Skip to content

Conversation

@anjiahao1
Copy link
Contributor

Summary

The loading method of modlib has been optimized to allow dynamic modules to be loaded using attribute((destructor)), eliminating the need for module_initialize.

1. use '__attribute__((constructor))' mark initialize function
2. use '__attribute__((destructor))' mark uninitialize function
3. compile module with -fvisibility=hidden. use `__attribute__((visibility("default")))`
mark is need export symbol.so not need module_initialize to initialize export symbol.

Impact

modlib

Testing

qemu with mps3 an547

1. use '__attribute__((constructor))' mark initialize function
2. use '__attribute__((destructor))' mark uninitialize function
3. compile module with -fvisibility=hidden. use `__attribute__((visibility("default")))`
mark is need export symbol.so not need module_initialize to initialize export symbol.

Signed-off-by: anjiahao <anjiahao@xiaomi.com>
Signed-off-by: anjiahao <anjiahao@xiaomi.com>
@github-actions github-actions bot added the Area: OS Components OS Components issues label Oct 4, 2024
@github-actions github-actions bot added the Size: M The size of the change in this PR is medium label Oct 4, 2024
@nuttxpr
Copy link

nuttxpr commented Oct 4, 2024

[Experimental Bot, please feedback here]

The PR summary is insufficient and lacks detail. Here's a breakdown of what's missing and how to improve it:

Summary Issues:

  • Vague Description: "The loading method of modlib has been optimized" is too general. Specify what optimization was made and the specific benefits (e.g., improved performance, reduced memory usage).
  • Missing Technical Detail:
    • How does __attribute__((destructor)) eliminate module_initialize? Explain the mechanism and why it's an improvement.
    • Why is -fvisibility=hidden needed? Clarify its purpose in this optimization.
  • No Issue Reference: Is this related to an existing NuttX issue? If so, provide the link.

Impact Issues:

  • Incomplete:
    • Feature Impact: Is this a new feature or a change to an existing one? Be explicit.
    • User Impact: Will users need to modify their modules? If so, how?
    • Build Impact: Does this change the build process (e.g., new compiler flags)?
    • Documentation: Does the documentation need updating to reflect this change?
  • Overly Concise: "modlib" is not a helpful description. List all affected components and potential ripple effects.

Testing Issues:

  • Insufficient Detail:
    • Build Host: Provide the OS, CPU architecture, compiler (including version), and any relevant build tools.
    • Target: Specify the exact architecture (e.g., ARM Cortex-M4) and board configuration used for testing.
  • Missing Logs: Include the relevant portions of the testing logs before and after your changes. Highlight the differences that demonstrate the improvement.

To make this PR meet NuttX requirements, you need to:

  1. Expand the Summary: Provide a clear, technical description of the changes, their benefits, and any related issues.
  2. Complete the Impact Section: Address all potential impacts, providing specific details where applicable.
  3. Provide Detailed Testing Information: Include complete build host and target specifications and relevant testing logs that showcase the changes.

@cederom cederom requested a review from patacongo October 4, 2024 15:04
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.

Thank you @anjiahao1 :-)

  • Are there any possible compiler / linker compatibility issues?
  • Do we need to update documentation as well?

@xiaoxiang781216
Copy link
Contributor

xiaoxiang781216 commented Oct 5, 2024

Thank you @anjiahao1 :-)

  • Are there any possible compiler / linker compatibility issues?

The new approach uses the same mechanism of C++ constructor and destructor, so it's more general than before. Actually, so with the global c++ object work now with this change.

  • Do we need to update documentation as well?

No documentation reference mod_initializer_t

@xiaoxiang781216 xiaoxiang781216 merged commit a613bcb into apache:master Oct 5, 2024
@tmedicci
Copy link
Contributor

Hi @anjiahao1 , this PR also broke esp32-devkitc:module defconfig. After it, it fails when executing module example app:

