Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

riscv: Suppress LOAD RWX linker warning #14724

Merged
merged 1 commit into from
Nov 11, 2024
Merged

Conversation

no1wudi
Copy link
Contributor

@no1wudi no1wudi commented Nov 11, 2024

Summary

Suppress the warning message "nuttx has a LOAD segment with RWX permissions" in case of RAM boot mode is selected. RAM MODE: BOOT_RUNFROMEXTSRAM/BOOT_RUNFROMISRAM/BOOT_RUNFROMSDRAM/BOOT_COPYTORAM

Similar to #14474

Impact

RISC-V GCC 12+

Testing

Local machine

Suppress the warning message "nuttx has a LOAD segment with RWX permissions" in case of RAM boot mode is selected.
RAM MODE: BOOT_RUNFROMEXTSRAM/BOOT_RUNFROMISRAM/BOOT_RUNFROMSDRAM/BOOT_COPYTORAM

Signed-off-by: Huang Qi <[email protected]>
@github-actions github-actions bot added Arch: risc-v Issues related to the RISC-V (32-bit or 64-bit) architecture Size: S The size of the change in this PR is small labels Nov 11, 2024
@nuttxpr
Copy link

nuttxpr commented Nov 11, 2024

[Experimental Bot, please feedback here]

No, this PR description does not fully meet the NuttX requirements. Here's why and how to fix it:

  • Insufficient Summary: While it states what is being changed, it lacks crucial information:

    • Why: Why is suppressing this warning necessary in RAM boot mode? Is it a false positive? Does it cause other issues? Explain the rationale.
    • How: How is the warning being suppressed? Code changes need to be described. Is a conditional being added? A flag being set? Be specific.
    • Related Issues: While a similar PR is mentioned, a specific related NuttX issue should be linked if one exists. This helps track the problem and its solution.
  • Incomplete Impact Assessment:

    • Is new feature added? Is existing feature changed?: This is unanswered. It's likely an existing feature change (warning suppression). State this explicitly.
    • Impact on user: While likely NO, explicitly state it.
    • Impact on build: Potentially YES if build flags are involved. Clarify.
    • Impact on hardware: Specify which RISC-V architectures and boards are affected. Don't just say "RISC-V".
    • Impact on documentation: If the behavior regarding this warning changes, documentation likely needs updating. Address this.
    • Impact on security: Suppressed warnings can sometimes mask real issues. Briefly justify why this suppression doesn't introduce security risks.
    • Impact on compatibility: Specify NO/YES and explain. Does suppressing this warning affect compatibility with any tools or other systems?
  • Missing Testing Detail:

    • Build Host(s): Provide details about your local machine (OS, CPU architecture, GCC version). "Local machine" is too vague.
    • Target(s): Be very specific. Which RISC-V architecture? Which simulator or board and its configuration? "sim" is insufficient.
    • Testing logs: Provide actual logs, both before and after the change. Empty code blocks are unacceptable. Demonstrate that the warning exists before the change and is absent after. Show that the system still functions correctly.

Example of an improved description:

Summary

This PR suppresses the "nuttx has a LOAD segment with RWX permissions" warning when building for RAM boot mode (BOOT_RUNFROMEXTSRAM/BOOT_RUNFROMISRAM/BOOT_RUNFROMSDRAM/BOOT_COPYTORAM) on RISC-V. This warning is a false positive in RAM boot scenarios because the entire image, including the .text segment, needs to be loaded into RAM and potentially modified before execution. The warning, while valid for other boot modes, is misleading in this context. The suppression is implemented by adding a conditional check around the warning emission in the linker script generation logic. This addresses [insert NuttX issue number here if applicable].

Impact

  • Is new feature added? Is existing feature changed?: Existing feature changed (warning suppression).
  • Impact on user: NO
  • Impact on build: YES. The build system will no longer emit the warning for RAM boot configurations on RISC-V.
  • Impact on hardware: YES. Affects RISC-V architectures (e.g., rv32imac, rv64imafdc) on boards like [board1], [board2] when configured for RAM boot.
  • Impact on documentation: YES. The documentation will be updated to reflect the change in warning behavior for RAM boot on RISC-V.
  • Impact on security: NO. This change only suppresses a false positive warning and does not introduce any new security vulnerabilities.
  • Impact on compatibility: NO.
  • Anything else to consider?: N/A

Testing

I confirm that changes are verified on local setup and works as intended:

  • Build Host(s): Ubuntu 22.04, x86_64, GCC 12.2.0
  • Target(s): QEMU RISC-V rv32imac, configuration: defconfig with CONFIG_BOOT_RUNFROMEXTSRAM=y; [Board Name], rv64imafdc, configuration: [config name]

Testing logs before change:

ld: warning: nuttx has a LOAD segment with RWX permissions
... other build logs ...

Testing logs after change:

... other build logs (no RWX warning) ...

By providing this level of detail, reviewers can understand the change, its rationale, and its impact much more effectively. This significantly increases the chances of a successful and timely PR merge.

@xiaoxiang781216 xiaoxiang781216 merged commit 8a6e832 into apache:master Nov 11, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Arch: risc-v Issues related to the RISC-V (32-bit or 64-bit) architecture 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.

4 participants