[lld] Explicitly ignore comdat groups when parsing LTO object

[sbc100]

Summary

Any symbols defined in the LTO object are by definition the ones we
want in the final output so we skip the comdat group checking in those
cases.

This change makes the ELF code more explicit about this and means
that wasm and ELF do this in the same way.

[sbc100]

• typo

[ruiu]

LGTM

lld/ELF/InputFiles.cpp

623 ↗

(On Diff #203048)

80 col?

[thakis]

lld/ELF/Driver.cpp

1506 ↗

(On Diff #203048)

fyi, the usual style is /*IgnoreComdats=*/true (with no space and a =): for true it doesn't make a big difference, but for false it's not clear without the = if the comment means "set IgnoreComdats to false, that is they are not ignored" or "passing false here _means_ IgnoreComdats, that is passing false means they _are_ ignored".

[sbc100]

• clang-format
• feedback

[christylee]

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. Is the assumption "any symbols defined in the LTO object are by definition the ones we want in the final output" only true for monolithic lto?

[sbc100]

In D62884#2044673, @christylee wrote:

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. Is the assumption "any symbols defined in the LTO object are by definition the ones we want in the final output" only true for monolithic lto?

You could be right here. I'm don't understand thingLTO enough to know that answer to that question of the top of my head. However the behavior before and after this change should be the same, since I believe they were being ignored before this change too. Do you see any other side effects of settings this to false?

[christylee]

However the behavior before and after this change should be the same, since I believe they were being ignored before this change too

I tried an earlier linker version without this patch and lld also did not de-duplicate. My guess is since with thinlto we are not doing true whole program analysis, there's still some leftover comdat groups we need to clean up at link time.

Do you see any other side effects of settings this to false?

I don't think so. My understanding is that in the monolithic lto case, if we set this to false then no extra comdat sections will be removed. If we set it to true for thinlto, lld will then link as normal.

Let me put up a diff to set ignoreComdats to false for thinlto and request more comments.

[MaskRay]

In D62884#2044673, @christylee wrote:

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. Is the assumption "any symbols defined in the LTO object are by definition the ones we want in the final output" only true for monolithic lto?

+@dblaikie

[dblaikie]

In D62884#2045111, @MaskRay wrote:

In D62884#2044673, @christylee wrote:

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. Is the assumption "any symbols defined in the LTO object are by definition the ones we want in the final output" only true for monolithic lto?

+@dblaikie

Hey, sorry I missed this - picking up the conversation over at https://reviews.llvm.org/D80765#2170820 I think.

lld/trunk/ELF/Driver.cpp

 // the compiler at once, it can do a whole-program optimization.
 template <class ELFT> void LinkerDriver::compileBitcodeFiles() {
   // Compile bitcode files and replace bitcode symbols.
   LTO.reset(new BitcodeCompiler);
   for (BitcodeFile *File : BitcodeFiles)
     LTO->add(*File);
 
   for (InputFile *File : LTO->compile()) {
-    DenseMap<CachedHashStringRef, const InputFile *> DummyGroups;
     auto *Obj = cast<ObjFile<ELFT>>(File);
-    Obj->parse(DummyGroups);
+    Obj->parse(/*IgnoreComdats=*/true);
     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/trunk/ELF/InputFiles.h

public:
 
   ArrayRef<Symbol *> getLocalSymbols();
   ArrayRef<Symbol *> getGlobalSymbols();
 
   ObjFile(MemoryBufferRef M, StringRef ArchiveName) : ELFFileBase(ObjKind, M) {
     this->ArchiveName = ArchiveName;
   }
 
-  void parse(llvm::DenseMap<llvm::CachedHashStringRef, const InputFile *>
-                 &ComdatGroups);
+  void parse(bool IgnoreComdats = false);
 
   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:
 
   // Pointer to this input file's .llvm_addrsig section, if it has one.
   const Elf_Shdr *AddrsigSec = nullptr;
 
   // SHT_LLVM_CALL_GRAPH_PROFILE table
   ArrayRef<Elf_CGProfile> CGProfile;
 
 private:
-  void initializeSections(llvm::DenseMap<llvm::CachedHashStringRef,
-                                         const InputFile *> &ComdatGroups);
+  void initializeSections(bool IgnoreComdats);
   void initializeSymbols();
   void initializeJustSymbols();
   void initializeDwarf();
   InputSectionBase *getRelocTarget(const Elf_Shdr &Sec);
   InputSectionBase *createInputSection(const Elf_Shdr &Sec);
   StringRef getSectionName(const Elf_Shdr &Sec);
 
   bool shouldMerge(const Elf_Shdr &Sec);
private:
   llvm::DenseSet<uint64_t> Seen;
 };
 
 class BitcodeFile : public InputFile {
 public:
   BitcodeFile(MemoryBufferRef M, StringRef ArchiveName,
               uint64_t OffsetInArchive);
   static bool classof(const InputFile *F) { return F->kind() == BitcodeKind; }
-  template <class ELFT>
-  void parse(llvm::DenseMap<llvm::CachedHashStringRef, const InputFile *>
-                 &ComdatGroups);
+  template <class ELFT> void parse();
   std::unique_ptr<llvm::lto::InputFile> Obj;
 };
 
 // .so file.
 class SharedFile : public ELFFileBase {
 public:
   SharedFile(MemoryBufferRef M, StringRef DefaultSoName)
       : ELFFileBase(SharedKind, M), SoName(DefaultSoName),

lld/trunk/ELF/InputFiles.cpp

template <class ELFT> static void doParseFile(InputFile *File) {
   if (auto *F = dyn_cast<SharedFile>(File)) {
     F->parse<ELFT>();
     return;
   }
 
   // LLVM bitcode file
   if (auto *F = dyn_cast<BitcodeFile>(File)) {
     BitcodeFiles.push_back(F);
-    F->parse<ELFT>(Symtab->ComdatGroups);
+    F->parse<ELFT>();
     return;
   }
 
   // Regular object file
   ObjectFiles.push_back(File);
-  cast<ObjFile<ELFT>>(File)->parse(Symtab->ComdatGroups);
+  cast<ObjFile<ELFT>>(File)->parse();
 }
 
 // Add symbols in File to the symbol table.
 void elf::parseFile(InputFile *File) {
   switch (Config->EKind) {
   case ELF32LEKind:
     doParseFile<ELF32LE>(File);
     return;
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(
-    DenseMap<CachedHashStringRef, const InputFile *> &ComdatGroups) {
+template <class ELFT> void ObjFile<ELFT>::parse(bool IgnoreComdats) {
   // Read a section table. JustSymbols is usually false.
   if (this->JustSymbols)
     initializeJustSymbols();
   else
-    initializeSections(ComdatGroups);
+    initializeSections(IgnoreComdats);
 
   // 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.
else if (Optional<std::string> S = searchLibraryBaseName(Specifier))
     Driver->addFile(*S, /*WithLOption=*/true);
   else
     error(toString(F) +
           ": unable to find library from dependent library specifier: " +
           Specifier);
 }
 
 template <class ELFT>
-void ObjFile<ELFT>::initializeSections(
-    DenseMap<CachedHashStringRef, const InputFile *> &ComdatGroups) {
+void ObjFile<ELFT>::initializeSections(bool IgnoreComdats) {
   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: {
       // are just skipped.
       if (Entries[0] == 0)
         continue;
 
       if (Entries[0] != GRP_COMDAT)
         fatal(toString(this) + ": unsupported SHT_GROUP format");
 
       bool IsNew =
-          ComdatGroups.try_emplace(CachedHashStringRef(Signature), this).second;
+          IgnoreComdats ||
+          Symtab->ComdatGroups.try_emplace(CachedHashStringRef(Signature), this)
+              .second;
       if (IsNew) {
         if (Config->Relocatable)
           this->Sections[I] = createInputSection(Sec);
         continue;
       }
 
       // Otherwise, discard group members.
       for (uint32_t SecIndex : Entries.slice(1)) {
return Symtab->addSymbol(
                      ObjSym.getCommonAlignment(), ObjSym.getCommonSize()});
 
   Defined New(&F, Name, Binding, Visibility, Type, 0, 0, nullptr);
   if (CanOmitFromDynSym)
     New.ExportDynamic = false;
   return Symtab->addSymbol(New);
 }
 
-template <class ELFT>
-void BitcodeFile::parse(
-    DenseMap<CachedHashStringRef, const InputFile *> &ComdatGroups) {
+template <class ELFT> void BitcodeFile::parse() {
   std::vector<bool> KeptComdats;
   for (StringRef S : Obj->getComdatTable())
     KeptComdats.push_back(
-        ComdatGroups.try_emplace(CachedHashStringRef(S), this).second);
+        Symtab->ComdatGroups.try_emplace(CachedHashStringRef(S), this).second);
 
   for (const lto::InputFile::Symbol &ObjSym : Obj->symbols())
     Symbols.push_back(createBitcodeSymbol<ELFT>(KeptComdats, ObjSym, *this));
 
   for (auto L : Obj->getDependentLibraries())
     addDependentLibrary(L, this);
 }
 
std::string elf::replaceThinLTOSuffix(StringRef Path) {
   StringRef Suffix = Config->ThinLTOObjectSuffixReplace.first;
   StringRef Repl = Config->ThinLTOObjectSuffixReplace.second;
 
   if (Path.consume_back(Suffix))
     return (Path + Repl).str();
   return Path;
 }
 
-template void
-BitcodeFile::parse<ELF32LE>(DenseMap<CachedHashStringRef, const InputFile *> &);
-template void
-BitcodeFile::parse<ELF32BE>(DenseMap<CachedHashStringRef, const InputFile *> &);
-template void
-BitcodeFile::parse<ELF64LE>(DenseMap<CachedHashStringRef, const InputFile *> &);
-template void
-BitcodeFile::parse<ELF64BE>(DenseMap<CachedHashStringRef, const InputFile *> &);
+template void BitcodeFile::parse<ELF32LE>();
+template void BitcodeFile::parse<ELF32BE>();
+template void BitcodeFile::parse<ELF64LE>();
+template void BitcodeFile::parse<ELF64BE>();
 
 template void LazyObjFile::parse<ELF32LE>();
 template void LazyObjFile::parse<ELF32BE>();
 template void LazyObjFile::parse<ELF64LE>();
 template void LazyObjFile::parse<ELF64BE>();
 
 template class elf::ObjFile<ELF32LE>;
 template class elf::ObjFile<ELF32BE>;
 template class elf::ObjFile<ELF64LE>;
 template class elf::ObjFile<ELF64BE>;
 
 template void SharedFile::parse<ELF32LE>();
 template void SharedFile::parse<ELF32BE>();
 template void SharedFile::parse<ELF64LE>();
 template void SharedFile::parse<ELF64BE>();

lld/trunk/wasm/InputFiles.cpp

for (const WasmRelocation &Reloc : Section.Relocations)
       if (Reloc.Type == R_WASM_FUNCTION_INDEX_LEB)
         IsCalledDirectly[Reloc.Index] = true;
   }
 
   TypeMap.resize(getWasmObj()->types().size());
   TypeIsUsed.resize(getWasmObj()->types().size(), false);
 
   ArrayRef<StringRef> Comdats = WasmObj->linkingData().Comdats;
-  for (unsigned I = 0; I < Comdats.size(); ++I)
-    if (IgnoreComdats)
-      KeptComdats.push_back(true);
-    else
-      KeptComdats.push_back(Symtab->addComdat(Comdats[I]));
+  for (unsigned I = 0; I < Comdats.size(); ++I) {
+    bool IsNew = IgnoreComdats || Symtab->addComdat(Comdats[I]);
+    KeptComdats.push_back(IsNew);
+  }
 
   // Populate `Segments`.
   for (const WasmSegment &S : WasmObj->dataSegments())
     Segments.emplace_back(make<InputSegment>(S, this));
   setRelocs(Segments, DataSection);
 
   // Populate `Functions`.
   ArrayRef<WasmFunction> Funcs = WasmObj->functions();
Obj = check(lto::InputFile::create(MemoryBufferRef(
       MB.getBuffer(), Saver.save(ArchiveName + MB.getBufferIdentifier()))));
   Triple T(Obj->getTargetTriple());
   if (T.getArch() != Triple::wasm32) {
     error(toString(MB.getBufferIdentifier()) + ": machine type must be wasm32");
     return;
   }
   std::vector<bool> KeptComdats;
   for (StringRef S : Obj->getComdatTable())
-    if (IgnoreComdats)
-      KeptComdats.push_back(true);
-    else
-      KeptComdats.push_back(Symtab->addComdat(S));
 
   for (const lto::InputFile::Symbol &ObjSym : Obj->symbols())
     Symbols.push_back(createBitcodeSymbol(KeptComdats, ObjSym, *this));
 }
 
 // Returns a string in the format of "foo.o" or "foo.a(bar.o)".
 std::string lld::toString(const wasm::InputFile *File) {
   if (!File)
     return "<internal>";
 
   if (File->ArchiveName.empty())
     return File->getName();
 
   return (File->ArchiveName + "(" + File->getName() + ")").str();
 }