Change linkInModule to take a std::unique_ptr

[• rafael]

Summary

Eric, please check that we are doing the right thing with regards to the C API.

[whitequark]

test/Bindings/OCaml/linker.ml

58

This is really terrible; this is a silent change that alters correctness of existing OCaml code, leading to double free, which will not be detected if you don't run a Debug build of LLVM (and even then I'm not sure).

Could you please rename link_modules function to link_modules' (note the apostrophe) and add a bit to documentation in the mli like you did in the C API? It is probably not necessary to keep the old one for compatibility.

This should be a straightforward search-and-replace job, but ping me if it causes issues.

[• rafael]

Rename the ocaml function

[echristo]

Couple of inline comments.

include/llvm-c/Linker.h

39

Typo.

44

For now please document the changes between the two functions. It would probably be best if you do so in the release notes as well.

[whitequark]

LGTM after changes described above.

bindings/ocaml/linker/llvm_linker.mli

17–19

There's no mode argument anymore--the signature should read [link_modules' dst src].

docs/ReleaseNotes.rst

92

Can you please add a notice similar to the one about the C API above about the link_modules' function?

[echristo]

Couple more inline comments, but the C API stuff as far as policy is ok now.

Thanks!

-eric

include/llvm-c/Linker.h

44

Typo.

Best to just describe the function completely with a note of what it also doesn't do so that the end of deprecation removal is easier for LLVMLinkModules?

include/llvm/Linker/Linker.h

51

"deprecated" and go ahead and specify which one so that a grep will find this.

[• rafael]

r255842. Thanks!

[mehdi_amini]

seems like it was committed, accept and close.

bindings/go/llvm/linker.go

 /*
 #include "llvm-c/Linker.h"
 #include <stdlib.h>
 */
 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

 #include "caml/memory.h"
 #include "caml/fail.h"
 #include "caml/callback.h"
 
 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

 (*===-- llvm_linker.ml - LLVM OCaml Interface ------------------*- OCaml -*-===*
  *
  *                     The LLVM Compiler Infrastructure
  *
  * This file is distributed under the University of Illinois Open Source
  * License. See LICENSE.TXT for details.
  *
  *===----------------------------------------------------------------------===*)
 
 exception Error of string
 
 let () = Callback.register_exception "Llvm_linker.Error" (Error "")
 
-external link_modules : Llvm.llmodule -> Llvm.llmodule -> unit
+external link_modules' : Llvm.llmodule -> Llvm.llmodule -> unit
                        = "llvm_link_modules"

bindings/ocaml/linker/llvm_linker.mli

 (*===-- llvm_linker.mli - LLVM OCaml Interface -----------------*- OCaml -*-===*
  *
  *                     The LLVM Compiler Infrastructure
  *
  * This file is distributed under the University of Illinois Open Source
  * License. See LICENSE.TXT for details.
  *
  *===----------------------------------------------------------------------===*)
 
 (** Linker.
 
     This interface provides an OCaml API for LLVM bitcode linker,
     the classes in the Linker library. *)
 
 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
 No newline at end of file

docs/ReleaseNotes.rst

 * With this release, the minimum Windows version required for running LLVM is
   Windows 7. Earlier versions, including Windows Vista and XP are no longer
   supported.
 
 * With this release, the autoconf build system is deprecated. It will be removed
   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
    point (e.g. maybe you would like to give an example of the
    functionality, or simply have a lot to talk about), see the `NOTE` below
    for adding a new subsection.
 
 * ... next change ...

 -----------------------------
 
  During this release ...
 
 
 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
 ============================================
 
 An exciting aspect of LLVM is that it is used as an enabling technology for
 a lot of other language and tools projects. This section lists some of the
 projects that have already been updated to work with LLVM 3.8.
 

