This is an archive of the discontinued LLVM Phabricator instance.

[lld] Fix --exclude-libs broken when --whole-archive is used
ClosedPublic

Authored by iid_iunknown on Oct 26 2017, 4:24 PM.

Details

Summary

Problem
--exclude-libs does not work for static libraries affected by the --whole-archive option.

Description
--exclude-libs creates a list of static library paths and does library lookups in this list.
--whole-archive splits the static libraries that follow it into separate objects. As a result, lld no longer sees static libraries among linked files and does no --exclude-libs lookups.

Solution
The proposed solution is to make --exclude-libs consider object files too. When lld finds an object file it checks whether this file originates from an archive and, if so, looks the archive up in the --exclude-libs list.

Diff Detail

Repository
rL LLVM

Event Timeline

iid_iunknown created this revision.Oct 26 2017, 4:24 PM
ruiu added inline comments.Oct 26 2017, 4:39 PM
ELF/Driver.cpp
986–987

I'd define a function

static StringRef getArchiveName(InputFile *File) {
  if (isa<ArchiveFile>(File))
    return File_>getName();
  return File->ArchiveName;
}

so that I can write

for (InputFile *File :Files)
  if ((All && isa<ArchiveFile>(File)) || Libs.count(path::filename(getArchiveName(File)))
    for (SymbolBody *Sym : F->getSymbols()
      if (!Sym->isLocal())
        Sym->symbol()->VersionId = VER_NDX_LOCAL;

Introducing getArchiveName

iid_iunknown added inline comments.Oct 26 2017, 5:20 PM
ELF/Driver.cpp
986–987

Thanks Rui!

This one is now fixed.
Please note that the suggested condition

if ((All && isa<ArchiveFile>(File)) || Libs.count(path::filename(getArchiveName(File)))

is not quite correct as it does not handle the --exclude-libs ALL --whole-archive case, so it was modified to produce the expected result.

iid_iunknown marked an inline comment as done.Oct 26 2017, 5:21 PM
ruiu added inline comments.Oct 26 2017, 5:56 PM
ELF/Driver.cpp
986–987

Do you actually have to check if ArchiveName is not empty? Since Libs can never contains an empty string, I don't think it is necessary.

iid_iunknown added inline comments.Oct 27 2017, 4:21 AM
ELF/Driver.cpp
986–987

Object files that do not originate from archives have empty ArchiveName.
Removing that check would involve all the objects and probably even shared libraries into symbols localization if --exclude-libs ALL is specified.

The check ensures that symbols localization is applied only to files associated with archives: either the file is an archive itself or an object extracted from an archive by e.g. --whole-archive.

ruiu accepted this revision.Oct 30 2017, 4:12 PM

LGTM with this change.

ELF/Driver.cpp
986–987

Ah, okay, then I'd make this change

static Optional<StringRef> getArchiveName(InputFile *File) {
  if (isa<ArchiveFile>(File))
    return File->getName();
  if (!File->ArchiveName.emty())
    return File->ArchiveName;
  return None;
}

so that I can change this function to

for (InputFile *File : Files) {
  if (Optional<StringRef> Archive = getArchiveName(File))
    if (All || Libs.count(path::filename(*Archive)))
      ...
This revision is now accepted and ready to land.Oct 30 2017, 4:12 PM
iid_iunknown added inline comments.Oct 31 2017, 6:42 AM
ELF/Driver.cpp
986–987

Good point.
I will update the patch.
Thanks!

Use Optional in getArchiveName()

iid_iunknown marked an inline comment as done.Oct 31 2017, 6:45 AM
iid_iunknown closed this revision.Oct 31 2017, 6:51 AM