From 2612a32ce588bfa8d363c78cf59dff90a679dfc0 Mon Sep 17 00:00:00 2001 From: Peter Collingbourne Date: Sat, 4 Jul 2015 05:28:41 +0000 Subject: [PATCH] COFF: Numerous fixes for interaction between LTO and weak externals. We were previously hitting assertion failures in the writer in cases where a regular object file defined a weak external symbol that was defined by a bitcode file. Because /export and /entry name mangling were implemented using weak externals, the same problem affected mangled symbol names in bitcode files. The underlying cause of the problem was that weak external symbols were being resolved before doing LTO, so the symbol table may have contained stale references to bitcode symbols. The fix here is to defer weak external symbol resolution until after LTO. Also implement support for weak external symbols in bitcode files by modelling them as replaceable DefinedBitcode symbols. Differential Revision: http://reviews.llvm.org/D10940 llvm-svn: 241391 --- lld/COFF/Driver.cpp | 8 ++--- lld/COFF/InputFiles.cpp | 7 ++-- lld/COFF/SymbolTable.cpp | 50 ++++++++++++-------------- lld/COFF/SymbolTable.h | 5 +-- lld/COFF/Symbols.cpp | 8 +++++ lld/COFF/Symbols.h | 5 +++ lld/test/COFF/Inputs/entry-mangled.ll | 6 ++++ lld/test/COFF/Inputs/weak-external.ll | 6 ++++ lld/test/COFF/Inputs/weak-external2.ll | 6 ++++ lld/test/COFF/Inputs/weak-external3.ll | 8 +++++ lld/test/COFF/entry-mangled.test | 2 ++ lld/test/COFF/weak-external.test | 9 ++++- lld/test/COFF/weak-external2.test | 30 ++++++++++++++++ lld/test/COFF/weak-external3.test | 32 +++++++++++++++++ 14 files changed, 146 insertions(+), 36 deletions(-) create mode 100644 lld/test/COFF/Inputs/entry-mangled.ll create mode 100644 lld/test/COFF/Inputs/weak-external.ll create mode 100644 lld/test/COFF/Inputs/weak-external2.ll create mode 100644 lld/test/COFF/Inputs/weak-external3.ll create mode 100644 lld/test/COFF/weak-external2.test create mode 100644 lld/test/COFF/weak-external3.test diff --git a/lld/COFF/Driver.cpp b/lld/COFF/Driver.cpp index ade0333a72c0..af80b4602549 100644 --- a/lld/COFF/Driver.cpp +++ b/lld/COFF/Driver.cpp @@ -594,10 +594,6 @@ bool LinkerDriver::link(llvm::ArrayRef ArgsArr) { } } - // Make sure we have resolved all symbols. - if (Symtab.reportRemainingUndefines()) - return false; - // Do LTO by compiling bitcode input files to a native COFF file // then link that file. if (auto EC = Symtab.addCombinedLTOObject()) { @@ -605,6 +601,10 @@ bool LinkerDriver::link(llvm::ArrayRef ArgsArr) { return false; } + // Make sure we have resolved all symbols. + if (Symtab.reportRemainingUndefines(/*Resolve=*/true)) + return false; + // Windows specific -- if no /subsystem is given, we need to infer // that from entry point name. if (Config->Subsystem == IMAGE_SUBSYSTEM_UNKNOWN) { diff --git a/lld/COFF/InputFiles.cpp b/lld/COFF/InputFiles.cpp index 3bb3c6f36d4e..87efd07e68c5 100644 --- a/lld/COFF/InputFiles.cpp +++ b/lld/COFF/InputFiles.cpp @@ -293,8 +293,11 @@ std::error_code BitcodeFile::parse() { if (SymbolDef == LTO_SYMBOL_DEFINITION_UNDEFINED) { SymbolBodies.push_back(new (Alloc) Undefined(SymName)); } else { - bool Replaceable = (SymbolDef == LTO_SYMBOL_DEFINITION_TENTATIVE || - (Attrs & LTO_SYMBOL_COMDAT)); + bool Replaceable = + (SymbolDef == LTO_SYMBOL_DEFINITION_TENTATIVE || // common + (Attrs & LTO_SYMBOL_COMDAT) || // comdat + (SymbolDef == LTO_SYMBOL_DEFINITION_WEAK && // weak external + (Attrs & LTO_SYMBOL_ALIAS))); SymbolBodies.push_back(new (Alloc) DefinedBitcode(this, SymName, Replaceable)); } diff --git a/lld/COFF/SymbolTable.cpp b/lld/COFF/SymbolTable.cpp index 00c0e21be873..44d943bf78e6 100644 --- a/lld/COFF/SymbolTable.cpp +++ b/lld/COFF/SymbolTable.cpp @@ -122,7 +122,7 @@ bool SymbolTable::queueEmpty() { return ArchiveQueue.empty() && ObjectQueue.empty(); } -bool SymbolTable::reportRemainingUndefines() { +bool SymbolTable::reportRemainingUndefines(bool Resolve) { bool Ret = false; for (auto &I : Symtab) { Symbol *Sym = I.second; @@ -130,20 +130,19 @@ bool SymbolTable::reportRemainingUndefines() { if (!Undef) continue; StringRef Name = Undef->getName(); - // A weak alias may have been resolved, so check for that. A weak alias - // may be a weak alias to another symbol, so check recursively. - for (SymbolBody *A = Undef->WeakAlias; A; - A = cast(A)->WeakAlias) { - if (auto *D = dyn_cast(A->repl())) { + // A weak alias may have been resolved, so check for that. + if (Defined *D = Undef->getWeakAlias()) { + if (Resolve) Sym->Body = D; - goto next; - } + continue; } // If we can resolve a symbol by removing __imp_ prefix, do that. // This odd rule is for compatibility with MSVC linker. if (Name.startswith("__imp_")) { Symbol *Imp = find(Name.substr(strlen("__imp_"))); if (Imp && isa(Imp->Body)) { + if (!Resolve) + continue; auto *D = cast(Imp->Body); auto *S = new (Alloc) DefinedLocalImport(Name, D); LocalImportChunks.push_back(S->getChunk()); @@ -155,11 +154,11 @@ bool SymbolTable::reportRemainingUndefines() { // Remaining undefined symbols are not fatal if /force is specified. // They are replaced with dummy defined symbols. if (Config->Force) { - Sym->Body = new (Alloc) DefinedAbsolute(Name, 0); + if (Resolve) + Sym->Body = new (Alloc) DefinedAbsolute(Name, 0); continue; } Ret = true; - next:; } return Ret; } @@ -299,6 +298,12 @@ std::error_code SymbolTable::addCombinedLTOObject() { if (BitcodeFiles.empty()) return std::error_code(); + // Diagnose any undefined symbols early, but do not resolve weak externals, + // as resolution breaks the invariant that each Symbol points to a unique + // SymbolBody, which we rely on to replace DefinedBitcode symbols correctly. + if (reportRemainingUndefines(/*Resolve=*/false)) + return make_error_code(LLDError::BrokenFile); + // Create an object file and add it to the symbol table by replacing any // DefinedBitcode symbols with the definitions in the object file. LTOCodeGenerator CG; @@ -310,21 +315,12 @@ std::error_code SymbolTable::addCombinedLTOObject() { for (SymbolBody *Body : Obj->getSymbols()) { if (!Body->isExternal()) continue; - // Find an existing Symbol. We should not see any new undefined symbols at - // this point. + // We should not see any new undefined symbols at this point, but we'll + // diagnose them later in reportRemainingUndefines(). StringRef Name = Body->getName(); Symbol *Sym = insert(Body); - if (Sym->Body == Body && !isa(Body)) { - llvm::errs() << "LTO: undefined symbol: " << Name << '\n'; - return make_error_code(LLDError::BrokenFile); - } if (isa(Sym->Body)) { - // The symbol should now be defined. - if (!isa(Body)) { - llvm::errs() << "LTO: undefined symbol: " << Name << '\n'; - return make_error_code(LLDError::BrokenFile); - } Sym->Body = Body; continue; } @@ -353,9 +349,6 @@ std::error_code SymbolTable::addCombinedLTOObject() { return make_error_code(LLDError::BrokenFile); } - // New runtime library symbol references may have created undefined references. - if (reportRemainingUndefines()) - return make_error_code(LLDError::BrokenFile); return std::error_code(); } @@ -375,9 +368,12 @@ ErrorOr SymbolTable::createLTOObject(LTOCodeGenerator *CG) { CG->addMustPreserveSymbol(Body->getName()); // Likewise for other symbols that must be preserved. - for (Undefined *U : Config->GCRoot) - if (isa(U->repl())) - CG->addMustPreserveSymbol(U->getName()); + for (Undefined *U : Config->GCRoot) { + if (auto *S = dyn_cast(U->repl())) + CG->addMustPreserveSymbol(S->getName()); + else if (auto *S = dyn_cast_or_null(U->getWeakAlias())) + CG->addMustPreserveSymbol(S->getName()); + } CG->setModule(BitcodeFiles[0]->releaseModule()); for (unsigned I = 1, E = BitcodeFiles.size(); I != E; ++I) diff --git a/lld/COFF/SymbolTable.h b/lld/COFF/SymbolTable.h index 0341eb1a6b2e..44c7ce4a7b45 100644 --- a/lld/COFF/SymbolTable.h +++ b/lld/COFF/SymbolTable.h @@ -46,8 +46,9 @@ public: std::error_code run(); bool queueEmpty(); - // Print an error message on undefined symbols. - bool reportRemainingUndefines(); + // Print an error message on undefined symbols. If Resolve is true, try to + // resolve any undefined symbols and update the symbol table accordingly. + bool reportRemainingUndefines(bool Resolve); // Returns a list of chunks of selected symbols. std::vector getChunks(); diff --git a/lld/COFF/Symbols.cpp b/lld/COFF/Symbols.cpp index baf60e97932d..7b82d36e2af9 100644 --- a/lld/COFF/Symbols.cpp +++ b/lld/COFF/Symbols.cpp @@ -236,5 +236,13 @@ ErrorOr> Lazy::getMember() { return std::move(Obj); } +Defined *Undefined::getWeakAlias() { + // A weak alias may be a weak alias to another symbol, so check recursively. + for (SymbolBody *A = WeakAlias; A; A = cast(A)->WeakAlias) + if (auto *D = dyn_cast(A->repl())) + return D; + return nullptr; +} + } // namespace coff } // namespace lld diff --git a/lld/COFF/Symbols.h b/lld/COFF/Symbols.h index e695c39feb3f..2c12af263ecd 100644 --- a/lld/COFF/Symbols.h +++ b/lld/COFF/Symbols.h @@ -254,6 +254,11 @@ public: // If it remains undefined, it'll be replaced with whatever the // Alias pointer points to. SymbolBody *WeakAlias = nullptr; + + // If this symbol is external weak, try to resolve it to a defined + // symbol by searching the chain of fallback symbols. Returns the symbol if + // successful, otherwise returns null. + Defined *getWeakAlias(); }; // Windows-specific classes. diff --git a/lld/test/COFF/Inputs/entry-mangled.ll b/lld/test/COFF/Inputs/entry-mangled.ll new file mode 100644 index 000000000000..b6fac214dfc1 --- /dev/null +++ b/lld/test/COFF/Inputs/entry-mangled.ll @@ -0,0 +1,6 @@ +target datalayout = "e-m:w-i64:64-f80:128-n8:16:32:64-S128" +target triple = "x86_64-pc-windows-msvc18.0.0" + +define void @"\01?main@@YAHXZ"() { + ret void +} diff --git a/lld/test/COFF/Inputs/weak-external.ll b/lld/test/COFF/Inputs/weak-external.ll new file mode 100644 index 000000000000..4775d50456c9 --- /dev/null +++ b/lld/test/COFF/Inputs/weak-external.ll @@ -0,0 +1,6 @@ +target datalayout = "e-m:w-i64:64-f80:128-n8:16:32:64-S128" +target triple = "x86_64-pc-windows-msvc" + +define void @g() { + ret void +} diff --git a/lld/test/COFF/Inputs/weak-external2.ll b/lld/test/COFF/Inputs/weak-external2.ll new file mode 100644 index 000000000000..2102c3b6a526 --- /dev/null +++ b/lld/test/COFF/Inputs/weak-external2.ll @@ -0,0 +1,6 @@ +target datalayout = "e-m:w-i64:64-f80:128-n8:16:32:64-S128" +target triple = "x86_64-pc-windows-msvc" + +define void @f() { + ret void +} diff --git a/lld/test/COFF/Inputs/weak-external3.ll b/lld/test/COFF/Inputs/weak-external3.ll new file mode 100644 index 000000000000..2aee0b774333 --- /dev/null +++ b/lld/test/COFF/Inputs/weak-external3.ll @@ -0,0 +1,8 @@ +target datalayout = "e-m:w-i64:64-f80:128-n8:16:32:64-S128" +target triple = "x86_64-pc-windows-msvc" + +@f = weak alias void()* @g + +define void @g() { + ret void +} diff --git a/lld/test/COFF/entry-mangled.test b/lld/test/COFF/entry-mangled.test index 770594093be1..8b719f1c81c7 100644 --- a/lld/test/COFF/entry-mangled.test +++ b/lld/test/COFF/entry-mangled.test @@ -1,5 +1,7 @@ # RUN: yaml2obj < %s > %t.obj # RUN: lld -flavor link2 /out:%t.exe /entry:main %t.obj +# RUN: llvm-as -o %t.lto.obj %S/Inputs/entry-mangled.ll +# RUN: lld -flavor link2 /out:%t.exe /entry:main %t.lto.obj --- header: diff --git a/lld/test/COFF/weak-external.test b/lld/test/COFF/weak-external.test index 294fecda8155..3f64483560d7 100644 --- a/lld/test/COFF/weak-external.test +++ b/lld/test/COFF/weak-external.test @@ -1,5 +1,12 @@ # RUN: yaml2obj %s > %t.obj -# RUN: lld -flavor link2 /out:%t.exe /entry:g /subsystem:console %t.obj +# RUN: llvm-as -o %t.lto.obj %S/Inputs/weak-external.ll +# RUN: lld -flavor link2 /out:%t1.exe /entry:g /subsystem:console %t.obj +# RUN: lld -flavor link2 /out:%t2.exe /entry:g /subsystem:console /lldmap:%t2.map %t.obj %t.lto.obj +# RUN: FileCheck %s < %t2.map + +# CHECK: lto-llvm{{.*}}: +# CHECK-NOT: : +# CHECK: {{ g$}} --- header: diff --git a/lld/test/COFF/weak-external2.test b/lld/test/COFF/weak-external2.test new file mode 100644 index 000000000000..f1aed8b27cec --- /dev/null +++ b/lld/test/COFF/weak-external2.test @@ -0,0 +1,30 @@ +# RUN: yaml2obj %s > %t.obj +# RUN: llvm-as -o %t.lto.obj %S/Inputs/weak-external2.ll +# RUN: lld -flavor link2 /out:%t.exe /entry:g /subsystem:console %t.obj %t.lto.obj + +--- +header: + Machine: IMAGE_FILE_MACHINE_AMD64 + Characteristics: [ ] +sections: + - Name: '.text' + Characteristics: [ IMAGE_SCN_CNT_CODE, IMAGE_SCN_MEM_EXECUTE, IMAGE_SCN_MEM_READ ] + Alignment: 16 + SectionData: 00 +symbols: + - Name: 'f' + Value: 0 + SectionNumber: 0 + SimpleType: IMAGE_SYM_TYPE_NULL + ComplexType: IMAGE_SYM_DTYPE_FUNCTION + StorageClass: IMAGE_SYM_CLASS_EXTERNAL + - Name: 'g' + Value: 0 + SectionNumber: 0 + SimpleType: IMAGE_SYM_TYPE_NULL + ComplexType: IMAGE_SYM_DTYPE_FUNCTION + StorageClass: IMAGE_SYM_CLASS_WEAK_EXTERNAL + WeakExternal: + TagIndex: 0 + Characteristics: IMAGE_WEAK_EXTERN_SEARCH_LIBRARY +... diff --git a/lld/test/COFF/weak-external3.test b/lld/test/COFF/weak-external3.test new file mode 100644 index 000000000000..da68e65daacc --- /dev/null +++ b/lld/test/COFF/weak-external3.test @@ -0,0 +1,32 @@ +# RUN: yaml2obj %s > %t.obj +# RUN: llvm-as -o %t.lto.obj %S/Inputs/weak-external3.ll +# RUN: lld -flavor link2 /out:%t1.exe /entry:f /subsystem:console /lldmap:%t1.map %t.lto.obj +# RUN: FileCheck --check-prefix=CHECK1 %s < %t1.map +# RUN: lld -flavor link2 /out:%t2.exe /entry:f /subsystem:console /lldmap:%t2.map %t.obj %t.lto.obj +# RUN: FileCheck --check-prefix=CHECK2 %s < %t2.map + +# CHECK1: lto-llvm{{.*}}: +# CHECK1-NOT: : +# CHECK1: {{ g$}} + +# CHECK2: weak-external3{{.*}}: +# CHECK2-NOT: : +# CHECK2: {{ f$}} + +--- +header: + Machine: IMAGE_FILE_MACHINE_AMD64 + Characteristics: [ ] +sections: + - Name: '.text' + Characteristics: [ IMAGE_SCN_CNT_CODE, IMAGE_SCN_MEM_EXECUTE, IMAGE_SCN_MEM_READ ] + Alignment: 16 + SectionData: 00 +symbols: + - Name: 'f' + Value: 0 + SectionNumber: 1 + SimpleType: IMAGE_SYM_TYPE_NULL + ComplexType: IMAGE_SYM_DTYPE_FUNCTION + StorageClass: IMAGE_SYM_CLASS_EXTERNAL +...