Page MenuHomePhabricator

ELF: Only add libcall symbols to the link if defined in bitcode.
ClosedPublic

Authored by pcc on Aug 8 2018, 2:00 PM.

Details

Summary

Adding all libcall symbols to the link can have undesired consequences.
For example, the libgcc implementation of __sync_val_compare_and_swap_8
on 32-bit ARM pulls in an .init_array entry that aborts the program if
the Linux kernel does not support 64-bit atomics, which would prevent
the program from running even if it does not use 64-bit atomics.

This change makes it so that we only add libcall symbols to the
link before LTO if we have to, i.e. if the symbol's definition is in
bitcode. Any other required libcall symbols will be added to the link
after LTO when we add the LTO object file to the link.

Diff Detail

Repository
rL LLVM

Event Timeline

pcc created this revision.Aug 8 2018, 2:00 PM
pcc updated this revision to Diff 159789.Aug 8 2018, 2:02 PM
  • Add a LazyObject test
ruiu added inline comments.Aug 8 2018, 2:37 PM
lld/ELF/Driver.cpp
1221 ↗(On Diff #159789)

Is it too expensive or is there any problem if we simply call Sym->fetch() and see if the returned value is of BitcodeFile (and if so, add to the symbol table)?

pcc added inline comments.Aug 8 2018, 2:42 PM
lld/ELF/Driver.cpp
1221 ↗(On Diff #159789)

I don't think that would work because subsequent calls to fetch would return nullptr. That means that if the object file actually turns out to be required, we wouldn't end up adding it.

ruiu added a comment.Aug 8 2018, 2:45 PM

You don't need to call fetch again to add a file that you already have to the symbol table. You can directly add it by calling SymbolTable::addFile().

pcc added a comment.Aug 8 2018, 2:54 PM

You don't need to call fetch again to add a file that you already have to the symbol table. You can directly add it by calling SymbolTable::addFile().

In more detail the scenario is:

  1. in handleLibcall we call fetch on the LazyArchive, which gives us an ObjFile. As a side effect we add an entry to Seen for that object file
  2. we do nothing with it
  3. while adding the LTO object file, we call SymbolTable::addLazyArchive on one of the libcall symbols that the LTO object file needs (via SymbolTable::addCombinedLTOObject -> ObjFile::parse)
  4. the call to F.fetch(Sym) in that function returns nullptr because we added the entry to Seen in step 1, so we never add the object file to the link
ruiu accepted this revision.Aug 8 2018, 3:00 PM

LGTM

lld/ELF/Driver.cpp
1221 ↗(On Diff #159789)

Can you return right here if it is not isLazy?

This revision is now accepted and ready to land.Aug 8 2018, 3:00 PM
ruiu added inline comments.Aug 8 2018, 3:02 PM
lld/ELF/Driver.cpp
1221 ↗(On Diff #159789)

I mean, I think something like this is a bit easier to grasp.

if (!Sym->isLazy())
  return

if (auto *LO = dyn_cast<LazyObject>(Sym))
  MB = LO->File->MB;
else
  MB = cast<LazyArchive>(Sym)->getMemberBuffer();
pcc added inline comments.Aug 8 2018, 4:41 PM
lld/ELF/Driver.cpp
1221 ↗(On Diff #159789)

Yes. Thinking about it more I think we can simplify a little further: we don't need to set IsUsedInRegularObj at all because the libcall symbols are kept alive because of D49434.

This revision was automatically updated to reflect the committed changes.