ELF: Undefine all symbols, not just those that we expect to be defined.

[pcc]

Summary

This allows the combined LTO object to provide a definition with the same
name as a symbol that was internalized without causing a duplicate symbol
error. This normally happens during parallel codegen which externalizes
originally-internal symbols, for example.

In order to make this work, I needed to relax the undefined symbol error to
only report an error for symbols that are used in regular objects.

[• rafael]

This will allow an undefined in a .bc file as long as lto optimizes it out, no?
I guess that is OK, but would be nice to add a comment about it.

test/ELF/lto/parallel-internalize.ll

4 ↗

(On Diff #56234)

Please also check the symbols in the final binary.

We should have only foo, right? Bar should still be internalized and when splitting it will be marked hidden.

lld/trunk/ELF/LTO.cpp

if (M.getDataLayoutStr().empty())
     fatal("invalid bitcode file: " + F.getName() + " has no datalayout");
 
   // If a symbol appears in @llvm.used, the linker is required
   // to treat the symbol as there is a reference to the symbol
   // that it cannot see. Therefore, we can't internalize.
   SmallPtrSet<GlobalValue *, 8> Used;
   collectUsedGlobalVariables(M, Used, /* CompilerUsed */ false);
 
-  // This function is called if we know that the combined LTO object will
-  // provide a definition of a symbol. It undefines the symbol so that the
-  // definition in the combined LTO object will replace it when parsed.
-  auto Undefine = [](Symbol *S) {
-    replaceBody<Undefined>(S, S->body()->getName(), STV_DEFAULT, 0);
-  };
-
   for (const BasicSymbolRef &Sym : Obj->symbols()) {
     uint32_t Flags = Sym.getFlags();
     GlobalValue *GV = Obj->getSymbolGV(Sym.getRawDataRefImpl());
     if (GV && GV->hasAppendingLinkage())
       Keep.push_back(GV);
     if (BitcodeFile::shouldSkip(Flags))
       continue;
     Symbol *S = Syms[BodyIndex++];
     if (Flags & BasicSymbolRef::SF_Undefined)
       continue;
-    if (!GV) {
-      // Module asm symbol.
-      Undefine(S);
-      continue;
-    }
     auto *B = dyn_cast<DefinedBitcode>(S->body());
     if (!B || B->File != &F)
       continue;
+
+    // We collect the set of symbols we want to internalize here
+    // and change the linkage after the IRMover executed, i.e. after
+    // we imported the symbols and satisfied undefined references
+    // to it. We can't just change linkage here because otherwise
+    // the IRMover will just rename the symbol.
+    if (GV && shouldInternalize(Used, S, GV))
+      InternalizedSyms.insert(GV->getName());
+
+    // At this point we know that either the combined LTO object will provide a
+    // definition of a symbol, or we will internalize it. In either case, we
+    // need to undefine the symbol. In the former case, the real definition
+    // needs to be able to replace the original definition without conflicting.
+    // In the latter case, we need to allow the combined LTO object to provide a
+    // definition with the same name, for example when doing parallel codegen.
+    replaceBody<Undefined>(S, S->body()->getName(), STV_DEFAULT, 0);
+
+    if (!GV)
+      // Module asm symbol.
+      continue;
+
     switch (GV->getLinkage()) {
     default:
       break;
     case llvm::GlobalValue::LinkOnceAnyLinkage:
       GV->setLinkage(GlobalValue::WeakAnyLinkage);
       break;
     case llvm::GlobalValue::LinkOnceODRLinkage:
       GV->setLinkage(GlobalValue::WeakODRLinkage);
       break;
     }
 
-    // We collect the set of symbols we want to internalize here
-    // and change the linkage after the IRMover executed, i.e. after
-    // we imported the symbols and satisfied undefined references
-    // to it. We can't just change linkage here because otherwise
-    // the IRMover will just rename the symbol.
-    if (shouldInternalize(Used, S, GV))
-      InternalizedSyms.insert(GV->getName());
-    else
-      Undefine(S);
-
     Keep.push_back(GV);
   }
 
   Mover.move(Obj->takeModule(), Keep,
              [](GlobalValue &, IRMover::ValueAdder) {});
 }
 
 static void internalize(GlobalValue &GV) {

lld/trunk/ELF/Writer.cpp

template <class ELFT> void Writer<ELFT>::createSections() {
   for (Symbol *S : Symtab.getSymbols()) {
     SymbolBody *Body = S->body();
 
     // Set "used" bit for --as-needed.
     if (S->IsUsedInRegularObj && !S->isWeak())
       if (auto *SS = dyn_cast<SharedSymbol<ELFT>>(Body))
         SS->File->IsUsed = true;
 
-    if (Body->isUndefined() && !S->isWeak())
+    // We only report undefined symbols in regular objects. This means that we
+    // will accept an undefined reference in bitcode if it can be optimized out.
+    if (S->IsUsedInRegularObj && Body->isUndefined() && !S->isWeak())
       reportUndefined<ELFT>(Symtab, Body);
 
     if (auto *C = dyn_cast<DefinedCommon>(Body))
       CommonSymbols.push_back(C);
 
     if (!includeInSymtab<ELFT>(*Body))
       continue;
     if (Out<ELFT>::SymTab)

lld/trunk/test/ELF/lto/parallel-internalize.ll

+; REQUIRES: x86
+; RUN: llvm-as -o %t.bc %s
+; RUN: ld.lld -m elf_x86_64 --lto-jobs=2 -save-temps -o %t %t.bc -e foo --lto-O0
+; RUN: llvm-readobj -t -dyn-symbols %t | FileCheck %s
+; RUN: llvm-nm %t0.lto.o | FileCheck --check-prefix=CHECK0 %s
+; RUN: llvm-nm %t1.lto.o | FileCheck --check-prefix=CHECK1 %s
+
+; CHECK:      Symbols [
+; CHECK-NEXT:   Symbol {
+; CHECK-NEXT:     Name:  (0)
+; CHECK-NEXT:     Value: 0x0
+; CHECK-NEXT:     Size: 0
+; CHECK-NEXT:     Binding: Local (0x0)
+; CHECK-NEXT:     Type: None (0x0)
+; CHECK-NEXT:     Other: 0
+; CHECK-NEXT:     Section: Undefined (0x0)
+; CHECK-NEXT:   }
+; CHECK-NEXT:   Symbol {
+; CHECK-NEXT:     Name: bar (5)
+; CHECK-NEXT:     Value: 0x11010
+; CHECK-NEXT:     Size: 8
+; CHECK-NEXT:     Binding: Local (0x0)
+; CHECK-NEXT:     Type: Function (0x2)
+; CHECK-NEXT:     Other [ (0x2)
+; CHECK-NEXT:       STV_HIDDEN (0x2)
+; CHECK-NEXT:     ]
+; CHECK-NEXT:     Section: .text (0x2)
+; CHECK-NEXT:   }
+; CHECK-NEXT:   Symbol {
+; CHECK-NEXT:     Name: foo (1)
+; CHECK-NEXT:     Value: 0x11000
+; CHECK-NEXT:     Size: 8
+; CHECK-NEXT:     Binding: Global (0x1)
+; CHECK-NEXT:     Type: Function (0x2)
+; CHECK-NEXT:     Other: 0
+; CHECK-NEXT:     Section: .text (0x2)
+; CHECK-NEXT:   }
+; CHECK-NEXT: ]
+; CHECK-NEXT: DynamicSymbols [
+; CHECK-NEXT: ]
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+; CHECK0: U bar
+; CHECK0: T foo
+define void @foo() {
+  call void @bar()
+  ret void
+}
+
+; CHECK1: T bar
+; CHECK1: U foo
+define void @bar() {
+  call void @foo()
+  ret void
+}