nsh> module
main: Registering romdisk at /dev/ram0
up_relocateadd: WARNING: Ignoring RELA relocation R_XTENSA_ASM_EXPAND 11
up_relocateadd: WARNING: Ignoring RELA relocation R_XTENSA_ASM_EXPAND 11
modlib_symname: ERROR: Symbol has no name
modlib_symvalue: ERROR: SHN_UNDEF: Failed to get symbol name: -3
modlib_relocateadd: ERROR: Section 2 reloc 14: Undefined symbol[0] has no name: -3
up_relocateadd: WARNING: Ignoring RELA relocation R_XTENSA_ASM_EXPAND 11
up_relocateadd: WARNING: Ignoring RELA relocation R_XTENSA_ASM_EXPAND 11
up_relocateadd: WARNING: Ignoring RELA relocation R_XTENSA_ASM_EXPAND 11
up_relocateadd: WARNING: Ignoring RELA relocation R_XTENSA_ASM_EXPAND 11
up_relocateadd: WARNING: Ignoring RELA relocation R_XTENSA_ASM_EXPAND 11
up_relocateadd: WARNING: Ignoring RELA relocation R_XTENSA_ASM_EXPAND 11
up_relocateadd: WARNING: Ignoring RELA relocation R_XTENSA_ASM_EXPAND 11
xtensa_user_panic: User Exception: EXCCAUSE=001c task: module
dump_assert_info: Current Version: NuttX  10.4.0 fb7fe185d8 Oct 23 2024 16:40:57 xtensa
dump_assert_info: Assertion failed user panic: at file: common/xtensa_assert.c:180 task: module process: module 0x400f5fd8
up_dump_register:    PC: 400f56e6    PS: 00060b30
up_dump_register:    A0: 800f4464    A1: 3ffaf8a0    A2: 0000000b    A3: 3ffafcd0
up_dump_register:    A4: 00000000    A5: 00000000    A6: 3ffe7b98    A7: 00000000
up_dump_register:    A8: 0000fff2    A9: 400862d8   A10: 00000000   A11: 3ffafcd0
up_dump_register:   A12: 00000010   A13: 00000001   A14: 00000000   A15: 00000000
up_dump_register:   SAR: 00000004 CAUSE: 0000001c VADDR: 00000000
up_dump_register:  LBEG: 4000c2e0  LEND: 4000c2f6  LCNT: ffffffff
dump_stack: User Stack:
dump_stack:   base: 0x3ffaf328
dump_stack:   size: 00002008
dump_stack:     sp: 0x3ffaf8a0
stack_dump: 0x3ffaf880: 00000010 000020fc 00000010 00000000 800f3014 3ffaf8c0 00000020 3ffaf930
stack_dump: 0x3ffaf8a0: 3ffafcd0 000020ec 00000000 00000000 800f0775 3ffaf930 3ffafc58 3f407dd8
stack_dump: 0x3ffaf8c0: fffffffd 3ffaf8f0 3ffaf930 00000000 00000003 00000215 00000003 00000000
stack_dump: 0x3ffaf8e0: 00000000 0000000e 3ffe80f0 3ffe80f0 00000001 3ffafc58 3ffe7d78 3ffafcd0
stack_dump: 0x3ffaf900: 00000000 3ffaf930 3f407dcd 3f407dd8 3ffafcc8 00000004 3ffe7da0 3ffafcc8
stack_dump: 0x3ffaf920: 800f60e4 3ffaf9e0 3f407dcd 3f407dd8 400862d8 3ffafcb8 000001e4 00000004
stack_dump: 0x3ffaf940: 00000004 00000001 00003d48 464c457f 00010101 00000000 00000000 005e0001
stack_dump: 0x3ffaf960: 00000001 000000b4 00000000 000037d0 00000300 00000034 00280000 00220023
stack_dump: 0x3ffaf980: 00000000 3ffe7b70 00000000 3ffafd08 00000000 00000000 00000000 00000000
stack_dump: 0x3ffaf9a0: 00000000 00000000 00200000 00000021 00000020 00000003 00000000 3ffafa90
stack_dump: 0x3ffaf9c0: 00000000 00000000 00000003 3ffafb10 800e3ddd 3ffafa00 3f407d5f 3f407d55
stack_dump: 0x3ffaf9e0: 3f407d64 00000001 00000000 3ffaeec0 800e293c 3ffafac0 400f5fd8 00000001
stack_dump: 0x3ffafa00: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
stack_dump: 0x3ffafa20: deadbeef deadbeef 400e28e4 00050020 00000000 3ffafb00 00000000 00000000
stack_dump: 0x3ffafa40: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
stack_dump: 0x3ffafa60: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
stack_dump: 0x3ffafa80: 00000000 00000100 00000040 3f407f1c 3f40bf60 00000005 00000000 00000000
stack_dump: 0x3ffafaa0: 800e3c7c 00000080 00000000 00000000 00000000 3ffafae0 00000000 400f5fd8
stack_dump: 0x3ffafac0: 3ffaf318 3ffb0d38 00000000 3ffb0d38 00000000 3ffafb00 00000000 00000000
stack_dump: 0x3ffafae0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
stack_dump: 0x3ffafb00: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
dump_tasks:    PID GROUP PRI POLICY   TYPE    NPX STATE   EVENT      SIGMASK          STACKBASE  STACKSIZE      USED   FILLED    COMMAND
dump_task:       0     0   0 FIFO     Kthread -   Ready              0000000000000000 0x3ffb0010      3056       768    25.1%    Idle_Task
dump_task:       1     1 100 RR       Task    -   Waiting Semaphore  0000000000000000 0x3ffe0490     30416      1232     4.0%    nsh_main
dump_task:       2     2 100 RR       Task    -   Running            0000000000000000 0x3ffaf328      2008      1912    95.2%!   module

