Page MenuHomePhabricator

[LLD] Allow usage of LLD as a library

Authored by aganea on Feb 4 2022, 6:08 PM.


Group Reviewers
Restricted Project

As discussed in this patch allows using LLD-as-a-lib. It also lets clients link-in only the drivers that they want (see tests).

This also adds the unittests infra as in the other LLVM projects. Among the test coverage, I've added the original issue from @krzysz00, see this.

Important note: this doesn't allow (yet) linking in parallel. This will come a bit later, in subsequent patches, for COFF at last.

Diff Detail

Unit TestsFailed

10 msx64 debian > LLD-Unit.AsLibELF/_/LLDAsLibELFTests/AsLib::ROCm
Script: -- /var/lib/buildkite-agent/builds/llvm-project/build/tools/lld/unittests/AsLibELF/./LLDAsLibELFTests --gtest_filter=AsLib.ROCm
60,050 msx64 debian > libFuzzer.libFuzzer::fuzzer-leak.test
Script: -- : 'RUN: at line 3'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -O2 -gline-tables-only -fsanitize=address,fuzzer -I/var/lib/buildkite-agent/builds/llvm-project/compiler-rt/lib/fuzzer -m64 /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/fuzzer/LeakTest.cpp -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/fuzzer/X86_64DefaultLinuxConfig/Output/fuzzer-leak.test.tmp-LeakTest
60,030 msx64 debian > libFuzzer.libFuzzer::minimize_crash.test
Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -O2 -gline-tables-only -fsanitize=address,fuzzer -I/var/lib/buildkite-agent/builds/llvm-project/compiler-rt/lib/fuzzer -m64 /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/fuzzer/NullDerefTest.cpp -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/fuzzer/X86_64DefaultLinuxConfig/Output/minimize_crash.test.tmp-NullDerefTest
60,040 msx64 debian > libFuzzer.libFuzzer::value-profile-load.test
Script: -- : 'RUN: at line 2'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -O2 -gline-tables-only -fsanitize=address,fuzzer -I/var/lib/buildkite-agent/builds/llvm-project/compiler-rt/lib/fuzzer -m64 /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/fuzzer/LoadTest.cpp -fsanitize-coverage=trace-gep -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/fuzzer/X86_64DefaultLinuxConfig/Output/value-profile-load.test.tmp-LoadTest

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Herald added a project: Restricted Project. · View Herald TranscriptJan 4 2023, 9:42 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
aganea added a comment.Jan 4 2023, 9:44 AM

@aganea If you'd be willing to pick this CL again within one/two month-ish timeline, that's great. I can help with testing since I'm a developer from ROCm side. Hopefully we can wrap this whole thing up by the end of this quarter. On the other hand, if this CL may not be your top priority to work on, and just want to let you know that I'd be willing to take over and finish the rest of work of it. But I have close to zero knowledge of the internals of LLD so may need your pointers and code reviews to make sure I'm on the right track.

Sorry for the delay, please see the updated patch.

aganea updated this revision to Diff 486326.Jan 4 2023, 9:51 AM
aganea marked an inline comment as done.

Fix casing in CMakeLists.txt


Fixed. Using the default OBJ-in-LIB override mechanism now, as described well by Raymond Chen here.

sbc100 added inline comments.Jan 4 2023, 9:59 AM

Can we come up with a better name for this file maybe? That name makes me think its specifically for the library use case but that isn't true.

Also, we don't capitalize lld elsewhere, with in the filenames or directory names.

How about lldMain.cpp, or perhaps we can come up with a name that doesn't start with lld at all?


Why not llvm::ArrayRef<const char *>?

aganea updated this revision to Diff 486638.Jan 5 2023, 11:22 AM
aganea marked 2 inline comments as done.

As suggested by @sbc100


DriversMain.cpp ?


Please see updated diff. We had std::vector just because of how the -flavor flag is treated and then stripped from args. I changed to llvm::ArrayRef as you suggested and added -flavor in all drivers as a shallow flag. If we want to go that route, I could send a separate patch for that change.

Thanks for your work @aganea. I abstain from giving review from lld's implementation side but thing looks good on the ROCm.cpp unit test. (BTW, do you aim to create that unit test as a demo and then to remove it later? Or else i'd expect to see the "Inputs" folder mocked up in certain way.) Although I did notice a fair amount of test breakages on windows. If you are still actively working on it, I'm willing to approve it once you fix the test failures. Also I plan to give this a test by pulling this diff locally assuming the base of this diff wouldn't cause too much conflicts with our tip of clone.

aganea updated this revision to Diff 491544.Jan 23 2023, 4:24 PM

Fix (hopefully) CI build.

@jerryyin The build errors were due to the fact that I only used LLD as stage-0 linker in my previous tests, assuming that its behavior is conformant with other system linkers. It is not, as described in this paragraph. The same holds for both ELF and COFF drivers.

It seems that there's no universal solution for overriding a symbol in a LIB by a symbol in another LIB. MSVC has /alternatename which works, but there's no such thing on ELF side -- weak symbols cannot be overriden by another LIB once they are visible in the module, or at least I don't know how. There's also the --include=symbol for MSVC and --u=symbol for Unix that can solve the issue by pushing the symbol early in the symbol table, but then we have to deal with the mangling and that doesn't sounds fun. In the end, if users want to use a partial set of the LLD drivers, I resorted to simply implementing the missing drivers with the LLD_IMPLEMENT_SHALLOW_DRIVER macro (see ROCm test). If there's a better/simpler solution for a user perspective, let me know and I'll try it.

aganea updated this revision to Diff 491742.Jan 24 2023, 6:09 AM

Add binaries into diff to fix the preflight build.

hmm the buildkite preflight is still broken, it seems Phabricator is stripping out the binary files from the diff.
My local diff is fine:

but after downloading from this page, they are empty:

It seems that there's no universal solution for overriding a symbol in a LIB by a symbol in another LIB...simply implementing the missing drivers with the LLD_IMPLEMENT_SHALLOW_DRIVER macro

Thanks for letting me know, sounds like a fair solution.

163–172 ↗(On Diff #491742)

I have a naïve question to ask:

  • In my limited knowledge, previously lldELF is dependent on lldCommon target. lld.cpp would compile just fine because its target is an executable.
  • Now, since DriversMain.cpp become part of lldCommon, and in the implementation of DriversMain,cpp, we start to invoke those specific link target, it implicitly requires, for example, lld::elf::link to be available before lldCommon is compiled. In other words, lldCommon is dependent on lldELF.

Did we just have a cyclic dependency here? And if it is, do you need to move the lldMain() and safeLldMain() into their own independent target such that we can avoid the existence of such a cycle in the compilation?

aganea added inline comments.Jan 24 2023, 1:43 PM
163–172 ↗(On Diff #491742)

The trickery is that we push lldCommon twice on the command-line, once in lldELF (same in all the other drivers), in another time in the final module. I retained the same mechanic for the unittests.

We could have another lib for LLD-as-lib usage, but I thought from the users' perspective it's simpler if we provided a single "common" lib + the specific drivers' libs.

arsenm added a subscriber: arsenm.Jan 24 2023, 2:30 PM
arsenm added inline comments.

Feels like this isn't something lld should have to roll its own handling for

162–172 ↗(On Diff #491742)

Should move to a separate function with a switch/return

aganea updated this revision to Diff 491980.Jan 24 2023, 6:36 PM
aganea marked 3 inline comments as done.

As suggested by @arsenm

jerryyin accepted this revision.Jan 25 2023, 7:15 AM

LGTM with my limited knowledge of lld. Approving it as this is an extremely useful feature to have, and I want it to make progress after almost a year of staging.

163–172 ↗(On Diff #491742)

Just want to let you know that I made an (very risky attempt) trying to test this patch in our fork, but this errors out unable to build lld. Based on Harbormaster build result, I don't believe this is due to issues from your patch, but rather our lld fork has been a couple of months old. (Refer to, I pulled the last 3 commits in between this period hoping they are enough narrow the gap.) However, ninja lld would yield a number of linker error as below:

ld.lld: error: undefined symbol: lld::elf::link(llvm::ArrayRef<char const*>, llvm::raw_ostream&, llvm::raw_ostream&, bool, bool)
referenced by DriversMain.cpp:0 (/root/rocMLIR/external/llvm-project/lld/Common/DriversMain.cpp:0)

external/llvm-project/llvm/tools/lld/Common/CMakeFiles/lldCommon.dir/DriversMain.cpp.o:(lld::lldMain(llvm::ArrayRef<char const*>, llvm::raw_ostream&, llvm::raw_ostream&, bool))

It would take a while for our fork to completely catch up with the tip of upstream, and I'll give this another test after then. In the mean time I will not hold this review up because of it.

This revision is now accepted and ready to land.Jan 25 2023, 7:15 AM

Thanks for reviewing @jerryyin !
Before going further, I'd like an review/approval from the regular lld contributors as well, whenever you got the chance @mstorsjo @MaskRay @hans @thieta

MaskRay added 1 blocking reviewer(s): Restricted Project.Jan 30 2023, 10:24 AM
This revision now requires review to proceed.Jan 30 2023, 10:24 AM

There is a layering issue. lld/Common/DriversMain.cpp references lld::elf::link which is defined in the library lldELF. lldELF depends on lldCommon so lldCommon cannot depend on lldELF.
You shall see a linker error with -DBUILD_SHARED_LIBS=on due to -Wl,-z,defs

MaskRay requested changes to this revision.Jan 30 2023, 10:58 PM
This revision now requires changes to proceed.Jan 30 2023, 10:58 PM
aganea updated this revision to Diff 494949.Feb 5 2023, 2:37 PM

As suggested by @MaskRay. This changes a few things, but it's for the best I think:

  • Fix circular dependency between lldCommon and the lld drivers.
  • Change safeLldMain -> lldMain and make it the only entry point.
  • Pass down drivers symbols from the caller to lldCommon. This also makes it easier to discriminate between what drivers are linked in the PE, and what it isn't available.
  • Remove all drivers' declarations from Drivers.h, the only usage allowed is to just use lldMain().
  • Tested on Linux with -DBUILD_SHARED_LIBS=ON.
MaskRay added inline comments.Feb 9 2023, 3:31 PM

Keep the file list sorted

1 ↗(On Diff #494949)

DriverMain instead of DriversMain? I wonder whether Dispatcher in the filename can make it clearer. We have many functions/files with main or Main in the names now...

Also, the header needs a fix.


This can call unsafeLldMain instead, then the parameter bool libUsage can be removed. The downside is that unsafeLldMain will be in an include header but that's probably fine (unsure whether moving it into a nested namespace to discourage its use is useful).

Thanks for the patch! I have gone through the code and it looks in a good shape. Left some suggestions.

60 ↗(On Diff #494949)

Hopefully share with other template instantiations to decrease the executable size.

152 ↗(On Diff #494949)
178 ↗(On Diff #494949)
739 ↗(On Diff #494949)

We should avoid ignoring -flavor in all drivers. It's probably not difficult to strip -flavor from the driver dispatcher.




Add a file-level comment explaining what AsLibAll means.


Add a file-level comment explaining what AsLibELF means.


Add a comment about the purpose.


remove braces





MaskRay added inline comments.Feb 9 2023, 3:43 PM
739 ↗(On Diff #494949)

Note: we can assert that -flavor is a very first option, i.e. lld -xxx -yyy -flavor gnu isn't allowed.
This may simplify striping the -flavor option.

MaskRay added inline comments.Feb 9 2023, 3:45 PM

Latest clang gives warnings in a -DLLD_DEFAULT_LD_LLD_IS_MINGW=on build.

/home/maskray/llvm/lld/tools/lld/lld.cpp:64:21: warning: extra ';' outside of a function is incompatible with C++98 [-Wc++98-compat-extra-semi]

The comment around lld/docs/index.rst:35 needs an update. It still suggests lld::elf::link.

aganea updated this revision to Diff 498530.Feb 17 2023, 5:19 PM
aganea marked 19 inline comments as done.

As suggested by @MaskRay

aganea added a comment.EditedFeb 17 2023, 5:19 PM

Thanks for the in-depth review @MaskRay, I appreciate!

60 ↗(On Diff #494949)

I checked with CruncherSharp, there are already ,256 instances in the PE image.


This pattern works only if the symbol is "public", as in, declared in a header that has been seen already by the current TU. Which is not the case here, I've removed declarations for direct drivers calls, only lld::lldMain remains in Drivers.h. Same comment for the other places where this pattern isn't applicable.


It isn't applicable here, since I remove the declarations from Driver.h


Hmm I don't see this with latest patch, would you have a chance to check again if it's still there?

MaskRay added inline comments.Feb 19 2023, 11:17 PM

This code introduces a new function. We can strip -flavor near whichDriver or its caller to move all -flavor handling to a single place and avoid polluting the common code.


delete blank line


d(...) returns a bool, not an int. exitLld(r) passes the bool to CrashRecoveryContext::throwIfCrash (isCrash expects an exit code) which seems incorrect.


warning: extra ';' outside of a function is incompatible with C++98 [-Wc++98-compat-extra-semi]


warning: extra ';' outside of a function is incompatible with C++98 [-Wc++98-compat-extra-semi]


warning: extra ';' outside of a function is incompatible with C++98 [-Wc++98-compat-extra-semi]

aganea updated this revision to Diff 498968.Feb 20 2023, 3:07 PM
aganea marked 6 inline comments as done.

As sugested by @MaskRay


Actually the bool to int conversion is correct. The drivers and thus d returns false on error, true on success. We invert the result and that makes r equal 1 on error, and 0 on success, which is the value that will be returned by the application. throwIfCrash will be really useful when getting back from the CrashRecoveryHandler below in lld::lldMain, but on this particular codepath in unsafeLldMain that will never be hit.
I've added an extra comment to clarify. Please let me know if you think more should be done.

MaskRay accepted this revision.Apr 1 2023, 7:36 AM

Looks great!


The warning goes away! Thanks.

This revision is now accepted and ready to land.Apr 1 2023, 7:36 AM