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.

llvm-svn: 255842

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

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;
 }

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"

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
+(** [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

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 <CMake.html>`_
 
+* 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
 ============================================

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

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<Module> Src, unsigned Flags = Flags::None,
                     const FunctionInfoIndex *Index = nullptr,
                     DenseSet<const GlobalValue *> *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<Module> Src,
+                          unsigned Flags = Flags::None);
 };
 
 /// Create a new module with exported local functions renamed and promoted
 /// for ThinLTO.
-std::unique_ptr<Module> renameModuleForThinLTO(std::unique_ptr<Module> &M,
+std::unique_ptr<Module> renameModuleForThinLTO(std::unique_ptr<Module> M,
                                                const FunctionInfoIndex *Index);
 
 } // End llvm namespace

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<const char *> &undefs = Mod->getAsmUndefinedRefs();
   for (int i = 0, e = undefs.size(); i != e; ++i)

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<Module> Src, unsigned Flags,
                           const FunctionInfoIndex *Index,
                           DenseSet<const GlobalValue *> *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<Module> Src,
+                         unsigned Flags) {
   Linker L(Dest);
-  return L.linkInModule(Src, Flags);
+  return L.linkInModule(std::move(Src), Flags);
 }
 
 std::unique_ptr<Module>
-llvm::renameModuleForThinLTO(std::unique_ptr<Module> &M,
+llvm::renameModuleForThinLTO(std::unique_ptr<Module> M,
                              const FunctionInfoIndex *Index) {
   std::unique_ptr<llvm::Module> 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<Module> M(unwrap(Src));
+  return Linker::linkModules(*D, std::move(M));
+}

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<Module> takeModule(StringRef FileName) {
+    auto I = ModuleMap.find(FileName);
+    assert(I != ModuleMap.end());
+    std::unique_ptr<Module> 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<StringRef, 64> &Worklist,
-    StringSet<> &CalledFunctions,
-    std::map<StringRef, std::pair<Module *, DenseSet<const GlobalValue *>>> &
-        ModuleToFunctionsToImportMap,
-    const FunctionInfoIndex &Index, ModuleLazyLoaderCache &ModuleLoaderCache) {
+static void GetImportList(Module &DestModule,
+                          SmallVector<StringRef, 64> &Worklist,
+                          StringSet<> &CalledFunctions,
+                          std::map<StringRef, DenseSet<const GlobalValue *>>
+                              &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<StringRef, std::pair<Module *, DenseSet<const GlobalValue *>>>
+  std::map<StringRef, DenseSet<const GlobalValue *>>
       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<Module> 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();

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

tools/bugpoint/BugDriver.cpp

@@ -132,7 +132,7 @@ bool BugDriver::addSources(const std::vector<std::string> &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;
   }
 

tools/bugpoint/Miscompilation.cpp

@@ -221,7 +221,7 @@ static std::unique_ptr<Module> testMergedProgram(const BugDriver &BD,
                                                  std::unique_ptr<Module> 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.

tools/llvm-link/llvm-link.cpp

@@ -204,7 +204,7 @@ static bool importFunctions(const char *argv0, LLVMContext &Context,
     // Link in the specified function.
     DenseSet<const GlobalValue *> 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;

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<Module> InternalM(getInternal(Ctx));
   std::unique_ptr<Module> 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<Module> InternalM(getInternal(Ctx));
   std::unique_ptr<Module> 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<Module> 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<Module> DestM(getExternal(Ctx, "foo"));
+  std::unique_ptr<Module> 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<std::string *>(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<Module> DestM(getExternal(Ctx, "foo"));
+  std::unique_ptr<Module> 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<Module>("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.