Index: bindings/go/llvm/linker.go =================================================================== --- bindings/go/llvm/linker.go +++ bindings/go/llvm/linker.go @@ -21,11 +21,9 @@ 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 Index: bindings/ocaml/linker/linker_ocaml.c =================================================================== --- bindings/ocaml/linker/linker_ocaml.c +++ bindings/ocaml/linker/linker_ocaml.c @@ -25,10 +25,8 @@ /* 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; } Index: bindings/ocaml/linker/llvm_linker.ml =================================================================== --- bindings/ocaml/linker/llvm_linker.ml +++ bindings/ocaml/linker/llvm_linker.ml @@ -11,5 +11,5 @@ 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" Index: bindings/ocaml/linker/llvm_linker.mli =================================================================== --- bindings/ocaml/linker/llvm_linker.mli +++ 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 Index: docs/ReleaseNotes.rst =================================================================== --- docs/ReleaseNotes.rst +++ docs/ReleaseNotes.rst @@ -41,6 +41,14 @@ 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 @@ 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 ============================================ Index: include/llvm-c/Linker.h =================================================================== --- include/llvm-c/Linker.h +++ include/llvm-c/Linker.h @@ -27,17 +27,27 @@ 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); +/* Unlike LLVMLinkerMode this function: + * Doesn't take an unused parameter. + * Destroys the source instead of only damaging it. + * Does not record a message. Use the diagnostic handler instead. +*/ +LLVMBool LLVMLinkModules2(LLVMModuleRef Dest, LLVMModuleRef Src); + #ifdef __cplusplus } #endif Index: include/llvm/Linker/Linker.h =================================================================== --- include/llvm/Linker/Linker.h +++ include/llvm/Linker/Linker.h @@ -35,7 +35,7 @@ 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,20 @@ /// 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 a deprecate 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 Index: lib/LTO/LTOCodeGenerator.cpp =================================================================== --- lib/LTO/LTOCodeGenerator.cpp +++ lib/LTO/LTOCodeGenerator.cpp @@ -106,7 +106,7 @@ 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) Index: lib/Linker/LinkModules.cpp =================================================================== --- lib/Linker/LinkModules.cpp +++ lib/Linker/LinkModules.cpp @@ -789,10 +789,15 @@ 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 @@ /// 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 @@ 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 @@ *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)); +} Index: lib/Transforms/IPO/FunctionImport.cpp =================================================================== --- lib/Transforms/IPO/FunctionImport.cpp +++ lib/Transforms/IPO/FunctionImport.cpp @@ -71,6 +71,14 @@ /// 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 @@ // // \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 @@ // 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 @@ 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 @@ // 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(); Index: test/Bindings/OCaml/linker.ml =================================================================== --- test/Bindings/OCaml/linker.ml +++ test/Bindings/OCaml/linker.ml @@ -39,23 +39,21 @@ 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 ------------------------------------------------------------===*) Index: tools/bugpoint/BugDriver.cpp =================================================================== --- tools/bugpoint/BugDriver.cpp +++ tools/bugpoint/BugDriver.cpp @@ -132,7 +132,7 @@ 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; } Index: tools/bugpoint/Miscompilation.cpp =================================================================== --- tools/bugpoint/Miscompilation.cpp +++ tools/bugpoint/Miscompilation.cpp @@ -221,7 +221,7 @@ 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 @@ 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 @@ // 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 @@ 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. Index: tools/llvm-link/llvm-link.cpp =================================================================== --- tools/llvm-link/llvm-link.cpp +++ tools/llvm-link/llvm-link.cpp @@ -204,7 +204,7 @@ // 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 @@ 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; Index: unittests/Linker/LinkModulesTest.cpp =================================================================== --- unittests/Linker/LinkModulesTest.cpp +++ unittests/Linker/LinkModulesTest.cpp @@ -98,10 +98,7 @@ 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 @@ 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 @@ 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 @@ 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 @@ 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.