[rfc] [lld] Do not ignore comdats when parsing LTO objects

[christylee]

Summary

We are trying to migrate from gold to lld, and we found that when building with thinlto, lld does not de-duplicate .debug_types. De-duplication is successful with monolithic lto. Repro: clang -flto=thin -fdebug-types-section -fuse-ld=lld -Wl,-plugin-opt=-generate-type-units

If we set ignoreComdats=false for lto builds, then the de-duplication is successful again. Looking at the thinlto code, it looks like there is no comdat folding at thinlto time.

For more context, see https://reviews.llvm.org/D62884

[sbc100]

If you are going to make the change here can you make the same change in the wasm linker too (in SymbolTable::addCombinedLTOObject)?

Also, if we land this then we can completely remove the argument ignoreComdats argument to parse.

[christylee]

Addressed comments. 1). made the same change to wasm. 2). removed the argument ignoreComdats.

[christylee]

I tried building a more complex workload and the build failed due to some undefined hidden symbol. For thinlto, is the linker or codegen supposed to handle comdat folding?

[ruiu]

In D80255#2047795, @christylee wrote:

I tried building a more complex workload and the build failed due to some undefined hidden symbol. For thinlto, is the linker or codegen supposed to handle comdat folding?

How did it work with gold with the LTO plugin?

[christylee]

In D80255#2048298, @ruiu wrote:

How did it work with gold with the LTO plugin?

As far as I can tell, the llvm gold-plugin.cpp takes the compiled files and pass to gold for normal linking, where comdat de-duplication happens in Sized_relobj_file<size, big_endian>::include_section_group() https://kernel.googlesource.com/pub/scm/linux/kernel/git/hjl/binutils/+/hjl/secondary/gold/object.cc?autodive=0%2F#856. The only difference I see is that for gold, if the first word of a SHT_GROUP section contains flags then it treats the section as a normal section, whereas lld would continue and not create a new section for it. However, the "undefined hidden symbols" come from SHT_GROUP sections so I don't think they are related.

[christylee]

@hoy and I figured out the issue, the problem is caused by de-duplicating twice in thinlto, once in thin link, once in final link. Abandoning this in favor of a new approach. New diff coming up soon.

[christylee]

New diff: https://reviews.llvm.org/D80765

lld/ELF/Driver.cpp

template <class ELFT> void LinkerDriver::compileBitcodeFiles() {
   llvm::TimeTraceScope timeScope("LTO");
   // Compile bitcode files and replace bitcode symbols.
   lto.reset(new BitcodeCompiler);
   for (BitcodeFile *file : bitcodeFiles)
     lto->add(*file);
 
   for (InputFile *file : lto->compile()) {
     auto *obj = cast<ObjFile<ELFT>>(file);
-    obj->parse(/*ignoreComdats=*/true);
+    obj->parse();
     for (Symbol *sym : obj->getGlobalSymbols())
       sym->parseSymbolVersion();
     objectFiles.push_back(file);
   }
 }
 
 // The --wrap option is a feature to rename symbols so that you can write
 // wrappers for existing functions. If you pass `-wrap=foo`, all

lld/ELF/InputFiles.h

public:
 
   ArrayRef<Symbol *> getLocalSymbols();
   ArrayRef<Symbol *> getGlobalSymbols();
 
   ObjFile(MemoryBufferRef m, StringRef archiveName) : ELFFileBase(ObjKind, m) {
     this->archiveName = std::string(archiveName);
   }
 
-  void parse(bool ignoreComdats = false);
+  void parse();
 
   StringRef getShtGroupSignature(ArrayRef<Elf_Shdr> sections,
                                  const Elf_Shdr &sec);
 
   Symbol &getSymbol(uint32_t symbolIndex) const {
     if (symbolIndex >= this->symbols.size())
       fatal(toString(this) + ": invalid symbol index");
     return *this->symbols[symbolIndex];
public:
 
   // SHT_LLVM_CALL_GRAPH_PROFILE table
   ArrayRef<Elf_CGProfile> cgProfile;
 
   // Get cached DWARF information.
   DWARFCache *getDwarf();
 
 private:
-  void initializeSections(bool ignoreComdats);
+  void initializeSections();
   void initializeSymbols();
   void initializeJustSymbols();
 
   InputSectionBase *getRelocTarget(const Elf_Shdr &sec);
   InputSectionBase *createInputSection(const Elf_Shdr &sec);
   StringRef getSectionName(const Elf_Shdr &sec);
 
   bool shouldMerge(const Elf_Shdr &sec, StringRef name);

lld/ELF/InputFiles.cpp

if (this->symbols.empty())
     return {};
   return makeArrayRef(this->symbols).slice(1, this->firstGlobal - 1);
 }
 
 template <class ELFT> ArrayRef<Symbol *> ObjFile<ELFT>::getGlobalSymbols() {
   return makeArrayRef(this->symbols).slice(this->firstGlobal);
 }
 
