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

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

Reviewed By: ruiu

Differential Revision: https://reviews.llvm.org/D59649

llvm-svn: 361359

9 files changed, 144 insertions, 37 deletions

diff --git a/lld/ELF/InputFiles.cpp b/lld/ELF/InputFiles.cpp
index add445cad1d..6db1217e08f 100644
--- a/lld/ELF/InputFiles.cpp
+++ b/lld/ELF/InputFiles.cpp
@@ -111,11 +111,6 @@ static bool isCompatible(InputFile *File) {
 }
 
 template <class ELFT> static void doParseFile(InputFile *File) {
-  // 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.
-  static llvm::DenseSet<llvm::CachedHashStringRef> ComdatGroups;
-
   if (!isCompatible(File))
     return;
 
@@ -151,13 +146,13 @@ template <class ELFT> static void doParseFile(InputFile *File) {
   // LLVM bitcode file
   if (auto *F = dyn_cast<BitcodeFile>(File)) {
     BitcodeFiles.push_back(F);
-    F->parse<ELFT>(ComdatGroups);
+    F->parse<ELFT>(Symtab->ComdatGroups);
     return;
   }
 
   // Regular object file
   ObjectFiles.push_back(File);
-  cast<ObjFile<ELFT>>(File)->parse(ComdatGroups);
+  cast<ObjFile<ELFT>>(File)->parse(Symtab->ComdatGroups);
 }
 
 // Add symbols in File to the symbol table.
@@ -396,7 +391,8 @@ template <class ELFT> ArrayRef<Symbol *> ObjFile<ELFT>::getGlobalSymbols() {
 }
 
 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();
@@ -523,7 +519,7 @@ static void addDependentLibrary(StringRef Specifier, const InputFile *F) {
 
 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);
@@ -582,7 +578,8 @@ void ObjFile<ELFT>::initializeSections(
       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);
@@ -949,9 +946,13 @@ template <class ELFT> Symbol *ObjFile<ELFT>::createSymbol(const Elf_Sym *Sym) {
 
   StringRef Name = CHECK(Sym->getName(this->StringTable), this);
 
-  if (Sym->st_shndx == SHN_UNDEF || Sec == &InputSection::Discarded)
+  if (Sym->st_shndx == SHN_UNDEF)
     return Symtab->addSymbol(Undefined{this, Name, Binding, StOther, Type});
 
+  if (Sec == &InputSection::Discarded)
+    return Symtab->addSymbol(Undefined{this, Name, Binding, StOther, Type,
+                                       /*DiscardedSecIdx=*/SecIdx});
+
   if (Sym->st_shndx == SHN_COMMON) {
     if (Value == 0 || Value >= UINT32_MAX)
       fatal(toString(this) + ": common symbol '" + Name +
@@ -1333,10 +1334,12 @@ static Symbol *createBitcodeSymbol(const std::vector<bool> &KeptComdats,
 }
 
 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));
@@ -1504,10 +1507,14 @@ std::string elf::replaceThinLTOSuffix(StringRef Path) {
   return Path;
 }
 
-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>();
diff --git a/lld/ELF/InputFiles.h b/lld/ELF/InputFiles.h
index 2208f334b9c..f3d9241b5db 100644
--- a/lld/ELF/InputFiles.h
+++ b/lld/ELF/InputFiles.h
@@ -187,9 +187,6 @@ template <class ELFT> class ObjFile : public ELFFileBase {
   using Elf_Word = typename ELFT::Word;
   using Elf_CGProfile = typename ELFT::CGProfile;
 
-  StringRef getShtGroupSignature(ArrayRef<Elf_Shdr> Sections,
-                                 const Elf_Shdr &Sec);
-
 public:
   static bool classof(const InputFile *F) { return F->kind() == ObjKind; }
 
@@ -201,7 +198,11 @@ public:
   ArrayRef<Symbol *> getGlobalSymbols();
 
   ObjFile(MemoryBufferRef M, StringRef ArchiveName);
-  void parse(llvm::DenseSet<llvm::CachedHashStringRef> &ComdatGroups);
+  void parse(llvm::DenseMap<llvm::CachedHashStringRef, const InputFile *>
+                 &ComdatGroups);
+
+  StringRef getShtGroupSignature(ArrayRef<Elf_Shdr> Sections,
+                                 const Elf_Shdr &Sec);
 
   Symbol &getSymbol(uint32_t SymbolIndex) const {
     if (SymbolIndex >= this->Symbols.size())
@@ -244,8 +245,8 @@ public:
   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();
@@ -338,7 +339,8 @@ public:
               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;
 };
 
diff --git a/lld/ELF/Relocations.cpp b/lld/ELF/Relocations.cpp
index 59fb807d86b..12c38c70dcc 100644
--- a/lld/ELF/Relocations.cpp
+++ b/lld/ELF/Relocations.cpp
@@ -671,8 +671,36 @@ static int64_t computeAddend(const RelTy &Rel, const RelTy *End,
   return Addend;
 }
 
+// Custom error message if Sym is defined in a discarded section.
+template <class ELFT>
+static std::string maybeReportDiscarded(Undefined &Sym, InputSectionBase &Sec,
+                                        uint64_t Offset) {
+  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);
+
+  Elf_Shdr_Impl<ELFT> ELFSec = ObjSections[Sym.DiscardedSecIdx - 1];
+  if (ELFSec.sh_type != SHT_GROUP)
+    return Msg;
+
+  // If the discarded section is a COMDAT.
+  StringRef Signature = File->getShtGroupSignature(ObjSections, ELFSec);
+  if (const InputFile *Prevailing =
+          Symtab->ComdatGroups.lookup(CachedHashStringRef(Signature)))
+    Msg += "\n>>> section group signature: " + Signature.str() +
+           "\n>>> prevailing definition is in " + toString(Prevailing);
+  return Msg;
+}
+
 // Report an undefined symbol if necessary.
 // Returns true if this function printed out an error message.
+template <class ELFT>
 static bool maybeReportUndefined(Symbol &Sym, InputSectionBase &Sec,
                                  uint64_t Offset) {
   if (!Sym.isUndefined() || Sym.isWeak())
@@ -683,15 +711,25 @@ static bool maybeReportUndefined(Symbol &Sym, InputSectionBase &Sec,
   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 = [&]() -> std::string {
+    switch (Sym.Visibility) {
+    case STV_INTERNAL:
+      return "internal ";
+    case STV_HIDDEN:
+      return "hidden ";
+    case STV_PROTECTED:
+      return "protected ";
+    default:
+      return "";
+    }
+  };
 
+  std::string Msg =
+      maybeReportDiscarded<ELFT>(cast<Undefined>(Sym), Sec, Offset);
+  if (Msg.empty())
+    Msg = "undefined " + Visibility() + "symbol: " + toString(Sym);
+
+  Msg += "\n>>> referenced by ";
   std::string Src = Sec.getSrcMsg(Sym, Offset);
   if (!Src.empty())
     Msg += Src + "\n>>>               ";
@@ -1031,7 +1069,7 @@ static void scanReloc(InputSectionBase &Sec, OffsetGetter &GetOffset, RelTy *&I,
 
   // Error if the target symbol is undefined. Symbol index 0 may be used by
   // marker relocations, e.g. R_*_NONE and R_ARM_V4BX. Don't error on them.
-  if (SymIndex != 0 && maybeReportUndefined(Sym, Sec, Rel.r_offset))
+  if (SymIndex != 0 && maybeReportUndefined<ELFT>(Sym, Sec, Rel.r_offset))
     return;
 
   const uint8_t *RelocatedAddr = Sec.data().begin() + Rel.r_offset;
diff --git a/lld/ELF/SymbolTable.cpp b/lld/ELF/SymbolTable.cpp
index 7bad4f92a69..b6d1741f856 100644
--- a/lld/ELF/SymbolTable.cpp
+++ b/lld/ELF/SymbolTable.cpp
@@ -46,7 +46,7 @@ template <class ELFT> void SymbolTable::addCombinedLTOObject() {
     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())
@@ -135,7 +135,11 @@ Symbol *SymbolTable::addSymbol(const Symbol &New) {
 static void addUndefined(Symbol *Old, const Undefined &New) {
   // An undefined symbol with non default visibility must be satisfied
   // in the same DSO.
-  if (Old->isShared() && New.Visibility != STV_DEFAULT) {
+  //
+  // If this is a non-weak defined symbol in a discarded section, override the
+  // existing undefined symbol for better error message later.
+  if ((Old->isShared() && New.Visibility != STV_DEFAULT) ||
+      (Old->isUndefined() && New.Binding != STB_WEAK && New.DiscardedSecIdx)) {
     Old->replace(New);
     return;
   }
diff --git a/lld/ELF/SymbolTable.h b/lld/ELF/SymbolTable.h
index 4765cb6b782..f77d04516b9 100644
--- a/lld/ELF/SymbolTable.h
+++ b/lld/ELF/SymbolTable.h
@@ -62,6 +62,11 @@ public:
   // Set of .so files to not link the same shared object file more than once.
   llvm::DenseMap<StringRef, SharedFile *> 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::vector<Symbol *> findByVersion(SymbolVersion Ver);
   std::vector<Symbol *> findAllByVersion(SymbolVersion Ver);
diff --git a/lld/ELF/Symbols.h b/lld/ELF/Symbols.h
index 1f7dc8da8c5..940767ea09e 100644
--- a/lld/ELF/Symbols.h
+++ b/lld/ELF/Symbols.h
@@ -280,10 +280,14 @@ public:
 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 = 0)
+      : 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 {
diff --git a/lld/test/ELF/comdat-discarded-error.s b/lld/test/ELF/comdat-discarded-error.s
new file mode 100644
index 00000000000..3584783cde0
--- /dev/null
+++ b/lld/test/ELF/comdat-discarded-error.s
@@ -0,0 +1,18 @@
+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=x86_64 %s -o %t1.o
+# RUN: echo '.section .text.foo,"axG",@progbits,foo,comdat; .globl foo; foo:' | \
+# RUN:   llvm-mc -filetype=obj -triple=x86_64 - -o %t2.o
+# RUN: echo '.section .text.foo,"axG",@progbits,foo,comdat; .globl bar; bar:' | \
+# RUN:   llvm-mc -filetype=obj -triple=x86_64 - -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
diff --git a/lld/test/ELF/exclude-discarded-error.s b/lld/test/ELF/exclude-discarded-error.s
new file mode 100644
index 00000000000..f51edfe850c
--- /dev/null
+++ b/lld/test/ELF/exclude-discarded-error.s
@@ -0,0 +1,15 @@
+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=x86_64 %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:
diff --git a/lld/test/ELF/exclude-discarded-error2.s b/lld/test/ELF/exclude-discarded-error2.s
new file mode 100644
index 00000000000..5474cfe8170
--- /dev/null
+++ b/lld/test/ELF/exclude-discarded-error2.s
@@ -0,0 +1,14 @@
+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=x86_64 %s -o %t.o
+# RUN: echo '.section .foo,"ae"; .weak foo; foo:' | \
+# RUN:   llvm-mc -filetype=obj -triple=x86_64 - -o %t1.o
+# RUN: not ld.lld %t.o %t1.o -o /dev/null 2>&1 | FileCheck %s
+
+# Because foo defined in %t1.o is weak, it does not override global undefined
+# in %t.o
+# CHECK-NOT: discarded section
+# CHECK: undefined symbol: foo
+
+.globl _start
+_start:
+  jmp foo