From d912be98f8ebfa99ab9fa2d985d3f4e9cddf2df6 Mon Sep 17 00:00:00 2001 From: Rafael Espindola Date: Wed, 16 Dec 2015 23:16:33 +0000 Subject: [PATCH] Change linkInModule to take a std::unique_ptr. Passing in a std::unique_ptr should help find errors when the module is used after being linked into another module. git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@255842 91177308-0d34-0410-b5e6-96231b3b80d8 --- bindings/go/llvm/linker.go | 6 ++-- bindings/ocaml/linker/linker_ocaml.c | 6 ++-- bindings/ocaml/linker/llvm_linker.ml | 4 +-- bindings/ocaml/linker/llvm_linker.mli | 6 ++-- docs/ReleaseNotes.rst | 11 +++++++ include/llvm-c/Linker.h | 22 ++++++++++---- include/llvm/Linker/Linker.h | 13 +++++--- lib/LTO/LTOCodeGenerator.cpp | 2 +- lib/Linker/LinkModules.cpp | 28 ++++++++++++----- lib/Transforms/IPO/FunctionImport.cpp | 35 +++++++++++++-------- test/Bindings/OCaml/linker.ml | 10 +++--- tools/bugpoint/BugDriver.cpp | 2 +- tools/bugpoint/Miscompilation.cpp | 9 +++--- tools/llvm-link/llvm-link.cpp | 4 +-- unittests/Linker/LinkModulesTest.cpp | 44 ++++++++++++++++++++++----- 15 files changed, 136 insertions(+), 66 deletions(-) diff --git a/bindings/go/llvm/linker.go b/bindings/go/llvm/linker.go index f64f66c858e..63979c2f5ac 100644 --- a/bindings/go/llvm/linker.go +++ b/bindings/go/llvm/linker.go @@ -21,11 +21,9 @@ import "C" import "errors" func LinkModules(Dest, Src Module) error { - var cmsg *C.char - failed := C.LLVMLinkModules(Dest.C, Src.C, C.LLVMLinkerDestroySource, &cmsg) + failed := C.LLVMLinkModules2(Dest.C, Src.C) if failed != 0 { - err := errors.New(C.GoString(cmsg)) - C.LLVMDisposeMessage(cmsg) + err := errors.New("Linking failed") return err } return nil diff --git a/bindings/ocaml/linker/linker_ocaml.c b/bindings/ocaml/linker/linker_ocaml.c index 3b8512aa595..498a5f0c845 100644 --- a/bindings/ocaml/linker/linker_ocaml.c +++ b/bindings/ocaml/linker/linker_ocaml.c @@ -25,10 +25,8 @@ void llvm_raise(value Prototype, char *Message); /* llmodule -> llmodule -> unit */ CAMLprim value llvm_link_modules(LLVMModuleRef Dst, LLVMModuleRef Src) { - char* Message; - - if (LLVMLinkModules(Dst, Src, 0, &Message)) - llvm_raise(*caml_named_value("Llvm_linker.Error"), Message); + if (LLVMLinkModules2(Dst, Src)) + llvm_raise(*caml_named_value("Llvm_linker.Error"), "Linking failed"); return Val_unit; } diff --git a/bindings/ocaml/linker/llvm_linker.ml b/bindings/ocaml/linker/llvm_linker.ml index 3044abd8b6c..f2b64eeee91 100644 --- a/bindings/ocaml/linker/llvm_linker.ml +++ b/bindings/ocaml/linker/llvm_linker.ml @@ -11,5 +11,5 @@ exception Error of string let () = Callback.register_exception "Llvm_linker.Error" (Error "") -external link_modules : Llvm.llmodule -> Llvm.llmodule -> unit - = "llvm_link_modules" +external link_modules' : Llvm.llmodule -> Llvm.llmodule -> unit + = "llvm_link_modules" diff --git a/bindings/ocaml/linker/llvm_linker.mli b/bindings/ocaml/linker/llvm_linker.mli index 06c3b92a577..5f558ffb116 100644 --- a/bindings/ocaml/linker/llvm_linker.mli +++ b/bindings/ocaml/linker/llvm_linker.mli @@ -14,6 +14,6 @@ exception Error of string -(** [link_modules dst src mode] links [src] into [dst], raising [Error] - if the linking fails. *) -val link_modules : Llvm.llmodule -> Llvm.llmodule -> unit \ No newline at end of file +(** [link_modules' dst src] links [src] into [dst], raising [Error] + if the linking fails. The src module is destroyed. *) +val link_modules' : Llvm.llmodule -> Llvm.llmodule -> unit \ No newline at end of file diff --git a/docs/ReleaseNotes.rst b/docs/ReleaseNotes.rst index ba7e436fb0e..b84c80777c9 100644 --- a/docs/ReleaseNotes.rst +++ b/docs/ReleaseNotes.rst @@ -41,6 +41,14 @@ Non-comprehensive list of changes in this release in the 3.9 release. Please migrate to using CMake. For more information see: `Building LLVM with CMake `_ +* The C API function LLVMLinkModules is deprecated. It will be removed in the + 3.9 release. Please migrate to LLVMLinkModules2. Unlike the old function the + new one + + * Doesn't take an unused parameter. + * Destroys the source instead of only damaging it. + * Does not record a message. Use the diagnostic handler instead. + .. NOTE For small 1-3 sentence descriptions, just add an entry at the end of this list. If your description won't fit comfortably in one bullet @@ -83,6 +91,9 @@ Changes to the OCaml bindings During this release ... +* The ocaml function link_modules has been replaced with link_modules' which + uses LLVMLinkModules2. + External Open Source Projects Using LLVM 3.8 ============================================ diff --git a/include/llvm-c/Linker.h b/include/llvm-c/Linker.h index 9f98a3342d0..94b65886d35 100644 --- a/include/llvm-c/Linker.h +++ b/include/llvm-c/Linker.h @@ -27,17 +27,27 @@ typedef enum { should not be used. */ } LLVMLinkerMode; -/* Links the source module into the destination module, taking ownership - * of the source module away from the caller. Optionally returns a - * human-readable description of any errors that occurred in linking. - * OutMessage must be disposed with LLVMDisposeMessage. The return value - * is true if an error occurred, false otherwise. +/* Links the source module into the destination module. The source module is + * damaged. The only thing that can be done is destroy it. Optionally returns a + * human-readable description of any errors that occurred in linking. OutMessage + * must be disposed with LLVMDisposeMessage. The return value is true if an + * error occurred, false otherwise. * * Note that the linker mode parameter \p Unused is no longer used, and has - * no effect. */ + * no effect. + * + * This function is deprecated. Use LLVMLinkModules2 instead. + */ LLVMBool LLVMLinkModules(LLVMModuleRef Dest, LLVMModuleRef Src, LLVMLinkerMode Unused, char **OutMessage); +/* Links the source module into the destination module. The source module is + * destroyed. + * The return value is true if an error occurred, false otherwise. + * Use the diagnostic handler to get any diagnostic message. +*/ +LLVMBool LLVMLinkModules2(LLVMModuleRef Dest, LLVMModuleRef Src); + #ifdef __cplusplus } #endif diff --git a/include/llvm/Linker/Linker.h b/include/llvm/Linker/Linker.h index 26c27c3aae8..eca0e9321a1 100644 --- a/include/llvm/Linker/Linker.h +++ b/include/llvm/Linker/Linker.h @@ -35,7 +35,7 @@ public: Linker(Module &M); - /// \brief Link \p Src into the composite. The source is destroyed. + /// \brief Link \p Src into the composite. /// /// Passing OverrideSymbols as true will have symbols from Src /// shadow those in the Dest. @@ -44,18 +44,21 @@ public: /// are part of the set will be imported from the source module. /// /// Returns true on error. - bool linkInModule(Module &Src, unsigned Flags = Flags::None, + bool linkInModule(std::unique_ptr Src, unsigned Flags = Flags::None, const FunctionInfoIndex *Index = nullptr, DenseSet *FunctionsToImport = nullptr); - static bool linkModules(Module &Dest, Module &Src, - unsigned Flags = Flags::None); + /// This exists to implement the deprecated LLVMLinkModules C api. Don't use + /// for anything else. + bool linkInModuleForCAPI(Module &Src); + static bool linkModules(Module &Dest, std::unique_ptr Src, + unsigned Flags = Flags::None); }; /// Create a new module with exported local functions renamed and promoted /// for ThinLTO. -std::unique_ptr renameModuleForThinLTO(std::unique_ptr &M, +std::unique_ptr renameModuleForThinLTO(std::unique_ptr M, const FunctionInfoIndex *Index); } // End llvm namespace diff --git a/lib/LTO/LTOCodeGenerator.cpp b/lib/LTO/LTOCodeGenerator.cpp index 525ca37c2f1..e51366c2930 100644 --- a/lib/LTO/LTOCodeGenerator.cpp +++ b/lib/LTO/LTOCodeGenerator.cpp @@ -106,7 +106,7 @@ bool LTOCodeGenerator::addModule(LTOModule *Mod) { assert(&Mod->getModule().getContext() == &Context && "Expected module in same context"); - bool ret = IRLinker->linkInModule(Mod->getModule()); + bool ret = IRLinker->linkInModule(Mod->takeModule()); const std::vector &undefs = Mod->getAsmUndefinedRefs(); for (int i = 0, e = undefs.size(); i != e; ++i) diff --git a/lib/Linker/LinkModules.cpp b/lib/Linker/LinkModules.cpp index 44b93696be1..1a10796a605 100644 --- a/lib/Linker/LinkModules.cpp +++ b/lib/Linker/LinkModules.cpp @@ -789,10 +789,15 @@ bool ModuleLinker::run() { Linker::Linker(Module &M) : Mover(M) {} -bool Linker::linkInModule(Module &Src, unsigned Flags, +bool Linker::linkInModule(std::unique_ptr Src, unsigned Flags, const FunctionInfoIndex *Index, DenseSet *FunctionsToImport) { - ModuleLinker TheLinker(Mover, Src, Flags, Index, FunctionsToImport); + ModuleLinker TheLinker(Mover, *Src, Flags, Index, FunctionsToImport); + return TheLinker.run(); +} + +bool Linker::linkInModuleForCAPI(Module &Src) { + ModuleLinker TheLinker(Mover, Src, 0, nullptr, nullptr); return TheLinker.run(); } @@ -805,18 +810,19 @@ bool Linker::linkInModule(Module &Src, unsigned Flags, /// true is returned and ErrorMsg (if not null) is set to indicate the problem. /// Upon failure, the Dest module could be in a modified state, and shouldn't be /// relied on to be consistent. -bool Linker::linkModules(Module &Dest, Module &Src, unsigned Flags) { +bool Linker::linkModules(Module &Dest, std::unique_ptr Src, + unsigned Flags) { Linker L(Dest); - return L.linkInModule(Src, Flags); + return L.linkInModule(std::move(Src), Flags); } std::unique_ptr -llvm::renameModuleForThinLTO(std::unique_ptr &M, +llvm::renameModuleForThinLTO(std::unique_ptr M, const FunctionInfoIndex *Index) { std::unique_ptr RenamedModule( new llvm::Module(M->getModuleIdentifier(), M->getContext())); Linker L(*RenamedModule.get()); - if (L.linkInModule(*M.get(), llvm::Linker::Flags::None, Index)) + if (L.linkInModule(std::move(M), llvm::Linker::Flags::None, Index)) return nullptr; return RenamedModule; } @@ -843,7 +849,9 @@ LLVMBool LLVMLinkModules(LLVMModuleRef Dest, LLVMModuleRef Src, std::string Message; Ctx.setDiagnosticHandler(diagnosticHandler, &Message, true); - LLVMBool Result = Linker::linkModules(*D, *unwrap(Src)); + Linker L(*D); + Module *M = unwrap(Src); + LLVMBool Result = L.linkInModuleForCAPI(*M); Ctx.setDiagnosticHandler(OldDiagnosticHandler, OldDiagnosticContext, true); @@ -851,3 +859,9 @@ LLVMBool LLVMLinkModules(LLVMModuleRef Dest, LLVMModuleRef Src, *OutMessages = strdup(Message.c_str()); return Result; } + +LLVMBool LLVMLinkModules2(LLVMModuleRef Dest, LLVMModuleRef Src) { + Module *D = unwrap(Dest); + std::unique_ptr M(unwrap(Src)); + return Linker::linkModules(*D, std::move(M)); +} diff --git a/lib/Transforms/IPO/FunctionImport.cpp b/lib/Transforms/IPO/FunctionImport.cpp index fe58bdbed19..b8c2ea7dce2 100644 --- a/lib/Transforms/IPO/FunctionImport.cpp +++ b/lib/Transforms/IPO/FunctionImport.cpp @@ -71,6 +71,14 @@ public: /// Retrieve a Module from the cache or lazily load it on demand. Module &operator()(StringRef FileName); + + std::unique_ptr takeModule(StringRef FileName) { + auto I = ModuleMap.find(FileName); + assert(I != ModuleMap.end()); + std::unique_ptr Ret = std::move(I->second); + ModuleMap.erase(I); + return Ret; + } }; // Get a Module for \p FileName from the cache, or load it lazily. @@ -149,12 +157,13 @@ static void findExternalCalls(const Module &DestModule, Function &F, // // \p ModuleToFunctionsToImportMap is filled with the set of Function to import // per Module. -static void GetImportList( - Module &DestModule, SmallVector &Worklist, - StringSet<> &CalledFunctions, - std::map>> & - ModuleToFunctionsToImportMap, - const FunctionInfoIndex &Index, ModuleLazyLoaderCache &ModuleLoaderCache) { +static void GetImportList(Module &DestModule, + SmallVector &Worklist, + StringSet<> &CalledFunctions, + std::map> + &ModuleToFunctionsToImportMap, + const FunctionInfoIndex &Index, + ModuleLazyLoaderCache &ModuleLoaderCache) { while (!Worklist.empty()) { auto CalledFunctionName = Worklist.pop_back_val(); DEBUG(dbgs() << DestModule.getModuleIdentifier() << ": Process import for " @@ -238,8 +247,7 @@ static void GetImportList( // Add the function to the import list auto &Entry = ModuleToFunctionsToImportMap[SrcModule.getModuleIdentifier()]; - Entry.first = &SrcModule; - Entry.second.insert(F); + Entry.insert(F); // Process the newly imported functions and add callees to the worklist. F->materialize(); @@ -274,7 +282,7 @@ bool FunctionImporter::importFunctions(Module &DestModule) { Linker TheLinker(DestModule); // Map of Module -> List of Function to import from the Module - std::map>> + std::map> ModuleToFunctionsToImportMap; // Analyze the summaries and get the list of functions to import by @@ -287,14 +295,15 @@ bool FunctionImporter::importFunctions(Module &DestModule) { // Do the actual import of functions now, one Module at a time for (auto &FunctionsToImportPerModule : ModuleToFunctionsToImportMap) { // Get the module for the import - auto &FunctionsToImport = FunctionsToImportPerModule.second.second; - auto *SrcModule = FunctionsToImportPerModule.second.first; + auto &FunctionsToImport = FunctionsToImportPerModule.second; + std::unique_ptr SrcModule = + ModuleLoaderCache.takeModule(FunctionsToImportPerModule.first); assert(&DestModule.getContext() == &SrcModule->getContext() && "Context mismatch"); // Link in the specified functions. - if (TheLinker.linkInModule(*SrcModule, Linker::Flags::None, &Index, - &FunctionsToImport)) + if (TheLinker.linkInModule(std::move(SrcModule), Linker::Flags::None, + &Index, &FunctionsToImport)) report_fatal_error("Function Import: link error"); ImportedCount += FunctionsToImport.size(); diff --git a/test/Bindings/OCaml/linker.ml b/test/Bindings/OCaml/linker.ml index 275a143d178..423905e489c 100644 --- a/test/Bindings/OCaml/linker.ml +++ b/test/Bindings/OCaml/linker.ml @@ -39,23 +39,21 @@ let test_linker () = let m1 = make_module "one" and m2 = make_module "two" in - link_modules m1 m2; + link_modules' m1 m2; dispose_module m1; - dispose_module m2; let m1 = make_module "one" and m2 = make_module "two" in - link_modules m1 m2; + link_modules' m1 m2; dispose_module m1; let m1 = make_module "one" and m2 = make_module "one" in try - link_modules m1 m2; + link_modules' m1 m2; failwith "must raise" with Error _ -> - dispose_module m1; - dispose_module m2 + dispose_module m1 (*===-- Driver ------------------------------------------------------------===*) diff --git a/tools/bugpoint/BugDriver.cpp b/tools/bugpoint/BugDriver.cpp index 39887d5d59d..030749fa9ea 100644 --- a/tools/bugpoint/BugDriver.cpp +++ b/tools/bugpoint/BugDriver.cpp @@ -132,7 +132,7 @@ bool BugDriver::addSources(const std::vector &Filenames) { if (!M.get()) return true; outs() << "Linking in input file: '" << Filenames[i] << "'\n"; - if (Linker::linkModules(*Program, *M)) + if (Linker::linkModules(*Program, std::move(M))) return true; } diff --git a/tools/bugpoint/Miscompilation.cpp b/tools/bugpoint/Miscompilation.cpp index 2c64e372256..16919f5a449 100644 --- a/tools/bugpoint/Miscompilation.cpp +++ b/tools/bugpoint/Miscompilation.cpp @@ -221,7 +221,7 @@ static std::unique_ptr testMergedProgram(const BugDriver &BD, std::unique_ptr M2, std::string &Error, bool &Broken) { - if (Linker::linkModules(*M1, *M2)) + if (Linker::linkModules(*M1, std::move(M2))) exit(1); // Execute the program. @@ -387,7 +387,8 @@ static bool ExtractLoops(BugDriver &BD, MisCompFunctions.emplace_back(F->getName(), F->getFunctionType()); } - if (Linker::linkModules(*ToNotOptimize, *ToOptimizeLoopExtracted)) + if (Linker::linkModules(*ToNotOptimize, + std::move(ToOptimizeLoopExtracted))) exit(1); MiscompiledFunctions.clear(); @@ -414,7 +415,7 @@ static bool ExtractLoops(BugDriver &BD, // extraction both didn't break the program, and didn't mask the problem. // Replace the current program with the loop extracted version, and try to // extract another loop. - if (Linker::linkModules(*ToNotOptimize, *ToOptimizeLoopExtracted)) + if (Linker::linkModules(*ToNotOptimize, std::move(ToOptimizeLoopExtracted))) exit(1); // All of the Function*'s in the MiscompiledFunctions list are in the old @@ -582,7 +583,7 @@ static bool ExtractBlocks(BugDriver &BD, if (!I->isDeclaration()) MisCompFunctions.emplace_back(I->getName(), I->getFunctionType()); - if (Linker::linkModules(*ProgClone, *Extracted)) + if (Linker::linkModules(*ProgClone, std::move(Extracted))) exit(1); // Set the new program and delete the old one. diff --git a/tools/llvm-link/llvm-link.cpp b/tools/llvm-link/llvm-link.cpp index 8030f4c9037..326ecba3ae9 100644 --- a/tools/llvm-link/llvm-link.cpp +++ b/tools/llvm-link/llvm-link.cpp @@ -204,7 +204,7 @@ static bool importFunctions(const char *argv0, LLVMContext &Context, // Link in the specified function. DenseSet FunctionsToImport; FunctionsToImport.insert(F); - if (L.linkInModule(*M, Linker::Flags::None, Index.get(), + if (L.linkInModule(std::move(M), Linker::Flags::None, Index.get(), &FunctionsToImport)) return false; } @@ -245,7 +245,7 @@ static bool linkFiles(const char *argv0, LLVMContext &Context, Linker &L, if (Verbose) errs() << "Linking in '" << File << "'\n"; - if (L.linkInModule(*M, ApplicableFlags, Index.get())) + if (L.linkInModule(std::move(M), ApplicableFlags, Index.get())) return false; // All linker flags apply to linking of subsequent files. ApplicableFlags = Flags; diff --git a/unittests/Linker/LinkModulesTest.cpp b/unittests/Linker/LinkModulesTest.cpp index ba8f4798172..5d3945e93b5 100644 --- a/unittests/Linker/LinkModulesTest.cpp +++ b/unittests/Linker/LinkModulesTest.cpp @@ -98,10 +98,7 @@ TEST_F(LinkModuleTest, BlockAddress) { Module *LinkedModule = new Module("MyModuleLinked", Ctx); Ctx.setDiagnosticHandler(expectNoDiags); - Linker::linkModules(*LinkedModule, *M); - - // Delete the original module. - M.reset(); + Linker::linkModules(*LinkedModule, std::move(M)); // Check that the global "@switch.bas" is well-formed. const GlobalVariable *LinkedGV = LinkedModule->getNamedGlobal("switch.bas"); @@ -175,14 +172,14 @@ TEST_F(LinkModuleTest, EmptyModule) { std::unique_ptr InternalM(getInternal(Ctx)); std::unique_ptr EmptyM(new Module("EmptyModule1", Ctx)); Ctx.setDiagnosticHandler(expectNoDiags); - Linker::linkModules(*EmptyM, *InternalM); + Linker::linkModules(*EmptyM, std::move(InternalM)); } TEST_F(LinkModuleTest, EmptyModule2) { std::unique_ptr InternalM(getInternal(Ctx)); std::unique_ptr EmptyM(new Module("EmptyModule1", Ctx)); Ctx.setDiagnosticHandler(expectNoDiags); - Linker::linkModules(*InternalM, *EmptyM); + Linker::linkModules(*InternalM, std::move(EmptyM)); } TEST_F(LinkModuleTest, TypeMerge) { @@ -198,7 +195,7 @@ TEST_F(LinkModuleTest, TypeMerge) { std::unique_ptr M2 = parseAssemblyString(M2Str, Err, C); Ctx.setDiagnosticHandler(expectNoDiags); - Linker::linkModules(*M1, *M2); + Linker::linkModules(*M1, std::move(M2)); EXPECT_EQ(M1->getNamedGlobal("t1")->getType(), M1->getNamedGlobal("t2")->getType()); @@ -228,6 +225,37 @@ TEST_F(LinkModuleTest, CAPIFailure) { LLVMDisposeMessage(errout); } +TEST_F(LinkModuleTest, NewCAPISuccess) { + std::unique_ptr DestM(getExternal(Ctx, "foo")); + std::unique_ptr SourceM(getExternal(Ctx, "bar")); + LLVMBool Result = + LLVMLinkModules2(wrap(DestM.get()), wrap(SourceM.release())); + EXPECT_EQ(0, Result); + // "bar" is present in destination module + EXPECT_NE(nullptr, DestM->getFunction("bar")); +} + +static void diagnosticHandler(LLVMDiagnosticInfoRef DI, void *C) { + auto *Err = reinterpret_cast(C); + char *CErr = LLVMGetDiagInfoDescription(DI); + *Err = CErr; + LLVMDisposeMessage(CErr); +} + +TEST_F(LinkModuleTest, NewCAPIFailure) { + // Symbol clash between two modules + LLVMContext Ctx; + std::string Err; + LLVMContextSetDiagnosticHandler(wrap(&Ctx), diagnosticHandler, &Err); + + std::unique_ptr DestM(getExternal(Ctx, "foo")); + std::unique_ptr SourceM(getExternal(Ctx, "foo")); + LLVMBool Result = + LLVMLinkModules2(wrap(DestM.get()), wrap(SourceM.release())); + EXPECT_EQ(1, Result); + EXPECT_EQ("Linking globals named 'foo': symbol multiply defined!", Err); +} + TEST_F(LinkModuleTest, MoveDistinctMDs) { LLVMContext C; SMDiagnostic Err; @@ -276,7 +304,7 @@ TEST_F(LinkModuleTest, MoveDistinctMDs) { auto Dst = llvm::make_unique("Linked", C); ASSERT_TRUE(Dst.get()); Ctx.setDiagnosticHandler(expectNoDiags); - Linker::linkModules(*Dst, *Src); + Linker::linkModules(*Dst, std::move(Src)); // Check that distinct metadata was moved, not cloned. Even !4, the uniqued // node, should effectively be moved, since its only operand hasn't changed. -- 2.34.1