Skip to content

linker: use wildcards in rel-sections.ld #10536

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 1 commit into from
Oct 12, 2018

Conversation

dcpleung
Copy link
Member

@dcpleung dcpleung commented Oct 11, 2018

Update rel-sections.ld to use wildcards instead of
spelling out those sections one by one.

Fixes #10493

Signed-off-by: Daniel Leung [email protected]

@codecov-io
Copy link

codecov-io commented Oct 11, 2018

Codecov Report

Merging #10536 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #10536   +/-   ##
=======================================
  Coverage   53.37%   53.37%           
=======================================
  Files         210      210           
  Lines       25825    25825           
  Branches     5686     5686           
=======================================
  Hits        13783    13783           
  Misses       9727     9727           
  Partials     2315     2315

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1a1df58...0678f5c. Read the comment docs.

Copy link
Member

@aescolar aescolar left a comment

Choose a reason for hiding this comment

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

shouldnt this be extended to .rel.shell_*.?
Otherwise shell_cmd_ and shell_module_ will be missing.

@dcpleung dcpleung force-pushed the posix_linker_10493_again branch from cadbd99 to 7605336 Compare October 12, 2018 08:45
@dcpleung
Copy link
Member Author

Thanks for letting me know. I was just looking at common-rom.ld.

@pfalcon
Copy link
Collaborator

pfalcon commented Oct 12, 2018

From #10528 (comment)

Good to know. I will update PR #10536 with wildcard ones.

@dcpleung : I wanted to add that to #10493, but apparently forgot: did you consider not doing any .ld file hacking for native_posix at all? Like, make it link with default host toolchain settings. (I don't know if that's actually easier, or we have a custom .ld even for native_posix, which is a portability hazard in general).

@dcpleung dcpleung changed the title linker: add .rel.shell_root_cmd_* to rel-sections.ld linker: use wildcards in rel-sections.ld Oct 12, 2018
@dcpleung
Copy link
Member Author

Thanks to @SebastianBoe for letting me know that ordering works. So updated the patch to use wildcards to match them all. :)

@dcpleung dcpleung force-pushed the posix_linker_10493_again branch from cea2650 to d9ad218 Compare October 12, 2018 09:03
@dcpleung
Copy link
Member Author

dcpleung commented Oct 12, 2018

@pfalcon We do have a custom linker script for posix at include/arch/posix/linker.ld, but it acts as an addition to the one provided by the host toolchain. Thanks for the note. I have removed it from the posix linker script in the latest revision.

@aescolar
Copy link
Member

aescolar commented Oct 12, 2018

did you consider not doing any .ld file hacking for native_posix at all?

I have some concerns in the same direction. Given that the posix arch is meant to be used with whatever the user wants/has locally, we have very little control over it. So I'm quite concerned about the amount of issues this may raise over time and the involved maintenance effort for this feature in this arch.
My thinking was more in the line of: "Should this be kconfigurable, so for those who have problems with it, they could easily disable it"

@aescolar
Copy link
Member

I have removed it from the posix

Just removing it will cause all the warnings to come back. You would want to ifdef the linker flag in the cmake file too

@dcpleung dcpleung force-pushed the posix_linker_10493_again branch from d9ad218 to 9ba0145 Compare October 12, 2018 09:29
@dcpleung
Copy link
Member Author

@aescolar I have disabled orphan section warning for native_posix. If this needs to work, we actually have to override the .rel.plt (as we need the ordering to work correctly) and it will probably be problematic in the future.

@aescolar
Copy link
Member

aescolar commented Oct 12, 2018

@dcpleung Thanks. Question:
In 8ce758a you added quite a few more things to the posix linker script (include/arch/posix/linker.ld). Are those necessary?

@dcpleung dcpleung force-pushed the posix_linker_10493_again branch from 9ba0145 to f64c480 Compare October 12, 2018 15:23
@dcpleung
Copy link
Member Author

@aescolar Those are still needed (with GCC 6.4.0 I am using on my machine).

@aescolar
Copy link
Member

Those are still needed (with GCC 6.

This is a sidetrack, and not a comment to this PR in particular, but could you describe the problem you had which required them?

@dcpleung
Copy link
Member Author

@aescolar Come to think of it, they are no longer needed as we are not warning about orphan sections for posix anymore... (I need more caffeine... :) )

Update rel-sections.ld to use wildcards instead of
spelling out those sections one by one.

Also, for POSIX, don't include this and turns off
the warnings. With different host toolchain across
different OS, it would be maintanence nightmare
to account for all those combinations. So this reverts
the POSIX linker script to before the first orphan
section changes.

Fixes zephyrproject-rtos#10493

Signed-off-by: Daniel Leung <[email protected]>
@dcpleung dcpleung force-pushed the posix_linker_10493_again branch from f64c480 to 0678f5c Compare October 12, 2018 16:35
@dcpleung
Copy link
Member Author

Also reverted include/arch/posix/linker.ld to the version before all the orphan section changes.

@nashif nashif merged commit 9ae3ea4 into zephyrproject-rtos:master Oct 12, 2018
@dcpleung dcpleung deleted the posix_linker_10493_again branch October 12, 2018 22:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants