[PATCH] D20550: Linker: error_code'ify the IR mover. NFC.
Rafael Espíndola via llvm-commits
llvm-commits at lists.llvm.org
Tue May 24 17:09:12 PDT 2016
With this patch the linker would be using all 3 error handling systems
in llvm, which is way too much.
What I think needs to happen is for it to have an Error class that is
not converted to error_code. We also have to decide if we are keeping
a list of error or just failing early. I don't think it is worth it
keeping multiple errors, so I would suggest refactoring the code to
return early and only store the error when it hits an interface that
returns void.
We should also stop logging error to the streamer and instead have the
caller uses log on the Error.
The attached patch implements part of that. What do you think?
Cheers,
Rafael
On 24 May 2016 at 18:53, Rafael Espíndola <rafael.espindola at gmail.com> wrote:
> Taking a look.
>
> On 24 May 2016 at 18:30, Peter Collingbourne <peter at pcc.me.uk> wrote:
>> pcc added a comment.
>>
>> Thanks Lang. This new patch does as you suggested: it retains the std::error_category, but converts the error_code to an Error at the last moment. I have also updated the call sites to handle a non-null Error in the same way as they handled a true return value before.
>>
>>
>> http://reviews.llvm.org/D20550
>>
>>
>>
-------------- next part --------------
diff --git a/include/llvm/Linker/IRMover.h b/include/llvm/Linker/IRMover.h
index a1a2d50..578940e 100644
--- a/include/llvm/Linker/IRMover.h
+++ b/include/llvm/Linker/IRMover.h
@@ -15,6 +15,7 @@
#include <functional>
namespace llvm {
+class Error;
class GlobalValue;
class Metadata;
class Module;
@@ -70,10 +71,8 @@ public:
/// not present in ValuesToLink. The GlobalValue and a ValueAdder callback
/// are passed as an argument, and the callback is expected to be called
/// if the GlobalValue needs to be added to the \p ValuesToLink and linked.
- ///
- /// Returns true on error.
- bool move(std::unique_ptr<Module> Src, ArrayRef<GlobalValue *> ValuesToLink,
- std::function<void(GlobalValue &GV, ValueAdder Add)> AddLazyFor);
+ Error move(std::unique_ptr<Module> Src, ArrayRef<GlobalValue *> ValuesToLink,
+ std::function<void(GlobalValue &GV, ValueAdder Add)> AddLazyFor);
Module &getModule() { return Composite; }
private:
diff --git a/lib/Linker/IRMover.cpp b/lib/Linker/IRMover.cpp
index 0d37ad1..e732e22 100644
--- a/lib/Linker/IRMover.cpp
+++ b/lib/Linker/IRMover.cpp
@@ -17,11 +17,33 @@
#include "llvm/IR/DiagnosticPrinter.h"
#include "llvm/IR/GVMaterializer.h"
#include "llvm/IR/TypeFinder.h"
+#include "llvm/Support/Error.h"
+#include "llvm/Support/ManagedStatic.h"
#include "llvm/Transforms/Utils/Cloning.h"
using namespace llvm;
+namespace {
+class StringError : public ErrorInfo<StringError> {
+public:
+ static char ID;
+ StringError(std::string S);
+ void log(raw_ostream &OS) const override;
+ std::error_code convertToErrorCode() const override;
+
+private:
+ std::string Msg;
+};
+
+StringError::StringError(std::string S) : Msg(S) {}
+char StringError::ID = 0;
+void StringError::log(raw_ostream &OS) const { OS << Msg; }
+std::error_code StringError::convertToErrorCode() const {
+ llvm_unreachable("foo");
+}
+}
+
//===----------------------------------------------------------------------===//
-// TypeMap implementation.
+// typemap implementation.
//===----------------------------------------------------------------------===//
namespace {
@@ -401,7 +423,11 @@ class IRLinker {
/// references.
bool DoneLinkingBodies = false;
- bool HasError = false;
+ Error FoundError;
+ void setError(Error E) {
+ if (E)
+ FoundError = std::move(E);
+ }
/// Entry point for mapping values and alternate context for mapping aliases.
ValueMapper Mapper;
@@ -412,10 +438,10 @@ class IRLinker {
GlobalValue *copyGlobalValueProto(const GlobalValue *SGV, bool ForDefinition);
/// Helper method for setting a message and returning an error code.
- bool emitError(const Twine &Message) {
+ Error emitError(const Twine &Message) {
SrcM->getContext().diagnose(LinkDiagnosticInfo(DS_Error, Message));
- HasError = true;
- return true;
+ StringError X(Message.str());
+ return make_error<StringError>(X);
}
void emitWarning(const Twine &Message) {
@@ -446,8 +472,8 @@ class IRLinker {
void computeTypeMapping();
- Constant *linkAppendingVarProto(GlobalVariable *DstGV,
- const GlobalVariable *SrcGV);
+ Expected<Constant *> linkAppendingVarProto(GlobalVariable *DstGV,
+ const GlobalVariable *SrcGV);
/// Given the GlobaValue \p SGV in the source module, and the matching
/// GlobalValue \p DGV (if any), return true if the linker will pull \p SGV
@@ -455,14 +481,14 @@ class IRLinker {
///
/// Note this code may call the client-provided \p AddLazyFor.
bool shouldLink(GlobalValue *DGV, GlobalValue &SGV);
- Constant *linkGlobalValueProto(GlobalValue *GV, bool ForAlias);
+ Expected<Constant *> linkGlobalValueProto(GlobalValue *GV, bool ForAlias);
- bool linkModuleFlagsMetadata();
+ Error linkModuleFlagsMetadata();
void linkGlobalInit(GlobalVariable &Dst, GlobalVariable &Src);
- bool linkFunctionBody(Function &Dst, Function &Src);
+ Error linkFunctionBody(Function &Dst, Function &Src);
void linkAliasBody(GlobalAlias &Dst, GlobalAlias &Src);
- bool linkGlobalValueBody(GlobalValue &Dst, GlobalValue &Src);
+ Error linkGlobalValueBody(GlobalValue &Dst, GlobalValue &Src);
/// Functions that take care of cloning a specific global value type
/// into the destination module.
@@ -489,7 +515,7 @@ public:
}
~IRLinker() { SharedMDs = std::move(*ValueMap.getMDMap()); }
- bool run();
+ llvm::Error run();
Value *materializeDeclFor(Value *V, bool ForAlias);
void materializeInitFor(GlobalValue *New, GlobalValue *Old, bool ForAlias);
};
@@ -539,11 +565,19 @@ Value *IRLinker::materializeDeclFor(Value *V, bool ForAlias) {
if (!SGV)
return nullptr;
- return linkGlobalValueProto(SGV, ForAlias);
+ auto E = linkGlobalValueProto(SGV, ForAlias);
+ if (!E) {
+ setError(E.takeError());
+ return nullptr;
+ }
+ return *E;
}
void IRLinker::materializeInitFor(GlobalValue *New, GlobalValue *Old,
bool ForAlias) {
+ if (FoundError)
+ return;
+
// If we already created the body, just return.
if (auto *F = dyn_cast<Function>(New)) {
if (!F->isDeclaration())
@@ -558,7 +592,7 @@ void IRLinker::materializeInitFor(GlobalValue *New, GlobalValue *Old,
}
if (ForAlias || shouldLink(New, *Old))
- linkGlobalValueBody(*New, *Old);
+ setError(linkGlobalValueBody(*New, *Old));
}
/// Loop through the global variables in the src module and merge them into the
@@ -720,8 +754,9 @@ static void getArrayElements(const Constant *C,
/// If there were any appending global variables, link them together now.
/// Return true on error.
-Constant *IRLinker::linkAppendingVarProto(GlobalVariable *DstGV,
- const GlobalVariable *SrcGV) {
+Expected<Constant *>
+IRLinker::linkAppendingVarProto(GlobalVariable *DstGV,
+ const GlobalVariable *SrcGV) {
Type *EltTy = cast<ArrayType>(TypeMap.get(SrcGV->getValueType()))
->getElementType();
@@ -751,46 +786,33 @@ Constant *IRLinker::linkAppendingVarProto(GlobalVariable *DstGV,
ArrayType *DstTy = cast<ArrayType>(DstGV->getValueType());
DstNumElements = DstTy->getNumElements();
- if (!SrcGV->hasAppendingLinkage() || !DstGV->hasAppendingLinkage()) {
- emitError(
+ if (!SrcGV->hasAppendingLinkage() || !DstGV->hasAppendingLinkage())
+ return emitError(
"Linking globals named '" + SrcGV->getName() +
- "': can only link appending global with another appending global!");
- return nullptr;
- }
+ "': can only link appending global with another appending "
+ "global!");
// Check to see that they two arrays agree on type.
- if (EltTy != DstTy->getElementType()) {
- emitError("Appending variables with different element types!");
- return nullptr;
- }
- if (DstGV->isConstant() != SrcGV->isConstant()) {
- emitError("Appending variables linked with different const'ness!");
- return nullptr;
- }
+ if (EltTy != DstTy->getElementType())
+ return emitError("Appending variables with different element types!");
+ if (DstGV->isConstant() != SrcGV->isConstant())
+ return emitError("Appending variables linked with different const'ness!");
- if (DstGV->getAlignment() != SrcGV->getAlignment()) {
- emitError(
+ if (DstGV->getAlignment() != SrcGV->getAlignment())
+ return emitError(
"Appending variables with different alignment need to be linked!");
- return nullptr;
- }
- if (DstGV->getVisibility() != SrcGV->getVisibility()) {
- emitError(
+ if (DstGV->getVisibility() != SrcGV->getVisibility())
+ return emitError(
"Appending variables with different visibility need to be linked!");
- return nullptr;
- }
- if (DstGV->hasUnnamedAddr() != SrcGV->hasUnnamedAddr()) {
- emitError(
+ if (DstGV->hasUnnamedAddr() != SrcGV->hasUnnamedAddr())
+ return emitError(
"Appending variables with different unnamed_addr need to be linked!");
- return nullptr;
- }
- if (DstGV->getSection() != SrcGV->getSection()) {
- emitError(
+ if (DstGV->getSection() != SrcGV->getSection())
+ return emitError(
"Appending variables with different section name need to be linked!");
- return nullptr;
- }
}
SmallVector<Constant *, 16> SrcElements;
@@ -865,7 +887,8 @@ bool IRLinker::shouldLink(GlobalValue *DGV, GlobalValue &SGV) {
return LazilyAdded;
}
-Constant *IRLinker::linkGlobalValueProto(GlobalValue *SGV, bool ForAlias) {
+Expected<Constant *> IRLinker::linkGlobalValueProto(GlobalValue *SGV,
+ bool ForAlias) {
GlobalValue *DGV = getLinkedToGlobal(SGV);
bool ShouldLink = shouldLink(DGV, *SGV);
@@ -939,12 +962,14 @@ void IRLinker::linkGlobalInit(GlobalVariable &Dst, GlobalVariable &Src) {
/// Copy the source function over into the dest function and fix up references
/// to values. At this point we know that Dest is an external function, and
/// that Src is not.
-bool IRLinker::linkFunctionBody(Function &Dst, Function &Src) {
+Error IRLinker::linkFunctionBody(Function &Dst, Function &Src) {
assert(Dst.isDeclaration() && !Src.isDeclaration());
// Materialize if needed.
- if (std::error_code EC = Src.materialize())
- return emitError(EC.message());
+ if (std::error_code EC = Src.materialize()) {
+ SrcM->getContext().diagnose(LinkDiagnosticInfo(DS_Error, EC.message()));
+ return make_error<ECError>(EC);
+ }
// Link in the operands without remapping.
if (Src.hasPrefixData())
@@ -966,22 +991,22 @@ bool IRLinker::linkFunctionBody(Function &Dst, Function &Src) {
// Everything has been moved over. Remap it.
Mapper.scheduleRemapFunction(Dst);
- return false;
+ return Error::success();
}
void IRLinker::linkAliasBody(GlobalAlias &Dst, GlobalAlias &Src) {
Mapper.scheduleMapGlobalAliasee(Dst, *Src.getAliasee(), AliasMCID);
}
-bool IRLinker::linkGlobalValueBody(GlobalValue &Dst, GlobalValue &Src) {
+Error IRLinker::linkGlobalValueBody(GlobalValue &Dst, GlobalValue &Src) {
if (auto *F = dyn_cast<Function>(&Src))
return linkFunctionBody(cast<Function>(Dst), *F);
if (auto *GVar = dyn_cast<GlobalVariable>(&Src)) {
linkGlobalInit(cast<GlobalVariable>(Dst), *GVar);
- return false;
+ return Error::success();
}
linkAliasBody(cast<GlobalAlias>(Dst), cast<GlobalAlias>(Src));
- return false;
+ return Error::success();
}
/// Insert all of the named MDNodes in Src into the Dest module.
@@ -999,11 +1024,11 @@ void IRLinker::linkNamedMDNodes() {
}
/// Merge the linker flags in Src into the Dest module.
-bool IRLinker::linkModuleFlagsMetadata() {
+Error IRLinker::linkModuleFlagsMetadata() {
// If the source module has no module flags, we are done.
const NamedMDNode *SrcModFlags = SrcM->getModuleFlagsMetadata();
if (!SrcModFlags)
- return false;
+ return Error::success();
// If the destination module doesn't have module flags yet, then just copy
// over the source module's flags.
@@ -1012,7 +1037,7 @@ bool IRLinker::linkModuleFlagsMetadata() {
for (unsigned I = 0, E = SrcModFlags->getNumOperands(); I != E; ++I)
DstModFlags->addOperand(SrcModFlags->getOperand(I));
- return false;
+ return Error::success();
}
// First build a map of the existing module flags and requirements.
@@ -1068,10 +1093,9 @@ bool IRLinker::linkModuleFlagsMetadata() {
if (DstBehaviorValue == Module::Override) {
// Diagnose inconsistent flags which both have override behavior.
if (SrcBehaviorValue == Module::Override &&
- SrcOp->getOperand(2) != DstOp->getOperand(2)) {
- emitError("linking module flags '" + ID->getString() +
- "': IDs have conflicting override values");
- }
+ SrcOp->getOperand(2) != DstOp->getOperand(2))
+ return emitError("linking module flags '" + ID->getString() +
+ "': IDs have conflicting override values");
continue;
} else if (SrcBehaviorValue == Module::Override) {
// Update the destination flag to that of the source.
@@ -1081,11 +1105,9 @@ bool IRLinker::linkModuleFlagsMetadata() {
}
// Diagnose inconsistent merge behavior types.
- if (SrcBehaviorValue != DstBehaviorValue) {
- emitError("linking module flags '" + ID->getString() +
- "': IDs have conflicting behaviors");
- continue;
- }
+ if (SrcBehaviorValue != DstBehaviorValue)
+ return emitError("linking module flags '" + ID->getString() +
+ "': IDs have conflicting behaviors");
auto replaceDstValue = [&](MDNode *New) {
Metadata *FlagOps[] = {DstOp->getOperand(0), ID, New};
@@ -1101,10 +1123,9 @@ bool IRLinker::linkModuleFlagsMetadata() {
llvm_unreachable("not possible");
case Module::Error: {
// Emit an error if the values differ.
- if (SrcOp->getOperand(2) != DstOp->getOperand(2)) {
- emitError("linking module flags '" + ID->getString() +
- "': IDs have conflicting values");
- }
+ if (SrcOp->getOperand(2) != DstOp->getOperand(2))
+ return emitError("linking module flags '" + ID->getString() +
+ "': IDs have conflicting values");
continue;
}
case Module::Warning: {
@@ -1147,14 +1168,11 @@ bool IRLinker::linkModuleFlagsMetadata() {
Metadata *ReqValue = Requirement->getOperand(1);
MDNode *Op = Flags[Flag].first;
- if (!Op || Op->getOperand(2) != ReqValue) {
- emitError("linking module flags '" + Flag->getString() +
- "': does not have the required value");
- continue;
- }
+ if (!Op || Op->getOperand(2) != ReqValue)
+ return emitError("linking module flags '" + Flag->getString() +
+ "': does not have the required value");
}
-
- return HasError;
+ return Error::success();
}
// This function returns true if the triples match.
@@ -1178,10 +1196,11 @@ static std::string mergeTriples(const Triple &SrcTriple,
return DstTriple.str();
}
-bool IRLinker::run() {
+Error IRLinker::run() {
// Ensure metadata materialized before value mapping.
- if (SrcM->getMaterializer() && SrcM->getMaterializer()->materializeMetadata())
- return true;
+ if (SrcM->getMaterializer())
+ if (std::error_code EC = SrcM->getMaterializer()->materializeMetadata())
+ return make_error<ECError>(EC);
// Inherit the target data from the source module if the destination module
// doesn't have one already.
@@ -1235,8 +1254,6 @@ bool IRLinker::run() {
assert(!GV->isDeclaration());
Mapper.mapValue(*GV);
- if (HasError)
- return true;
}
// Note that we are done linking global value bodies. This prevents
@@ -1250,10 +1267,9 @@ bool IRLinker::run() {
linkNamedMDNodes();
// Merge the module flags into the DstM module.
- if (linkModuleFlagsMetadata())
- return true;
-
- return false;
+ if (auto E = linkModuleFlagsMetadata())
+ return E;
+ return std::move(FoundError);
}
IRMover::StructTypeKeyInfo::KeyTy::KeyTy(ArrayRef<Type *> E, bool P)
@@ -1357,12 +1373,12 @@ IRMover::IRMover(Module &M) : Composite(M) {
}
}
-bool IRMover::move(
+Error IRMover::move(
std::unique_ptr<Module> Src, ArrayRef<GlobalValue *> ValuesToLink,
std::function<void(GlobalValue &, ValueAdder Add)> AddLazyFor) {
IRLinker TheIRLinker(Composite, SharedMDs, IdentifiedStructTypes,
std::move(Src), ValuesToLink, AddLazyFor);
- bool RetCode = TheIRLinker.run();
+ Error E = TheIRLinker.run();
Composite.dropTriviallyDeadConstantArrays();
- return RetCode;
+ return E;
}
diff --git a/lib/Linker/LinkModules.cpp b/lib/Linker/LinkModules.cpp
index 5b713fe..8476639 100644
--- a/lib/Linker/LinkModules.cpp
+++ b/lib/Linker/LinkModules.cpp
@@ -18,6 +18,7 @@
#include "llvm/IR/DiagnosticPrinter.h"
#include "llvm/IR/LLVMContext.h"
#include "llvm/Linker/Linker.h"
+#include "llvm/Support/Error.h"
#include "llvm/Transforms/Utils/FunctionImportUtils.h"
using namespace llvm;
@@ -574,11 +575,24 @@ bool ModuleLinker::run() {
Internalize.insert(GV->getName());
}
- if (Mover.move(std::move(SrcM), ValuesToLink.getArrayRef(),
- [this](GlobalValue &GV, IRMover::ValueAdder Add) {
- addLazyFor(GV, Add);
- }))
+ // FIXME: Once we no longer need to be able to losslessly convert from
+ // llvm::Error to std::error_code, we should teach IRMover to encode error
+ // messages directly as Errors and return them here. Until then, an Error from
+ // IRMover causes us to return true here, and we rely on IRMover to send a
+ // more descriptive error message to the DiagnosticHandler.
+ bool HasErrors = false;
+ if (Error E = Mover.move(std::move(SrcM), ValuesToLink.getArrayRef(),
+ [this](GlobalValue &GV, IRMover::ValueAdder Add) {
+ addLazyFor(GV, Add);
+ })) {
+ handleAllErrors(std::move(E), [&](ErrorInfoBase &EIB) {
+ HasErrors = true;
+ return Error::success();
+ });
+ }
+ if (HasErrors)
return true;
+
for (auto &P : Internalize) {
GlobalValue *GV = DstM.getNamedValue(P.first());
GV->setLinkage(GlobalValue::InternalLinkage);
diff --git a/tools/gold/gold-plugin.cpp b/tools/gold/gold-plugin.cpp
index a72ae07..1de5794 100644
--- a/tools/gold/gold-plugin.cpp
+++ b/tools/gold/gold-plugin.cpp
@@ -1097,8 +1097,8 @@ void CodeGen::runAll() {
}
/// Links the module in \p View from file \p F into the combined module
-/// saved in the IRMover \p L. Returns true on error, false on success.
-static bool linkInModule(LLVMContext &Context, IRMover &L, claimed_file &F,
+/// saved in the IRMover \p L.
+static void linkInModule(LLVMContext &Context, IRMover &L, claimed_file &F,
const void *View, ld_plugin_input_file &File,
raw_fd_ostream *ApiFile, StringSet<> &Internalize,
StringSet<> &Maybe) {
@@ -1107,7 +1107,7 @@ static bool linkInModule(LLVMContext &Context, IRMover &L, claimed_file &F,
std::unique_ptr<Module> M = getModuleForFile(
Context, F, View, File, ApiFile, Internalize, Maybe, Keep, Realign);
if (!M.get())
- return false;
+ return;
if (!options::triple.empty())
M->setTargetTriple(options::triple.c_str());
else if (M->getTargetTriple().empty()) {
@@ -1115,7 +1115,7 @@ static bool linkInModule(LLVMContext &Context, IRMover &L, claimed_file &F,
}
if (L.move(std::move(M), Keep, [](GlobalValue &, IRMover::ValueAdder) {}))
- return true;
+ message(LDPL_FATAL, "Failed to link module");
for (const auto &I : Realign) {
GlobalValue *Dst = L.getModule().getNamedValue(I.first());
@@ -1123,8 +1123,6 @@ static bool linkInModule(LLVMContext &Context, IRMover &L, claimed_file &F,
continue;
cast<GlobalVariable>(Dst)->setAlignment(I.second);
}
-
- return false;
}
/// Perform the ThinLTO backend on a single module, invoking the LTO and codegen
@@ -1145,8 +1143,7 @@ static void thinLTOBackendTask(claimed_file &F, const void *View,
IRMover L(*NewModule.get());
StringSet<> Dummy;
- if (linkInModule(Context, L, F, View, File, ApiFile, Dummy, Dummy))
- message(LDPL_FATAL, "Failed to rename module for ThinLTO");
+ linkInModule(Context, L, F, View, File, ApiFile, Dummy, Dummy);
if (renameModuleForThinLTO(*NewModule, CombinedIndex))
message(LDPL_FATAL, "Failed to rename module for ThinLTO");
@@ -1368,9 +1365,8 @@ static ld_plugin_status allSymbolsReadHook(raw_fd_ostream *ApiFile) {
const void *View = getSymbolsAndView(F);
if (!View)
continue;
- if (linkInModule(Context, L, F, View, InputFile.file(), ApiFile,
- Internalize, Maybe))
- message(LDPL_FATAL, "Failed to link module");
+ linkInModule(Context, L, F, View, InputFile.file(), ApiFile, Internalize,
+ Maybe);
}
for (const auto &Name : Internalize) {
More information about the llvm-commits
mailing list