[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

739

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

750–752

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

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;

707–718

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

723

... 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

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

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/Driver.cpp

 // created from the DSO. Otherwise, they become dangling references
 // that point to a non-existent DSO.
 template <class ELFT> static void demoteSharedSymbols() {
   for (Symbol *Sym : Symtab->getSymbols()) {
     if (auto *S = dyn_cast<SharedSymbol>(Sym)) {
       if (!S->getFile<ELFT>().IsNeeded) {
         bool Used = S->Used;
         replaceSymbol<Undefined>(S, nullptr, S->getName(), STB_WEAK, S->StOther,
-                                 S->Type);
+                                 S->Type, 0);
         S->Used = Used;
       }
     }
   }
 }
 
 // The section referred to by S is considered address-significant. Set the
 // KeepUnique flag on the section if appropriate.
if (Obj->AddrsigSec) {
       for (Symbol *S : Syms)
         markAddrsig(S);
     }
   }
 }
 
 template <class ELFT> static Symbol *addUndefined(StringRef Name) {
   return Symtab->addUndefined<ELFT>(Name, STB_GLOBAL, STV_DEFAULT, 0, false,
-                                    nullptr);
+                                    nullptr, 0);
 }
 
 // The --wrap option is a feature to rename symbols so that you can write
 // wrappers for existing functions. If you pass `-wrap=foo`, all
 // occurrences of symbol `foo` are resolved to `wrap_foo` (so, you are
 // expected to write `wrap_foo` function as a wrapper). The original
 // symbol becomes accessible as `real_foo`, so you can call that from your
 // wrapper.

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)) {
if (Binding == STB_LOCAL) {
     if (Sym->getType() == STT_FILE)
       SourceFile = CHECK(Sym->getName(this->StringTable), this);
 
     if (this->StringTable.size() <= Sym->st_name)
       fatal(toString(this) + ": invalid symbol name offset");
 
     StringRefZ Name = this->StringTable.data() + Sym->st_name;
     if (Sym->st_shndx == SHN_UNDEF)
-      return make<Undefined>(this, Name, Binding, StOther, Type);
+      return make<Undefined>(this, Name, Binding, StOther, Type, 0);
 
     return make<Defined>(this, Name, Binding, StOther, Type, Value, Size, Sec);
   }
 
   StringRef Name = CHECK(Sym->getName(this->StringTable), this);
 
   switch (Sym->st_shndx) {
   case SHN_UNDEF:
     return Symtab->addUndefined<ELFT>(Name, Binding, StOther, Type,
-                                      /*CanOmitFromDynSym=*/false, this);
+                                      /*CanOmitFromDynSym=*/false, this, 0);
   case SHN_COMMON:
     if (Value == 0 || Value >= UINT32_MAX)
       fatal(toString(this) + ": common symbol '" + Name +
             "' has invalid alignment: " + Twine(Value));
     return Symtab->addCommon(Name, Size, Value, Binding, StOther, Type, *this);
   }
 
   switch (Binding) {
   default:
     fatal(toString(this) + ": unexpected binding: " + Twine(Binding));
   case STB_GLOBAL:
   case STB_WEAK:
   case STB_GNU_UNIQUE:
-    if (Sec == &InputSection::Discarded)
+    if (Sec == &InputSection::Discarded) {
       return Symtab->addUndefined<ELFT>(Name, Binding, StOther, Type,
-                                        /*CanOmitFromDynSym=*/false, this);
+                                        /*CanOmitFromDynSym=*/false, this,
+                                        SecIdx);
+    }
     return Symtab->addDefined(Name, StOther, Type, Value, Size, Binding, Sec,
                               this);
   }
 }
 
 ArchiveFile::ArchiveFile(std::unique_ptr<Archive> &&File)
     : InputFile(ArchiveKind, File->getMemoryBufferRef()),
       File(std::move(File)) {}
for (size_t I = 0; I < Syms.size(); ++I) {
     StringRef Name = CHECK(Sym.getName(this->StringTable), this);
     if (Sym.getBinding() == STB_LOCAL) {
       warn("found local symbol '" + Name +
            "' in global part of symbol table in file " + toString(this));
       continue;
     }
 
     if (Sym.isUndefined()) {
-      Symbol *S = Symtab->addUndefined<ELFT>(Name, Sym.getBinding(),
-                                             Sym.st_other, Sym.getType(),
-                                             /*CanOmitFromDynSym=*/false, this);
+      Symbol *S = Symtab->addUndefined<ELFT>(
+          Name, Sym.getBinding(), Sym.st_other, Sym.getType(),
+          /*CanOmitFromDynSym=*/false, this, 0);
       S->ExportDynamic = true;
       continue;
     }
 
     // MIPS BFD linker puts _gp_disp symbol into DSO files and incorrectly
     // assigns VER_NDX_LOCAL to this section global symbol. Here is a
     // workaround for this bug.
     uint32_t Idx = Versyms[I] & ~VERSYM_HIDDEN;
static Symbol *createBitcodeSymbol(const std::vector<bool> &KeptComdats,
 
   uint8_t Type = ObjSym.isTLS() ? STT_TLS : STT_NOTYPE;
   uint8_t Visibility = mapVisibility(ObjSym.getVisibility());
   bool CanOmitFromDynSym = ObjSym.canBeOmittedFromSymbolTable();
 
   int C = ObjSym.getComdatIndex();
   if (C != -1 && !KeptComdats[C])
     return Symtab->addUndefined<ELFT>(Name, Binding, Visibility, Type,
-                                      CanOmitFromDynSym, &F);
+                                      CanOmitFromDynSym, &F, 0);
 
   if (ObjSym.isUndefined())
     return Symtab->addUndefined<ELFT>(Name, Binding, Visibility, Type,
-                                      CanOmitFromDynSym, &F);
+                                      CanOmitFromDynSym, &F, 0);
 
   if (ObjSym.isCommon())
     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/LTO.cpp