include/llvm-c/Linker.h

 
 /* This enum is provided for backwards-compatibility only. It has no effect. */
 typedef enum {
   LLVMLinkerDestroySource = 0, /* This is the default behavior. */
   LLVMLinkerPreserveSource_Removed = 1 /* This option has been deprecated and
                                           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
 
 #endif

include/llvm/Linker/Linker.h

enum Flags {
     None = 0,
     OverrideFromSrc = (1 << 0),
     LinkOnlyNeeded = (1 << 1),
     InternalizeLinkedSymbols = (1 << 2)
   };
 
   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.
   /// For ThinLTO function importing/exporting the \p FunctionInfoIndex
   /// is passed. If \p FunctionsToImport is provided, only the functions that
   /// 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 a deprecate 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
 
 #endif

lib/LTO/LTOCodeGenerator.cpp

void LTOCodeGenerator::initializeLTOPasses() {
   initializeDCEPass(R);
   initializeCFGSimplifyPassPass(R);
 }
 
 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)
     AsmUndefinedRefs[undefs[i]] = 1;
 
   return !ret;
 }
 

lib/Linker/LinkModules.cpp

for (auto &P : Internalize) {
     GV->setLinkage(GlobalValue::InternalLinkage);
   }
 
   return false;
 }
 
 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();
 }
 
 //===----------------------------------------------------------------------===//
 // LinkModules entrypoint.
 //===----------------------------------------------------------------------===//
 
 /// This function links two modules together, with the resulting Dest module
 /// modified to be the composite of the two input modules. If an error occurs,
 /// 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;
 }
 
 //===----------------------------------------------------------------------===//
 // C API.
 //===----------------------------------------------------------------------===//
 
LLVMBool LLVMLinkModules(LLVMModuleRef Dest, LLVMModuleRef Src,
   LLVMContext &Ctx = D->getContext();
 
   LLVMContext::DiagnosticHandlerTy OldDiagnosticHandler =
       Ctx.getDiagnosticHandler();
   void *OldDiagnosticContext = Ctx.getDiagnosticContext();
   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);
 
   if (OutMessages && Result)
     *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

 public:
   /// Create the loader, Module will be initialized in \p Context.
   ModuleLazyLoaderCache(std::function<
       std::unique_ptr<Module>(StringRef FileName)> createLazyModule)
       : createLazyModule(createLazyModule) {}
 
   /// 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.
 Module &ModuleLazyLoaderCache::operator()(StringRef Identifier) {
   auto &Module = ModuleMap[Identifier];
   if (!Module)
     Module = createLazyModule(Identifier);
   return *Module;

 // Helper function: given a worklist and an index, will process all the worklist
 // and decide what to import based on the summary information.
 //
 // Nothing is actually imported, functions are materialized in their source
 // module and analyzed there.
 //
 // \p ModuleToFunctionsToImportMap is filled with the set of Function to import
 // per Module.
-static void GetImportList(
-    Module &DestModule, SmallVector<StringRef, 64> &Worklist,
+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) {
+                          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 "
                  << CalledFunctionName << "\n");
 
     // Try to get a summary for this function call.
     auto InfoList = Index.findFunctionInfoList(CalledFunctionName);
     if (InfoList == Index.end()) {
if (SGV->hasWeakAnyLinkage()) {
                    << (isa<Function>(SGV) ? "function " : "alias ")
                    << CalledFunctionName << " from "
                    << SrcModule.getModuleIdentifier() << "\n");
       continue;
     }
 
     // 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();
     findExternalCalls(DestModule, *F, Index, CalledFunctions, Worklist);
   }
 }
 
 // Automatically import functions in Module \p DestModule based on the summaries
if (Worklist.empty())
     return false;
 
   /// Second step: for every call to an external function, try to import it.
 
   // Linker that will be used for importing function
   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
   // populating ModuleToFunctionsToImportMap
   ModuleLazyLoaderCache ModuleLoaderCache(ModuleLoader);
   GetImportList(DestModule, Worklist, CalledFunctions,
                 ModuleToFunctionsToImportMap, Index, ModuleLoaderCache);
   assert(Worklist.empty() && "Worklist hasn't been flushed in GetImportList");
 
   // 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();
   }
   DEBUG(dbgs() << "Imported " << ImportedCount << " functions for Module "
                << DestModule.getModuleIdentifier() << "\n");
   return ImportedCount;
 }

test/Bindings/OCaml/linker.ml

let make_module name =
     let m = create_module context name in
     let fn = define_function ("fn_" ^ name) fty m in
     ignore (build_ret_void (builder_at_end context (entry_block fn)));
     m
   in
 
   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 ------------------------------------------------------------===*)
 
 let _ =
   suite "linker" test_linker

tools/bugpoint/BugDriver.cpp

bool BugDriver::addSources(const std::vector<std::string> &Filenames) {
 
   outs() << "Read input file      : '" << Filenames[0] << "'\n";
 
   for (unsigned i = 1, e = Filenames.size(); i != e; ++i) {
     std::unique_ptr<Module> M = parseInputFile(Filenames[i], Context);
     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;
   }
 
   outs() << "*** All input ok\n";
 
   // All input files read successfully!
   return false;
 }

tools/bugpoint/Miscompilation.cpp

 /// output is different. If the DeleteInputs argument is set to true then this
 /// function deletes both input modules before it returns.
 ///
 static std::unique_ptr<Module> testMergedProgram(const BugDriver &BD,
                                                  std::unique_ptr<Module> M1,
                                                  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.
   Broken = BD.diffProgram(M1.get(), "", "", false, &Error);
   if (!Error.empty())
     return nullptr;
   return M1;
 }
if (!Failure) {
       // If the program is not still broken, then loop extraction did something
       // that masked the error.  Stop loop extraction now.
 
       std::vector<std::pair<std::string, FunctionType*> > MisCompFunctions;
       for (Function *F : MiscompiledFunctions) {
         MisCompFunctions.emplace_back(F->getName(), F->getFunctionType());
       }
 
-      if (Linker::linkModules(*ToNotOptimize, *ToOptimizeLoopExtracted))
+      if (Linker::linkModules(*ToNotOptimize,
+                              std::move(ToOptimizeLoopExtracted)))
         exit(1);
 
       MiscompiledFunctions.clear();
       for (unsigned i = 0, e = MisCompFunctions.size(); i != e; ++i) {
         Function *NewF = ToNotOptimize->getFunction(MisCompFunctions[i].first);
 
         assert(NewF && "Function not found??");
         MiscompiledFunctions.push_back(NewF);
for (Module::iterator I = ToOptimizeLoopExtracted->begin(),
            E = ToOptimizeLoopExtracted->end(); I != E; ++I)
       if (!I->isDeclaration())
         MisCompFunctions.emplace_back(I->getName(), I->getFunctionType());
 
     // Okay, great!  Now we know that we extracted a loop and that loop
     // 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
     // module.  Update this list to include all of the functions in the
     // optimized and loop extracted module.
     MiscompiledFunctions.clear();
     for (unsigned i = 0, e = MisCompFunctions.size(); i != e; ++i) {
       Function *NewF = ToNotOptimize->getFunction(MisCompFunctions[i].first);
static bool ExtractBlocks(BugDriver &BD,
   delete ToExtract;
 
   std::vector<std::pair<std::string, FunctionType*> > MisCompFunctions;
   for (Module::iterator I = Extracted->begin(), E = Extracted->end();
        I != E; ++I)
     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.
   BD.setNewProgram(ProgClone);
 
   // Update the list of miscompiled functions.
   MiscompiledFunctions.clear();
 

tools/llvm-link/llvm-link.cpp

if (!FunctionIndex.empty()) {
         return false;
       }
       Index = std::move(IndexOrErr.get());
     }
 
     // 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;
   }
   return true;
 }
 
 static bool linkFiles(const char *argv0, LLVMContext &Context, Linker &L,
                       const cl::list<std::string> &Files,
if (!FunctionIndex.empty()) {
         return false;
       }
       Index = std::move(IndexOrErr.get());
     }
 
     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;
 
     // If requested for testing, preserve modules by releasing them from
     // the unique_ptr before the are freed. This can help catch any
     // cross-module references from e.g. unneeded metadata references
     // that aren't properly set to null but instead mapped to the source

unittests/Linker/LinkModulesTest.cpp

TEST_F(LinkModuleTest, BlockAddress) {
   Builder.SetInsertPoint(SwitchCase2BB);
   Builder.CreateBr(ExitBB);
 
   Builder.SetInsertPoint(ExitBB);
   Builder.CreateRet(ConstantPointerNull::get(Type::getInt8PtrTy(Ctx)));
 
   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");
   const Constant *Init = LinkedGV->getInitializer();
 
   // @switch.bas = internal global [3 x i8*]
   //   [i8* blockaddress(@ba_func, %switch.case.1),
   //    i8* blockaddress(@ba_func, %switch.case.2),
static Module *getInternal(LLVMContext &Ctx) {
   GV->setInitializer(ConstantStruct::get(STy, F));
   return InternalM;
 }
 
 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) {
   LLVMContext C;
   SMDiagnostic Err;
 
   const char *M1Str = "%t = type {i32}\n"
                       "@t1 = weak global %t zeroinitializer\n";
   std::unique_ptr<Module> M1 = parseAssemblyString(M1Str, Err, C);
 
   const char *M2Str = "%t = type {i32}\n"
                       "@t2 = weak global %t zeroinitializer\n";
   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());
 }
 
 TEST_F(LinkModuleTest, CAPISuccess) {
   std::unique_ptr<Module> DestM(getExternal(Ctx, "foo"));
   std::unique_ptr<Module> SourceM(getExternal(Ctx, "bar"));
TEST_F(LinkModuleTest, CAPIFailure) {
   char *errout = nullptr;
   LLVMBool result = LLVMLinkModules(wrap(DestM.get()), wrap(SourceM.get()),
                                     LLVMLinkerDestroySource, &errout);
   EXPECT_EQ(1, result);
   EXPECT_STREQ("Linking globals named 'foo': symbol multiply defined!", errout);
   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;
 
   const char *SrcStr = "define void @foo() !attach !0 {\n"
                        "entry:\n"
                        "  call void @llvm.md(metadata !1)\n"
                        "  ret void, !attach !2\n"
TEST_F(LinkModuleTest, MoveDistinctMDs) {
   EXPECT_TRUE(M3->isDistinct());
   EXPECT_TRUE(M4->isUniqued());
   EXPECT_EQ(M3, M4->getOperand(0));
 
   // Link into destination module.
   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.
   F = &*Dst->begin();
   BB = &F->getEntryBlock();
   CI = cast<CallInst>(&BB->front());
   RI = cast<ReturnInst>(BB->getTerminator());
   NMD = &*Dst->named_metadata_begin();