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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

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.

endif

LDELFFLAGS += -e main -T $(call CONVERT_PATH,$(TOPDIR)$(DELIM)libs$(DELIM)libc$(DELIM)modlib$(DELIM)gnu-elf.ld)
ifneq ($(CONFIG_BINFMT_ELF_EXECUTABLE),y)
LDELFFLAGS += -e main -T $(call CONVERT_PATH,$(TOPDIR)$(DELIM)libs$(DELIM)libc$(DELIM)modlib$(DELIM)gnu-elf.ld)
Copy link
Contributor

Choose a reason for hiding this comment

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

can we share the same gnu-elf.ld for relacation and exeutable?

Copy link
Contributor Author

@yf13 yf13 Jan 7, 2025

Choose a reason for hiding this comment

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

Currently the gnu-elf.ld in boards/ folder contains target specific .text address, which is specific to target virtual space design now. If later NuttX kernel build has a unified virtual space design (e.g. kernel at highmem, userspace in lowmem), I believe one userspace gnu-elf.ld will be enough. So maybe we should track unified userspace memory layout separately?

Copy link
Contributor

Choose a reason for hiding this comment

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

but why not unify the layout now?

Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't the linker script board specific data anyway, why must we change (or for that matter, have) anything in the common modlib folder, I don't understand ?

I presume the linker file in the modlib folder is there as an example, not intended for production use in any case.

I question the motivation to have this change at all in upstream.

Copy link
Contributor

Choose a reason for hiding this comment

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

elf file linker script is different from board linker script, since the layout should be general for all board like how Linux done.

Copy link
Contributor

Choose a reason for hiding this comment

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

You are right, the Linux toolchain has a generic linker script, so maybe this change is not as bad as I initially thought. I personally have the gnu-elf.ld file in the board folder for my projects because I still think it is board specific (for now).

Copy link
Contributor

Choose a reason for hiding this comment

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

but why not unify the layout now?

Us having a unified virtual address spacing (e.g. user in lowmem, kernel in himem) for now is impossible, as we do not have a good mechanism to move memory mapped devices into the kernel virtual memory area (like fdt and vmap). We have the pieces in place but not a single platform implements this AFAIK ?

This is why I for example configure MPFS the other way around; userspace starts from 0xc000_0000 and kernel resides in the lower part.

I guess this patch does not break this functionality, so I'm fine with it.

@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 <[email protected]>
@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
Yanfeng Liu added 2 commits January 9, 2025 10:09
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 <[email protected]>
This avoids using `-r` option when linking executable programs.

Signed-off-by: Yanfeng Liu <[email protected]>
@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
This allows using BINFMT_ELF_EXECUTABLE for qemu-armv7a target.

Signed-off-by: Yanfeng Liu <[email protected]>
@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.

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.

6 participants