Skip to content

Delete the start hook implementation and use crt0 instead. The module will initialize itself in crt0. #16154

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

Merged
merged 4 commits into from
Apr 9, 2025

Conversation

anjiahao1
Copy link
Contributor

@anjiahao1 anjiahao1 commented Apr 8, 2025

Summary

Delete the start hook implementation and use crt0 instead. The module will initialize itself in crt0.
and with delete start hook, we can unfiy tcb_s task_tcb_s pthread_tcb

the modification is required to support c++ constructor for protected and kernel mode since start_hook can't call directly inside kernel space.

Impact

module build

Testing

mps3an547 module test with cxx
qemu armv7 module test with cxx

@github-actions github-actions bot added 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: 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: z80 Issues related to the Z80 architecture Area: OS Components OS Components issues Area: BINFMT Board: arm Board: risc-v Board: simulator Board: xtensa Size: L The size of the change in this PR is large labels Apr 8, 2025
@github-actions github-actions bot added Size: M The size of the change in this PR is medium and removed Board: simulator Size: L The size of the change in this PR is large labels Apr 8, 2025
@github-actions github-actions bot added Size: L The size of the change in this PR is large and removed Size: M The size of the change in this PR is medium labels Apr 8, 2025
@nuttxpr
Copy link

nuttxpr commented Apr 8, 2025

[Experimental Bot, please feedback here]

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

  • Insufficient Summary: While the summary mentions what is changed, it lacks crucial details about why this change is necessary. What problem does using crt0 solve? What are the benefits of unifying tcb_s, task_tcb_s, and pthread_tcb? How does deleting the start hook achieve this unification?

  • Incomplete Impact Assessment: "module build" is too vague. Be specific. Does this change the build process for all modules? Only certain ones? Will users need to change their Makefiles or configurations? All other impact categories are missing (user impact, hardware impact, documentation, security, compatibility). Even if the answer is "NO" for each, it must be explicitly stated.

  • Missing Testing Details: The testing description is also too vague. Provide specific details about the build host (OS, compiler version) and the target (specific board/configuration within qemu-armv7). The logs sections are empty; actual log output demonstrating the functionality before and after the change is required.

To make this PR compliant, address the following:

  • Expand the Summary: Explain the rationale behind the change. What problem does it solve? What are the advantages of using crt0 and unifying the TCB structures?
  • Complete the Impact Assessment: Address all impact categories, even if the answer is "NO". Provide specific details when the answer is "YES". For example, if the build process changes, explain how.
  • Provide Detailed Testing Information: Specify the build host OS, compiler version, and the exact target configuration used (qemu-armv7 board/config). Include actual log output demonstrating the functionality before and after the change. Show that the module initializes correctly using crt0 and that the intended unification of TCB structures has been achieved.

@xiaoxiang781216 xiaoxiang781216 requested review from anchao and pussuw April 8, 2025 06:40
@pussuw
Copy link
Contributor

pussuw commented Apr 8, 2025

Should https://github.com/apache/nuttx/blob/master/libs/libc/misc/lib_cxx_initialize.c be removed by this change as well ?

@lupyuen
Copy link
Member

lupyuen commented Apr 9, 2025

@anjiahao1 Please fix the conflict. Thanks :-)

@github-actions github-actions bot added Size: M The size of the change in this PR is medium and removed Area: Documentation Improvements or additions to documentation Arch: simulator Issues related to the SIMulator Size: XL The size of the change in this PR is very large. Consider breaking down the PR into smaller pieces. labels Apr 9, 2025
@anjiahao1
Copy link
Contributor Author

Should https://github.com/apache/nuttx/blob/master/libs/libc/misc/lib_cxx_initialize.c be removed by this change as well ?

ok,remove it

@github-actions github-actions bot added Size: L The size of the change in this PR is large and removed Size: M The size of the change in this PR is medium labels Apr 9, 2025
@lupyuen
Copy link
Member

lupyuen commented Apr 9, 2025

@nuttxpr test avaota-a1:nsh

@nuttxpr
Copy link

nuttxpr commented Apr 9, 2025

[Experimental Bot, please feedback here]

Build and Test Successful (avaota-a1:nsh)
https://gitlab.com/lupyuen/nuttx-build-log/-/snippets/4832906

$ git clone https://github.com/anjiahao1/nuttx nuttx --branch 20250408_modlib
$ git clone https://github.com/apache/nuttx-apps apps --branch master
$ pushd nuttx
$ git reset --hard HEAD
HEAD is now at 97fe263c11 arch/arm64: remove unrecognized command-line option
$ popd
$ pushd apps
$ git reset --hard HEAD
HEAD is now at 21a6a1369 kernel build:avoid multiple definition ld script
$ popd
NuttX Source: https://github.com/apache/nuttx/tree/97fe263c11f62160e35357fdbc654a6ec3ac23df
NuttX Apps: https://github.com/apache/nuttx-apps/tree/21a6a13698d1418fbaafebea12d46cd7cc957351
$ cd nuttx
$ tools/configure.sh avaota-a1:nsh
$ make -j
$ make -j export
$ pushd ../apps
$ ./tools/mkimport.sh -z -x ../nuttx/nuttx-export-10.1.0.tar.gz
$ make -j import
$ popd
$ genromfs -f initrd -d ../apps/bin -V NuttXBootVol
$ head -c 65536 /dev/zero
$ cat nuttx.bin /tmp/nuttx.pad initrd
$ scp Image thinkcentre:/tmp/Image
$ ssh thinkcentre sudo /home/user/copy-image.sh
$ sd-mux-ctrl --device-serial=sd-wire_02-09 --ts
$ mkdir -p /tmp/sda1
$ mount /dev/sda1 /tmp/sda1
$ cp /tmp/Image /tmp/sda1/
$ rm /tmp/Image
$ umount /tmp/sda1
$ sd-mux-ctrl --device-serial=sd-wire_02-09 --dut
$ cd /home/luppy/nuttx-build-farm
$ ssh thinkcentre
nsh> uname -a
NuttX 10.1.0 97fe263c11 Apr  9 2025 15:19:15 arm64 avaota-a1
nsh> ostest
arena        a000    28000
ordblks         2        4
mxordblk     5ff8    1bff8
uordblks     27e8     6700
fordblks     7818    21900
user_main: Exiting
ostest_main: Exiting with status 0
nshNow running https://github.com/lupyuen/nuttx-build-farm/blob/main/avaota-power.sh off
----- Power off Avaota-A1
[]

@lupyuen
Copy link
Member

lupyuen commented Apr 9, 2025

@nuttxpr test milkv_duos:nsh

@nuttxpr
Copy link

nuttxpr commented Apr 9, 2025

[Experimental Bot, please feedback here]

Build and Test Successful (milkv_duos:nsh)
https://gitlab.com/lupyuen/nuttx-build-log/-/snippets/4832909

$ git clone https://github.com/anjiahao1/nuttx nuttx --branch 20250408_modlib
$ git clone https://github.com/apache/nuttx-apps apps --branch master
$ pushd nuttx
$ git reset --hard HEAD
HEAD is now at 97fe263c11 arch/arm64: remove unrecognized command-line option
$ popd
$ pushd apps
$ git reset --hard HEAD
HEAD is now at 21a6a1369 kernel build:avoid multiple definition ld script
$ popd
NuttX Source: https://github.com/apache/nuttx/tree/97fe263c11f62160e35357fdbc654a6ec3ac23df
NuttX Apps: https://github.com/apache/nuttx-apps/tree/21a6a13698d1418fbaafebea12d46cd7cc957351
$ cd nuttx
$ tools/configure.sh milkv_duos:nsh
$ make -j
$ make -j export
$ pushd ../apps
$ ./tools/mkimport.sh -z -x ../nuttx/nuttx-export-10.1.0.tar.gz
$ make -j import
$ popd
$ genromfs -f initrd -d ../apps/bin -V NuttXBootVol
$ head -c 65536 /dev/zero
$ cat nuttx.bin /tmp/nuttx.pad initrd
$ scp Image tftpserver:/tftpboot/Image-sg2000
$ cd /home/luppy/nuttx-build-farm
$ ssh tftpserver
OpenSBI v0.9
nsh> uname -a
NuttX 10.1.0 97fe263c11 Apr  9 2025 15:23:58 risc-v milkv_duos
nsh> ostest
arena       81000    81000
ordblks         2        3
mxordblk    7cff8    78ff8
uordblks     2660     4570
fordblks    7e9a0    7ca90
user_main: Exiting
ostest_main: Exiting with status 0
nsh> Now running https://github.com/lupyuen/nuttx-build-farm/blob/main/oz64-power.sh off
----- Power off Oz64
[]

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.

test:
1.use mps3-an547 build helloxx as module and run it
2.use qemu-armv7a:knsh test kernel build helloxx and run it

Signed-off-by: anjiahao <[email protected]>
aarch64-none-elf-gcc: error: unrecognized command-line option '-mlong-calls'

Signed-off-by: anjiahao <[email protected]>
@xiaoxiang781216 xiaoxiang781216 merged commit c9a1507 into apache:master Apr 9, 2025
39 checks passed
anjiahao1 added a commit to anjiahao1/nuttx that referenced this pull request Apr 15, 2025
we use crt0 inside of start hook in pr apache#16154, so xtensa also need add it.

Signed-off-by: anjiahao <[email protected]>
xiaoxiang781216 pushed a commit that referenced this pull request Apr 15, 2025
we use crt0 inside of start hook in pr #16154, so xtensa also need add it.

Signed-off-by: anjiahao <[email protected]>
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: 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: z80 Issues related to the Z80 architecture Area: BINFMT Area: OS Components OS Components issues Board: arm Board: arm64 Board: risc-v Board: xtensa Size: L The size of the change in this PR is large
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants