Page MenuHomePhabricator

[lld] Add CMake options to disable individual linkers
AcceptedPublic

Authored by arichardson on Aug 16 2021, 4:34 AM.

Details

Reviewers
MaskRay
peter.smith
sbc100
mstorsjo
rnk
gkm
dim
Group Reviewers
Restricted Project
Summary

This is useful in downstream toolchain builds that only support a single
(or maybe two) output formats. For CHERI LLVM, most users compile
the toolchain from source, so disabling the non-ELF linkers reduces
build time (about 100 C++ source files fewer to compile) and reduces
the lld binary size by about 4MB (106MB -> 102MB) for a statically
linked build.

Another example where this can be useful is the FreeBSD lld build,
which has a local patch to only include ELF support in lld.cpp.
This change will allow removing the downstream patch and
instead use the relevant LLD_ENABLE_*_LINKER defines.

This patch should not result in any long-term maintainance issues,
as it only adds a few if() lines to the CMake build and skips tests
for some subdirectories.

Diff Detail

Event Timeline

arichardson created this revision.Aug 16 2021, 4:34 AM
Herald added a project: Restricted Project. · View Herald Transcript
arichardson requested review of this revision.Aug 16 2021, 4:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 16 2021, 4:34 AM

Is the motivation disk size? How much of a difference does this make?

arichardson added a comment.EditedAug 17 2021, 6:06 AM

Is the motivation disk size? How much of a difference does this make?

For a release+asserts build it's about 3.8MB difference. Not huge but noticeable:

$ ls -l bin/lld.*.s*
-rwxrwxr-x 1 alr48 alr48 106799760 Aug 17 13:57 bin/lld.all.stripped
-rwxrwxr-x 1 alr48 alr48 102797984 Aug 17 13:53 bin/lld.elf.stripped
$ size bin/lld.*.s*
   text	   data	    bss	    dec	    hex	filename
99852875	6934040	 500314	107287229	66512bd	bin/lld.all.stripped
95985096	6805040	 474962	103265098	627b34a	bin/lld.elf.stripped

My main motivation was actually build time and looking for files that can easily be omitted.
Building all LLD flavours results in 156 ninja commands (after rm -rf tools/lld) whereas only building the ELF linker results in 54 build commands.
FreeBSD builds LLVM using a separate Makefile based system, so they only have a small downstream #if 0 patch in lld.cpp. Therefore it's not a huge local diff that would be removed by this patch.

My real motivation is only building the ELF linker for our CHERI toolchain (and I think Arm might do the same for their Morello toolchain once our changes have been merged).
Right now we don't ship binaries, so we expect all our users to compile LLVM from source (possibly on laptops with only a few CPU cores), so not building approximately 100 source files could be a noticeable speedup.

@thakis If you want I can also have a look at updating the GN build files (assuming this patch is considered acceptable).

Fix unused function warning

Looks fine to me.

lld/include/lld/Common/config.h.cmake
1

The header is not needed. Many config.h.cmake files don't use the header.

dim accepted this revision.Aug 17 2021, 12:22 PM

Indeed in the FreeBSD base system we only build the ELF part and patch lldMain to stub out the rest, see https://github.com/freebsd/freebsd-src/commit/59948e95d8deadafca3acd659b6dfc78b708f117

But this is nicer to have, and can save you some time when using the CMake build.

This revision is now accepted and ready to land.Aug 17 2021, 12:22 PM
arichardson added inline comments.Aug 17 2021, 2:31 PM
lld/include/lld/Common/config.h.cmake
1

Will drop it, I believe I just copied it from the LLVM config header.

If this doesn't help you much upstream, and since it's not a big size change, is it really worth to add the complexity here upstream?

(Don't worry about the gn build though, that's easy to update if this lands.)

If this doesn't help you much upstream, and since it's not a big size change, is it really worth to add the complexity here upstream?

(Don't worry about the gn build though, that's easy to update if this lands.)

We're interested in this as well, and it'd be nice to have it upstream :) (I haven't run the size numbers on our end yet, to be fair.)

If this doesn't help you much upstream, and since it's not a big size change, is it really worth to add the complexity here upstream?

(Don't worry about the gn build though, that's easy to update if this lands.)

We're interested in this as well, and it'd be nice to have it upstream :) (I haven't run the size numbers on our end yet, to be fair.)

+1 to avoiding extra complexity unless the size wins really are significant (I'm not sure how we go about assessing that).

If this doesn't help you much upstream, and since it's not a big size change, is it really worth to add the complexity here upstream?

(Don't worry about the gn build though, that's easy to update if this lands.)

While it doesn't help very much for FreeBSD (one of the downstream forks I contribute to) since FreeBSD doesn't use the CMake build system, it does help for the CHERI downstream (the downstream fork I work on for my job).
I think this doesn't add very much complexity: just a few ifdefs in a file that rarely changes and a few if() statements in CMake.

To me a 3.8MB size reduction seems worth this complexity but if you disagree I can commit this downstream instead.

dim added a comment.Aug 18 2021, 3:15 PM

It's mostly about shaving off quite a bit of build time, if you can skip all the formats that you never use. The size reduction is also nice, but is not really the essential part.

(I would compare this more to e.g. the LLVM_TARGETS_TO_BUILD list, which allows you to select only the architectures you're interested in, saving you lots of build time in many cases.)

arichardson edited the summary of this revision. (Show Details)Sep 24 2021, 5:41 AM

Drop unnecessary copyright from config.h header.

@sbc100 are you still strongly opposed to this patch?

arichardson marked an inline comment as done.Sep 24 2021, 5:43 AM

IMO this is worth it for the build time reduction, the size reduction is a nice bonus but not the main benefit.

Drop unnecessary copyright from config.h header.

@sbc100 are you still strongly opposed to this patch?

Nope, not a strong objection. Just not sure the incremental wins are worth it. If folks downstream see real value in shaving a few cycles of the build time this seems like a reasonable approach.

MaskRay added inline comments.Sep 24 2021, 8:58 AM
lld/tools/lld/lld.cpp
154

then and else should be swapped.

arichardson added inline comments.Sep 24 2021, 9:00 AM
lld/tools/lld/lld.cpp
154

Like this?

#if LLD_ENABLE_COFF_LINKER
      return !mingw::link(args, exitEarly, stdoutOS, stderrOS);
#else
      missingLinkerSupport("COFF");
#endif

I originally used ! to catch cases where the macro is not defined (since we don't build with -Wundef), but I could use != 0 to get the same error with the branches inverted.

MaskRay added inline comments.Sep 24 2021, 9:38 AM
lld/tools/lld/lld.cpp
154

!= 0 looks good

We're supportive of this, as stated before.