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

modlib: preprocess gnu-elf.ld for executable ELF #15444

Merged
merged 4 commits into from
Jan 11, 2025

Conversation

yf13
Copy link
Contributor

@yf13 yf13 commented Jan 7, 2025

Summary

This allows apps for kernel mode NuttX (e.g. on arm/qemu-armv7a) can be built as executable type ELF files. Benefits of executable programs are smaller in size and easier GDB debugging.

Initial scope was only for qemu-armv7a but it used an almost duplicated gnu-elf.ld which differs only at .text and .data addresses from the gnu-elf.ld in modlib.

To avoid such duplications, a preprocessed gnu-elf.ld in modlib is added so that to adapt to target config. This requires minor tweaks for addrenv.h so that some macros can be included in gnu-elf.ld.in.

Impacts

Hardware: qemu-armv7a or using kernel build.

Testing

  • local checks with QEMU v6.2 on Ubuntu 22.04: qemu-armv7a:knsh, bl602evb:elf.
  • CI checks

@github-actions github-actions bot added Arch: arm Issues related to ARM (32-bit) architecture Board: arm Size: M The size of the change in this PR is medium labels Jan 7, 2025
@nuttxpr
Copy link

nuttxpr commented Jan 7, 2025

[Experimental Bot, please feedback here]

Yes, this PR appears to meet the basic NuttX requirements, although it could be more thorough. While it addresses the key points, it lacks specific details in several areas which would make review easier.

Here's a breakdown:

Strengths:

  • Clear Summary: The summary explains the what and why of the change, and the benefit (smaller size, easier debugging).
  • Identifies Impacted Area: Correctly identifies the impact as hardware-specific (qemu-armv7a).
  • Mentions Testing: Indicates testing was performed locally and that CI checks are expected.

Weaknesses:

  • Missing Detail in Summary: The how is somewhat vague. It states "allows apps... to be built as executable type ELF files," but doesn't explain the mechanism of the change. What was modified to achieve this?
  • Impact Lacks Detail: The impact section needs significant expansion. While "qemu-armv7a" is mentioned, it doesn't specify whether other ARM architectures or boards are affected. All other impact categories are simply omitted (user impact, build process, documentation, security, compatibility). These should be explicitly stated as "NO" or "YES" with explanations.
  • Insufficient Testing Information: "Local checks" and "CI checks" are too general. The PR needs specific details about the host system (OS version, compiler version). The target information is slightly better but should specify the NuttX configuration used. Most importantly, the "Testing logs before change" and "Testing logs after change" sections are empty. These are crucial for demonstrating the change's effect and should include relevant output showing the difference in behavior.

Recommendation:

The PR author should add the missing details to strengthen it. Specifically:

  • Summary: Elaborate on the how. What files were changed? What build system modifications were made?
  • Impact: Address all impact categories. Even if the answer is "NO," it should be explicitly stated. For "hardware," be more specific about affected architectures/boards.
  • Testing: Provide detailed host and target information. Include actual log output demonstrating the change's effect (e.g., file sizes before/after, GDB debugging experience comparison). If logs are extensive, consider linking to a separate file or using a diff tool.

@yf13
Copy link
Contributor Author

yf13 commented Jan 7, 2025

Seems that CI is blocked by issue similar to #15441?

Copy link
Contributor

@acassis acassis left a comment

Choose a reason for hiding this comment

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

Please include a Documentation/ at qemu page showing a usage example

This allows the header file to be useful from non-C sources such as
assembly code or linker scripts.

Signed-off-by: Yanfeng Liu <p-liuyanfeng9@xiaomi.com>
@github-actions github-actions bot added Size: S The size of the change in this PR is small and removed Area: Tooling Size: M The size of the change in this PR is medium labels Jan 9, 2025
@yf13 yf13 changed the title arm/qemu-armv7a: add EXE apps support modlib: preprocessed gnu-elf.ld for executable ELF Jan 9, 2025
@yf13 yf13 added Arch: all Issues that apply to all architectures and removed Arch: arm Issues related to ARM (32-bit) architecture labels Jan 9, 2025
@yf13 yf13 changed the title modlib: preprocessed gnu-elf.ld for executable ELF modlib: preprocess gnu-elf.ld for executable ELF Jan 9, 2025
@github-actions github-actions bot added the Arch: arm Issues related to ARM (32-bit) architecture label Jan 9, 2025
@yf13
Copy link
Contributor Author

yf13 commented Jan 9, 2025

@acassis, the usage is rather simple: just enable BINFMT_ELF_EXECUTABLE config for a kernel build.

Actually this isn't new or specific to QEMU, I used it before for k230 devices, all canmv230 kernel mode have it enable by default, see pull/11513 for the old story.

@acassis
Copy link
Contributor

acassis commented Jan 9, 2025

@acassis, the usage is rather simple: just enable BINFMT_ELF_EXECUTABLE config for a kernel build.

Actually this isn't new or specific to QEMU, I used it before for k230 devices, all canmv230 kernel mode have it enable by default, see pull/11513 for the old story.

Thank you for explanation, I thought it was specific for QEMU. BTW, but you should create a board config demo and include it in the sim page

@yf13
Copy link
Contributor Author

yf13 commented Jan 9, 2025

BTW, but you should create a board config demo and include it in the sim page

@acassis I am unsure if sim device has KERNEL build because existing defconfigs there are all for FLAT build. My current understanding is that executable ELFs depend on SYSCALL thus not proper for FLAT build?

@acassis
Copy link
Contributor

acassis commented Jan 10, 2025 via email

@yf13
Copy link
Contributor Author

yf13 commented Jan 11, 2025

@acassis, thanks.

Here I tried sim:elf. by default ELF type is relocatable and it works.

I then tried to blindly enable BINFMT_ELF_EXECUTABLE after selecting ARCH_HAVE_ELF_EXECUTABLE in arch/sim/Kconfig first, but the built ELFs in ../apps/examples/elf/tests/romfs are still relocatable.

So if there is a way to make executable ELFs for sim, please teach.

libs/libc/modlib/gnu-elf.ld.in Show resolved Hide resolved
libs/libc/Makefile Show resolved Hide resolved
yf13 added 3 commits January 11, 2025 14:58
This generates gnu-elf.ld via preprocessing of gnu-elf.ld.in so
that to reduce board specific app linker scripts in kernel mode
when BINFMT_ELF_EXECUTABLE is enabled.

Signed-off-by: Yanfeng Liu <p-liuyanfeng9@xiaomi.com>
This avoids using `-r` option when linking executable programs.

Signed-off-by: Yanfeng Liu <p-liuyanfeng9@xiaomi.com>
This allows using BINFMT_ELF_EXECUTABLE for qemu-armv7a target.

Signed-off-by: Yanfeng Liu <p-liuyanfeng9@xiaomi.com>
@xiaoxiang781216 xiaoxiang781216 merged commit ff48813 into apache:master Jan 11, 2025
39 checks passed
@acassis
Copy link
Contributor

acassis commented Jan 11, 2025

@acassis, thanks.

Here I tried sim:elf. by default ELF type is relocatable and it works.

I then tried to blindly enable BINFMT_ELF_EXECUTABLE after selecting ARCH_HAVE_ELF_EXECUTABLE in arch/sim/Kconfig first, but the built ELFs in ../apps/examples/elf/tests/romfs are still relocatable.

So if there is a way to make executable ELFs for sim, please teach.

Hi YF, I never tried it on SIM, only did it on ARM (STM32) some years ago.

@lupyuen
Copy link
Member

lupyuen commented Jan 12, 2025

Sorry @yf13: This PR is causing "Instruction page fault" for rv-virt:knsh64. Wonder if there's something I missed in my testing steps? Thanks!
https://gist.github.com/lupyuen/60d54514ce9a8589b56ed6207c356d95#file-special-qemu-riscv-knsh64-log-L1396

+ git reset --hard 657247bda89d60112d79bb9b8d223eca5f9641b5
HEAD is now at 657247bda8 libc/modlib: preprocess gnu-elf.ld
NuttX Source: https://github.com/apache/nuttx/tree/657247bda89d60112d79bb9b8d223eca5f9641b5
NuttX Apps: https://github.com/apache/nuttx-apps/tree/a6b9e718460a56722205c2a84a9b07b94ca664aa
+ tools/configure.sh rv-virt:knsh64
+ make -j
+ make export
+ pushd ../apps
+ ./tools/mkimport.sh -z -x ../nuttx/nuttx-export-12.8.0.tar.gz
+ make import
+ popd
+ qemu-system-riscv64 -semihosting -M virt,aclint=on -cpu rv64 -kernel nuttx -nographic
QEMU emulator version 9.2.0
OpenSBI v1.5.1
ABC
riscv_exception: EXCEPTION: Instruction page fault. MCAUSE: 000000000000000c, EPC: 000000018000001a, MTVAL: 000000018000001a
riscv_exception: Segmentation fault in PID 2: /system/bin/init

(Earlier Commit is OK)

@yf13
Copy link
Contributor Author

yf13 commented Jan 12, 2025 via email

@lupyuen
Copy link
Member

lupyuen commented Jan 12, 2025

Thank you so much YF! I think we need a NuttX Mirror to make things easier for you :-)

"Forgejo Git Forge for Apache NuttX RTOS (Experimental)"

@lupyuen
Copy link
Member

lupyuen commented Jan 12, 2025

Fixed by YF:

Thank you so much YF :-)
https://gist.github.com/lupyuen/7888125f58fe3034bc41b91b42fdabd7

## NuttX Source: https://github.com/apache/nuttx/tree/2149d893363119e5368a495d9658acd1a5ea7668
## NuttX Apps: https://github.com/apache/nuttx-apps/tree/a6b9e718460a56722205c2a84a9b07b94ca664aa

$ tools/configure.sh rv-virt:knsh64
$ qemu-system-riscv64 -semihosting -M virt,aclint=on -cpu rv64 -kernel nuttx -nographic

NuttShell (NSH) NuttX-12.8.0
nsh> uname -a
NuttX 12.8.0  risc-v rv-virt
## Hmmm why is Commit Hash missing?

nsh> ostest
ostest_main: Exiting with status 0

@lupyuen
Copy link
Member

lupyuen commented Jan 13, 2025

Sorry again @yf13: Static Variables don't seem to work correctly for rv-virt:knsh64, wonder if there's a problem with the Linking of Static Vars?

Here's the code that prints a Static Var: hello_main.c

static char test_static[] = "Testing Static Var"; 
int main(int argc, FAR char *argv[]) {
  printf("test_static=%s\n", test_static);
  printf("Address of test_static=%p\n", test_static);

rv-virt:knsh64 shows that the Static Var is incorrectly empty: knsh64 Log

nsh> hello
test_static=
Address of test_static=0xc0100200

rv-virt:nsh64 shows that the Static Var is correctly non-empty: nsh64 Log

nsh> hello
test_static=Testing Static Var
Address of test_static=0x8002d6d8

This might explain why uname -a for rv-virt:knsh64 doesn't print the Commit Hash. Thanks!

nsh> uname -a
NuttX 12.8.0  risc-v rv-virt

knsh64 Log

@yf13
Copy link
Contributor Author

yf13 commented Jan 13, 2025

@lupyuen, thanks, but I got different test result here:

$ git diff
--- a/examples/hello/hello_main.c
+++ b/examples/hello/hello_main.c
@@ -28,6 +28,7 @@
 /****************************************************************************
  * Public Functions
  ****************************************************************************/
+static const char *NAME = "NuttX";

 /****************************************************************************
  * hello_main
@@ -35,6 +36,6 @@

 int main(int argc, FAR char *argv[])
 {
-  printf("Hello, World!!\n");
+  printf("Hello, World of %s!!\n", NAME);
   return 0;
 }

Check with rv-virt:knsh:

$ qemu-system-riscv32 -M virt,aclint=on -semihosting -nographic -kernel nuttx
nsh> hello
Hello, World of NuttX!!

The commit string in uname might be a different story.

@lupyuen
Copy link
Member

lupyuen commented Jan 13, 2025

@yf13 What about knsh64?
https://gist.github.com/lupyuen/75c326a01c5e9110cac56b6e47f1a85f

## NuttX Source: https://github.com/apache/nuttx/tree/5f4a15b690d31776081f1bf1f28103863a88e38b
## NuttX Apps: https://github.com/apache/nuttx-apps/tree/6908ba52c7c5719dfecceab99c2606113e0cf6b1
+ tools/configure.sh rv-virt:knsh64
+ qemu-system-riscv64 -semihosting -M virt,aclint=on -cpu rv64 -kernel nuttx -nographic
nsh> hello
test_static=
Address of test_static=0xc0100200

@lupyuen
Copy link
Member

lupyuen commented Jan 13, 2025

@yf13 Can you also try static char instead of static const char? They show different results for knsh64:

static char test_static[] = "Testing Static Var";
static const char test_static_const[] = "Testing Static Const Var";
 int main(int argc, FAR char *argv[]) {
  printf("test_static=%s\n", test_static);
  printf("Address of test_static=%p\n", test_static);
  printf("test_static_const=%s\n", test_static_const);
  printf("Address of test_static_const=%p\n", test_static_const);

test_static=
Address of test_static=0xc0100200
test_static_const=Testing Static Const Var
Address of test_static_const=0xc0001ee0

@yf13
Copy link
Contributor Author

yf13 commented Jan 13, 2025

I removed the const modifier and tried on knsh64, also works:

$ git diff examples/hello
diff --git a/examples/hello/hello_main.c b/examples/hello/hello_main.c
index 2cd6cb400..270f3f453 100644
--- a/examples/hello/hello_main.c
+++ b/examples/hello/hello_main.c
@@ -28,6 +28,7 @@
 /****************************************************************************
  * Public Functions
  ****************************************************************************/
+static char *NAME = "NuttX";

 /****************************************************************************
  * hello_main
@@ -35,6 +36,6 @@

 int main(int argc, FAR char *argv[])
 {
-  printf("Hello, World!!\n");
+  printf("Hello, World of %s!!\n", NAME);
   return 0;
 }
$ cd ../nuttx
$ qemu-system-riscv64 -M virt,aclint=on -semihosting -nographic -kernel nuttx
nsh> hello
Hello, World of NuttX!!

@lupyuen
Copy link
Member

lupyuen commented Jan 13, 2025

Hmmm which commit for NuttX and Apps are you using? Lemme try your branch. Thanks :-)

@lupyuen
Copy link
Member

lupyuen commented Jan 13, 2025

Erm @yf13 could you try static char NAME[] instead of static char *NAME? It makes a difference...

static char test_static[] = "Testing Static Var"; //// TODO: Added this
static const char test_static_const[] = "Testing Static Const Var"; //// TODO: Added this
static char *NAME = "NuttX";

test_static=
Address of test_static=0xc0100200
test_static_const=Testing Static Const Var
Address of test_static_const=0xc0001f10
Hello, World of NuttX!!
Address of NAME=0xc0001e10

The reason why I chose static char [] is because uname uses that:

#if defined(__DATE__) && defined(__TIME__) && \
!defined(CONFIG_LIBC_UNAME_DISABLE_TIMESTAMP)
static char g_version[] = CONFIG_VERSION_BUILD " " __DATE__ " " __TIME__;
#else
static char g_version[] = CONFIG_VERSION_BUILD;
#endif

@yf13
Copy link
Contributor Author

yf13 commented Jan 13, 2025

I finally reproduced it here with static char NAME[] = xxx.

The issue looks like somehow initialized data section is not properly populated. I don't see how it relates to this PR as current rv-virt kernel build is still using board specific linker script.

I will spend time later on this and update if I have findings. Do you know if this PR is the first broken commit?

@lupyuen
Copy link
Member

lupyuen commented Jan 13, 2025

Do you know if this PR is the first broken commit?

The problem doesn't happen for the commit just before this PR. Thanks for looking into this :-)

https://gist.github.com/lupyuen/21167b5ce4f2c2e8b23e2057dcefbe64

$ git reset --hard be40c01ddd6f43a527abeae31042ba7978aabb58
HEAD is now at be40c01ddd6 nuttx/addrenv.h: revise assembly guard
NuttX Source: https://github.com/apache/nuttx/tree/be40c01ddd6f43a527abeae31042ba7978aabb58
NuttX Apps: https://github.com/apache/nuttx-apps/tree/04f0b8e19274ff65ad8744d4b5b37f522c0097c0

$ qemu-system-riscv64 -semihosting -M virt,aclint=on -cpu rv64 -kernel nuttx -nographic
NuttShell (NSH) NuttX-12.7.0
nsh> uname -a
NuttX 12.7.0 be40c01ddd6 Jan 13 2025 14:22:45 risc-v rv-virt

nsh> hello
test_static=Testing Static Var
Address of test_static=0xc0101000
test_static_const=Testing Static Const Var
Address of test_static_const=0xc0002058
Hello, World of NuttX!!
Address of NAME=0xc0002020

Update: Milk-V Duo S / SG2000 still works OK today for uname -a. So I'm thinking the Linker Script for SG2000 is different from knsh64, that's why uname works OK?
https://github.com/lupyuen/nuttx-sg2000/releases/tag/nuttx-sg2000-2025-01-13

nsh> uname -a
NuttX 12.8.0 5f4a15b690 Jan 13 2025 00:17:37 risc-v milkv_duos

@yf13
Copy link
Contributor Author

yf13 commented Jan 13, 2025

It seems that rv-virt:knsh has this issue since 5076b0c74cc which is pull/14064. Please help to review as my git bisect skill is still poor.

Criteria I used as a good commit:

  • Hello app can show static char NAME[] = "NuttX" on console;
  • file ../apps/bin/hello shows executable program.

@lupyuen
Copy link
Member

lupyuen commented Jan 13, 2025

It seems that rv-virt:knsh has this issue since 5076b0c74cc which is pull/14064.

Sorry @yf13 I can't reproduce the bug with rv-virt:knsh and rv-virt:knsh64, based on 5076b0c and #14064. Did I miss something?

I compiled NuttX for 5076b0c:

+ git clone https://github.com/apache/nuttx --branch master nuttx
+ git clone https://github.com/lupyuen2/wip-nuttx-apps apps --branch uname2
+ cd nuttx
+ git reset --hard 5076b0c74cc
HEAD is now at 5076b0c74c risc-v:Unify module compilation options
NuttX Source: https://github.com/apache/nuttx/tree/5076b0c74ccc280e4af6042219f6a5d07e78b7ac
NuttX Apps: https://github.com/apache/nuttx-apps/tree/bf6d2f2be67b138f9bf1f4f69a9663cd10eb7663
+ tools/configure.sh rv-virt:knsh
Or tools/configure.sh rv-virt:knsh64
+ make -j ; make export
+ pushd ../apps ; ./tools/mkimport.sh -z -x ../nuttx/nuttx-export-12.8.0.tar.gz ; make import ; popd

rv-virt:knsh tested OK:
https://gist.github.com/lupyuen/a1544d3298035e0dd983ef636ebb728a

+ tools/configure.sh rv-virt:knsh
+ qemu-system-riscv32 -semihosting -M virt,aclint=on -cpu rv32 -kernel nuttx -bios opensbi-1.5-rv-bin/share/opensbi/ilp32/generic/firmware/fw_dynamic.bin -nographic
QEMU emulator version 9.2.0
OpenSBI v1.5
nsh> uname -a
NuttX 12.8.0 5076b0c74c Jan 14 2025 07:07:44 risc-v rv-virt
nsh> hello
NAME=NuttX
Address of NAME=0xc0101000

rv-virt:knsh64 also tested OK:
https://gist.github.com/lupyuen/54727956551cd6bea113891ddfd09288

+ tools/configure.sh rv-virt:knsh64
+ qemu-system-riscv64 -semihosting -M virt,aclint=on -cpu rv64 -kernel nuttx -nographic
QEMU emulator version 9.2.0
OpenSBI v1.5.1
nsh> uname -a
NuttX 12.8.0 5076b0c74c Jan 14 2025 07:08:20 risc-v rv-virt
nsh> hello
NAME=NuttX
Address of NAME=0xc0101000

@yf13
Copy link
Contributor Author

yf13 commented Jan 14, 2025

How about the file ../apps/bin/hello check?
Previously the type was ET_EXEC, since this it changes to ET_REL. The ET_EXEC type works fine before this commit. Can you confirm this?

The ET_REL support seems working since then, but we need find out when ET_EXEC type starts having the issue.

@lupyuen
Copy link
Member

lupyuen commented Jan 14, 2025

Sorry @yf13 I don't quite understand, what are the exact steps I need to run to reproduce the Static Vars bug?

I'm not sure if the file type of ../apps/bin/hello matters, so long as it works correctly with Static Vars?

Do we agree that 5076b0c and #14064 are not faulty, since they tested OK with rv-virt:knsh and rv-virt:knsh64? Thanks :-)

@lupyuen
Copy link
Member

lupyuen commented Jan 14, 2025

Hi @yf13: About ET_EXEC vs ET_REL, could you post this as a NuttX Issue for the NuttX Community to decide? Perhaps also link the issue to the Mailing List?

Meanwhile I think it's important to fix the Static Vars, since it will break a lot of code. Would you consider rolling back your PR? Thanks :-)

@yf13
Copy link
Contributor Author

yf13 commented Jan 14, 2025

I am hesitate to hide the issue, please allow more investigation, we'd better find the root cause.

@lupyuen
Copy link
Member

lupyuen commented Jan 14, 2025

But shall we revert the PR for now? I'm hoping we can clear the bug before the Lunar New Year holidays, so everyone can be a little less stressed thanks :-)

@lupyuen
Copy link
Member

lupyuen commented Jan 14, 2025

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Arch: all Issues that apply to all architectures Arch: arm Issues related to ARM (32-bit) architecture Area: OS Components OS Components issues 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.

7 participants