Skip to content

Conversation

@anjiahao1
Copy link
Contributor

Summary

modlib bug fix

  1. allow 64bit elf load
  2. so need export symbols, exec elf don't need export symbols

Impact

modlib

Testing

rv-virt:knsh and rv-virt:knsh64 with ostest and getprime, pass it

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

nuttxpr commented Oct 13, 2024

[Experimental Bot, please feedback here]

No, this PR does not meet the NuttX requirements.

Here's why and how to improve it:

  • Insufficient Summary:

    • Vague Description: "modlib bug fix" is too general. Clearly state the bug the PR fixes. For example, "Fix modlib's inability to load 64-bit ELF files".
    • Missing Explanation: Explain why exporting symbols is necessary for 64-bit ELF loading but not for regular ELF execution.
    • No References: Link any relevant NuttX Issues that this PR addresses.
  • Incomplete Impact:

    • Which Feature? Be specific about the modlib feature impacted (e.g., "ELF loading functionality").
    • User Impact: Will users need to make any changes to their code or configuration due to this fix? Be explicit.
    • Build/Hardware/Documentation: Even if the answer is "NO" for most impact categories, state it explicitly for clarity.
  • Inadequate Testing:

    • Insufficient Detail: "rv-virt:knsh" is not descriptive enough. Specify the exact NuttX configuration (board, architecture) and versions used.
    • Missing Logs: Provide actual testing logs before and after the change. Demonstrate the bug and its resolution.
    • Limited Scope: Ideally, test on more than one platform/configuration to ensure the fix doesn't introduce regressions.

How to Improve the PR:

  1. Expand the Summary: Provide a clear, concise, and informative description of the bug, the solution, and its rationale.
  2. Complete the Impact Analysis: Address all impact categories specifically, even if it's to say "NO" with a brief justification.
  3. Provide Detailed Testing Information: Specify the exact test environments, include relevant log snippets, and consider expanding test coverage.

Copy link
Member

@lupyuen lupyuen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested OK on rv-virt:knsh, rv-virt:knsh64 and milkv_duos:nsh. Stress Test OK with 20 iterations of knsh64. Thanks!

NuttShell (NSH) NuttX-10.1.0
nsh> uname -a
NuttX 10.1.0 203b37992e Oct 13 2024 17:38:01 risc-v rv-virt
nsh> ostest
ostest_main: Exiting with status 0

@anjiahao1
Copy link
Contributor Author

look like ci build break
image

@lupyuen
Copy link
Member

lupyuen commented Oct 14, 2024

@anjiahao1 The build should be fixed by apache/nuttx-apps#2716

Signed-off-by: anjiahao <anjiahao@xiaomi.com>
Signed-off-by: anjiahao <anjiahao@xiaomi.com>
@xiaoxiang781216 xiaoxiang781216 merged commit 4156eca into apache:master Oct 14, 2024
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: M The size of the change in this PR is medium

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants