[ELF] Improve error message for relocations to symbols defined in discarded sections

[MaskRay]

Summary

Rather than report "undefined symbol: ", give more informative message
about the object file that defines the discarded section.

In particular, PR41133, if the section is a discarded COMDAT, print the
section group signature and the object file with the prevailing
definition. This is useful to track down some ODR issues.

We need to

• add uint32_t DiscardedSecIdx to Undefined for this feature.
• make ComdatGroups public and change its type to DenseMap<CachedHashStringRef, const InputFile *>

[smeenai]

Thank you!

If I'm understanding correctly, for the case where the relocation is not in the same object file as the discarded section, we'll still get the "undefined symbol" error message. I don't know if it would be possible to enhance that case to print the "relocation against discarded section" message as well. (gold does print that message in this case; it just doesn't print the additional information about the section group signature and prevailing definition.)

[ruiu]

ELF/Relocations.cpp

743

This is a big chunk of new code. Is there any way to factor out?

754–756

I think the following message would be better:

relocation refers to a symbol in a discarded section: <symbol name here>

[MaskRay]

Factor out maybeReportDiscardedComdat

[MaskRay]

In D59649#1438330, @smeenai wrote:

Thank you!

If I'm understanding correctly, for the case where the relocation is not in the same object file as the discarded section, we'll still get the "undefined symbol" error message. I don't know if it would be possible to enhance that case to print the "relocation against discarded section" message as well. (gold does print that message in this case; it just doesn't print the additional information about the section group signature and prevailing definition.)

Symbol has the File field but not the section index (st_shndx). If the relocation is not in the same object as the discarded section, we can't get the defined section easily.

[smeenai]

In D59649#1438809, @MaskRay wrote:

In D59649#1438330, @smeenai wrote:

Thank you!

If I'm understanding correctly, for the case where the relocation is not in the same object file as the discarded section, we'll still get the "undefined symbol" error message. I don't know if it would be possible to enhance that case to print the "relocation against discarded section" message as well. (gold does print that message in this case; it just doesn't print the additional information about the section group signature and prevailing definition.)

Symbol has the File field but not the section index (st_shndx). If the relocation is not in the same object as the discarded section, we can't get the defined section easily.

Could we do so via symbol table lookup? Rui said in https://bugs.llvm.org/show_bug.cgi?id=41133 that "We already insert all symbols (including ones for discarded sections) to the symbol table".

[MaskRay]

In D59649#1438837, @smeenai wrote:

In D59649#1438809, @MaskRay wrote:

In D59649#1438330, @smeenai wrote:

Thank you!

If I'm understanding correctly, for the case where the relocation is not in the same object file as the discarded section, we'll still get the "undefined symbol" error message. I don't know if it would be possible to enhance that case to print the "relocation against discarded section" message as well. (gold does print that message in this case; it just doesn't print the additional information about the section group signature and prevailing definition.)

Symbol has the File field but not the section index (st_shndx). If the relocation is not in the same object as the discarded section, we can't get the defined section easily.

Could we do so via symbol table lookup? Rui said in https://bugs.llvm.org/show_bug.cgi?id=41133 that "We already insert all symbols (including ones for discarded sections) to the symbol table".

We do insert all symbols into the symbol table (InputFiles.cpp:ObjFile<ELFT>::initializeSymbols()), however, Symbol does not encode all fields of Elf_Sym. Symbol has several derived classes, among them, only Defined keeps st_shndx, Undefined doesn't. Symbols defined in discarded COMDAT sections are represented by Undefined so the section index is lost. I think I can probably add SectionBase *Sec to Undefined so that we do not have the limitation.

[MaskRay]

generalize to SHF_EXCLUDE and other discarded sections

[MaskRay]

.

[MaskRay]

Binding != STB_WEAK

[MaskRay]

.

[grimar]

ELF/Driver.cpp

1296 ↗

