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

make: fix linking #5757

Closed
wants to merge 12 commits into from
Closed

make: fix linking #5757

wants to merge 12 commits into from

Conversation

kaspar030
Copy link
Contributor

@kaspar030 kaspar030 commented Aug 17, 2016

Ever wondered why exactly files needed to be put into "UNDEF" in order to link correctly?

When considering archives, ld only considers objects in archives if there is at least one symbol in them needed elsewhere. Also, it doesn't seem to consider a symbol if it already found a weak symbol before.

In the current make system, this leads to:

  • weak symbols not overridden by strong symbols unless the object is passed to the linker outside of an archive (in "UNDEF")
  • weak symbols don't get overridden unless at least one symbol of that object is used somewhere (this is why overridden ISRs are only actually used if some other function of that file is used, e.g., isr_timer0 will only be used instead of the weak-default dummy handler if e.g., timer_init() is called)

Both feel hacky to me.

This PR encloses processing of module-archives in "--whole-archive", causing all objects to be always considered for linking. That way, the UNDEF-objects are not needed anymore.

Unfortunately this always overrides all defined isr defined in periph_*, as they are currently all always built. So to not get somewhat bloated binaries, this depends on #5065 #7241.

Also, the config module breaks, so #5756 is also required. (#5756 merged)

@kaspar030 kaspar030 added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet Area: build system Area: Build system State: waiting for other PR State: The PR requires another PR to be merged first labels Aug 17, 2016
@Kijewski
Copy link
Contributor

Ever wondered why exactly files needed to be put into "UNDEF" in order to link correctly?

Dunno if that's still true, but once upon a time (probably in 2014) GCC considered all the interrupt handlers as unused, and removed them. Turns out boards are rather boring if you don't have any external interrupts. Since then there was a lot of work to make -fLTO and --gc-sections work, so probably this problem was fixed. too.

@jnohlgard
Copy link
Member

@Kijewski The issue with missing ISRs or interrupt vectors is solved by marking the functions with __attribute__((used)). Like you wrote, it should be fixed with the various LTO improvements lately.

@haukepetersen
Copy link
Contributor

would you mind rebasing, as #5756 is merged by now? Thanx

I really like this PR! If the only thing is now to wait for #5065, I again call for help fixing the last Makefile.buildtest issue in that PR so we can merge this one also.

@kaspar030 kaspar030 force-pushed the fix_linking branch 2 times, most recently from e33bca3 to 1893e5a Compare October 7, 2016 10:48
@kaspar030
Copy link
Contributor Author

  • rebased

@miri64
Copy link
Member

miri64 commented Oct 18, 2016

@kaspar030 what makes this still WIP?

@kaspar030
Copy link
Contributor Author

@miri64 I need to re-check that this is still working as expected once #5065 is in.

@miri64
Copy link
Member

miri64 commented Oct 31, 2016

Feature freeze -> postponed

@miri64 miri64 modified the milestones: Release 2017.01, Release 2016.10 Oct 31, 2016
@PeterKietzmann PeterKietzmann modified the milestones: Release 2017.01, Release 2017.04 Jan 26, 2017
@kaspar030 kaspar030 removed this from the Release 2017.04 milestone Apr 21, 2017
@kaspar030 kaspar030 added the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Nov 14, 2017
@smlng smlng removed this from the Release 2018.01 milestone Jan 12, 2018
@jnohlgard
Copy link
Member

jnohlgard commented Feb 28, 2018

@cladmi I created a proof of concept for switching to using ld -r instead of ar for the modules. Will post a PR later this week.

@cladmi
Copy link
Contributor

cladmi commented Mar 1, 2018

@gebart Cool :) Using ld -r for modules could also help if trying to dynamically load elf objects on a node and I could be interested in that.

The work done by @kaspar030 will help for the modules that would break if not handled as libraries. Do you remember if other modules required to be moved to EXTRA_LIBS ?

@jnohlgard
Copy link
Member

@cladmi see #8711 for a proof of concept

@kaspar030
Copy link
Contributor Author

Closing in favour of #8711.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: build system Area: Build system CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.