-template <class ELFT> void ObjFile<ELFT>::parse(bool ignoreComdats) {
+template <class ELFT> void ObjFile<ELFT>::parse() {
   // Read a section table. justSymbols is usually false.
   if (this->justSymbols)
     initializeJustSymbols();
   else
-    initializeSections(ignoreComdats);
+    initializeSections();
 
   // Read a symbol table.
   initializeSymbols();
 }
 
 // Sections with SHT_GROUP and comdat bits define comdat section groups.
 // They are identified and deduplicated by group name. This function
 // returns a group name.
for (uint32_t index : entries.slice(1)) {
     else
       head = s;
     prev = s;
   }
   if (prev)
     prev->nextInSectionGroup = head;
 }
 
-template <class ELFT>
-void ObjFile<ELFT>::initializeSections(bool ignoreComdats) {
+template <class ELFT> void ObjFile<ELFT>::initializeSections() {
   const ELFFile<ELFT> &obj = this->getObj();
 
   ArrayRef<Elf_Shdr> objSections = CHECK(obj.sections(), this);
   uint64_t size = objSections.size();
   this->sections.resize(size);
   this->sectionStringTable =
       CHECK(obj.getSectionStringTable(objSections), this);
 
case SHT_GROUP: {
       // An group with the empty flag doesn't define anything; such sections
       // are just skipped.
       if (entries[0] == 0)
         continue;
 
       if (entries[0] != GRP_COMDAT)
         fatal(toString(this) + ": unsupported SHT_GROUP format");
 
-      bool isNew =
-          ignoreComdats ||
-          symtab->comdatGroups.try_emplace(CachedHashStringRef(signature), this)
-              .second;
-      if (isNew) {
+      if (symtab->comdatGroups.try_emplace(CachedHashStringRef(signature), this)
+              .second) {
         if (config->relocatable)
           this->sections[i] = createInputSection(sec);
         selectedGroups.push_back(entries);
         continue;
       }
 
       // Otherwise, discard group members.
       for (uint32_t secIndex : entries.slice(1)) {

lld/test/ELF/lto/comdat.ll

 ; REQUIRES: x86
 ; RUN: llvm-as %s -o %t.o
 ; RUN: ld.lld %t.o %t.o -o %t.so -shared
 ; RUN: llvm-readobj --symbols %t.so | FileCheck %s
 
 ; CHECK:      Name: foo
 ; CHECK-NEXT: Value:
-; CHECK-NEXT: Size: 1
+; CHECK-NEXT: Size: 0
 ; CHECK-NEXT: Binding: Global
 ; CHECK-NEXT: Type: Function
 ; CHECK-NEXT: Other: 0
-; CHECK-NEXT: Section: .text
+; CHECK-NEXT: Section: Undefined
 
 target triple = "x86_64-unknown-linux-gnu"
 target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
 
 $foo = comdat any
 define void @foo() comdat {
   ret void
 }
-

lld/test/ELF/lto/comdat2.ll

 
 define protected void @foo() comdat($c) {
   ret void
 }
 
 ; CHECK: Symbol {
 ; CHECK:   Name: foo
 ; CHECK-NEXT:   Value:
-; CHECK-NEXT:   Size: 1
+; CHECK-NEXT:   Size: 0
 ; CHECK-NEXT:   Binding: Global
 ; CHECK-NEXT:   Type: Function
 ; CHECK-NEXT:   Other [
 ; CHECK-NEXT:     STV_PROTECTED
 ; CHECK-NEXT:   ]
-; CHECK-NEXT:   Section: .text
+; CHECK-NEXT:   Section: Undefined
 ; CHECK-NEXT: }
 
 ; OTHER: Symbol {
 ; OTHER:   Name: foo
 ; OTHER-NEXT:   Value:
 ; 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: }

lld/wasm/InputFiles.h

 class ObjFile : public InputFile {
 public:
   explicit ObjFile(MemoryBufferRef m, StringRef archiveName)
       : InputFile(ObjectKind, m) {
     this->archiveName = std::string(archiveName);
   }
   static bool classof(const InputFile *f) { return f->kind() == ObjectKind; }
 
-  void parse(bool ignoreComdats = false);
+  void parse();
 
   // Returns the underlying wasm file.
   const WasmObjectFile *getWasmObj() const { return wasmObj.get(); }
 
   void dumpInfo() const;
 
   uint32_t calcNewIndex(const WasmRelocation &reloc) const;
   uint32_t calcNewValue(const WasmRelocation &reloc) const;

lld/wasm/InputFiles.cpp

auto relocsStart = std::lower_bound(relocsNext, relocsEnd,
                                         c->getInputSectionOffset(), relocLess);
     relocsNext = std::lower_bound(
         relocsStart, relocsEnd, c->getInputSectionOffset() + c->getInputSize(),
         relocLess);
     c->setRelocations(ArrayRef<WasmRelocation>(relocsStart, relocsNext));
   }
 }
 
-void ObjFile::parse(bool ignoreComdats) {
+void ObjFile::parse() {
   // Parse a memory buffer as a wasm file.
   LLVM_DEBUG(dbgs() << "Parsing object: " << toString(this) << "\n");
   std::unique_ptr<Binary> bin = CHECK(createBinary(mb), toString(this));
 
   auto *obj = dyn_cast<WasmObjectFile>(bin.get());
   if (!obj)
     fatal(toString(this) + ": not a wasm file");
   if (!obj->isRelocatableObject())
for (const WasmRelocation &reloc : section.Relocations)
         isCalledDirectly[reloc.Index] = true;
   }
 
   typeMap.resize(getWasmObj()->types().size());
   typeIsUsed.resize(getWasmObj()->types().size(), false);
 
   ArrayRef<StringRef> comdats = wasmObj->linkingData().Comdats;
   for (StringRef comdat : comdats) {
-    bool isNew = ignoreComdats || symtab->addComdat(comdat);
-    keptComdats.push_back(isNew);
+    keptComdats.push_back(symtab->addComdat(comdat));
   }
 
   // Populate `Segments`.
   for (const WasmSegment &s : wasmObj->dataSegments()) {
     auto* seg = make<InputSegment>(s, this);
     seg->discarded = isExcludedByComdat(seg);
     segments.emplace_back(seg);
   }

lld/wasm/SymbolTable.cpp

void SymbolTable::addFile(InputFile *file) {
   if (auto *f = dyn_cast<BitcodeFile>(file)) {
     f->parse();
     bitcodeFiles.push_back(f);
     return;
   }
 
   // Regular object file
   auto *f = cast<ObjFile>(file);
-  f->parse(false);
+  f->parse();
   objectFiles.push_back(f);
 }
 
 // This function is where all the optimizations of link-time
 // optimization happens. When LTO is in use, some input files are
 // not in native object file format but in the LLVM bitcode format.
 // This function compiles bitcode files into a few big native files
 // using LLVM functions and replaces bitcode symbols with the results.
 // Because all bitcode files that the program consists of are passed
 // to the compiler at once, it can do whole-program optimization.
 void SymbolTable::addCombinedLTOObject() {
   // Prevent further LTO objects being included
   BitcodeFile::doneLTO = true;
 
   if (bitcodeFiles.empty())
     return;
 
   // Compile bitcode files and replace bitcode symbols.
   lto.reset(new BitcodeCompiler);
   for (BitcodeFile *f : bitcodeFiles)
     lto->add(*f);
 
   for (StringRef filename : lto->compile()) {
     auto *obj = make<ObjFile>(MemoryBufferRef(filename, "lto.tmp"), "");
-    obj->parse(true);
+    obj->parse();
     objectFiles.push_back(obj);
   }
 }
 
 Symbol *SymbolTable::find(StringRef name) {
   auto it = symMap.find(CachedHashStringRef(name));
   if (it == symMap.end() || it->second == -1)
     return nullptr;