COFF: Do not create SectionChunks for discarded comdat sections.

[pcc]

Summary

With this change, instead of creating a SectionChunk for each section
in the object file, we only create them when we encounter a prevailing
comdat section.

Also change how symbol resolution occurs between comdat symbols. Now
only the comdat leader participates in comdat resolution, and not any
other external associated symbols. This is more in line with how COFF
semantics are defined, and should allow for a more straightforward
implementation of non-ANY comdat types.

On my machine, this change reduces our runtime linking a release
build of chrome_child.dll with /nopdb from 5.65s to 4.54s (median of
50 runs).

[pcc]

• Finish LTO implementation

[ruiu]

lld/COFF/InputFiles.cpp

142 ↗

(On Diff #123708)

It seems a bit odd that you ignored a return value here. Turns out that readSection mutates SparseChunks as a side effect. But can you remove that assignment from that function and instead doing it explicitly in this line?

214 ↗

(On Diff #123708)

I'd return early if Parent is PendingComdat.

222 ↗

(On Diff #123708)

Replace auto with its actual type.

276 ↗

(On Diff #123708)

Do you think you can make readSectionDefinition to have fewer side effects? Not always using its return value seems a bit odd.

lld/COFF/InputFiles.h

148 ↗

(On Diff #123708)

Can you add a blank line after a multi-line declaration?

lld/COFF/SymbolTable.cpp

241 ↗

(On Diff #123708)

Maybe just returning {S, true} and {S, false} is easier to read than using Prevailing variable.

242 ↗

(On Diff #123708)

nit: add {}

[pcc]

• Address review comments

[ruiu]

LGTM

lld/COFF/InputFiles.cpp

251–252 ↗

(On Diff #124013)

else if

333 ↗

(On Diff #124013)

auto -> const coff_aux_section_definition *

lld/COFF/SymbolTable.cpp

241 ↗

(On Diff #124013)

Remove else after return.

lld/trunk/COFF/Chunks.h

public:
   // in a final output. It's supposed to print out a log message to stdout.
   void printDiscardedMessage() const;
 
   // Adds COMDAT associative sections to this COMDAT section. A chunk
   // and its children are treated as a group by the garbage collector.
   void addAssociative(SectionChunk *Child);
 
   StringRef getDebugName() override;
-  void setSymbol(DefinedRegular *S) { if (!Sym) Sym = S; }
 
-  // Returns true if the chunk was not dropped by GC or COMDAT deduplication.
-  bool isLive() { return Live && !Discarded; }
+  // Returns true if the chunk was not dropped by GC.
+  bool isLive() { return Live; }
 
   // Used by the garbage collector.
   void markLive() {
     assert(Config->DoGC && "should only mark things live from GC");
     assert(!isLive() && "Cannot mark an already live section!");
     Live = true;
   }
 
-  // Returns true if this chunk was dropped by COMDAT deduplication.
-  bool isDiscarded() const { return Discarded; }
-
-  // Used by the SymbolTable when discarding unused comdat sections. This is
-  // redundant when GC is enabled, as all comdat sections will start out dead.
-  void markDiscarded() { Discarded = true; }
-
   // True if this is a codeview debug info chunk. These will not be laid out in
   // the image. Instead they will end up in the PDB, if one is requested.
   bool isCodeView() const {
     return SectionName == ".debug" || SectionName.startswith(".debug$");
   }
 
   // True if this is a DWARF debug info or exception handling chunk.
   bool isDWARF() const {
public:
   // Auxiliary Format 5: Section Definitions. Used for ICF.
   uint32_t Checksum = 0;
 
   const coff_section *Header;
 
   // The file that this chunk was created from.
   ObjFile *File;
 
+  // The COMDAT leader symbol if this is a COMDAT chunk.
+  DefinedRegular *Sym = nullptr;
+
 private:
   StringRef SectionName;
   std::vector<SectionChunk *> AssocChildren;
   llvm::iterator_range<const coff_relocation *> Relocs;
   size_t NumRelocs;
 
-  // True if this chunk was discarded because it was a duplicate comdat section.
-  bool Discarded;
-
   // Used by the garbage collector.
   bool Live;
 
   // Used for ICF (Identical COMDAT Folding)
   void replace(SectionChunk *Other);
   uint32_t Class[2] = {0, 0};
-
-  // Sym points to a section symbol if this is a COMDAT chunk.
-  DefinedRegular *Sym = nullptr;
 };
 
 // A chunk for common symbols. Common chunks don't have actual data.
 class CommonChunk : public Chunk {
 public:
   CommonChunk(const COFFSymbolRef Sym);
   size_t getSize() const override { return Sym.getValue(); }
   bool hasData() const override { return false; }

lld/trunk/COFF/Chunks.cpp

SectionChunk::SectionChunk(ObjFile *F, const coff_section *H)
     : Chunk(SectionKind), Repl(this), Header(H), File(F),
       Relocs(File->getCOFFObj()->getRelocations(Header)),
       NumRelocs(std::distance(Relocs.begin(), Relocs.end())) {
   // Initialize SectionName.
   File->getCOFFObj()->getSectionName(Header, SectionName);
 
   Alignment = Header->getAlignment();
 
-  // Chunks may be discarded during comdat merging.
-  Discarded = false;
-
   // If linker GC is disabled, every chunk starts out alive.  If linker GC is
   // enabled, treat non-comdat sections as roots. Generally optimized object
   // files will be built with -ffunction-sections or /Gy, so most things worth
   // stripping will be in a comdat.
   Live = !Config->DoGC || !isCOMDAT();
 }
 
 static void add16(uint8_t *P, int16_t V) { write16le(P, read16le(P) + V); }

 
 bool SectionChunk::isCOMDAT() const {
   return Header->Characteristics & IMAGE_SCN_LNK_COMDAT;
 }
 
 void SectionChunk::printDiscardedMessage() const {
   // Removed by dead-stripping. If it's removed by ICF, ICF already
   // printed out the name, so don't repeat that here.
-  if (Sym && this == Repl) {
-    if (Discarded)
-      message("Discarded comdat symbol " + Sym->getName());
-    else if (!Live)
-      message("Discarded " + Sym->getName());
-  }
-}
+  if (Sym && this == Repl)
 
 StringRef SectionChunk::getDebugName() {
   if (Sym)
     return Sym->getName();
   return "";
 }
 
 ArrayRef<uint8_t> SectionChunk::getContents() const {

lld/trunk/COFF/InputFiles.h

public:
   // if we are not producing a PDB.
   llvm::pdb::DbiModuleDescriptorBuilder *ModuleDBI = nullptr;
 
 private:
   void initializeChunks();
   void initializeSymbols();
   void initializeSEH();
 
-  Symbol *createDefined(COFFSymbolRef Sym, const void *Aux, bool IsFirst);
+  SectionChunk *
+  readSection(uint32_t SectionNumber,
+              const llvm::object::coff_aux_section_definition *Def);
+
+  void readAssociativeDefinition(
+      COFFSymbolRef COFFSym,
+      const llvm::object::coff_aux_section_definition *Def);
+
+  llvm::Optional<Symbol *>
+  createDefined(COFFSymbolRef Sym,
+                std::vector<const llvm::object::coff_aux_section_definition *>
+                    &ComdatDefs);
+  Symbol *createRegular(COFFSymbolRef Sym);
   Symbol *createUndefined(COFFSymbolRef Sym);
 
   std::unique_ptr<COFFObjectFile> COFFObj;
   const coff_section *SXData = nullptr;
 
   // List of all chunks defined by this file. This includes both section
   // chunks and non-section chunks for common symbols.
   std::vector<Chunk *> Chunks;

lld/trunk/COFF/InputFiles.cpp

void ObjFile::parse() {
   }
 
   // Read section and symbol tables.
   initializeChunks();
   initializeSymbols();
   initializeSEH();
 }
 
+// We set SectionChunk pointers in the SparseChunks vector to this value
+// temporarily to mark comdat sections as having an unknown resolution. As we
+// walk the object file's symbol table, once we visit either a leader symbol or
+// an associative section definition together with the parent comdat's leader,
+// we set the pointer to either nullptr (to mark the section as discarded) or a
+// valid SectionChunk for that section.
+static SectionChunk *const PendingComdat = reinterpret_cast<SectionChunk *>(1);
+
 void ObjFile::initializeChunks() {
   uint32_t NumSections = COFFObj->getNumberOfSections();
   Chunks.reserve(NumSections);
   SparseChunks.resize(NumSections + 1);
   for (uint32_t I = 1; I < NumSections + 1; ++I) {
     const coff_section *Sec;
-    StringRef Name;
     if (auto EC = COFFObj->getSection(I, Sec))
       fatal("getSection failed: #" + Twine(I) + ": " + EC.message());
-    if (auto EC = COFFObj->getSectionName(Sec, Name))
-      fatal("getSectionName failed: #" + Twine(I) + ": " + EC.message());
-    if (Name == ".sxdata") {
-      SXData = Sec;
-      continue;
-    }
-    if (Name == ".drectve") {
-      ArrayRef<uint8_t> Data;
-      COFFObj->getSectionContents(Sec, Data);
-      Directives = std::string((const char *)Data.data(), Data.size());
-      continue;
-    }
+
+    if (Sec->Characteristics & IMAGE_SCN_LNK_COMDAT)
+      SparseChunks[I] = PendingComdat;
+    else
+      SparseChunks[I] = readSection(I, nullptr);
+  }
+}
+
+SectionChunk *ObjFile::readSection(uint32_t SectionNumber,
+                                   const coff_aux_section_definition *Def) {
+  const coff_section *Sec;
+  StringRef Name;
+  if (auto EC = COFFObj->getSection(SectionNumber, Sec))
+    fatal("getSection failed: #" + Twine(SectionNumber) + ": " + EC.message());
+    fatal("getSectionName failed: #" + Twine(SectionNumber) + ": " +
+          EC.message());
+    return nullptr;
+    return nullptr;
 
-    // Object files may have DWARF debug info or MS CodeView debug info
-    // (or both).
-    //
-    // DWARF sections don't need any special handling from the perspective
-    // of the linker; they are just a data section containing relocations.
-    // We can just link them to complete debug info.
-    //
-    // CodeView needs a linker support. We need to interpret and debug
-    // info, and then write it to a separate .pdb file.
 
-    // Ignore debug info unless /debug is given.
-    if (!Config->Debug && Name.startswith(".debug"))
-      continue;
+    return nullptr;
 
-    if (Sec->Characteristics & llvm::COFF::IMAGE_SCN_LNK_REMOVE)
-      continue;
-    auto *C = make<SectionChunk>(this, Sec);
+    return nullptr;
+  if (Def)
+    C->Checksum = Def->CheckSum;
 
-    // CodeView sections are stored to a different vector because they are not
-    // linked in the regular manner.
-    if (C->isCodeView())
-      DebugChunks.push_back(C);
-    else
-      Chunks.push_back(C);
 
-    SparseChunks[I] = C;
+  return C;
+}
+
+void ObjFile::readAssociativeDefinition(
+    COFFSymbolRef Sym, const coff_aux_section_definition *Def) {
+  SectionChunk *Parent = SparseChunks[Def->getNumber(Sym.isBigObj())];
+
+  // If the parent is pending, it probably means that its section definition
+  // appears after us in the symbol table. Leave the associated section as
+  // pending; we will handle it during the second pass in initializeSymbols().
+  if (Parent == PendingComdat)
+    return;
+
+  // Check whether the parent is prevailing. If it is, so are we, and we read
+  // the section; otherwise mark it as discarded.
+  int32_t SectionNumber = Sym.getSectionNumber();
+  if (Parent) {
+    SparseChunks[SectionNumber] = readSection(SectionNumber, Def);
+    Parent->addAssociative(SparseChunks[SectionNumber]);
+  } else {
+    SparseChunks[SectionNumber] = nullptr;
+  }
+}
+
+Symbol *ObjFile::createRegular(COFFSymbolRef Sym) {
+  SectionChunk *SC = SparseChunks[Sym.getSectionNumber()];
+  if (Sym.isExternal()) {
+    StringRef Name;
+    COFFObj->getSymbolName(Sym, Name);
+    if (SC)
+      return Symtab->addRegular(this, Name, Sym.getGeneric(), SC);
+    return Symtab->addUndefined(Name, this, false);
   }
+  if (SC)
+    return make<DefinedRegular>(this, /*Name*/ "", false,
+                                /*IsExternal*/ false, Sym.getGeneric(), SC);
+  return nullptr;
 }
 
 void ObjFile::initializeSymbols() {
   uint32_t NumSymbols = COFFObj->getNumberOfSymbols();
   Symbols.resize(NumSymbols);
 
   SmallVector<std::pair<Symbol *, uint32_t>, 8> WeakAliases;
-  int32_t LastSectionNumber = 0;
+  std::vector<uint32_t> PendingIndexes;
+  PendingIndexes.reserve(NumSymbols);
+
+  std::vector<const coff_aux_section_definition *> ComdatDefs(
+      COFFObj->getNumberOfSections() + 1);
 
   for (uint32_t I = 0; I < NumSymbols; ++I) {
     COFFSymbolRef COFFSym = check(COFFObj->getSymbol(I));
-
-    const void *AuxP = nullptr;
-    if (COFFSym.getNumberOfAuxSymbols())
-      AuxP = check(COFFObj->getSymbol(I + 1)).getRawPtr();
-    bool IsFirst = (LastSectionNumber != COFFSym.getSectionNumber());
-
-    Symbol *Sym = nullptr;
     if (COFFSym.isUndefined()) {
-      Sym = createUndefined(COFFSym);
+      Symbols[I] = createUndefined(COFFSym);
     } else if (COFFSym.isWeakExternal()) {
-      Sym = createUndefined(COFFSym);
-      uint32_t TagIndex =
-          static_cast<const coff_aux_weak_external *>(AuxP)->TagIndex;
-      WeakAliases.emplace_back(Sym, TagIndex);
+      Symbols[I] = createUndefined(COFFSym);
+      uint32_t TagIndex = COFFSym.getAux<coff_aux_weak_external>()->TagIndex;
+      WeakAliases.emplace_back(Symbols[I], TagIndex);
+    } else if (Optional<Symbol *> OptSym = createDefined(COFFSym, ComdatDefs)) {
+      Symbols[I] = *OptSym;
     } else {
-      Sym = createDefined(COFFSym, AuxP, IsFirst);
+      // createDefined() returns None if a symbol belongs to a section that
+      // was pending at the point when the symbol was read. This can happen in
+      // two cases:
+      // 1) section definition symbol for a comdat leader;
+      // 2) symbol belongs to a comdat section associated with a section whose
+      //    section definition symbol appears later in the symbol table.
+      // In both of these cases, we can expect the section to be resolved by
+      // the time we finish visiting the remaining symbols in the symbol
+      // table. So we postpone the handling of this symbol until that time.
+      PendingIndexes.push_back(I);
     }
-    Symbols[I] = Sym;
     I += COFFSym.getNumberOfAuxSymbols();
-    LastSectionNumber = COFFSym.getSectionNumber();
+  }
+
+  for (uint32_t I : PendingIndexes) {
+    COFFSymbolRef Sym = check(COFFObj->getSymbol(I));
+    if (auto *Def = Sym.getSectionDefinition())
+      if (Def->Selection == IMAGE_COMDAT_SELECT_ASSOCIATIVE)
+        readAssociativeDefinition(Sym, Def);
+    Symbols[I] = createRegular(Sym);
   }
 
   for (auto &KV : WeakAliases) {
     Symbol *Sym = KV.first;
     uint32_t Idx = KV.second;
     checkAndSetWeakAlias(Symtab, this, Sym, Symbols[Idx]);
   }
 }
 
 Symbol *ObjFile::createUndefined(COFFSymbolRef Sym) {
   StringRef Name;
   COFFObj->getSymbolName(Sym, Name);
   return Symtab->addUndefined(Name, this, Sym.isWeakExternal());
 }
 
-Symbol *ObjFile::createDefined(COFFSymbolRef Sym, const void *AuxP,
-                               bool IsFirst) {
+Optional<Symbol *> ObjFile::createDefined(
+    COFFSymbolRef Sym,
+    std::vector<const coff_aux_section_definition *> &ComdatDefs) {
   StringRef Name;
   if (Sym.isCommon()) {
     auto *C = make<CommonChunk>(Sym);
     Chunks.push_back(C);
     COFFObj->getSymbolName(Sym, Name);
     Symbol *S =
         Symtab->addCommon(this, Name, Sym.getValue(), Sym.getGeneric(), C);
     return S;
Optional<Symbol *> ObjFile::createDefined(
   if (llvm::COFF::isReservedSectionNumber(SectionNumber))
     fatal("broken object file: " + toString(this));
 
   // This symbol references a section which is not present in the section
   // header.
   if ((uint32_t)SectionNumber >= SparseChunks.size())
     fatal("broken object file: " + toString(this));
 
-  // Nothing else to do without a section chunk.
-  auto *SC = SparseChunks[SectionNumber];
-  if (!SC)
-    return nullptr;
-
-  // Handle section definitions
-  if (IsFirst && AuxP) {
-    auto *Aux = reinterpret_cast<const coff_aux_section_definition *>(AuxP);
-    if (Aux->Selection == IMAGE_COMDAT_SELECT_ASSOCIATIVE)
-      if (auto *ParentSC = SparseChunks[Aux->getNumber(Sym.isBigObj())]) {
-        ParentSC->addAssociative(SC);
-        // If we already discarded the parent, discard the child.
-        if (ParentSC->isDiscarded())
-          SC->markDiscarded();
-      }
-    SC->Checksum = Aux->CheckSum;
+  // Handle comdat leader symbols.
+  if (const coff_aux_section_definition *Def = ComdatDefs[SectionNumber]) {
+    ComdatDefs[SectionNumber] = nullptr;
+    Symbol *Leader;
+    bool Prevailing;
+    if (Sym.isExternal()) {
+      COFFObj->getSymbolName(Sym, Name);
+      std::tie(Leader, Prevailing) =
+          Symtab->addComdat(this, Name, Sym.getGeneric());
+    } else {
+      Leader = make<DefinedRegular>(this, /*Name*/ "", false,
+                                    /*IsExternal*/ false, Sym.getGeneric());
+      Prevailing = true;
+    }
+    if (Prevailing) {
+      SectionChunk *C = readSection(SectionNumber, Def);
+      SparseChunks[SectionNumber] = C;
+      C->Sym = cast<DefinedRegular>(Leader);
+      cast<DefinedRegular>(Leader)->Data = &C->Repl;
+    } else {
+      SparseChunks[SectionNumber] = nullptr;
+    return Leader;
   }
 
-  DefinedRegular *B;
-  if (Sym.isExternal()) {
-    COFFObj->getSymbolName(Sym, Name);
-    Symbol *S =
-        Symtab->addRegular(this, Name, SC->isCOMDAT(), Sym.getGeneric(), SC);
-    B = cast<DefinedRegular>(S);
-  } else
-    B = make<DefinedRegular>(this, /*Name*/ "", SC->isCOMDAT(),
-                             /*IsExternal*/ false, Sym.getGeneric(), SC);
-  if (SC->isCOMDAT() && Sym.getValue() == 0 && !AuxP)
-    SC->setSymbol(B);
+  // Read associative section definitions and prepare to handle the comdat
+  // leader symbol by setting the section's ComdatDefs pointer if we encounter a
+  // non-associative comdat.
+  if (SparseChunks[SectionNumber] == PendingComdat) {
+    if (auto *Def = Sym.getSectionDefinition()) {
+      if (Def->Selection == IMAGE_COMDAT_SELECT_ASSOCIATIVE)
+        readAssociativeDefinition(Sym, Def);
+      else
+        ComdatDefs[SectionNumber] = Def;
+    }
+  }
 
-  return B;
+  if (SparseChunks[SectionNumber] == PendingComdat)
+    return None;
+  return createRegular(Sym);
 }
 
 void ObjFile::initializeSEH() {
   if (!SEHCompat || !SXData)
     return;
   ArrayRef<uint8_t> A;
   COFFObj->getSectionContents(SXData, A);
   if (A.size() % 4 != 0)
void ImportFile::parse() {
   // DLL functions just like regular non-DLL functions.)
   if (Hdr->getType() == llvm::COFF::IMPORT_CODE)
     ThunkSym = Symtab->addImportThunk(Name, ImpSym, Hdr->Machine);
 }
 
 void BitcodeFile::parse() {
   Obj = check(lto::InputFile::create(MemoryBufferRef(
       MB.getBuffer(), Saver.save(ParentName + MB.getBufferIdentifier()))));
+  std::vector<std::pair<Symbol *, bool>> Comdat(Obj->getComdatTable().size());
+  for (size_t I = 0; I != Obj->getComdatTable().size(); ++I)
+    Comdat[I] = Symtab->addComdat(this, Saver.save(Obj->getComdatTable()[I]));
   for (const lto::InputFile::Symbol &ObjSym : Obj->symbols()) {
     StringRef SymName = Saver.save(ObjSym.getName());
+    int ComdatIndex = ObjSym.getComdatIndex();
     Symbol *Sym;
     if (ObjSym.isUndefined()) {
       Sym = Symtab->addUndefined(SymName, this, false);
     } else if (ObjSym.isCommon()) {
       Sym = Symtab->addCommon(this, SymName, ObjSym.getCommonSize());
     } else if (ObjSym.isWeak() && ObjSym.isIndirect()) {
       // Weak external.
       Sym = Symtab->addUndefined(SymName, this, true);
       std::string Fallback = ObjSym.getCOFFWeakExternalFallback();
       Symbol *Alias = Symtab->addUndefined(Saver.save(Fallback));
       checkAndSetWeakAlias(Symtab, this, Sym, Alias);
+    } else if (ComdatIndex != -1) {
+      if (SymName == Obj->getComdatTable()[ComdatIndex])
+        Sym = Comdat[ComdatIndex].first;
+      else if (Comdat[ComdatIndex].second)
+        Sym = Symtab->addRegular(this, SymName);
+      else
+        Sym = Symtab->addUndefined(SymName, this, false);
     } else {
-      bool IsCOMDAT = ObjSym.getComdatIndex() != -1;
-      Sym = Symtab->addRegular(this, SymName, IsCOMDAT);
+      Sym = Symtab->addRegular(this, SymName);
     }
     SymbolBodies.push_back(Sym);
   }
   Directives = Obj->getCOFFLinkerOpts();
 }
 
 MachineTypes BitcodeFile::getMachineType() {
   switch (Triple(Obj->getTargetTriple()).getArch()) {

lld/trunk/COFF/MarkLive.cpp

void markLive(const std::vector<Chunk *> &Chunks) {
   };
 
   // Add GC root chunks.
   for (Symbol *B : Config->GCRoot)
     AddSym(B);
 
   while (!Worklist.empty()) {
     SectionChunk *SC = Worklist.pop_back_val();
-
-    // If this section was discarded, there are relocations referring to
-    // discarded sections. Ignore these sections to avoid crashing. They will be
-    // diagnosed during relocation processing.
-    if (SC->isDiscarded())
-      continue;
-
     assert(SC->isLive() && "We mark as live when pushing onto the worklist!");
 
     // Mark all symbols listed in the relocation table for this section.
     for (Symbol *B : SC->symbols())
       if (B)
         AddSym(B);
 
     // Mark associative sections if any.
     for (SectionChunk *C : SC->children())
       Enqueue(C);
   }
 }
 
 }
 }

lld/trunk/COFF/SymbolTable.h

public:
   Symbol *addUndefined(StringRef Name);
 
   Symbol *addSynthetic(StringRef N, Chunk *C);
   Symbol *addAbsolute(StringRef N, uint64_t VA);
 
   Symbol *addUndefined(StringRef Name, InputFile *F, bool IsWeakAlias);
   void addLazy(ArchiveFile *F, const Archive::Symbol Sym);
   Symbol *addAbsolute(StringRef N, COFFSymbolRef S);
-  Symbol *addRegular(InputFile *F, StringRef N, bool IsCOMDAT,
+  Symbol *addRegular(InputFile *F, StringRef N,
                      const llvm::object::coff_symbol_generic *S = nullptr,
                      SectionChunk *C = nullptr);
+  std::pair<Symbol *, bool>
+  addComdat(InputFile *F, StringRef N,
+            const llvm::object::coff_symbol_generic *S = nullptr);
   Symbol *addCommon(InputFile *F, StringRef N, uint64_t Size,
                     const llvm::object::coff_symbol_generic *S = nullptr,
                     CommonChunk *C = nullptr);
   DefinedImportData *addImportData(StringRef N, ImportFile *F);
   DefinedImportThunk *addImportThunk(StringRef Name, DefinedImportData *S,
                                      uint16_t Machine);
 
   void reportDuplicate(Symbol *Existing, InputFile *NewFile);

lld/trunk/COFF/SymbolTable.cpp

 #include "llvm/Support/raw_ostream.h"
 #include <utility>
 
 using namespace llvm;
 
 namespace lld {
 namespace coff {
 
-enum SymbolPreference {
-  SP_EXISTING = -1,
-  SP_CONFLICT = 0,
-  SP_NEW = 1,
-};
-
-/// Checks if an existing symbol S should be kept or replaced by a new symbol.
-/// Returns SP_EXISTING when S should be kept, SP_NEW when the new symbol
-/// should be kept, and SP_CONFLICT if no valid resolution exists.
-static SymbolPreference compareDefined(Symbol *S, bool WasInserted,
-                                       bool NewIsCOMDAT) {
-  // If the symbol wasn't previously known, the new symbol wins by default.
-  if (WasInserted || !isa<Defined>(S))
-    return SP_NEW;
-
-  // If the existing symbol is a DefinedRegular, both it and the new symbol
-  // must be comdats. In that case, we have no reason to prefer one symbol
-  // over the other, and we keep the existing one. If one of the symbols
-  // is not a comdat, we report a conflict.
-  if (auto *R = dyn_cast<DefinedRegular>(S)) {
-    if (NewIsCOMDAT && R->isCOMDAT())
-      return SP_EXISTING;
-    else
-      return SP_CONFLICT;
-  }
-
-  // Existing symbol is not a DefinedRegular; new symbol wins.
-  return SP_NEW;
-}
-
 SymbolTable *Symtab;
 
 void SymbolTable::addFile(InputFile *File) {
   log("Reading " + toString(File));
   File->parse();
 
   MachineTypes MT = File->getMachineType();
   if (Config->Machine == IMAGE_FILE_MACHINE_UNKNOWN) {
Symbol *SymbolTable::addSynthetic(StringRef N, Chunk *C) {
   S->IsUsedInRegularObj = true;
   if (WasInserted || isa<Undefined>(S) || isa<Lazy>(S))
     replaceSymbol<DefinedSynthetic>(S, N, C);
   else if (!isa<DefinedCOFF>(S))
     reportDuplicate(S, nullptr);
   return S;
 }
 
-Symbol *SymbolTable::addRegular(InputFile *F, StringRef N, bool IsCOMDAT,
+Symbol *SymbolTable::addRegular(InputFile *F, StringRef N,
                                 const coff_symbol_generic *Sym,
                                 SectionChunk *C) {
   Symbol *S;
   bool WasInserted;
   std::tie(S, WasInserted) = insert(N);
   if (!isa<BitcodeFile>(F))
     S->IsUsedInRegularObj = true;
-  SymbolPreference SP = compareDefined(S, WasInserted, IsCOMDAT);
-  if (SP == SP_CONFLICT) {
+  if (WasInserted || !isa<DefinedRegular>(S))
+    replaceSymbol<DefinedRegular>(S, F, N, /*IsCOMDAT*/ false,
+                                  /*IsExternal*/ true, Sym, C);
+  else
     reportDuplicate(S, F);
-  } else if (SP == SP_NEW) {
-    replaceSymbol<DefinedRegular>(S, F, N, IsCOMDAT, /*IsExternal*/ true, Sym,
-                                  C);
-  } else if (SP == SP_EXISTING && IsCOMDAT && C) {
-    C->markDiscarded();
-    // Discard associative chunks that we've parsed so far. No need to recurse
-    // because an associative section cannot have children.
-    for (SectionChunk *Child : C->children())
-      Child->markDiscarded();
-  }
   return S;
 }
 
+std::pair<Symbol *, bool>
+SymbolTable::addComdat(InputFile *F, StringRef N,
+                       const coff_symbol_generic *Sym) {
+  Symbol *S;
+  bool WasInserted;
+  std::tie(S, WasInserted) = insert(N);
+  if (!isa<BitcodeFile>(F))
+    S->IsUsedInRegularObj = true;
+  if (WasInserted || !isa<DefinedRegular>(S)) {
+    replaceSymbol<DefinedRegular>(S, F, N, /*IsCOMDAT*/ true,
+                                  /*IsExternal*/ true, Sym, nullptr);
+    return {S, true};
+  }
+  if (!cast<DefinedRegular>(S)->isCOMDAT())
+    reportDuplicate(S, F);
+  return {S, false};
+}
+
 Symbol *SymbolTable::addCommon(InputFile *F, StringRef N, uint64_t Size,
                                const coff_symbol_generic *Sym, CommonChunk *C) {
   Symbol *S;
   bool WasInserted;
   std::tie(S, WasInserted) = insert(N);
   if (!isa<BitcodeFile>(F))
     S->IsUsedInRegularObj = true;
   if (WasInserted || !isa<DefinedCOFF>(S))

lld/trunk/COFF/Symbols.h

static bool classof(const Symbol *S) {
     return S->kind() == DefinedRegularKind;
   }
 
   uint64_t getRVA() const { return (*Data)->getRVA() + Sym->Value; }
   bool isCOMDAT() const { return IsCOMDAT; }
   SectionChunk *getChunk() const { return *Data; }
   uint32_t getValue() const { return Sym->Value; }
 
-private:
   SectionChunk **Data;
 };
 
 class DefinedCommon : public DefinedCOFF {
 public:
   DefinedCommon(InputFile *F, StringRef N, uint64_t Size,
                 const coff_symbol_generic *S = nullptr,
                 CommonChunk *C = nullptr)

lld/trunk/test/COFF/reloc-discarded.s

# CHECK: error: relocation against symbol in discarded section: assoc_global
   .section  .bss,"bw",discard,main_global
   .globl  main_global
   .p2align  2
 main_global:
   .long 0
 
   .section  .CRT$XCU,"dr",associative,main_global
   .p2align  3
-  .globl assoc_global
 assoc_global:
   .quad main_global
 
   .text
   .globl main
 main:
   movq assoc_global(%rip), %rax
   movl (%rax), %eax
   retq

llvm/trunk/include/llvm/Object/COFF.h

 struct coff_symbol_generic {
   union {
     char ShortName[COFF::NameSize];
     StringTableOffset Offset;
   } Name;
   support::ulittle32_t Value;
 };
 
+struct coff_aux_section_definition;
+
 class COFFSymbolRef {
 public:
   COFFSymbolRef() = default;
   COFFSymbolRef(const coff_symbol16 *CS) : CS16(CS) {}
   COFFSymbolRef(const coff_symbol32 *CS) : CS32(CS) {}
 
   const void *getRawPtr() const {
     return CS16 ? static_cast<const void *>(CS16) : CS32;
public:
   }
 
   uint8_t getBaseType() const { return getType() & 0x0F; }
 
   uint8_t getComplexType() const {
     return (getType() & 0xF0) >> COFF::SCT_COMPLEX_TYPE_SHIFT;
   }
 
+  template <typename T> const T *getAux() const {
+    return CS16 ? reinterpret_cast<const T *>(CS16 + 1)
+                : reinterpret_cast<const T *>(CS32 + 1);
+  }
+
+  const coff_aux_section_definition *getSectionDefinition() const {
+    if (!getNumberOfAuxSymbols() ||
+        getStorageClass() != COFF::IMAGE_SYM_CLASS_STATIC)
+      return nullptr;
+    return getAux<coff_aux_section_definition>();
+  }
+
   bool isAbsolute() const {
     return getSectionNumber() == -1;
   }
 
   bool isExternal() const {
     return getStorageClass() == COFF::IMAGE_SYM_CLASS_EXTERNAL;
   }