[LTO] Create Undefined Bitcode symbol when we drop a comdat member

[• rafael]

ELF/InputFiles.cpp

486

With this Body never gets to

if (GV && GV->isThreadLocal())
  Body->Type = STT_TLS;

so STT_TLS (and on trunk CanOmitFromDynSym) is never set.

Maybe we could split this function in two? One uses early returns to just construct the Body and the second one sets STT_TLS and CanOmitFromDynSym.

test/ELF/lto/comdat2.ll

3

I think this test already passes when linking in this order, no?

I think it is "%t2.o %t.o" that is currently broken. How about just testing both orders?

[davide]

Addressed comments. If you don't like the helper names, feel free to suggest better ones.

[• rafael]

LGTM with a nit.

ELF/InputFiles.cpp

507

This helper doesn't help much. Please inline it.

ELF/InputFiles.cpp

static uint8_t getGvVisibility(const GlobalValue *GV) {
   llvm_unreachable("unknown visibility");
 }
 
 SymbolBody *
 BitcodeFile::createSymbolBody(const DenseSet<const Comdat *> &KeptComdats,
                               const IRObjectFile &Obj,
                               const BasicSymbolRef &Sym) {
   const GlobalValue *GV = Obj.getSymbolGV(Sym.getRawDataRefImpl());
-  if (GV)
-    if (const Comdat *C = GV->getComdat())
-      if (!KeptComdats.count(C))
-        return nullptr;
+  SmallString<64> Name;
+  raw_svector_ostream OS(Name);
+  Sym.printName(OS);
+  StringRef NameRef = Saver.save(StringRef(Name));
+  SymbolBody *Body;
 
   uint32_t Flags = Sym.getFlags();
+  bool IsWeak = Flags & BasicSymbolRef::SF_Weak;
+
   uint8_t Visibility;
   if (GV)
     Visibility = getGvVisibility(GV);
   else
     // FIXME: Set SF_Hidden flag correctly for module asm symbols, and expose
     // protected visibility.
     Visibility = STV_DEFAULT;
 
-  SmallString<64> Name;
-  raw_svector_ostream OS(Name);
-  Sym.printName(OS);
-  StringRef NameRef = Saver.save(StringRef(Name));
+  if (GV)
+    if (const Comdat *C = GV->getComdat())
+      if (!KeptComdats.count(C)) {
+        Body = new (Alloc)
+          UndefinedBitcode(NameRef, IsWeak, Visibility);
+        return Body;
+      }
 
   const Module &M = Obj.getModule();
-  SymbolBody *Body;
-  bool IsWeak = Flags & BasicSymbolRef::SF_Weak;
   if (Flags & BasicSymbolRef::SF_Undefined) {
     Body = new (Alloc) UndefinedBitcode(NameRef, IsWeak, Visibility);
   } else if (Flags & BasicSymbolRef::SF_Common) {
     // FIXME: Set SF_Common flag correctly for module asm symbols, and expose
     // size and alignment.
     assert(GV);
     const DataLayout &DL = M.getDataLayout();
     uint64_t Size = DL.getTypeAllocSize(GV->getValueType());
     Body = new (Alloc)
         DefinedCommon(NameRef, Size, GV->getAlignment(),
                       IsWeak ? STB_WEAK : STB_GLOBAL, Visibility, /*Type*/ 0);
   } else {
     Body = new (Alloc) DefinedBitcode(NameRef, IsWeak, Visibility);
   }
   // FIXME: Expose a thread-local flag for module asm symbols.
   if (GV && GV->isThreadLocal())
     Body->Type = STT_TLS;
   return Body;
 }
 
 bool BitcodeFile::shouldSkip(const BasicSymbolRef &Sym) {
   uint32_t Flags = Sym.getFlags();
   if (!(Flags & BasicSymbolRef::SF_Global))
     return true;
   if (Flags & BasicSymbolRef::SF_FormatSpecific)
     return true;

test/ELF/lto/Inputs/comdat.s

+    .section .text.f,"axG",@progbits,c,comdat
+    .globl foo
+
+foo:
+    retq

test/ELF/lto/comdat2.ll

+; RUN: llvm-as %s -o %t.o
+; RUN: llvm-mc -triple=x86_64-pc-linux %p/Inputs/comdat.s -o %t2.o -filetype=obj
+; RUN: ld.lld -m elf_x86_64 %t.o %t2.o -o %t.so -shared
+; RUN: llvm-readobj -t %t.so | FileCheck %s
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+$c = comdat any
+
+define protected void @foo() comdat($c) {
+  ret void
+}
+
+; CHECK: Symbol {
+; CHECK:   Name: foo
+; CHECK-NEXT:   Value: 0x1000
+; CHECK-NEXT:   Size: 1
+; CHECK-NEXT:   Binding: Global
+; CHECK-NEXT:   Type: Function
+; CHECK-NEXT:   Other [
+; CHECK-NEXT:     STV_PROTECTED
+; CHECK-NEXT:   ]
+; CHECK-NEXT:   Section: .text
+; CHECK-NEXT: }