(On Diff #191830)

I would use something like 0 /*DiscardedSecIdx*/ because it is not clear what is 0.
(here and below)

ELF/Relocations.cpp

682

Early return?

684

Use lookup?

691

I.e. this code seems a bit better looking as:

// If the discarded section is a COMDAT.
Elf_Shdr_Impl<ELFT> ELFSec = ObjSections[Sym.DiscardedSecIdx - 1];
if (ELFSec.sh_type != SHT_GROUP)
  return Msg;

StringRef Signature = File->getShtGroupSignature(ObjSections, ELFSec);
if (const InputFile *InFile =
        Symtab->ComdatGroups.lookup(CachedHashStringRef(Signature)))
  Msg += "\n>>> section group signature: " + Signature.str() +
         "\n>>> prevailing definition is in " + toString(InFile);
return Msg;

711–722

If you'll do -> std::string here, then ...

727

... you can remove .str() here.

test/ELF/exclude-discarded-error2.s

9 ↗

(On Diff #191830)

What about testing the full error message explicitly here?

[MaskRay]

Address grimar's comments

[MaskRay]

ELF/Relocations.cpp

684

Thank you! I didn't know the useful lookup...

[MaskRay]

0 -> /*DiscardedSecIdx=*/0

[grimar]

I have no more comments, thanks!

ELF/Driver.cpp

1296 ↗

(On Diff #191830)

Side note: Hmmm. We are not consistent. We use both /*DiscardedSecIdx*/ and /*DiscardedSecIdx=*/ forms across LLD.

[MaskRay]

/*DiscardedSecIdx*/ 0 -> /*DiscardedSecIdx=*/0

[MaskRay]

ELF/Driver.cpp

1296 ↗

(On Diff #191830)

Missed that case. Fixed!

[smeenai]

Thank you! This does even better than gold now :)

[MaskRay]

Ping :)

[MaskRay]

Ping :) I had come across an ODR violation case (which took me quite a while to figure out the cause) before I noticed smeenai's report so I believe this improved error message will be helpful.

[ruiu]

I'm honestly not very happy about the plumbing work in this patch, as the new value, DiscardedSecIdx, is not semantically required but handed over to many functions just to get to the location where the error message is generated.

I wonder if we can do this without that variable. When we need that variable, we are generating an error message, so the code doesn't have to be fast. I.e. you can scan through object file's symbol table to find an undefined symbol, for example. What do you think?

[MaskRay]

I wonder if we can do this without that variable. ... you can scan through object file's symbol table to find an undefined symbol, for example. What do you think?

I have thought about this option. I believe that will make code slower and more complex.

We use Undefined for two purposes: 1) undefined symbol 2) symbol in a discarded section (has only one occurrence: InputFiles.cpp: ObjFile<ELFT>::createSymbol). (I added some comments in this patch).

If we don't store Undefined::DiscardedSecIdx, we can iterate the symbol table of Symbol::File to figure out the section index and then check if the section is discarded. That would repeat some work done by ObjFile<ELFT>::createSymbol. Because we may emit many maybeReportUndefined errors, we don't want symbol table iterating to be too slow. We may end up with some cache. I think that is more complex than an extra DiscardedSecIdx.

If /*DiscardedSecIdx=0*/ looks too messy, it can be made a default argument.
That is simpler than having two member functions SymbolTable::addSymbolInDiscarded and SymbolTable::addUndefined.

[MaskRay]

explict /*DiscardedSecIdx=0*/ -> default argument

[MaskRay]

Does the default argument DiscardedSecIdx look good?

[MaskRay]

Remove template parameter class RelTy

[MaskRay]

Rebase after recent SymbolTable changes. Combined with D61583, we will give good warnings for PR41693

[MaskRay]

🏓 Ping:)

[ruiu]

Overall looking good.

Somewhat unrelated, but maybe we should clear ComdatGroups map once name resolution pass is done? COMDAT signatures are long strings, and discarding them from memory might save nontrivial amount of memory.

ELF/InputFiles.cpp

80–81

Why did you move this to Symtab?

[MaskRay]

In D59649#1511494, @ruiu wrote:

Overall looking good.

Somewhat unrelated, but maybe we should clear ComdatGroups map once name resolution pass is done? COMDAT signatures are long strings, and discarding them from memory might save nontrivial amount of memory.

Good point. I can do that in a separate change.

ELF/InputFiles.cpp

80–81

Relocations.cpp needs to access the variable to check which section is prevailing.

It was originally in Symtab until you moved it to a function-local static variable.. It can also be a global variable if you prefer.

[ruiu]

LGTM

[MaskRay]

lld/trunk/ELF/Relocations.cpp

688 ↗

(On Diff #200668)

We may improve the search for SHT_GROUP. Probably some validation is required. Sym.DiscardedSecIdx - 1 may not find the belonged SHT_GROUP...

ELF/InputFiles.h

template <class ELFT> class ObjFile : public ELFFileBase<ELFT> {
   typedef ELFFileBase<ELFT> Base;
   typedef typename ELFT::Rel Elf_Rel;
   typedef typename ELFT::Rela Elf_Rela;
   typedef typename ELFT::Sym Elf_Sym;
   typedef typename ELFT::Shdr Elf_Shdr;
   typedef typename ELFT::Word Elf_Word;
   typedef typename ELFT::CGProfile Elf_CGProfile;
 
-  StringRef getShtGroupSignature(ArrayRef<Elf_Shdr> Sections,
-                                 const Elf_Shdr &Sec);
-
 public:
   static bool classof(const InputFile *F) { return F->kind() == Base::ObjKind; }
 
+  StringRef getShtGroupSignature(ArrayRef<Elf_Shdr> Sections,
+                                 const Elf_Shdr &Sec);
+
   ArrayRef<Symbol *> getLocalSymbols();
   ArrayRef<Symbol *> getGlobalSymbols();
 
   ObjFile(MemoryBufferRef M, StringRef ArchiveName);
-  void parse(llvm::DenseSet<llvm::CachedHashStringRef> &ComdatGroups);
+  void parse(llvm::DenseMap<llvm::CachedHashStringRef, const InputFile *>
+                 &ComdatGroups);
 
   Symbol &getSymbol(uint32_t SymbolIndex) const {
     if (SymbolIndex >= this->Symbols.size())
       fatal(toString(this) + ": invalid symbol index");
     return *this->Symbols[SymbolIndex];
   }
 
   template <typename RelT> Symbol &getRelocTargetSym(const RelT &Rel) const {
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::DenseSet<llvm::CachedHashStringRef> &ComdatGroups);
+  void initializeSections(llvm::DenseMap<llvm::CachedHashStringRef,
+                                         const InputFile *> &ComdatGroups);
   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);

 };
 
 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::DenseSet<llvm::CachedHashStringRef> &ComdatGroups);
+  void parse(llvm::DenseMap<llvm::CachedHashStringRef, const InputFile *>
+                 &ComdatGroups);
   std::unique_ptr<llvm::lto::InputFile> Obj;
 };
 
 // .so file.
 template <class ELFT> class SharedFile : public ELFFileBase<ELFT> {
   typedef ELFFileBase<ELFT> Base;
   typedef typename ELFT::Dyn Elf_Dyn;
   typedef typename ELFT::Shdr Elf_Shdr;

ELF/InputFiles.cpp

Optional<MemoryBufferRef> elf::readFile(StringRef Path) {
   std::unique_ptr<MemoryBuffer> &MB = *MBOrErr;
   MemoryBufferRef MBRef = MB->getMemBufferRef();
   make<std::unique_ptr<MemoryBuffer>>(std::move(MB)); // take MB ownership
 
   if (Tar)
     Tar->append(relativeToRoot(Path), MBRef.getBuffer());
   return MBRef;
 }
 
 // Concatenates arguments to construct a string representing an error location.
 static std::string createFileLineMsg(StringRef Path, unsigned Line) {
   std::string Filename = path::filename(Path);
   std::string Lineno = ":" + std::to_string(Line);
   if (Filename == Path)
     return Filename + Lineno;
   return Filename + Lineno + " (" + Path.str() + Lineno + ")";
 }
 
template <class ELFT> ArrayRef<Symbol *> ObjFile<ELFT>::getLocalSymbols() {
   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(DenseSet<CachedHashStringRef> &ComdatGroups) {
+void ObjFile<ELFT>::parse(
+    DenseMap<CachedHashStringRef, const InputFile *> &ComdatGroups) {
   // Read a section table. JustSymbols is usually false.
   if (this->JustSymbols)
     initializeJustSymbols();
   else
     initializeSections(ComdatGroups);
 
   // Read a symbol table.
   initializeSymbols();
if (Sec.sh_type != SHT_SYMTAB)
       continue;
     this->initSymtab(ObjSections, &Sec);
     return;
   }
 }
 
 template <class ELFT>
 void ObjFile<ELFT>::initializeSections(
-    DenseSet<CachedHashStringRef> &ComdatGroups) {
+    DenseMap<CachedHashStringRef, const InputFile *> &ComdatGroups) {
   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 = ComdatGroups.insert(CachedHashStringRef(Signature)).second;
+      bool IsNew =
+          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->addCommon(Name, ObjSym.getCommonSize(),
                              ObjSym.getCommonAlignment(), Binding, Visibility,
                              STT_OBJECT, F);
 
   return Symtab->addBitcode(Name, Binding, Visibility, Type, CanOmitFromDynSym,
                             F);
 }
 
 template <class ELFT>
-void BitcodeFile::parse(DenseSet<CachedHashStringRef> &ComdatGroups) {
+void BitcodeFile::parse(
+    DenseMap<CachedHashStringRef, const InputFile *> &ComdatGroups) {
   std::vector<bool> KeptComdats;
   for (StringRef S : Obj->getComdatTable())
-    KeptComdats.push_back(ComdatGroups.insert(CachedHashStringRef(S)).second);
+    KeptComdats.push_back(
+        ComdatGroups.try_emplace(CachedHashStringRef(S), this).second);
 
   for (const lto::InputFile::Symbol &ObjSym : Obj->symbols())
     Symbols.push_back(createBitcodeSymbol<ELFT>(KeptComdats, ObjSym, *this));
 }
 
 static ELFKind getELFKind(MemoryBufferRef MB, StringRef ArchiveName) {
   unsigned char Size;
   unsigned char Endian;
std::string elf::replaceThinLTOSuffix(StringRef Path) {
   return Path;
 }
 
 template void ArchiveFile::parse<ELF32LE>();
 template void ArchiveFile::parse<ELF32BE>();
 template void ArchiveFile::parse<ELF64LE>();
 template void ArchiveFile::parse<ELF64BE>();
 
-template void BitcodeFile::parse<ELF32LE>(DenseSet<CachedHashStringRef> &);
-template void BitcodeFile::parse<ELF32BE>(DenseSet<CachedHashStringRef> &);
-template void BitcodeFile::parse<ELF64LE>(DenseSet<CachedHashStringRef> &);
-template void BitcodeFile::parse<ELF64BE>(DenseSet<CachedHashStringRef> &);
+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 LazyObjFile::parse<ELF32LE>();
 template void LazyObjFile::parse<ELF32BE>();
 template void LazyObjFile::parse<ELF64LE>();
 template void LazyObjFile::parse<ELF64BE>();
 
 template class elf::ELFFileBase<ELF32LE>;
 template class elf::ELFFileBase<ELF32BE>;

ELF/Relocations.cpp

static int64_t computeAddend(const RelTy &Rel, const RelTy *End,
   if (Config->EMachine == EM_PPC64 && Config->Pic && Type == R_PPC64_TOC)
     Addend += getPPC64TocBase();
   if (Config->EMachine == EM_MIPS)
     Addend += computeMipsAddend<ELFT>(Rel, End, Sec, Expr, IsLocal);
 
   return Addend;
 }
 
+// If Rel is in the same file where Sym was created, we know its st_shndx and
+// can thus check if sh_shndx refers to a discarded COMDAT section. For that
+// case, Sym is defined but created as Undefined to catch errors.
+template <class ELFT, class RelTy>
+static std::string maybeReportDiscardedComdat(Symbol &Sym,
+                                              InputSectionBase &Sec,
+                                              const RelTy &Rel) {
+  ObjFile<ELFT> *File = Sec.getFile<ELFT>();
+  if (File != Sym.File)
+    return "";
+  uint32_t SecIdx = File->getSectionIndex(
+      File->getELFSyms()[Rel.getSymbol(Config->IsMips64EL)]);
+  if (File->getSections()[SecIdx] != &InputSection::Discarded)
+    return "";
+  ArrayRef<Elf_Shdr_Impl<ELFT>> ObjSections =
+      CHECK(File->getObj().sections(), File);
+  Elf_Shdr_Impl<ELFT> ELFSec = ObjSections[SecIdx - 1];
+  if (ELFSec.sh_type != SHT_GROUP)
+    return "";
+
+  StringRef Signature = File->getShtGroupSignature(ObjSections, ELFSec);
+  auto It = Symtab->ComdatGroups.find(CachedHashStringRef(Signature));
+  if (It == Symtab->ComdatGroups.end())
+    return "";
+  return ("relocation refers to a symbol in a discarded section: " +
+          toString(Sym) + "\n>>> section group signature: " + Signature +
+          "\n>>> defined in " + toString(File) +
+          "\n>>> prevailing definition is in " + toString(It->second) +
+          "\n>>> referenced by ")
+      .str();
+}
+
 // Report an undefined symbol if necessary.
 // Returns true if this function printed out an error message.
+template <class ELFT, class RelTy>
 static bool maybeReportUndefined(Symbol &Sym, InputSectionBase &Sec,
-                                 uint64_t Offset) {
+                                 const RelTy &Rel) {
   if (Sym.isLocal() || !Sym.isUndefined() || Sym.isWeak())
     return false;
 
   bool CanBeExternal =
       Sym.computeBinding() != STB_LOCAL && Sym.Visibility == STV_DEFAULT;
   if (Config->UnresolvedSymbols == UnresolvedPolicy::Ignore && CanBeExternal)
     return false;
 
-  std::string Msg = "undefined ";
-  if (Sym.Visibility == STV_INTERNAL)
-    Msg += "internal ";
-  else if (Sym.Visibility == STV_HIDDEN)
-    Msg += "hidden ";
-  else if (Sym.Visibility == STV_PROTECTED)
-    Msg += "protected ";
-  Msg += "symbol: " + toString(Sym) + "\n>>> referenced by ";
+  auto Visibility = [&]() -> StringRef {
+    switch (Sym.Visibility) {
+    case STV_INTERNAL:
+      return "internal ";
+    case STV_HIDDEN:
+      return "hidden ";
+    case STV_PROTECTED:
+      return "protected ";
+    default:
+      return "";
+    }
+  };
+
+  std::string Msg = maybeReportDiscardedComdat<ELFT, RelTy>(Sym, Sec, Rel);
+  if (Msg.empty())
+    Msg = ("undefined " + Visibility() + "symbol: " + toString(Sym) +
+           "\n>>> referenced by ")
+              .str();
 
-  std::string Src = Sec.getSrcMsg(Sym, Offset);
+  std::string Src = Sec.getSrcMsg(Sym, Rel.r_offset);
   if (!Src.empty())
     Msg += Src + "\n>>>               ";
-  Msg += Sec.getObjMsg(Offset);
+  Msg += Sec.getObjMsg(Rel.r_offset);
 
   if (Sym.getName().startswith("_ZTV"))
     Msg += "\nthe vtable symbol may be undefined because the class is missing "
            "its key function (see https://lld.llvm.org/missingkeyfunction)";
 
   if ((Config->UnresolvedSymbols == UnresolvedPolicy::Warn && CanBeExternal) ||
       Config->NoinhibitExec) {
     warn(Msg);
     return false;
   }
 
   error(Msg);
   return true;
 }
 
 // MIPS N32 ABI treats series of successive relocations with the same offset
 // as a single relocation. The similar approach used by N64 ABI, but this ABI
 // packs all relocations into the single relocation record. Here we emulate
 // this for the N32 ABI. Iterate over relocation with the same offset and put
 // theirs types into the single bit-set.
 template <class RelTy> static RelType getMipsN32RelType(RelTy *&Rel, RelTy *End) {
   RelType Type = 0;
   uint64_t Offset = Rel->r_offset;
 
   int N = 0;
   while (Rel != End && Rel->r_offset == Offset)
     Type |= (Rel++)->getType(Config->IsMips64EL) << (8 * N++);
   return Type;
 }
 
 // .eh_frame sections are mergeable input sections, so their input
static void scanReloc(InputSectionBase &Sec, OffsetGetter &GetOffset, RelTy *&I,
   }
 
   // Get an offset in an output section this relocation is applied to.
   uint64_t Offset = GetOffset.get(Rel.r_offset);
   if (Offset == uint64_t(-1))
     return;
 
   // Skip if the target symbol is an erroneous undefined symbol.
-  if (maybeReportUndefined(Sym, Sec, Rel.r_offset))
+  if (maybeReportUndefined<ELFT, RelTy>(Sym, Sec, Rel))
     return;
 
   const uint8_t *RelocatedAddr = Sec.data().begin() + Rel.r_offset;
   RelExpr Expr = Target->getRelExpr(Type, Sym, RelocatedAddr);
 
   // Ignore "hint" relocations because they are only markers for relaxation.
   if (oneof<R_HINT, R_NONE>(Expr))
     return;

ELF/SymbolTable.h

public:
 
   void trace(StringRef Name);
 
   void handleDynamicList();
 
   // Set of .so files to not link the same shared object file more than once.
   llvm::DenseMap<StringRef, InputFile *> SoNames;
 
+  // Comdat groups define "link once" sections. If two comdat groups have the
+  // same name, only one of them is linked, and the other is ignored. This map
+  // is used to uniquify them.
+  llvm::DenseMap<llvm::CachedHashStringRef, const InputFile *> ComdatGroups;
+
 private:
   std::pair<Symbol *, bool> insertName(StringRef Name);
 
   std::vector<Symbol *> findByVersion(SymbolVersion Ver);
   std::vector<Symbol *> findAllByVersion(SymbolVersion Ver);
 
   llvm::StringMap<std::vector<Symbol *>> &getDemangledSyms();
   void handleAnonymousVersion();
   void assignExactVersion(SymbolVersion Ver, uint16_t VersionId,
                           StringRef VersionName);
   void assignWildcardVersion(SymbolVersion Ver, uint16_t VersionId);
 
   // The order the global symbols are in is not defined. We can use an arbitrary
   // order, but it has to be reproducible. That is true even when cross linking.
   // The default hashing of StringRef produces different results on 32 and 64
   // bit systems so we use a map to a vector. That is arbitrary, deterministic
   // but a bit inefficient.
   // FIXME: Experiment with passing in a custom hashing or sorting the symbols
   // once symbol resolution is finished.
   llvm::DenseMap<llvm::CachedHashStringRef, int> SymMap;
   std::vector<Symbol *> SymVector;
 
-  // Comdat groups define "link once" sections. If two comdat groups have the
-  // same name, only one of them is linked, and the other is ignored. This set
-  // is used to uniquify them.
-  llvm::DenseSet<llvm::CachedHashStringRef> ComdatGroups;
-
   // A map from demangled symbol names to their symbol objects.
   // This mapping is 1:N because two symbols with different versions
   // can have the same name. We use this map to handle "extern C++ {}"
   // directive in version scripts.
   llvm::Optional<llvm::StringMap<std::vector<Symbol *>>> DemangledSyms;
 
   // For LTO.
   std::unique_ptr<BitcodeCompiler> LTO;
 };
 
 extern SymbolTable *Symtab;
 } // namespace elf
 } // namespace lld
 
 #endif

ELF/SymbolTable.cpp

if (BitcodeFiles.empty())
     return;
 
   // Compile bitcode files and replace bitcode symbols.
   LTO.reset(new BitcodeCompiler);
   for (BitcodeFile *F : BitcodeFiles)
     LTO->add(*F);
 
   for (InputFile *File : LTO->compile()) {
-    DenseSet<CachedHashStringRef> DummyGroups;
+    DenseMap<CachedHashStringRef, const InputFile *> DummyGroups;
     auto *Obj = cast<ObjFile<ELFT>>(File);
     Obj->parse(DummyGroups);
     for (Symbol *Sym : Obj->getGlobalSymbols())
       Sym->parseSymbolVersion();
     ObjectFiles.push_back(File);
   }
 }
 

test/ELF/Inputs/comdat-discarded-error.s

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

test/ELF/comdat-discarded-error.s

+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %S/Inputs/comdat-discarded-error.s -o %t1.o
+# RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s -o %t2.o
+# RUN: not ld.lld %t1.o %t2.o -o /dev/null 2>&1 | FileCheck %s
+
+# CHECK:      error: relocation refers to a symbol in a discarded section: bar
+# CHECK-NEXT: >>> section group signature: foo
+# CHECK-NEXT: >>> defined in {{.*}}2.o
+# CHECK-NEXT: >>> prevailing definition is in {{.*}}1.o
+# CHECK-NEXT: >>> referenced by {{.*}}2.o:(.text+0x1)
+
+.globl _start
+_start:
+  jmp bar
+
+.section .text.foo,"axG",@progbits,foo,comdat
+.globl bar
+bar: