Page MenuHomePhabricator

[lld-link] implement -start-lib and -end-lib
ClosedPublic

Authored by inglorion on Aug 27 2019, 5:13 PM.

Details

Summary

This implements -start-lib and -end-lib flags for lld-link, analogous
to the similarly named options in ld.lld. Object files after
-start-lib are included in the link only when needed to resolve
undefined symbols. The -end-lib flag goes back to the normal behavior
of always including object files in the link. This mimics the
semantics of static libraries, but without needing to actually create
the archive file.

Diff Detail

Repository
rL LLVM

Event Timeline

inglorion created this revision.Aug 27 2019, 5:13 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 27 2019, 5:13 PM
inglorion planned changes to this revision.Aug 27 2019, 5:16 PM

Seeing the code here, I see I still need to handle LazyObject in symbol resolution.

inglorion updated this revision to Diff 217547.Aug 27 2019, 6:01 PM

fix places where I needed to add handling of LazyObject symbols

Primarily, this change adds a LazyObject Symbol kind, and a LazyObjFile InputFile kind. A LazyObject represents a symbol defined in a LazyObjFile, and a LazyObjFile is either a native object file or a bitcode file that we know of, but have not yet decided to link in. As with the LazyArchive symbols we already had, when we encounter both a lazy symbol and an Undefined symbol, we resolve this by linking in the file that defines the symbol, so the result is a Defined symbol.

The change also modifies the way we track wholearchive. This was previously passed as a boolean. Because it doesn't make sense to have both wholearchive (which says to link in all objects in an archive regardless of whether their symbols are referenced) and -start-lib (which says to only link in objects only when their symbols are referenced), this change replaces this with a mode enum which can be wholearchive, lazy, or neither.

Finally, -start-lib and -end-lib are added to the options the linker accepts, and we perform some sanity checks (no -end-lib without -start-lib, for example).

ruiu added inline comments.Aug 28 2019, 1:26 AM
lld/COFF/DebugTypes.cpp
234 ↗(On Diff #217547)

AddMode is not an enum class, so it is in the namespace of LinkerDriver, so it can just be LinkerDriver::NORMAL

lld/COFF/Driver.cpp
1578–1581 ↗(On Diff #217547)

Too much nesting I guess? I'd write this way

if (InLib)
  enqueuePath(*path, LAZY);
else if (isWholeArchive(*Path))
  enqueuePath(*path, WHOLE_ARCHIVE);
else
  enqueuePath(*path, NORMAL);
1594 ↗(On Diff #217547)

I think you can omit AddMode::.

MaskRay added inline comments.Aug 28 2019, 1:48 AM
lld/COFF/InputFiles.cpp
831 ↗(On Diff #217547)

symbols(std::move(symbols)) to avoid a copy.

lld/COFF/SymbolTable.cpp
620 ↗(On Diff #217547)

auto * can be used here because the type is obvious from the right hand side. (in lld's code base this idiom is used a lot)

MaskRay added inline comments.Aug 28 2019, 1:51 AM
lld/COFF/InputFiles.h
133 ↗(On Diff #217547)

symbols(std::move(symbols))

inglorion updated this revision to Diff 217712.Aug 28 2019, 1:32 PM
inglorion marked 9 inline comments as done.

comments by @ruiu and @MaskRay (thanks!)

  • made AddMode an enum class
  • simplified AddMode computation in OPT_INPUT case
  • added missing std::moves
  • use auto instead of explicitly naming LazyArchive
inglorion added inline comments.Aug 28 2019, 1:32 PM
lld/COFF/DebugTypes.cpp
234 ↗(On Diff #217547)

I had actually meant for this to be an enum class. Added the missing "class". Thanks!

lld/COFF/Driver.cpp
1578–1581 ↗(On Diff #217547)

Thanks! That is much clearer.

lld/COFF/InputFiles.h
133 ↗(On Diff #217547)

Thanks!

MaskRay added inline comments.Aug 28 2019, 7:02 PM
lld/test/COFF/start-lib-cmd-diagnostics.ll
14 ↗(On Diff #217712)

What made you decide -start-lib and -wholearchive: cannot be used together?

I noticed that -wholearchive: can apply on both archives and regular object files. However, there is no test before this patch that test -wholearchive: can apply on object files.

In ELF, the -wholearchive: counterpart is a pair of --whole-archive and --no-whole-archive. --start-lib is allowed to be used together because

  • --start-lib applies on object files
  • --whole-archive applies on archives

Their use cases don't overlap so their combination is not rejected.

ruiu added inline comments.Aug 28 2019, 7:57 PM
lld/test/COFF/start-lib-cmd-diagnostics.ll
14 ↗(On Diff #217712)

I agree with MaskRay. Giving static libraries object file semantics and object files library file semantics at the same time seems odd but they don't technically conflict. I can't think of a sane use case of the combination of the features, but I don't think we should ban the combination.

  • allow mixing -start-lib and -wholearchive:
inglorion marked 2 inline comments as done.Aug 29 2019, 11:16 AM
ruiu accepted this revision.Aug 29 2019, 9:49 PM

LGTM

lld/COFF/InputFiles.cpp
142 ↗(On Diff #217942)

nit: if you flip the condition you can write this way:

for (const lto::InputFile::Symbol &sym : obj->symbols()) {
  if (!sym.isUndefined())
    symtab->addLazyObject(this, sym.getName());
lld/COFF/Options.td
180 ↗(On Diff #217942)

Can you add the same help message as the ELF linker?

This revision is now accepted and ready to land.Aug 29 2019, 9:49 PM
MaskRay accepted this revision.Aug 29 2019, 10:04 PM

--start-lib semantics look very similar to ELF now :)

This revision was automatically updated to reflect the committed changes.
inglorion marked an inline comment as done.
inglorion marked an inline comment as done.Aug 30 2019, 9:51 AM

Implemented ruiu's requests before committing. Thanks for reviewing!