Skip to content

Conversation

@realprocrastinator
Copy link

@realprocrastinator realprocrastinator commented Oct 12, 2024

Summary

According to POSIX semantics https://pubs.opengroup.org/onlinepubs/009604599/functions/longjmp.html. The setjmp should return 1 instead of 0 when calling longjmp with 0 as the second argument.

Impact

The current implementation of setjmp on some architectures misbehaves. In my case, it affects the CMocka framework.

Testing

The testing code lives int the nuttx apps repository as a seperate patch.

@github-actions github-actions bot added Area: OS Components OS Components issues Size: S The size of the change in this PR is small labels Oct 12, 2024
@nuttxpr
Copy link

nuttxpr commented Oct 12, 2024

[Experimental Bot, please feedback here]

This PR description is missing crucial information and does not fully meet the NuttX requirements. Here's a breakdown:

Missing Information:

  • What functional part of the code is being changed? Be specific. Which files, functions, or modules are modified to correct the setjmp behavior?
  • How does the change exactly work? Explain the technical implementation details of the fix.
  • Impact on user: Will any existing applications relying on the incorrect behavior break? How should they adapt?
  • Impact on hardware: While the description mentions "some architectures", it doesn't specify which ones. List the affected architectures explicitly.
  • Testing:
    • Incomplete: "Testing code lives in the nuttx apps repository" is not sufficient. Include relevant snippets of the testing logs demonstrating the issue before and the fix after the change.
    • Unclear: "as a separate patch" is confusing. Is the testing part of this PR or a separate one? If separate, provide a link.
  • Links: Include links to the related NuttX issue and the testing patch in the NuttX Apps repository.

Recommendations:

  1. Expand the Summary: Provide specific details about the code changes and their mechanism.
  2. Clarify Impact: Clearly state the affected architectures and potential user impact (if any).
  3. Complete Testing Section: Include relevant testing log snippets directly in the PR description. If the testing is a separate patch, provide a link to it.
  4. Add Links: Include links to the related NuttX and NuttX Apps issues/PRs.

By addressing these points, you'll make your PR much stronger and easier for reviewers to understand and approve.

…d argument

Signed-off-by: Gao Jiawei <gaojiawei@xiaomi.com>
@xiaoxiang781216 xiaoxiang781216 merged commit 89d6abf into apache:master Oct 15, 2024
@xiaoxiang781216
Copy link
Contributor

@realprocrastinator please resubmit your apps side change.

@realprocrastinator
Copy link
Author

@realprocrastinator please resubmit your apps side change.

Gottcha. Thx for reminding.

@realprocrastinator
Copy link
Author

@xiaoxiang781216 Hi Xiaoxiang, I've just resubmitted the test code for nuttx-app repo. Pls check the link apache/nuttx-apps#2725. Thx.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: OS Components OS Components issues Size: S The size of the change in this PR is small

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants