Index: include/llvm/Linker/IRMover.h =================================================================== --- include/llvm/Linker/IRMover.h +++ include/llvm/Linker/IRMover.h @@ -15,6 +15,7 @@ #include namespace llvm { +class Error; class GlobalValue; class Metadata; class Module; @@ -70,10 +71,8 @@ /// 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 Src, ArrayRef ValuesToLink, - std::function AddLazyFor); + Error move(std::unique_ptr Src, ArrayRef ValuesToLink, + std::function AddLazyFor); Module &getModule() { return Composite; } private: Index: include/llvm/Support/Error.h =================================================================== --- include/llvm/Support/Error.h +++ include/llvm/Support/Error.h @@ -26,6 +26,7 @@ class Error; class ErrorList; +class Twine; /// Base class for error info classes. Do not extend this directly: Extend /// the ErrorInfo template subclass instead. @@ -883,6 +884,23 @@ return std::move(*E); } +/// This class wraps a string in an Error. +/// +/// StringError is useful in cases where the client is not expected to be able +/// to consume the specific error message programmatically (for example, if the +/// error message is to be presented to the user). It cannot be converted to a +/// std::error_code. +class StringError : public ErrorInfo { +public: + static char ID; + StringError(const Twine &S); + void log(raw_ostream &OS) const override; + std::error_code convertToErrorCode() const override; + +private: + std::string Msg; +}; + /// Helper for check-and-exit error handling. /// /// For tool use only. NOT FOR USE IN LIBRARY CODE. Index: lib/Linker/IRMover.cpp =================================================================== --- lib/Linker/IRMover.cpp +++ lib/Linker/IRMover.cpp @@ -17,6 +17,7 @@ #include "llvm/IR/DiagnosticPrinter.h" #include "llvm/IR/GVMaterializer.h" #include "llvm/IR/TypeFinder.h" +#include "llvm/Support/Error.h" #include "llvm/Transforms/Utils/Cloning.h" using namespace llvm; @@ -399,7 +400,13 @@ /// references. bool DoneLinkingBodies = false; - bool HasError = false; + /// The Error encountered during materialization. We use an Optional here to + /// avoid needing to manage an unconsumed success value. + Optional FoundError; + void setError(Error E) { + if (E) + FoundError = std::move(E); + } /// Entry point for mapping values and alternate context for mapping aliases. ValueMapper Mapper; @@ -409,13 +416,6 @@ /// the destination module, including setting the attributes and visibility. GlobalValue *copyGlobalValueProto(const GlobalValue *SGV, bool ForDefinition); - /// Helper method for setting a message and returning an error code. - bool emitError(const Twine &Message) { - SrcM->getContext().diagnose(LinkDiagnosticInfo(DS_Error, Message)); - HasError = true; - return true; - } - void emitWarning(const Twine &Message) { SrcM->getContext().diagnose(LinkDiagnosticInfo(DS_Warning, Message)); } @@ -444,8 +444,8 @@ void computeTypeMapping(); - Constant *linkAppendingVarProto(GlobalVariable *DstGV, - const GlobalVariable *SrcGV); + Expected 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 @@ -453,14 +453,14 @@ /// /// Note this code may call the client-provided \p AddLazyFor. bool shouldLink(GlobalValue *DGV, GlobalValue &SGV); - Constant *linkGlobalValueProto(GlobalValue *GV, bool ForAlias); + Expected 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. @@ -487,7 +487,7 @@ } ~IRLinker() { SharedMDs = std::move(*ValueMap.getMDMap()); } - bool run(); + Error run(); Value *materialize(Value *V, bool ForAlias); }; } @@ -526,13 +526,17 @@ if (!SGV) return nullptr; - Constant *NewProto = linkGlobalValueProto(SGV, ForAlias); - if (!NewProto) - return NewProto; + Expected NewProto = linkGlobalValueProto(SGV, ForAlias); + if (!NewProto) { + setError(NewProto.takeError()); + return nullptr; + } + if (!*NewProto) + return nullptr; - GlobalValue *New = dyn_cast(NewProto); + GlobalValue *New = dyn_cast(*NewProto); if (!New) - return NewProto; + return *NewProto; // If we already created the body, just return. if (auto *F = dyn_cast(New)) { @@ -559,7 +563,7 @@ return New; if (ForAlias || shouldLink(New, *SGV)) - linkGlobalValueBody(*New, *SGV); + setError(linkGlobalValueBody(*New, *SGV)); return New; } @@ -727,9 +731,9 @@ } /// If there were any appending global variables, link them together now. -/// Return true on error. -Constant *IRLinker::linkAppendingVarProto(GlobalVariable *DstGV, - const GlobalVariable *SrcGV) { +Expected +IRLinker::linkAppendingVarProto(GlobalVariable *DstGV, + const GlobalVariable *SrcGV) { Type *EltTy = cast(TypeMap.get(SrcGV->getValueType())) ->getElementType(); @@ -759,46 +763,35 @@ ArrayType *DstTy = cast(DstGV->getValueType()); DstNumElements = DstTy->getNumElements(); - if (!SrcGV->hasAppendingLinkage() || !DstGV->hasAppendingLinkage()) { - emitError( + if (!SrcGV->hasAppendingLinkage() || !DstGV->hasAppendingLinkage()) + return make_error( "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 (DstGV->getAlignment() != SrcGV->getAlignment()) { - emitError( + if (EltTy != DstTy->getElementType()) + return make_error( + "Appending variables with different element types!"); + if (DstGV->isConstant() != SrcGV->isConstant()) + return make_error( + "Appending variables linked with different const'ness!"); + + if (DstGV->getAlignment() != SrcGV->getAlignment()) + return make_error( "Appending variables with different alignment need to be linked!"); - return nullptr; - } - if (DstGV->getVisibility() != SrcGV->getVisibility()) { - emitError( + if (DstGV->getVisibility() != SrcGV->getVisibility()) + return make_error( "Appending variables with different visibility need to be linked!"); - return nullptr; - } - if (DstGV->hasUnnamedAddr() != SrcGV->hasUnnamedAddr()) { - emitError( + if (DstGV->hasUnnamedAddr() != SrcGV->hasUnnamedAddr()) + return make_error( "Appending variables with different unnamed_addr need to be linked!"); - return nullptr; - } - if (DstGV->getSection() != SrcGV->getSection()) { - emitError( + if (DstGV->getSection() != SrcGV->getSection()) + return make_error( "Appending variables with different section name need to be linked!"); - return nullptr; - } } SmallVector SrcElements; @@ -873,7 +866,8 @@ return LazilyAdded; } -Constant *IRLinker::linkGlobalValueProto(GlobalValue *SGV, bool ForAlias) { +Expected IRLinker::linkGlobalValueProto(GlobalValue *SGV, + bool ForAlias) { GlobalValue *DGV = getLinkedToGlobal(SGV); bool ShouldLink = shouldLink(DGV, *SGV); @@ -947,12 +941,12 @@ /// 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()); + return errorCodeToError(EC); // Link in the operands without remapping. if (Src.hasPrefixData()) @@ -974,22 +968,22 @@ // 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(&Src)) return linkFunctionBody(cast(Dst), *F); if (auto *GVar = dyn_cast(&Src)) { linkGlobalInit(cast(Dst), *GVar); - return false; + return Error::success(); } linkAliasBody(cast(Dst), cast(Src)); - return false; + return Error::success(); } /// Insert all of the named MDNodes in Src into the Dest module. @@ -1007,11 +1001,11 @@ } /// 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. @@ -1020,7 +1014,7 @@ 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. @@ -1076,10 +1070,10 @@ 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 make_error( + "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. @@ -1089,11 +1083,10 @@ } // Diagnose inconsistent merge behavior types. - if (SrcBehaviorValue != DstBehaviorValue) { - emitError("linking module flags '" + ID->getString() + - "': IDs have conflicting behaviors"); - continue; - } + if (SrcBehaviorValue != DstBehaviorValue) + return make_error("linking module flags '" + + ID->getString() + + "': IDs have conflicting behaviors"); auto replaceDstValue = [&](MDNode *New) { Metadata *FlagOps[] = {DstOp->getOperand(0), ID, New}; @@ -1109,10 +1102,10 @@ 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 make_error("linking module flags '" + + ID->getString() + + "': IDs have conflicting values"); continue; } case Module::Warning: { @@ -1155,14 +1148,12 @@ 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 make_error("linking module flags '" + + Flag->getString() + + "': does not have the required value"); } - - return HasError; + return Error::success(); } // This function returns true if the triples match. @@ -1186,10 +1177,11 @@ 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 errorCodeToError(EC); // Inherit the target data from the source module if the destination module // doesn't have one already. @@ -1243,8 +1235,8 @@ assert(!GV->isDeclaration()); Mapper.mapValue(*GV); - if (HasError) - return true; + if (FoundError) + return std::move(*FoundError); } // Note that we are done linking global value bodies. This prevents @@ -1258,10 +1250,7 @@ linkNamedMDNodes(); // Merge the module flags into the DstM module. - if (linkModuleFlagsMetadata()) - return true; - - return false; + return linkModuleFlagsMetadata(); } IRMover::StructTypeKeyInfo::KeyTy::KeyTy(ArrayRef E, bool P) @@ -1365,12 +1354,12 @@ } } -bool IRMover::move( +Error IRMover::move( std::unique_ptr Src, ArrayRef ValuesToLink, std::function 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; } Index: lib/Linker/LinkModules.cpp =================================================================== --- lib/Linker/LinkModules.cpp +++ 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,22 @@ Internalize.insert(GV->getName()); } - if (Mover.move(std::move(SrcM), ValuesToLink.getArrayRef(), - [this](GlobalValue &GV, IRMover::ValueAdder Add) { - addLazyFor(GV, Add); - })) + // FIXME: Propagate Errors through to the caller instead of emitting + // diagnostics. + 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) { + DstM.getContext().diagnose(LinkDiagnosticInfo(DS_Error, EIB.message())); + HasErrors = true; + return Error::success(); + }); + } + if (HasErrors) return true; + for (auto &P : Internalize) { GlobalValue *GV = DstM.getNamedValue(P.first()); GV->setLinkage(GlobalValue::InternalLinkage); Index: lib/Support/Error.cpp =================================================================== --- lib/Support/Error.cpp +++ lib/Support/Error.cpp @@ -7,6 +7,7 @@ // //===----------------------------------------------------------------------===// +#include "llvm/ADT/Twine.h" #include "llvm/Support/Error.h" #include "llvm/Support/ErrorHandling.h" #include "llvm/Support/ManagedStatic.h" @@ -48,3 +49,12 @@ return std::error_code(static_cast(ErrorErrorCode::MultipleErrors), *ErrorErrorCat); } + +StringError::StringError(const Twine &S) : Msg(S.str()) {} +char StringError::ID = 0; +void StringError::log(raw_ostream &OS) const { OS << Msg; } +std::error_code StringError::convertToErrorCode() const { + // A StringError cannot be converted to an error_code. Bail and report our + // error message to the user, which is probably better than nothing. + report_fatal_error(Msg); +} Index: test/Linker/module-flags-4-a.ll =================================================================== --- test/Linker/module-flags-4-a.ll +++ test/Linker/module-flags-4-a.ll @@ -5,6 +5,6 @@ ; CHECK: linking module flags 'bar': does not have the required value !0 = !{ i32 1, !"foo", i32 37 } -!1 = !{ i32 1, !"bar", i32 927 } +!1 = !{ i32 4, !"bar", i32 927 } !llvm.module.flags = !{ !0, !1 } Index: test/Linker/module-flags-4-b.ll =================================================================== --- test/Linker/module-flags-4-b.ll +++ test/Linker/module-flags-4-b.ll @@ -2,6 +2,6 @@ ; RUN: true !0 = !{i32 3, !"foo", !{!"bar", i32 42}} -!1 = !{i32 2, !"bar", i32 42} +!1 = !{i32 1, !"bar", i32 42} !llvm.module.flags = !{ !0, !1 } Index: test/tools/gold/X86/Inputs/irmover-error.ll =================================================================== --- /dev/null +++ test/tools/gold/X86/Inputs/irmover-error.ll @@ -0,0 +1,3 @@ +!0 = !{ i32 1, !"foo", i32 2 } + +!llvm.module.flags = !{ !0 } Index: test/tools/gold/X86/irmover-error.ll =================================================================== --- /dev/null +++ test/tools/gold/X86/irmover-error.ll @@ -0,0 +1,9 @@ +; RUN: llvm-as -o %t1.bc %s +; RUN: llvm-as -o %t2.bc %S/Inputs/irmover-error.ll +; RUN: not %gold -plugin %llvmshlibdir/LLVMgold.so -o %t %t1.bc %t2.bc 2>&1 | FileCheck %s + +; CHECK: fatal error: Failed to link module {{.*}}2.bc: linking module flags 'foo': IDs have conflicting values + +!0 = !{ i32 1, !"foo", i32 1 } + +!llvm.module.flags = !{ !0 } Index: tools/gold/gold-plugin.cpp =================================================================== --- tools/gold/gold-plugin.cpp +++ tools/gold/gold-plugin.cpp @@ -1131,8 +1131,8 @@ } /// 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, StringRef Name, raw_fd_ostream *ApiFile, StringSet<> &Internalize, StringSet<> &Maybe) { @@ -1141,15 +1141,21 @@ std::unique_ptr M = getModuleForFile( Context, F, View, Name, 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()) { M->setTargetTriple(DefaultTriple); } - if (L.move(std::move(M), Keep, [](GlobalValue &, IRMover::ValueAdder) {})) - return true; + if (Error E = L.move(std::move(M), Keep, + [](GlobalValue &, IRMover::ValueAdder) {})) { + handleAllErrors(std::move(E), [&](const llvm::ErrorInfoBase &EIB) { + message(LDPL_FATAL, "Failed to link module %s: %s", Name.str().c_str(), + EIB.message().c_str()); + return Error::success(); + }); + } for (const auto &I : Realign) { GlobalValue *Dst = L.getModule().getNamedValue(I.first()); @@ -1157,8 +1163,6 @@ continue; cast(Dst)->setAlignment(I.second); } - - return false; } /// Perform the ThinLTO backend on a single module, invoking the LTO and codegen @@ -1179,8 +1183,7 @@ IRMover L(*NewModule.get()); StringSet<> Dummy; - if (linkInModule(Context, L, F, View, Name, ApiFile, Dummy, Dummy)) - message(LDPL_FATAL, "Failed to rename module for ThinLTO"); + linkInModule(Context, L, F, View, Name, ApiFile, Dummy, Dummy); if (renameModuleForThinLTO(*NewModule, CombinedIndex)) message(LDPL_FATAL, "Failed to rename module for ThinLTO"); @@ -1420,8 +1423,7 @@ const void *View = getSymbolsAndView(F); if (!View) continue; - if (linkInModule(Context, L, F, View, F.name, ApiFile, Internalize, Maybe)) - message(LDPL_FATAL, "Failed to link module"); + linkInModule(Context, L, F, View, F.name, ApiFile, Internalize, Maybe); } for (const auto &Name : Internalize) {