[Patch] Change linkInModule to take a std::unique_ptr
Rafael Espíndola via llvm-commits
llvm-commits at lists.llvm.org
Wed Dec 9 07:55:26 PST 2015
Looking a bit more at the C API I realized it is probably used as
implemented, not as documented. I.E.: callers do free the second
argument.
Given that, it seems we will have to take the long road: Add a new API
and deprecate the old one. That is what the attached patch does.
Eric, could you please check that the patch does the deprecation dance
correctly? Thanks.
Cheers,
Rafael
On 8 December 2015 at 21:50, Mehdi Amini <mehdi.amini at apple.com> wrote:
> LGTM.
>
> —
> Mehdi
>
>> On Dec 8, 2015, at 5:07 PM, Rafael Espíndola <rafael.espindola at gmail.com> wrote:
>>
>> Passing in a std::unique_ptr should help find errors when the module
>> is used after being linked into another module.
>>
>> This changes a C api, but changes it to what it is documented to do:
>>
>> * Links the source module into the destination module, taking ownership
>> * of the source module away from the caller.
>>
>> Cheers,
>> Rafael
>> <t.diff>
>
-------------- next part --------------
diff --git a/bindings/go/llvm/linker.go b/bindings/go/llvm/linker.go
index f64f66c..63979c2 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 3b8512a..33c028d 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 (LLVMLinkModules(Dst, Src))
+ llvm_raise(*caml_named_value("Llvm_linker.Error"), "Linking failed");
return Val_unit;
}
diff --git a/docs/ReleaseNotes.rst b/docs/ReleaseNotes.rst
index ba7e436..6faa2ae 100644
--- a/docs/ReleaseNotes.rst
+++ b/docs/ReleaseNotes.rst
@@ -41,6 +41,9 @@ 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.
+
.. 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
diff --git a/include/llvm-c/Linker.h b/include/llvm-c/Linker.h
index 9f98a33..f958aad 100644
--- a/include/llvm-c/Linker.h
+++ b/include/llvm-c/Linker.h
@@ -27,17 +27,22 @@ 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 funciton is deprecated. Use LLVMLinkModules2 instead.
+ */
LLVMBool LLVMLinkModules(LLVMModuleRef Dest, LLVMModuleRef Src,
LLVMLinkerMode Unused, char **OutMessage);
+LLVMBool LLVMLinkModules2(LLVMModuleRef Dest, LLVMModuleRef Src);
+
#ifdef __cplusplus
}
#endif
diff --git a/include/llvm/Linker/Linker.h b/include/llvm/Linker/Linker.h
index aa43009..321444e 100644
--- a/include/llvm/Linker/Linker.h
+++ b/include/llvm/Linker/Linker.h
@@ -70,7 +70,7 @@ public:
Linker(Module &M, DiagnosticHandlerFunction DiagnosticHandler);
- /// \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.
@@ -79,11 +79,14 @@ 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,
+ /// 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,
DiagnosticHandlerFunction DiagnosticHandler,
unsigned Flags = Flags::None);
diff --git a/lib/LTO/LTOCodeGenerator.cpp b/lib/LTO/LTOCodeGenerator.cpp
index bf3cde5..03a2c74 100644
--- a/lib/LTO/LTOCodeGenerator.cpp
+++ b/lib/LTO/LTOCodeGenerator.cpp
@@ -108,7 +108,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)
diff --git a/lib/Linker/LinkModules.cpp b/lib/Linker/LinkModules.cpp
index a9fcee7..0004f5c 100644
--- a/lib/Linker/LinkModules.cpp
+++ b/lib/Linker/LinkModules.cpp
@@ -1969,16 +1969,24 @@ Linker::Linker(Module &M, DiagnosticHandlerFunction DiagnosticHandler)
}
}
-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(Composite, IdentifiedStructTypes, Src,
+ ModuleLinker TheLinker(Composite, IdentifiedStructTypes, *Src,
DiagnosticHandler, Flags, Index, FunctionsToImport);
bool RetCode = TheLinker.run();
Composite.dropTriviallyDeadConstantArrays();
return RetCode;
}
+bool Linker::linkInModuleForCAPI(Module &Src) {
+ ModuleLinker TheLinker(Composite, IdentifiedStructTypes, Src,
+ DiagnosticHandler, 0, nullptr, nullptr);
+ bool RetCode = TheLinker.run();
+ Composite.dropTriviallyDeadConstantArrays();
+ return RetCode;
+}
+
//===----------------------------------------------------------------------===//
// LinkModules entrypoint.
//===----------------------------------------------------------------------===//
@@ -1988,11 +1996,11 @@ 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,
+bool Linker::linkModules(Module &Dest, std::unique_ptr<Module> Src,
DiagnosticHandlerFunction DiagnosticHandler,
unsigned Flags) {
Linker L(Dest, DiagnosticHandler);
- return L.linkInModule(Src, Flags);
+ return L.linkInModule(std::move(Src), Flags);
}
std::unique_ptr<Module>
@@ -2002,7 +2010,7 @@ llvm::renameModuleForThinLTO(std::unique_ptr<Module> &M,
std::unique_ptr<llvm::Module> RenamedModule(
new llvm::Module(M->getModuleIdentifier(), M->getContext()));
Linker L(*RenamedModule.get(), DiagnosticHandler);
- 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;
}
@@ -2017,9 +2025,10 @@ LLVMBool LLVMLinkModules(LLVMModuleRef Dest, LLVMModuleRef Src,
std::string Message;
raw_string_ostream Stream(Message);
DiagnosticPrinterRawOStream DP(Stream);
+ Module *M = unwrap(Src);
- LLVMBool Result = Linker::linkModules(
- *D, *unwrap(Src), [&](const DiagnosticInfo &DI) { DI.print(DP); });
+ Linker L(*D, [&](const DiagnosticInfo &DI) { DI.print(DP); });
+ LLVMBool Result = L.linkInModuleForCAPI(*M);
if (OutMessages && Result) {
Stream.flush();
@@ -2027,3 +2036,11 @@ LLVMBool LLVMLinkModules(LLVMModuleRef Dest, LLVMModuleRef Src,
}
return Result;
}
+
+LLVMBool LLVMLinkModules2(LLVMModuleRef Dest, LLVMModuleRef Src) {
+ Module *D = unwrap(Dest);
+ LLVMContext &C = D->getContext();
+ std::unique_ptr<Module> M(unwrap(Src));
+ return Linker::linkModules(*D, std::move(M),
+ [&](const DiagnosticInfo &DI) { C.diagnose(DI); });
+}
diff --git a/lib/Transforms/IPO/FunctionImport.cpp b/lib/Transforms/IPO/FunctionImport.cpp
index c6a7038..7956236 100644
--- a/lib/Transforms/IPO/FunctionImport.cpp
+++ b/lib/Transforms/IPO/FunctionImport.cpp
@@ -71,6 +71,12 @@ 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());
+ return std::move(I->second);
+ }
};
// Get a Module for \p FileName from the cache, or load it lazily.
@@ -288,13 +294,14 @@ bool FunctionImporter::importFunctions(Module &DestModule) {
for (auto &FunctionsToImportPerModule : ModuleToFunctionsToImportMap) {
// Get the module for the import
auto &FunctionsToImport = FunctionsToImportPerModule.second.second;
- auto *SrcModule = FunctionsToImportPerModule.second.first;
+ 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();
diff --git a/test/Bindings/OCaml/linker.ml b/test/Bindings/OCaml/linker.ml
index 275a143..d93f58c 100644
--- a/test/Bindings/OCaml/linker.ml
+++ b/test/Bindings/OCaml/linker.ml
@@ -41,7 +41,6 @@ let test_linker () =
and m2 = make_module "two" in
link_modules m1 m2;
dispose_module m1;
- dispose_module m2;
let m1 = make_module "one"
and m2 = make_module "two" in
@@ -54,8 +53,7 @@ let test_linker () =
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 9edc242..4f7a747 100644
--- a/tools/bugpoint/BugDriver.cpp
+++ b/tools/bugpoint/BugDriver.cpp
@@ -139,7 +139,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, diagnosticHandler))
+ if (Linker::linkModules(*Program, std::move(M), diagnosticHandler))
return true;
}
diff --git a/tools/bugpoint/Miscompilation.cpp b/tools/bugpoint/Miscompilation.cpp
index 5c9f027..5d704a8 100644
--- a/tools/bugpoint/Miscompilation.cpp
+++ b/tools/bugpoint/Miscompilation.cpp
@@ -230,7 +230,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, diagnosticHandler))
+ if (Linker::linkModules(*M1, std::move(M2), diagnosticHandler))
exit(1);
// Execute the program.
@@ -396,7 +396,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),
diagnosticHandler))
exit(1);
@@ -424,7 +425,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),
diagnosticHandler))
exit(1);
@@ -593,7 +594,7 @@ static bool ExtractBlocks(BugDriver &BD,
if (!I->isDeclaration())
MisCompFunctions.emplace_back(I->getName(), I->getFunctionType());
- if (Linker::linkModules(*ProgClone, *Extracted, diagnosticHandler))
+ if (Linker::linkModules(*ProgClone, std::move(Extracted), diagnosticHandler))
exit(1);
// Set the new program and delete the old one.
diff --git a/tools/gold/gold-plugin.cpp b/tools/gold/gold-plugin.cpp
index 8eacdc3..f7b65c5 100644
--- a/tools/gold/gold-plugin.cpp
+++ b/tools/gold/gold-plugin.cpp
@@ -956,7 +956,7 @@ static ld_plugin_status allSymbolsReadHook(raw_fd_ostream *ApiFile) {
M->setTargetTriple(DefaultTriple);
}
- if (L.linkInModule(*M))
+ if (L.linkInModule(std::move(M)))
message(LDPL_FATAL, "Failed to link module");
if (release_input_file(F.handle) != LDPS_OK)
message(LDPL_FATAL, "Failed to release file information");
diff --git a/tools/llvm-link/llvm-link.cpp b/tools/llvm-link/llvm-link.cpp
index 6f90f40..3b91109 100644
--- a/tools/llvm-link/llvm-link.cpp
+++ b/tools/llvm-link/llvm-link.cpp
@@ -200,7 +200,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;
}
@@ -241,7 +241,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 e56a692..03fb630 100644
--- a/unittests/Linker/LinkModulesTest.cpp
+++ b/unittests/Linker/LinkModulesTest.cpp
@@ -95,10 +95,7 @@ TEST_F(LinkModuleTest, BlockAddress) {
Builder.CreateRet(ConstantPointerNull::get(Type::getInt8PtrTy(Ctx)));
Module *LinkedModule = new Module("MyModuleLinked", Ctx);
- Linker::linkModules(*LinkedModule, *M, expectNoDiags);
-
- // Delete the original module.
- M.reset();
+ Linker::linkModules(*LinkedModule, std::move(M), expectNoDiags);
// Check that the global "@switch.bas" is well-formed.
const GlobalVariable *LinkedGV = LinkedModule->getNamedGlobal("switch.bas");
@@ -171,13 +168,13 @@ static Module *getInternal(LLVMContext &Ctx) {
TEST_F(LinkModuleTest, EmptyModule) {
std::unique_ptr<Module> InternalM(getInternal(Ctx));
std::unique_ptr<Module> EmptyM(new Module("EmptyModule1", Ctx));
- Linker::linkModules(*EmptyM, *InternalM, expectNoDiags);
+ Linker::linkModules(*EmptyM, std::move(InternalM), expectNoDiags);
}
TEST_F(LinkModuleTest, EmptyModule2) {
std::unique_ptr<Module> InternalM(getInternal(Ctx));
std::unique_ptr<Module> EmptyM(new Module("EmptyModule1", Ctx));
- Linker::linkModules(*InternalM, *EmptyM, expectNoDiags);
+ Linker::linkModules(*InternalM, std::move(EmptyM), expectNoDiags);
}
TEST_F(LinkModuleTest, TypeMerge) {
@@ -192,7 +189,7 @@ TEST_F(LinkModuleTest, TypeMerge) {
"@t2 = weak global %t zeroinitializer\n";
std::unique_ptr<Module> M2 = parseAssemblyString(M2Str, Err, C);
- Linker::linkModules(*M1, *M2, [](const llvm::DiagnosticInfo &) {});
+ Linker::linkModules(*M1, std::move(M2), [](const llvm::DiagnosticInfo &) {});
EXPECT_EQ(M1->getNamedGlobal("t1")->getType(),
M1->getNamedGlobal("t2")->getType());
@@ -222,6 +219,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;
@@ -269,7 +297,8 @@ TEST_F(LinkModuleTest, MoveDistinctMDs) {
// Link into destination module.
auto Dst = llvm::make_unique<Module>("Linked", C);
ASSERT_TRUE(Dst.get());
- Linker::linkModules(*Dst, *Src, [](const llvm::DiagnosticInfo &) {});
+ Linker::linkModules(*Dst, std::move(Src),
+ [](const llvm::DiagnosticInfo &) {});
// Check that distinct metadata was moved, not cloned. Even !4, the uniqued
// node, should effectively be moved, since its only operand hasn't changed.
More information about the llvm-commits
mailing list