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

libc/elf: rename modlib to elf #15765

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

libc/elf: rename modlib to elf #15765

wants to merge 1 commit into from

Conversation

anchao
Copy link
Contributor

@anchao anchao commented Feb 5, 2025

Summary

libc/elf: rename modlib to elf

Renaming "modlib" to "elf" is more in line with the implementation content,
which makes it easier for individual developers to understand the capabilities of this module.

CONFIG_LIBC_MODLIB -> CONFIG_LIBC_ELF

Signed-off-by: chao an [email protected]

Impact

N/A, depends on apache/nuttx-apps#2985

Testing

sim/sotest

@github-actions github-actions bot added Area: Documentation Improvements or additions to documentation Area: Tooling Area: Build system Arch: arm Issues related to ARM (32-bit) architecture Arch: arm64 Issues related to ARM64 (64-bit) architecture Arch: avr Issues related to all AVR(8-bit or 32-bit) architectures Arch: ceva Issues related to CEVA architecture Arch: mips Issues related to the MIPS architecture Arch: openrisc Issues related to the OpenRISC architecture Arch: risc-v Issues related to the RISC-V (32-bit or 64-bit) architecture Arch: simulator Issues related to the SIMulator Arch: sparc Issues related to the SPARC architecture Arch: tricore Issues related to the TriCore architecture from Infineon Arch: x86 Issues related to the x86 architecture Arch: x86_64 Issues related to the x86_64 architecture Arch: xtensa Issues related to the Xtensa architecture Arch: z16 Issues related to the Z16 architecture Arch: z80 Issues related to the Z80 architecture Area: Memory Management Memory Management issues Area: OS Components OS Components issues Area: BINFMT Board: arm Board: arm64 Board: renesas Board: risc-v Board: simulator Board: sparc Board: xtensa Size: XL The size of the change in this PR is very large. Consider breaking down the PR into smaller pieces. labels Feb 5, 2025
@nuttxpr
Copy link

nuttxpr commented Feb 5, 2025

[Experimental Bot, please feedback here]

No. This PR, as described, does not fully meet the NuttX requirements. Here's why:

  • Insufficient Impact Assessment: Simply stating "N/A" for impact is unacceptable. Even a seemingly simple rename can have impacts. At a minimum, the developer needs to confirm:

    • Impact on user: Will any application code need to be changed due to the config option rename? Even if the answer is no, it needs to be explicitly stated.
    • Impact on documentation: The documentation needs to be updated to reflect the name change. The PR should either include these changes or explain why they are not necessary.
    • Impact on compatibility: Does this change break any backward compatibility? Again, even if the answer is no, it needs to be explicitly stated and justified.
  • Insufficient Testing Information: "sim/sotest" is not enough information. The PR needs to specify:

    • Build Host: What operating system, compiler version, etc. was used to build NuttX?
    • Target Details: What simulator was used (qemu, etc.)? What architecture? What configuration?
    • Testing Logs: The PR includes placeholders for logs, but no actual logs are provided. There needs to be concrete evidence that the change works as intended. What tests were run? What were the results before and after the change?

In short, while the summary provides a reasonable explanation for the why, the impact and testing sections lack crucial details required for proper review and acceptance.

@anchao
Copy link
Contributor Author

anchao commented Feb 5, 2025

Sorry @anchao I tested the new branch anchao:25020501 for NuttX and Apps, it fails to build for rv-virt:knsh64. Did I miss something? Thanks :-) https://gist.github.com/lupyuen/58c3b955a0c222776af523662561c494

+ git clone https://github.com/anchao/nuttx --branch 25020501 nuttx
+ git clone https://github.com/anchao/nuttx-apps --branch 25020501 apps
+ tools/configure.sh rv-virt:knsh64
+ make -j

CC:  driver/fs_registerblockdriver.c In file included from binfmt_globals.c:29:
binfmt.h:261:5: error: conflicting types for 'elf_initialize'; have 'int(void)'
  261 | int elf_initialize(void);
      |     ^~~~~~~~~~~~~~

@lupyuen Thank you, I updated my modification and changed elf to libelf, so that rv-virt:knsh64 can be compiled successfully, but I found a new issue, rv-virt:knsh64 cannot be started normally on my platform, even if I roll back my changes

@xiaoxiang781216 xiaoxiang781216 added the breaking change This change requires a mitigation entry in the release notes. label Feb 5, 2025
@anchao
Copy link
Contributor Author

anchao commented Feb 5, 2025

@lupyuen Thank you, I updated my modification and changed elf to libelf, so that rv-virt:knsh64 can be compiled successfully, but I found a new issue, rv-virt:knsh64 cannot be started normally on my platform, even if I roll back my changes

@lupyuen I just asked @yf13 to help verify this PR, which could be bringup normally on qemu6.2. My qemu version is 8.2.2. It seems that there are some compatibility issues

@lupyuen
Copy link
Member

lupyuen commented Feb 5, 2025

Cool lemme know when we're ready to retest, I'll run my script again. Thanks :-)

@anchao
Copy link
Contributor Author

anchao commented Feb 5, 2025

Cool lemme know when we're ready to retest, I'll run my script again. Thanks :-)

It seems that qemu 8.2.2 cannot execute rv-virt/knsh64 not caused by my PR. If your test environment is lower than this version, please restart your script. Thank you.

@github-actions github-actions bot added Size: L The size of the change in this PR is large and removed Size: XL The size of the change in this PR is very large. Consider breaking down the PR into smaller pieces. labels Feb 5, 2025
@github-actions github-actions bot added Size: XL The size of the change in this PR is very large. Consider breaking down the PR into smaller pieces. and removed Size: L The size of the change in this PR is large labels Feb 5, 2025
Renaming "modlib" to "elf" is more in line with the implementation content,
which makes it easier for individual developers to understand the capabilities of this module.

CONFIG_LIBC_MODLIB -> CONFIG_LIBC_ELF

Signed-off-by: chao an <[email protected]>
@anchao
Copy link
Contributor Author

anchao commented Feb 5, 2025

Cool lemme know when we're ready to retest, I'll run my script again. Thanks :-)

It seems that qemu 8.2.2 cannot execute rv-virt/knsh64 not caused by my PR. If your test environment is lower than this version, please restart your script. Thank you.

@lupyuen Sorry to bother you again. I've updated the naming of some relevant macros and code directories. we may need to run the script again.

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:knsh64. Thanks :-)
https://gist.github.com/lupyuen/edb4747d1235c77be624bd4c6a24a500

+ git clone https://github.com/anchao/nuttx --branch 25020501 nuttx
+ git clone https://github.com/anchao/nuttx-apps --branch 25020501 apps
NuttX Source: https://github.com/apache/nuttx/tree/fa059c19fad275324afdfec023d24a85827516e9
NuttX Apps: https://github.com/apache/nuttx-apps/tree/6d0afa6c9b8d4ecb896f9aa177dbdfcd40218f48
+ tools/configure.sh rv-virt:knsh64
nsh> uname -a
NuttX 10.4.0 fa059c19fa Feb  5 2025 19:25:45 risc-v rv-virt
nsh> ostest
ostest_main: Exiting with status 0

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.

Also tested OK on real hardware: Oz64 SG2000 RISC-V SBC (milkv_duos:nsh). Thanks :-)
https://gist.github.com/lupyuen/15fd5e68f66b03f19f476b5ef3c066f1

+ git clone https://github.com/anchao/nuttx nuttx --branch 25020501
+ git clone https://github.com/anchao/nuttx-apps apps --branch 25020501
NuttX Source: https://github.com/apache/nuttx/tree/fa059c19fad275324afdfec023d24a85827516e9
NuttX Apps: https://github.com/apache/nuttx-apps/tree/6d0afa6c9b8d4ecb896f9aa177dbdfcd40218f48
+ tools/configure.sh milkv_duos:nsh
nsh> uname -a
NuttX 10.4.0 fa059c19fa Feb  6 2025 22:00:28 risc-v milkv_duos
nsh> ostest
ostest_main: Exiting with status 0

Copy link
Contributor

@cederom cederom left a comment

Choose a reason for hiding this comment

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

Thanks @anchao :-)

  • Why this change is really necessary? What it fixes / improves?
  • Modlib library [1] is meant for loadable modules of various formats. So far we only support ELF but other formats may be added. When other formats are added then elf name will be confusing again. We should stick to name modlib as this is loadable module component, not just elf handler.
  • How does that change impact existing configurations and backward-compatibility (i.e. older nuttx-apps)? You marked impact N/A but this seems breaking change so description is invalid? Also providing reference to different change is not an impact factor just a reference / dependency and should belong to summary section.
  • This change will breaks existing libc and configuration. It will break compatibility with nuttx-apps out of sync with nuttx. This change should be marked as breaking change. It would be good to first discuss this kind of change on mailing lists.
  • Where are build / runtime test logs? Did you verify build and runtime with latest release of nuttx-apps to see if it does not break things?
  • How does that align with other standards (libc) and OS/RTOS implementations?
  • Okay I can see that modlib_ calls [2] are renamed to libelf_. Is this in align and compatibility with existing libelf library?

[1] https://nuttx.apache.org/docs/latest/applications/examples/module/index.html
[2] https://github.com/apache/nuttx/tree/fdc0b608b5893a207ea37a276e0adade6ded3b62/libs/libc/modlib

@anchao
Copy link
Contributor Author

anchao commented Feb 7, 2025

Thanks @anchao :-)

  • Why this change is really necessary? What it fixes / improves?
  • Modlib library [1] is meant for loadable modules of various formats. So far we only support ELF but other formats may be added. When other formats are added then elf name will be confusing again. We should stick to name modlib as this is loadable module component, not just elf handler.
  • How does that change impact existing configurations and backward-compatibility (i.e. older nuttx-apps)? You marked impact N/A but this seems breaking change so description is invalid? Also providing reference to different change is not an impact factor just a reference / dependency and should belong to summary section.
  • This change will breaks existing libc and configuration. It will break compatibility with nuttx-apps out of sync with nuttx. This change should be marked as breaking change. It would be good to first discuss this kind of change on mailing lists.
  • Where are build / runtime test logs? Did you verify build and runtime with latest release of nuttx-apps to see if it does not break things?
  • How does that align with other standards (libc) and OS/RTOS implementations?
  • Okay I can see that modlib_ calls [2] are renamed to libelf_. Is this in align and compatibility with existing libelf library?

[1] https://nuttx.apache.org/docs/latest/applications/examples/module/index.html [2] https://github.com/apache/nuttx/tree/fdc0b608b5893a207ea37a276e0adade6ded3b62/libs/libc/modlib

apache/nuttx-apps#2985 (comment)

@pussuw
Copy link
Contributor

pussuw commented Feb 26, 2025

I know about the implementation duplication before, libelf and modlib were about the same. However, will we have a need to "re-introduce" modlib, if it gets capability to load modules other than elf format ? Or will we just implement the new module loader into whateverlib and re-create modlib as a wrapper ?

Because I suspect the original intention of modlib was not at all to duplicate code, it was supposed to be higher in the code hierarchy compared to libelf ?

@anchao
Copy link
Contributor Author

anchao commented Feb 27, 2025

I know about the implementation duplication before, libelf and modlib were about the same. However, will we have a need to "re-introduce" modlib, if it gets capability to load modules other than elf format ? Or will we just implement the new module loader into whateverlib and re-create modlib as a wrapper ?

Because I suspect the original intention of modlib was not at all to duplicate code, it was supposed to be higher in the code hierarchy compared to libelf ?

Actually, that's not the case. In the architectural design, the structure of binfmt should be reused to complete the glue code. modlib is just a part of libelf. If we consider the design from the user's perspective, some future loadable types, such as coff, fdpic, flat, misc, script, etc., should be integrated with binfmt rather than with modlib.

https://github.com/torvalds/linux/blob/master/fs/binfmt_elf.c
https://github.com/torvalds/linux/blob/master/fs/binfmt_elf_fdpic.c
https://github.com/torvalds/linux/blob/master/fs/binfmt_flat.c
https://github.com/torvalds/linux/blob/master/fs/binfmt_misc.c
https://github.com/torvalds/linux/blob/master/fs/binfmt_script.c
https://github.com/alexbousso/kernel_2.4.18-14/blob/master/fs/binfmt_coff.c

This is where they belong:
https://github.com/apache/nuttx/tree/master/binfmt

@anchao
Copy link
Contributor Author

anchao commented Feb 27, 2025

I'm not saying that the structure of Linux must be correct. However, I think that @gregory-nutt and Linus had similar ideas in the initial design. Compared with the kernels implements of the other 3-parties, I believe that the initial design of NuttX is more reasonable.

Copy link
Contributor

@pussuw pussuw left a comment

Choose a reason for hiding this comment

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

Thanks for the explanation, LGTM!

@jerpelea
Copy link
Contributor

@anchao please rebase

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Arch: arm Issues related to ARM (32-bit) architecture Arch: arm64 Issues related to ARM64 (64-bit) architecture Arch: avr Issues related to all AVR(8-bit or 32-bit) architectures Arch: ceva Issues related to CEVA architecture Arch: mips Issues related to the MIPS architecture Arch: openrisc Issues related to the OpenRISC architecture Arch: risc-v Issues related to the RISC-V (32-bit or 64-bit) architecture Arch: simulator Issues related to the SIMulator Arch: sparc Issues related to the SPARC architecture Arch: tricore Issues related to the TriCore architecture from Infineon Arch: x86 Issues related to the x86 architecture Arch: x86_64 Issues related to the x86_64 architecture Arch: xtensa Issues related to the Xtensa architecture Arch: z16 Issues related to the Z16 architecture Arch: z80 Issues related to the Z80 architecture Area: BINFMT Area: Build system Area: Documentation Improvements or additions to documentation Area: Memory Management Memory Management issues Area: OS Components OS Components issues Area: Tooling Board: arm Board: arm64 Board: renesas Board: risc-v Board: simulator Board: sparc Board: xtensa breaking change This change requires a mitigation entry in the release notes. Size: XL The size of the change in this PR is very large. Consider breaking down the PR into smaller pieces.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants