From 027ddd19fbb44cca1679de2751c6af59a651d1bb Mon Sep 17 00:00:00 2001 From: Fangrui Song Date: Wed, 22 May 2019 09:06:42 +0000 Subject: [PATCH] [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 Reviewed By: ruiu Differential Revision: https://reviews.llvm.org/D59649 llvm-svn: 361359 --- lld/ELF/InputFiles.cpp | 41 ++++++++++-------- lld/ELF/InputFiles.h | 16 +++---- lld/ELF/Relocations.cpp | 56 +++++++++++++++++++++---- lld/ELF/SymbolTable.cpp | 8 +++- lld/ELF/SymbolTable.h | 5 +++ lld/ELF/Symbols.h | 8 +++- lld/test/ELF/comdat-discarded-error.s | 18 ++++++++ lld/test/ELF/exclude-discarded-error.s | 15 +++++++ lld/test/ELF/exclude-discarded-error2.s | 14 +++++++ 9 files changed, 144 insertions(+), 37 deletions(-) create mode 100644 lld/test/ELF/comdat-discarded-error.s create mode 100644 lld/test/ELF/exclude-discarded-error.s create mode 100644 lld/test/ELF/exclude-discarded-error2.s diff --git a/lld/ELF/InputFiles.cpp b/lld/ELF/InputFiles.cpp index add445cad1d72..6db1217e08fc5 100644 --- a/lld/ELF/InputFiles.cpp +++ b/lld/ELF/InputFiles.cpp @@ -111,11 +111,6 @@ static bool isCompatible(InputFile *File) { } template 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 ComdatGroups; - if (!isCompatible(File)) return; @@ -151,13 +146,13 @@ template static void doParseFile(InputFile *File) { // LLVM bitcode file if (auto *F = dyn_cast(File)) { BitcodeFiles.push_back(F); - F->parse(ComdatGroups); + F->parse(Symtab->ComdatGroups); return; } // Regular object file ObjectFiles.push_back(File); - cast>(File)->parse(ComdatGroups); + cast>(File)->parse(Symtab->ComdatGroups); } // Add symbols in File to the symbol table. @@ -396,7 +391,8 @@ template ArrayRef ObjFile::getGlobalSymbols() { } template -void ObjFile::parse(DenseSet &ComdatGroups) { +void ObjFile::parse( + DenseMap &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 void ObjFile::initializeSections( - DenseSet &ComdatGroups) { + DenseMap &ComdatGroups) { const ELFFile &Obj = this->getObj(); ArrayRef ObjSections = CHECK(Obj.sections(), this); @@ -582,7 +578,8 @@ void ObjFile::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 Symbol *ObjFile::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 &KeptComdats, } template -void BitcodeFile::parse(DenseSet &ComdatGroups) { +void BitcodeFile::parse( + DenseMap &ComdatGroups) { std::vector 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(KeptComdats, ObjSym, *this)); @@ -1504,10 +1507,14 @@ std::string elf::replaceThinLTOSuffix(StringRef Path) { return Path; } -template void BitcodeFile::parse(DenseSet &); -template void BitcodeFile::parse(DenseSet &); -template void BitcodeFile::parse(DenseSet &); -template void BitcodeFile::parse(DenseSet &); +template void +BitcodeFile::parse(DenseMap &); +template void +BitcodeFile::parse(DenseMap &); +template void +BitcodeFile::parse(DenseMap &); +template void +BitcodeFile::parse(DenseMap &); template void LazyObjFile::parse(); template void LazyObjFile::parse(); diff --git a/lld/ELF/InputFiles.h b/lld/ELF/InputFiles.h index 2208f334b9c90..f3d9241b5db49 100644 --- a/lld/ELF/InputFiles.h +++ b/lld/ELF/InputFiles.h @@ -187,9 +187,6 @@ template class ObjFile : public ELFFileBase { using Elf_Word = typename ELFT::Word; using Elf_CGProfile = typename ELFT::CGProfile; - StringRef getShtGroupSignature(ArrayRef Sections, - const Elf_Shdr &Sec); - public: static bool classof(const InputFile *F) { return F->kind() == ObjKind; } @@ -201,7 +198,11 @@ template class ObjFile : public ELFFileBase { ArrayRef getGlobalSymbols(); ObjFile(MemoryBufferRef M, StringRef ArchiveName); - void parse(llvm::DenseSet &ComdatGroups); + void parse(llvm::DenseMap + &ComdatGroups); + + StringRef getShtGroupSignature(ArrayRef Sections, + const Elf_Shdr &Sec); Symbol &getSymbol(uint32_t SymbolIndex) const { if (SymbolIndex >= this->Symbols.size()) @@ -244,8 +245,8 @@ template class ObjFile : public ELFFileBase { ArrayRef CGProfile; private: - void - initializeSections(llvm::DenseSet &ComdatGroups); + void initializeSections(llvm::DenseMap &ComdatGroups); void initializeSymbols(); void initializeJustSymbols(); void initializeDwarf(); @@ -338,7 +339,8 @@ class BitcodeFile : public InputFile { uint64_t OffsetInArchive); static bool classof(const InputFile *F) { return F->kind() == BitcodeKind; } template - void parse(llvm::DenseSet &ComdatGroups); + void parse(llvm::DenseMap + &ComdatGroups); std::unique_ptr Obj; }; diff --git a/lld/ELF/Relocations.cpp b/lld/ELF/Relocations.cpp index 59fb807d86ba5..12c38c70dcc41 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 +static std::string maybeReportDiscarded(Undefined &Sym, InputSectionBase &Sec, + uint64_t Offset) { + auto *File = dyn_cast_or_null>(Sym.File); + if (!File || !Sym.DiscardedSecIdx || + File->getSections()[Sym.DiscardedSecIdx] != &InputSection::Discarded) + return ""; + ArrayRef> 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 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 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(cast(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(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 7bad4f92a6965..b6d1741f856b2 100644 --- a/lld/ELF/SymbolTable.cpp +++ b/lld/ELF/SymbolTable.cpp @@ -46,7 +46,7 @@ template void SymbolTable::addCombinedLTOObject() { LTO->add(*F); for (InputFile *File : LTO->compile()) { - DenseSet DummyGroups; + DenseMap DummyGroups; auto *Obj = cast>(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 4765cb6b782e7..f77d04516b901 100644 --- a/lld/ELF/SymbolTable.h +++ b/lld/ELF/SymbolTable.h @@ -62,6 +62,11 @@ class SymbolTable { // Set of .so files to not link the same shared object file more than once. llvm::DenseMap 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 ComdatGroups; + private: std::vector findByVersion(SymbolVersion Ver); std::vector findAllByVersion(SymbolVersion Ver); diff --git a/lld/ELF/Symbols.h b/lld/ELF/Symbols.h index 1f7dc8da8c58a..940767ea09e0c 100644 --- a/lld/ELF/Symbols.h +++ b/lld/ELF/Symbols.h @@ -280,10 +280,14 @@ class CommonSymbol : public Symbol { 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 0000000000000..3584783cde09d --- /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 0000000000000..f51edfe850c84 --- /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 0000000000000..5474cfe8170fe --- /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