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

[pcc]

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.

[pcc]

• Add a LazyObject test

[ruiu]

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]

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]

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]

In D50475#1193055, @ruiu wrote:

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]

LGTM

lld/ELF/Driver.cpp

1221 ↗

(On Diff #159789)

Can you return right here if it is not isLazy?

[ruiu]

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]

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.

lld/trunk/ELF/Driver.cpp

template <class ELFT> static void handleUndefined(StringRef Name) {
   // Since symbol S may not be used inside the program, LTO may
   // eliminate it. Mark the symbol as "used" to prevent it.
   Sym->IsUsedInRegularObj = true;
 
   if (Sym->isLazy())
     Symtab->fetchLazy<ELFT>(Sym);
 }
 
+template <class ELFT> static void handleLibcall(StringRef Name) {
+  Symbol *Sym = Symtab->find(Name);
+  if (!Sym || !Sym->isLazy())
+    return;
+
+  MemoryBufferRef MB;
+  if (auto *LO = dyn_cast<LazyObject>(Sym))
+    MB = LO->File->MB;
+  else
+    MB = cast<LazyArchive>(Sym)->getMemberBuffer();
+
+  if (isBitcode(MB))
+    Symtab->fetchLazy<ELFT>(Sym);
+}
+
 template <class ELFT> static bool shouldDemote(Symbol &Sym) {
   // If all references to a DSO happen to be weak, the DSO is not added to
   // DT_NEEDED. If that happens, we need to eliminate shared symbols created
   // from the DSO. Otherwise, they become dangling references that point to a
   // non-existent DSO.
   if (auto *S = dyn_cast<SharedSymbol>(&Sym))
     return !S->getFile<ELFT>().IsNeeded;
 
template <class ELFT> void LinkerDriver::link(opt::InputArgList &Args) {
   handleUndefined<ELFT>(Config->Entry);
 
   // If any of our inputs are bitcode files, the LTO code generator may create
   // references to certain library functions that might not be explicit in the
   // bitcode file's symbol table. If any of those library functions are defined
   // in a bitcode file in an archive member, we need to arrange to use LTO to
   // compile those archive members by adding them to the link beforehand.
   //
-  // With this the symbol table should be complete. After this, no new names
-  // except a few linker-synthesized ones will be added to the symbol table.
+  // However, 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.
+  //
+  // Therefore, 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.
   if (!BitcodeFiles.empty())
     for (const char *S : LibcallRoutineNames)
-      handleUndefined<ELFT>(S);
+      handleLibcall<ELFT>(S);
 
   // Return if there were name resolution errors.
   if (errorCount())
     return;
 
   // Now when we read all script files, we want to finalize order of linker
   // script commands, which can be not yet final because of INSERT commands.
   Script->processInsertCommands();
if (!Config->Relocatable)
     Symtab->scanVersionScript();
 
   // Create wrapped symbols for -wrap option.
   for (auto *Arg : Args.filtered(OPT_wrap))
     Symtab->addSymbolWrap<ELFT>(Arg->getValue());
 
   // Do link-time optimization if given files are LLVM bitcode files.
   // This compiles bitcode files into real object files.
+  //
+  // With this the symbol table should be complete. After this, no new names
+  // except a few linker-synthesized ones will be added to the symbol table.
   Symtab->addCombinedLTOObject<ELFT>();
   if (errorCount())
     return;
 
   // If -thinlto-index-only is given, we should create only "index
   // files" and not object files. Index file creation is already done
   // in addCombinedLTOObject, so we are done if that's the case.
   if (Config->ThinLTOIndexOnly)

lld/trunk/ELF/Symbols.h

LazyArchive(InputFile &File, uint8_t Type,
               const llvm::object::Archive::Symbol S)
       : Symbol(LazyArchiveKind, &File, S.getName(), llvm::ELF::STB_GLOBAL,
                llvm::ELF::STV_DEFAULT, Type),
         Sym(S) {}
 
   static bool classof(const Symbol *S) { return S->kind() == LazyArchiveKind; }
 
   InputFile *fetch();
+  MemoryBufferRef getMemberBuffer();
 
 private:
   const llvm::object::Archive::Symbol Sym;
 };
 
 // LazyObject symbols represents symbols in object files between
 // --start-lib and --end-lib options.
 class LazyObject : public Symbol {

lld/trunk/ELF/Symbols.cpp

void Symbol::parseSymbolVersion() {
   // symbol table.
   if (Config->Shared && VersionId != VER_NDX_LOCAL)
     error(toString(File) + ": symbol " + S + " has undefined version " +
           Verstr);
 }
 
 InputFile *LazyArchive::fetch() { return cast<ArchiveFile>(File)->fetch(Sym); }
 
+MemoryBufferRef LazyArchive::getMemberBuffer() {
+  Archive::Child C = CHECK(
+      Sym.getMember(), "could not get the member for symbol " + Sym.getName());
+
+  return CHECK(C.getMemoryBufferRef(),
+               "could not get the buffer for the member defining symbol " +
+                   Sym.getName());
+}
+
 uint8_t Symbol::computeBinding() const {
   if (Config->Relocatable)
     return Binding;
   if (Visibility != STV_DEFAULT && Visibility != STV_PROTECTED)
     return STB_LOCAL;
   if (VersionId == VER_NDX_LOCAL && isDefined())
     return STB_LOCAL;
   if (!Config->GnuUnique && Binding == STB_GNU_UNIQUE)

lld/trunk/test/ELF/lto/Inputs/libcall-archive.s

+.globl __sync_val_compare_and_swap_8
+__sync_val_compare_and_swap_8:

lld/trunk/test/ELF/lto/libcall-archive.ll

 ; REQUIRES: x86
 ; RUN: rm -f %t.a
 ; RUN: llvm-as -o %t.o %s
 ; RUN: llvm-as -o %t2.o %S/Inputs/libcall-archive.ll
-; RUN: llvm-ar rcs %t.a %t2.o
+; RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux -o %t3.o %S/Inputs/libcall-archive.s
+; RUN: llvm-ar rcs %t.a %t2.o %t3.o
 ; RUN: ld.lld -o %t %t.o %t.a
 ; RUN: llvm-nm %t | FileCheck %s
+; RUN: ld.lld -o %t2 %t.o --start-lib %t2.o %t3.o --end-lib
+; RUN: llvm-nm %t2 | FileCheck %s
 
+; CHECK-NOT: T __sync_val_compare_and_swap_8
 ; CHECK: T _start
 ; CHECK: T memcpy
 
 target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-unknown-linux-gnu"
 
 define void @_start(i8* %a, i8* %b) {
 entry:
   call void @llvm.memcpy.p0i8.p0i8.i64(i8* %a, i8* %b, i64 1024, i1 false)
   ret void
 }
 
 declare void @llvm.memcpy.p0i8.p0i8.i64(i8* nocapture, i8* nocapture, i64, i1)