[Patch] Change linkInModule to take a std::unique_ptr
Rafael Espíndola via llvm-commits
llvm-commits at lists.llvm.org
Wed Dec 9 14:59:31 PST 2015
rebased patch attached.
On 9 December 2015 at 10:55, Rafael Espíndola
<rafael.espindola at gmail.com> wrote:
> 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 3d40c12..9801a4d 100644
--- a/lib/Linker/LinkModules.cpp
+++ b/lib/Linker/LinkModules.cpp
@@ -1974,16 +1974,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.
//===----------------------------------------------------------------------===//
@@ -1993,11 +2001,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>
@@ -2007,7 +2015,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;
}
@@ -2022,9 +2030,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();
@@ -2032,3 +2041,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 6325a72..e0f56e8 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 9f8b0b2..bc5eabe 100644
--- a/tools/gold/gold-plugin.cpp
+++ b/tools/gold/gold-plugin.cpp
@@ -959,7 +959,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