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

linker: section_tags: fix missing include #76100

Merged
merged 1 commit into from
Aug 1, 2024

Conversation

Finomnis
Copy link
Contributor

@Finomnis Finomnis commented Jul 19, 2024

Fixes #76254.

Using section tags like __dtcm_noinit_section should resolve to:

__attribute__((section(".dtcm_noinit"), used))

However, due to a missing include, they resolve to:

__attribute__((section("_DTCM_NOINIT_SECTION_NAME"), used))

... failing to move the variable to the proper section and causing the following warning:

ld.bfd.exe: warning: orphan section `_DTCM_NOINIT_SECTION_NAME' from `app/libapp.a(main.c.obj)' being placed in section `_DTCM_NOINIT_SECTION_NAME'

Copy link
Member

@ycsin ycsin left a comment

Choose a reason for hiding this comment

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

Is the build error reproducible with an upstream board & application?

include/zephyr/linker/section_tags.h Outdated Show resolved Hide resolved
@ycsin ycsin requested review from stephanosio and dcpleung July 22, 2024 03:38
@Finomnis
Copy link
Contributor Author

Finomnis commented Jul 22, 2024

Is the build error reproducible with an upstream board & application?

Sadly no. I thought this one would:

west build -b mimxrt1170_evk/mimxrt1176/cm7 zephyr/samples/boards/mimxrt1170_evk_cm7/magic_addr/

... but it seems that the header is in fact included there, through:

  • #include <zephyr/kernel.h>
  • #include <zephyr/kernel_includes.h>
  • #include <zephyr/linker/sections.h>

If the official 'solution' is to include <zephyr/kernel.h> in user code, then we should at least add a compile time check for it somewhere. But IMO it would be better to force the include in linker_sections.h.

@zephyrbot zephyrbot added the size: XS A PR changing only a single line of code label Jul 22, 2024
Copy link
Member

@ycsin ycsin left a comment

Choose a reason for hiding this comment

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

<zephyr/linker/sections.h> includes section_tags.h at the bottom, but nothing suggests that section_tags.h is an internal/hidden header, in that sense this PR is a valid fix, since it is not including things that it required.

@Finomnis
Copy link
Contributor Author

Finomnis commented Jul 22, 2024

<zephyr/linker/sections.h> includes section_tags.h at the bottom, but nothing suggests that section_tags.h is an internal/hidden header, in that sense this PR is a valid fix, since it is not including things that it required.

If that is the case, then this change causes a circular header dependency :/

Would it be better to indicate that section_tags.h is an internal header and instead tell users to use sections.h instead?

Or remove section_tags.h from sections.h as well? Although that would be a breaking API change.

EDIT:
The more I think about it the more I am of the opinion that sections.h should not include section_tags.h. That's a dependency inversion; a thing should include its dependencies. But the current case is that the dependency includes its dependee.

If using `<zephyr/linker/section_tags.h>` without including
`zephyr/linker/sections.h` as well, we get a warning an the linker fails
to place the data in the desired section.

Signed-off-by: Martin Stumpf <finomnis@gmail.com>
@tomi-font tomi-font added the bug The issue is a bug, or the PR is fixing a bug label Jul 22, 2024
@Finomnis
Copy link
Contributor Author

@nashif @tomi-font Now that 3.7 is out, could this be added to 3.7.1?

@tomi-font tomi-font added backport v3.7-branch Request backport to the v3.7-branch and removed backport v3.7-branch Request backport to the v3.7-branch labels Jul 29, 2024
@tomi-font
Copy link
Collaborator

Added the relevant backport tag. This PR should be picked up for merging at some point, I think.

@carlescufi carlescufi merged commit 77eafac into zephyrproject-rtos:main Aug 1, 2024
30 checks passed
@Finomnis Finomnis deleted the Finomnis-patch-2 branch September 4, 2024 16:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Linker Scripts backport v3.7-branch Request backport to the v3.7-branch bug The issue is a bug, or the PR is fixing a bug size: XS A PR changing only a single line of code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

linker: section_tags: missing include
6 participants