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: Use ld -r for intermediate linking of each module #8711

Closed
wants to merge 18 commits into from

Conversation

jnohlgard
Copy link
Member

@jnohlgard jnohlgard commented Mar 1, 2018

Contribution description

As discussed in #5757, using ld -r --unique and creating intermediate object files can be preferable to creating archives (using ar rcs, current practice). This eliminates hidden issues such as a weak symbol may override a strong symbol even though both are included in the final link.
This means that each module will first be linked into a single object file module.o instead of a static archive module.a.

New build system environment variables are introduced:

  • LD: target low level linker, default $(PREFIX)ld, must be ld, gcc does not work here.
  • ILINK: command used for intermediate linking (incremental linking), default $(LD).
  • ILINKFLAGS: flags for incremental linking, default -r. Certain platforms may need to specify the CPU type or other flags here, e.g. AVR, MIPS

This PR eliminates the variable UNDEF entirely because it is no longer needed.

Issues/PRs references

Initial suggestion in #5757

@jnohlgard jnohlgard added 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 Impact: major The PR changes a significant part of the code base. It should be reviewed carefully CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Mar 1, 2018
@jnohlgard
Copy link
Member Author

jnohlgard commented Mar 1, 2018

TODO:

  • verify that binaries are still correct.
  • Check if any pkgs need updating
  • Check if the special case for MacOS native builds is still necessary in Makefile.base

@cladmi
Copy link
Contributor

cladmi commented Mar 1, 2018

In #5757 PR, @kaspar030 needed to handle certain libraries without the '--whole-archive' and so with the lazy linking algorithm.

Even if some/(all?) of the packages could be fixed to produce an object file, there may be reasons for an external libraries or a module to need to be built as an archive, or also we could be lazy to migrate external build scripts all at once.
So starting with a mechanism that allows some code to be compiled and used as library could help migrate in a more smoother way. It could be adapted from the EXTRAL_LIBS of #5757 PR.

Also, if moving away from 'libraries' archives, would it make sense to rename BASELIBS?

I found some references to the '.a' output in bindist:

examples/bindist/Makefile:# and that the resulting .a should be saved when doing "make bindist"
examples/bindist/README.md:compiled and linked binary, bindist.a, abc.a and Makefiles.
makefiles/bindist.inc.mk:DIST_FILES += $(BIN_USEMODULE:%=bin/$(BOARD)/%.a)

And the bindist example does not build for the moment because it does not find main.

# An empty object file is included in the link to avoid situations where modules
# do not provide any code (e.g. sys, some boards), causing the build to fail
# with a missing object file.
EMPTYOBJ = $(BINDIR)/$(MODULE)/.empty.o
Copy link
Contributor

@Kijewski Kijewski Mar 1, 2018

Choose a reason for hiding this comment

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

I am not sure that using a generic empty.o will work. If I'm not mistaken (I did not test it), the linker will reject an object file if it's not the right architecture.

I guess something like that would be more robust:

ifeq $(,$(OBJ))
  OBJ := $(BINDIR)/$(MODULE)/empty.o

  $(BINDIR)/$(MODULE)/empty.o: $(RIOTBASE)/empty.c
        $(CC) $(CFLAGS) -w -c -o $@ $<
ifeq
static void __attribute__((unused)) _placeholder(void) {}

Copy link
Member Author

Choose a reason for hiding this comment

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

It works, try it. It is the correct architecture because of using the real CFLAGS when generating it.
Not sure the ifeq suggestion is robust because of lazy evaluation of recursive variables, does OBJ always have its final value when interpreting this makefile?

@kaspar030
Copy link
Contributor

I'm very looking forward to this one!

@jnohlgard
Copy link
Member Author

Feel free to join in if you feel like working on this. Currently some pkgs have problems linking because of multiple inclusions of the same object file in the final link

@cladmi
Copy link
Contributor

cladmi commented Mar 14, 2018

I really like the concept of packages registering themselves as building an .a but I do not really like the name USEARCHIVE. For me it does not match what we have currently in USEMODULE USEPKG used in dependencies. Its more something like MODULES_PRODUCING_ARCHIVES_INSTEAD_OF_OBJECTS but should have a better name than this.

I will create sub pull requests to fix what is currently broken in master.

@jnohlgard
Copy link
Member Author

rebased to fix conflicts

@jnohlgard
Copy link
Member Author

@cladmi I take any suggestions for a name better than USEARCHIVE. I think it's a bit too cryptic right now.

@kaspar030
Copy link
Contributor

I take any suggestions for a name better than USEARCHIVE. I think it's a bit too cryptic right now.

Traditionally statically linked libraries are linked as .a. Thus I'd suggest LINKLIBS or just LIBS.

@cladmi
Copy link
Contributor

cladmi commented Apr 13, 2018

I would like to have a precise name, even maybe a strange name so that it cannot be understood directly as something else, and I would prefer that people look it up in the documentation as it should be an exception when not possible to do otherwise.

LIBSMODULES, ARCHIVEMODULES, ARCHIVELIBS ?

As it's meant to be used in modules.inc.mk when setting BASELIBS, with the following I would understand it

_OBJECTMODULES = $(filter-out $(ARCHIVEMODULES), $(REALMODULES))
BASELIBS += $(_OBJECTMODULES:%=$(BINDIR)/%.o)
BASELIBS += $(ARCHIVEMODULES:%=$(BINDIR)/%.a)

@kaspar030
Copy link
Contributor

As it's meant to be used in modules.inc.mk when setting BASELIBS

The name BASELIBS is not a set-in-stone one either, more something from the past that can be re-thought when oit makes sense.

Joakim Nohlgård and others added 16 commits March 5, 2019 15:13
makefiles/modules.inc.mk must be cleaned.
There is no need for special 'PKGS' handling.
Just find all real modules and define both

Add variables names for '$(sort $(USEMODULE) $(USEPKG))' and build from
this. Or just add "USEPKG" to USEMODULE at that moment. Should be
removed later anyway.

REALLIBS and something to describe modules that are not 'LIBS'

This needs proper names and refactoring. I will do it outside.
This will allow special handling as a library.
This will allow special handling as a library.
This will allow special handling as a library.
This will allow special handling as a library.
This will allow special handling as a library.
… pkg/oonf_api: Update build to use ld -r for submodules

Also using '$(BINDIR)/oonf_*.a as a dependency' is wrong…

This commit should be this at the end with a commit with prerequisites
to fix issues.

pkg/oonf_api: define 'oonf_api' as LIBS

This will allow special handling as a library.
It is only needed to overwrite it if `LIBS` is exported in the build
system. But resetting it anyway does not hurt.

Could be removed if `LIBS` is not exported.
Add an exception to not depend on periph_uart when using MIPS UHI
syscalls for newlib syscalls.
Do not make 'oonf_api' group all oonf_* objects.

I find it strange to merge modules there.
@kaspar030
Copy link
Contributor

Seems like "ld -r" is incompatible with LTO: https://lwn.net/Articles/744507/

@jcarrano
Copy link
Contributor

Seems like "ld -r" is incompatible with LTO:

If that's the case, we may have to go with --whole-archive, which is the route I prefer, as it is less invasive.

@stale
Copy link

stale bot commented Mar 1, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

@stale stale bot added the State: stale State: The issue / PR has no activity for >185 days label Mar 1, 2020
@stale stale bot closed this Apr 1, 2020
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 Area: toolchain Area: toolchains; everything related to compilation, libc, linking, … CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Community: help wanted The contributors require help from other members of the community Impact: major The PR changes a significant part of the code base. It should be reviewed carefully State: stale State: The issue / PR has no activity for >185 days
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants