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

[• rafael]

ELF/InputFiles.cpp

486 ↗

(On Diff #54574)

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 ↗

(On Diff #54574)

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

522 ↗

(On Diff #54683)

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

lld/trunk/ELF/InputFiles.h

 private:
   std::vector<SymbolBody *> SymbolBodies;
   llvm::BumpPtrAllocator Alloc;
   llvm::StringSaver Saver{Alloc};
   SymbolBody *
   createSymbolBody(const llvm::DenseSet<const llvm::Comdat *> &KeptComdats,
                    const llvm::object::IRObjectFile &Obj,
                    const llvm::object::BasicSymbolRef &Sym);
+  SymbolBody *
+  createBody(const llvm::DenseSet<const llvm::Comdat *> &KeptComdats,
+             const llvm::object::IRObjectFile &Obj,
+             const llvm::object::BasicSymbolRef &Sym,
+             const llvm::GlobalValue *GV);
 };
 
 // .so file.
 template <class ELFT> class SharedFile : public ELFFileBase<ELFT> {
   typedef ELFFileBase<ELFT> Base;
   typedef typename ELFT::Shdr Elf_Shdr;
   typedef typename ELFT::Sym Elf_Sym;
   typedef typename ELFT::Word Elf_Word;

lld/trunk/ELF/InputFiles.cpp

case GlobalValue::HiddenVisibility:
     return STV_HIDDEN;
   case GlobalValue::ProtectedVisibility:
     return STV_PROTECTED;
   }
   llvm_unreachable("unknown visibility");
 }
 
 SymbolBody *
-BitcodeFile::createSymbolBody(const DenseSet<const Comdat *> &KeptComdats,
+BitcodeFile::createBody(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;
+                              const BasicSymbolRef &Sym,
+                              const GlobalValue *GV) {
+  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) {
+  if (Flags & BasicSymbolRef::SF_Undefined)
+    return new (Alloc) UndefinedBitcode(NameRef, IsWeak, Visibility);
+  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)
+    return new (Alloc)
         DefinedCommon(NameRef, Size, GV->getAlignment(),
                       IsWeak ? STB_WEAK : STB_GLOBAL, Visibility, /*Type*/ 0);
-  } else {
-    Body = new (Alloc) DefinedBitcode(NameRef, IsWeak, Visibility);
   }
+  return new (Alloc) DefinedBitcode(NameRef, IsWeak, Visibility);
+}
+
+SymbolBody *
+BitcodeFile::createSymbolBody(const DenseSet<const Comdat *> &KeptComdats,
+                              const IRObjectFile &Obj,
+                              const BasicSymbolRef &Sym) {
+  const GlobalValue *GV = Obj.getSymbolGV(Sym.getRawDataRefImpl());
+  SymbolBody *Body = createBody(KeptComdats, Obj, Sym, GV);
+
   // FIXME: Expose a thread-local flag for module asm symbols.
   if (GV) {
     if (GV->isThreadLocal())
       Body->Type = STT_TLS;
     Body->CanOmitFromDynSym = canBeOmittedFromSymbolTable(GV);
   }
   return Body;
 }

lld/trunk/test/ELF/lto/Inputs/comdat.s

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

lld/trunk/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
+; RUN: ld.lld -m elf_x86_64 %t2.o %t.o -o %t2.so -shared
+; RUN: llvm-readobj -t %t2.so | FileCheck %s --check-prefix=OTHER
+
+
+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: }
+
+; OTHER: Symbol {
+; OTHER:   Name: foo
+; OTHER-NEXT:   Value: 0x1000
+; OTHER-NEXT:   Size: 0
+; OTHER-NEXT:   Binding: Global
+; OTHER-NEXT:   Type: None
+; OTHER-NEXT:   Other [
+; OTHER-NEXT:     STV_PROTECTED
+; OTHER-NEXT:   ]
+; OTHER-NEXT:   Section: .text
+; OTHER-NEXT: }