Any idea?

@tmedicci
Copy link
Contributor

Hi @anjiahao1 , this PR also broke esp32-devkitc:module defconfig. After it, it fails when executing module example app:

nsh> module
main: Registering romdisk at /dev/ram0
up_relocateadd: WARNING: Ignoring RELA relocation R_XTENSA_ASM_EXPAND 11
up_relocateadd: WARNING: Ignoring RELA relocation R_XTENSA_ASM_EXPAND 11
modlib_symname: ERROR: Symbol has no name
modlib_symvalue: ERROR: SHN_UNDEF: Failed to get symbol name: -3
modlib_relocateadd: ERROR: Section 2 reloc 14: Undefined symbol[0] has no name: -3
up_relocateadd: WARNING: Ignoring RELA relocation R_XTENSA_ASM_EXPAND 11
up_relocateadd: WARNING: Ignoring RELA relocation R_XTENSA_ASM_EXPAND 11
up_relocateadd: WARNING: Ignoring RELA relocation R_XTENSA_ASM_EXPAND 11
up_relocateadd: WARNING: Ignoring RELA relocation R_XTENSA_ASM_EXPAND 11
up_relocateadd: WARNING: Ignoring RELA relocation R_XTENSA_ASM_EXPAND 11
up_relocateadd: WARNING: Ignoring RELA relocation R_XTENSA_ASM_EXPAND 11
up_relocateadd: WARNING: Ignoring RELA relocation R_XTENSA_ASM_EXPAND 11
xtensa_user_panic: User Exception: EXCCAUSE=001c task: module
dump_assert_info: Current Version: NuttX  10.4.0 fb7fe185d8 Oct 23 2024 16:40:57 xtensa
dump_assert_info: Assertion failed user panic: at file: common/xtensa_assert.c:180 task: module process: module 0x400f5fd8
up_dump_register:    PC: 400f56e6    PS: 00060b30
up_dump_register:    A0: 800f4464    A1: 3ffaf8a0    A2: 0000000b    A3: 3ffafcd0
up_dump_register:    A4: 00000000    A5: 00000000    A6: 3ffe7b98    A7: 00000000
up_dump_register:    A8: 0000fff2    A9: 400862d8   A10: 00000000   A11: 3ffafcd0
up_dump_register:   A12: 00000010   A13: 00000001   A14: 00000000   A15: 00000000
up_dump_register:   SAR: 00000004 CAUSE: 0000001c VADDR: 00000000
up_dump_register:  LBEG: 4000c2e0  LEND: 4000c2f6  LCNT: ffffffff
dump_stack: User Stack:
dump_stack:   base: 0x3ffaf328
dump_stack:   size: 00002008
dump_stack:     sp: 0x3ffaf8a0
stack_dump: 0x3ffaf880: 00000010 000020fc 00000010 00000000 800f3014 3ffaf8c0 00000020 3ffaf930
stack_dump: 0x3ffaf8a0: 3ffafcd0 000020ec 00000000 00000000 800f0775 3ffaf930 3ffafc58 3f407dd8
stack_dump: 0x3ffaf8c0: fffffffd 3ffaf8f0 3ffaf930 00000000 00000003 00000215 00000003 00000000
stack_dump: 0x3ffaf8e0: 00000000 0000000e 3ffe80f0 3ffe80f0 00000001 3ffafc58 3ffe7d78 3ffafcd0
stack_dump: 0x3ffaf900: 00000000 3ffaf930 3f407dcd 3f407dd8 3ffafcc8 00000004 3ffe7da0 3ffafcc8
stack_dump: 0x3ffaf920: 800f60e4 3ffaf9e0 3f407dcd 3f407dd8 400862d8 3ffafcb8 000001e4 00000004
stack_dump: 0x3ffaf940: 00000004 00000001 00003d48 464c457f 00010101 00000000 00000000 005e0001
stack_dump: 0x3ffaf960: 00000001 000000b4 00000000 000037d0 00000300 00000034 00280000 00220023
stack_dump: 0x3ffaf980: 00000000 3ffe7b70 00000000 3ffafd08 00000000 00000000 00000000 00000000
stack_dump: 0x3ffaf9a0: 00000000 00000000 00200000 00000021 00000020 00000003 00000000 3ffafa90
stack_dump: 0x3ffaf9c0: 00000000 00000000 00000003 3ffafb10 800e3ddd 3ffafa00 3f407d5f 3f407d55
stack_dump: 0x3ffaf9e0: 3f407d64 00000001 00000000 3ffaeec0 800e293c 3ffafac0 400f5fd8 00000001
stack_dump: 0x3ffafa00: deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef deadbeef
stack_dump: 0x3ffafa20: deadbeef deadbeef 400e28e4 00050020 00000000 3ffafb00 00000000 00000000
stack_dump: 0x3ffafa40: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
stack_dump: 0x3ffafa60: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
stack_dump: 0x3ffafa80: 00000000 00000100 00000040 3f407f1c 3f40bf60 00000005 00000000 00000000
stack_dump: 0x3ffafaa0: 800e3c7c 00000080 00000000 00000000 00000000 3ffafae0 00000000 400f5fd8
stack_dump: 0x3ffafac0: 3ffaf318 3ffb0d38 00000000 3ffb0d38 00000000 3ffafb00 00000000 00000000
stack_dump: 0x3ffafae0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
stack_dump: 0x3ffafb00: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
dump_tasks:    PID GROUP PRI POLICY   TYPE    NPX STATE   EVENT      SIGMASK          STACKBASE  STACKSIZE      USED   FILLED    COMMAND
dump_task:       0     0   0 FIFO     Kthread -   Ready              0000000000000000 0x3ffb0010      3056       768    25.1%    Idle_Task
dump_task:       1     1 100 RR       Task    -   Waiting Semaphore  0000000000000000 0x3ffe0490     30416      1232     4.0%    nsh_main
dump_task:       2     2 100 RR       Task    -   Running            0000000000000000 0x3ffaf328      2008      1912    95.2%!   module

Any idea?

Hi @anjiahao1 , were you able to verify anything further?

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.

5 participants