for (StringRef Prefix : {"__start_", "__stop_"})
         UsedStartStop.insert(S.substr(Prefix.size()));
   }
 }
 
 BitcodeCompiler::~BitcodeCompiler() = default;
 
 static void undefine(Symbol *S) {
   replaceSymbol<Undefined>(S, nullptr, S->getName(), STB_GLOBAL, STV_DEFAULT,
-                           S->Type);
+                           S->Type, 0);
 }
 
 void BitcodeCompiler::add(BitcodeFile &F) {
   lto::InputFile &Obj = *F.Obj;
   bool IsExec = !Config->Shared && !Config->Relocatable;
 
   if (Config->ThinLTOIndexOnly)
     ThinIndices.insert(Obj.getName());

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;
 }
 
+// Custom error message if Sym is defined in a discarded section.
+template <class ELFT, class RelTy>
+static std::string maybeReportDiscarded(Undefined &Sym, InputSectionBase &Sec,
+                                        const RelTy &Rel) {
+  auto *File = dyn_cast_or_null<ObjFile<ELFT>>(Sym.File);
+  if (!File || !Sym.DiscardedSecIdx ||
+      File->getSections()[Sym.DiscardedSecIdx] != &InputSection::Discarded)
+    return "";
+  ArrayRef<Elf_Shdr_Impl<ELFT>> ObjSections =
+      CHECK(File->getObj().sections(), File);
+  std::string Msg =
+      "relocation refers to a symbol in a discarded section: " + toString(Sym) +
+      "\n>>> defined in " + toString(File);
+
+  // If the discarded section is a COMDAT.
+  Elf_Shdr_Impl<ELFT> ELFSec = ObjSections[Sym.DiscardedSecIdx - 1];
+  if (ELFSec.sh_type == SHT_GROUP) {
+    StringRef Signature = File->getShtGroupSignature(ObjSections, ELFSec);
+    auto It = Symtab->ComdatGroups.find(CachedHashStringRef(Signature));
+    if (It != Symtab->ComdatGroups.end())
+      Msg += ("\n>>> section group signature: " + Signature +
+              "\n>>> prevailing definition is in " + toString(It->second))
+                 .str();
+  }
+
+  return Msg;
+}
+
 // 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 Src = Sec.getSrcMsg(Sym, Offset);
+  std::string Msg =
+      maybeReportDiscarded<ELFT, RelTy>(cast<Undefined>(Sym), Sec, Rel);
+  if (Msg.empty())
+    Msg = ("undefined " + Visibility() + "symbol: " + toString(Sym)).str();
+
+  Msg += "\n>>> referenced by ";
+  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:
   template <class ELFT> void addFile(InputFile *File);
   template <class ELFT> void addCombinedLTOObject();
   void wrap(Symbol *Sym, Symbol *Real, Symbol *Wrap);
 
   ArrayRef<Symbol *> getSymbols() const { return SymVector; }
 
   template <class ELFT>
   Symbol *addUndefined(StringRef Name, uint8_t Binding, uint8_t StOther,
-                       uint8_t Type, bool CanOmitFromDynSym, InputFile *File);
+                       uint8_t Type, bool CanOmitFromDynSym, InputFile *File,
+                       uint32_t DiscardedSecIdx);
 
   Defined *addDefined(StringRef Name, uint8_t StOther, uint8_t Type,
                       uint64_t Value, uint64_t Size, uint8_t Binding,
                       SectionBase *Section, InputFile *File);
 
   template <class ELFT>
   void addShared(StringRef Name, SharedFile<ELFT> &F,
                  const typename ELFT::Sym &Sym, uint32_t Alignment,
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);
   }
 }
 
std::pair<Symbol *, bool> SymbolTable::insert(StringRef Name,
   return {S, WasInserted};
 }
 
 static uint8_t getVisibility(uint8_t StOther) { return StOther & 3; }
 
 template <class ELFT>
 Symbol *SymbolTable::addUndefined(StringRef Name, uint8_t Binding,
                                   uint8_t StOther, uint8_t Type,
-                                  bool CanOmitFromDynSym, InputFile *File) {
+                                  bool CanOmitFromDynSym, InputFile *File,
+                                  uint32_t DiscardedSecIdx) {
   Symbol *S;
   bool WasInserted;
   uint8_t Visibility = getVisibility(StOther);
   std::tie(S, WasInserted) = insert(Name, Visibility, CanOmitFromDynSym, File);
 
   // An undefined symbol with non default visibility must be satisfied
   // in the same DSO.
-  if (WasInserted || (isa<SharedSymbol>(S) && Visibility != STV_DEFAULT)) {
-    replaceSymbol<Undefined>(S, File, Name, Binding, StOther, Type);
+  //
+  // If Name is in a discarded section, override the existing undefined symbol
+  // for better error message later.
+  if (WasInserted || (isa<SharedSymbol>(S) && Visibility != STV_DEFAULT) ||
+      (S->isUndefined() && DiscardedSecIdx)) {
+    replaceSymbol<Undefined>(S, File, Name, Binding, StOther, Type,
+                             DiscardedSecIdx);
     return S;
   }
 
   if (S->isShared() || S->isLazy() || (S->isUndefined() && Binding != STB_WEAK))
     S->Binding = Binding;
 
   if (S->isLazy()) {
     // An undefined weak will not fetch archive members. See comment on Lazy in

 }
 
 template void SymbolTable::addFile<ELF32LE>(InputFile *);
 template void SymbolTable::addFile<ELF32BE>(InputFile *);
 template void SymbolTable::addFile<ELF64LE>(InputFile *);
 template void SymbolTable::addFile<ELF64BE>(InputFile *);
 
 template Symbol *SymbolTable::addUndefined<ELF32LE>(StringRef, uint8_t, uint8_t,
-                                                    uint8_t, bool, InputFile *);
+                                                    uint8_t, bool, InputFile *,
+                                                    uint32_t);
 template Symbol *SymbolTable::addUndefined<ELF32BE>(StringRef, uint8_t, uint8_t,
-                                                    uint8_t, bool, InputFile *);
+                                                    uint8_t, bool, InputFile *,
+                                                    uint32_t);
 template Symbol *SymbolTable::addUndefined<ELF64LE>(StringRef, uint8_t, uint8_t,
-                                                    uint8_t, bool, InputFile *);
+                                                    uint8_t, bool, InputFile *,
+                                                    uint32_t);
 template Symbol *SymbolTable::addUndefined<ELF64BE>(StringRef, uint8_t, uint8_t,
-                                                    uint8_t, bool, InputFile *);
+                                                    uint8_t, bool, InputFile *,
+                                                    uint32_t);
 
 template void SymbolTable::addCombinedLTOObject<ELF32LE>();
 template void SymbolTable::addCombinedLTOObject<ELF32BE>();
 template void SymbolTable::addCombinedLTOObject<ELF64LE>();
 template void SymbolTable::addCombinedLTOObject<ELF64BE>();
 
 template void
 SymbolTable::addLazyArchive<ELF32LE>(StringRef, ArchiveFile &,

ELF/Symbols.h

public:
 
   static bool classof(const Symbol *S) { return S->isDefined(); }
 
   uint64_t Value;
   uint64_t Size;
   SectionBase *Section;
 };
 
+// Represents an undefined symbol or a defined symbol in a discarded section.
 class Undefined : public Symbol {
 public:
   Undefined(InputFile *File, StringRefZ Name, uint8_t Binding, uint8_t StOther,
-            uint8_t Type)
-      : Symbol(UndefinedKind, File, Name, Binding, StOther, Type) {}
+            uint8_t Type, uint32_t DiscardedSecIdx)
+      : Symbol(UndefinedKind, File, Name, Binding, StOther, Type),
+        DiscardedSecIdx(DiscardedSecIdx) {}
 
   static bool classof(const Symbol *S) { return S->kind() == UndefinedKind; }
+
+  // The section index if in a discarded section, 0 otherwise.
+  uint32_t DiscardedSecIdx;
 };
 
 class SharedSymbol : public Symbol {
 public:
   static bool classof(const Symbol *S) { return S->kind() == SharedKind; }
 
   SharedSymbol(InputFile &File, StringRef Name, uint8_t Binding,
                uint8_t StOther, uint8_t Type, uint64_t Value, uint64_t Size,

test/ELF/comdat-discarded-error.s

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

test/ELF/exclude-discarded-error.s

+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s -o %t.o
+# RUN: not ld.lld %t.o -o /dev/null 2>&1 | FileCheck %s
+
+# CHECK:      error: relocation refers to a symbol in a discarded section: foo
+# CHECK-NEXT: >>> defined in {{.*}}.o
+# CHECK-NEXT: >>> referenced by {{.*}}.o:(.text+0x1)
+
+.globl _start
+_start:
+  jmp foo
+
+.section .foo,"ae"
+.globl foo
+foo: