[LTO] Fix error reporting from lto_module_create_in_local_context()

[petpav01]

Summary

Function lto_module_create_in_local_context() would previously rely on the default LLVMContext being created for it by LTOModule::makeLTOModule(). This context exits the program on error and is not arranged to update sLastStringError in tools/lto/lto.cpp.

Function lto_module_create_in_local_context() now creates an LLVMContext by itself, sets it up correctly to its needs and then passes it to LTOModule::createInLocalContext() which takes ownership of the context and keeps it present for the lifetime of the returned LTOModule.

Function LTOModule::makeLTOModule() is modified to take a reference to LLVMContext (instead of a pointer) and no longer creates a default context when nullptr is passed to it. Method LTOModule::createInContext() that takes a pointer to LLVMContext is removed because it allows to pass a nullptr to it. Instead LTOModule::createFromBuffer() (that takes a reference to LLVMContext) should be used.

[• rafael]

I really like it, thanks!

I will leave the final LGTM to Duncan or some other Apple to make sure it works for them.

[dexonsmith]

LGTM. Thanks for the fix!

include/llvm/LTO/LTOModule.h

private:
   std::vector<NameAndAttributes> _symbols;
 
   // _defines and _undefines only needed to disambiguate tentative definitions
   StringSet<>                             _defines;
   StringMap<NameAndAttributes> _undefines;
   std::vector<const char*>                _asm_undefines;
 
   LTOModule(std::unique_ptr<object::IRObjectFile> Obj, TargetMachine *TM);
-  LTOModule(std::unique_ptr<object::IRObjectFile> Obj, TargetMachine *TM,
-            std::unique_ptr<LLVMContext> Context);
 
 public:
   ~LTOModule();
 
   /// Returns 'true' if the file or memory contents is LLVM bitcode.
   static bool isBitcodeFile(const void *mem, size_t length);
   static bool isBitcodeFile(const char *path);
 
public:
   createFromOpenFile(LLVMContext &Context, int fd, const char *path,
                      size_t size, TargetOptions options);
   static ErrorOr<std::unique_ptr<LTOModule>>
   createFromOpenFileSlice(LLVMContext &Context, int fd, const char *path,
                           size_t map_size, off_t offset, TargetOptions options);
   static ErrorOr<std::unique_ptr<LTOModule>>
   createFromBuffer(LLVMContext &Context, const void *mem, size_t length,
                    TargetOptions options, StringRef path = "");
-
-  static ErrorOr<std::unique_ptr<LTOModule>>
-  createInLocalContext(const void *mem, size_t length, TargetOptions options,
-                       StringRef path);
   static ErrorOr<std::unique_ptr<LTOModule>>
-  createInContext(const void *mem, size_t length, TargetOptions options,
-                  StringRef path, LLVMContext *Context);
+  createInLocalContext(std::unique_ptr<LLVMContext> Context, const void *mem,
+                       size_t length, TargetOptions options, StringRef path);
 
   const Module &getModule() const {
     return const_cast<LTOModule*>(this)->getModule();
   }
   Module &getModule() {
     return IRFile->getModule();
   }
 
private:
   void addObjCClassRef(const GlobalVariable *clgv);
 
   /// Get string that the data pointer points to.
   bool objcClassNameFromExpression(const Constant *c, std::string &name);
 
   /// Create an LTOModule (private version).
   static ErrorOr<std::unique_ptr<LTOModule>>
   makeLTOModule(MemoryBufferRef Buffer, TargetOptions options,
-                LLVMContext *Context);
+                LLVMContext &Context, bool ShouldBeLazy);
 };
 }
 #endif

lib/LTO/LTOModule.cpp

 #include <system_error>
 using namespace llvm;
 using namespace llvm::object;
 
 LTOModule::LTOModule(std::unique_ptr<object::IRObjectFile> Obj,
                      llvm::TargetMachine *TM)
     : IRFile(std::move(Obj)), _target(TM) {}
 
-LTOModule::LTOModule(std::unique_ptr<object::IRObjectFile> Obj,
-                     llvm::TargetMachine *TM,
-                     std::unique_ptr<LLVMContext> Context)
-    : OwnedContext(std::move(Context)), IRFile(std::move(Obj)), _target(TM) {}
-
 LTOModule::~LTOModule() {}
 
 /// isBitcodeFile - Returns 'true' if the file (or memory contents) is LLVM
 /// bitcode.
 bool LTOModule::isBitcodeFile(const void *Mem, size_t Length) {
   ErrorOr<MemoryBufferRef> BCData = IRObjectFile::findBitcodeInMemBuffer(
       MemoryBufferRef(StringRef((const char *)Mem, Length), "<mem>"));
   return bool(BCData);
LTOModule::createFromFile(LLVMContext &Context, const char *path,
                           TargetOptions options) {
   ErrorOr<std::unique_ptr<MemoryBuffer>> BufferOrErr =
       MemoryBuffer::getFile(path);
   if (std::error_code EC = BufferOrErr.getError()) {
     Context.emitError(EC.message());
     return EC;
   }
   std::unique_ptr<MemoryBuffer> Buffer = std::move(BufferOrErr.get());
-  return makeLTOModule(Buffer->getMemBufferRef(), options, &Context);
+  return makeLTOModule(Buffer->getMemBufferRef(), options, Context,
+                       /* ShouldBeLazy*/ false);
 }
 
 ErrorOr<std::unique_ptr<LTOModule>>
 LTOModule::createFromOpenFile(LLVMContext &Context, int fd, const char *path,
                               size_t size, TargetOptions options) {
   return createFromOpenFileSlice(Context, fd, path, size, 0, options);
 }
 
 ErrorOr<std::unique_ptr<LTOModule>>
 LTOModule::createFromOpenFileSlice(LLVMContext &Context, int fd,
                                    const char *path, size_t map_size,
                                    off_t offset, TargetOptions options) {
   ErrorOr<std::unique_ptr<MemoryBuffer>> BufferOrErr =
       MemoryBuffer::getOpenFileSlice(fd, path, map_size, offset);
   if (std::error_code EC = BufferOrErr.getError()) {
     Context.emitError(EC.message());
     return EC;
   }
   std::unique_ptr<MemoryBuffer> Buffer = std::move(BufferOrErr.get());
-  return makeLTOModule(Buffer->getMemBufferRef(), options, &Context);
+  return makeLTOModule(Buffer->getMemBufferRef(), options, Context,
+                       /* ShouldBeLazy */ false);
 }
 
 ErrorOr<std::unique_ptr<LTOModule>>
 LTOModule::createFromBuffer(LLVMContext &Context, const void *mem,
                             size_t length, TargetOptions options,
                             StringRef path) {
-  return createInContext(mem, length, options, path, &Context);
+  StringRef Data((const char *)mem, length);
+  MemoryBufferRef Buffer(Data, path);
+  return makeLTOModule(Buffer, options, Context, /* ShouldBeLazy */ false);
 }
 
 ErrorOr<std::unique_ptr<LTOModule>>
-LTOModule::createInLocalContext(const void *mem, size_t length,
+LTOModule::createInLocalContext(std::unique_ptr<LLVMContext> Context,
+                                const void *mem, size_t length,
                                 TargetOptions options, StringRef path) {
-  return createInContext(mem, length, options, path, nullptr);
-}
-
-ErrorOr<std::unique_ptr<LTOModule>>
-LTOModule::createInContext(const void *mem, size_t length,
-                           TargetOptions options, StringRef path,
-                           LLVMContext *Context) {
   StringRef Data((const char *)mem, length);
   MemoryBufferRef Buffer(Data, path);
-  return makeLTOModule(Buffer, options, Context);
+  // If we own a context, we know this is being used only for symbol extraction,
+  // not linking.  Be lazy in that case.
+  ErrorOr<std::unique_ptr<LTOModule>> Ret =
+      makeLTOModule(Buffer, options, *Context, /* ShouldBeLazy */ true);
+  if (Ret)
+    (*Ret)->OwnedContext = std::move(Context);
+  return std::move(Ret);
 }
 
 static ErrorOr<std::unique_ptr<Module>>
 parseBitcodeFileImpl(MemoryBufferRef Buffer, LLVMContext &Context,
                      bool ShouldBeLazy) {
 
   // Find the buffer.
   ErrorOr<MemoryBufferRef> MBOrErr =
ErrorOr<std::unique_ptr<Module>> M = getLazyBitcodeModule(
       std::move(LightweightBuf), Context, true /*ShouldLazyLoadMetadata*/);
   if (std::error_code EC = M.getError())
     return EC;
   return std::move(*M);
 }
 
 ErrorOr<std::unique_ptr<LTOModule>>
 LTOModule::makeLTOModule(MemoryBufferRef Buffer, TargetOptions options,
-                         LLVMContext *Context) {
-  std::unique_ptr<LLVMContext> OwnedContext;
-  if (!Context) {
-    OwnedContext = llvm::make_unique<LLVMContext>();
-    Context = OwnedContext.get();
-  }
-
-  // If we own a context, we know this is being used only for symbol
-  // extraction, not linking.  Be lazy in that case.
+                         LLVMContext &Context, bool ShouldBeLazy) {
   ErrorOr<std::unique_ptr<Module>> MOrErr =
-      parseBitcodeFileImpl(Buffer, *Context,
-                           /* ShouldBeLazy */ static_cast<bool>(OwnedContext));
+      parseBitcodeFileImpl(Buffer, Context, ShouldBeLazy);
   if (std::error_code EC = MOrErr.getError())
     return EC;
   std::unique_ptr<Module> &M = *MOrErr;
 
   std::string TripleStr = M->getTargetTriple();
   if (TripleStr.empty())
     TripleStr = sys::getDefaultTargetTriple();
   llvm::Triple Triple(TripleStr);
LTOModule::makeLTOModule(MemoryBufferRef Buffer, TargetOptions options,
 
   TargetMachine *target = march->createTargetMachine(TripleStr, CPU, FeatureStr,
                                                      options);
   M->setDataLayout(target->createDataLayout());
 
   std::unique_ptr<object::IRObjectFile> IRObj(
       new object::IRObjectFile(Buffer, std::move(M)));
 
-  std::unique_ptr<LTOModule> Ret;
-  if (OwnedContext)
-    Ret.reset(new LTOModule(std::move(IRObj), target, std::move(OwnedContext)));
-  else
-    Ret.reset(new LTOModule(std::move(IRObj), target));
-
+  std::unique_ptr<LTOModule> Ret(new LTOModule(std::move(IRObj), target));
   Ret->parseSymbols();
   Ret->parseMetadata();
 
   return std::move(Ret);
 }
 
 /// Create a MemoryBuffer from a memory range with an optional name.
 std::unique_ptr<MemoryBuffer>

test/tools/llvm-lto/error.ll

 ; RUN: not llvm-lto foobar 2>&1 | FileCheck %s
 ; CHECK: llvm-lto: error loading file 'foobar': {{N|n}}o such file or directory
+
+; RUN: not llvm-lto --list-symbols-only %S/Inputs/empty.bc 2>&1 | FileCheck %s --check-prefix=CHECK-LIST
+; CHECK-LIST: llvm-lto: error loading file '{{.*}}/Inputs/empty.bc': The file was not recognized as a valid object file

tools/llvm-lto/llvm-lto.cpp

static void diagnosticHandler(const DiagnosticInfo &DI) {
   DiagnosticPrinterRawOStream DP(OS);
   DI.print(DP);
   OS << '\n';
 
   if (DI.getSeverity() == DS_Error)
     exit(1);
 }
 
-static void diagnosticHandlerWithContenxt(const DiagnosticInfo &DI,
-                                          void *Context) {
+static void diagnosticHandlerWithContext(const DiagnosticInfo &DI,
   diagnosticHandler(DI);
 }
 
 static void error(const Twine &Msg) {
   errs() << "llvm-lto: " << Msg << '\n';
   exit(1);
 }
 

 static std::unique_ptr<LTOModule>
 getLocalLTOModule(StringRef Path, std::unique_ptr<MemoryBuffer> &Buffer,
                   const TargetOptions &Options) {
   ErrorOr<std::unique_ptr<MemoryBuffer>> BufferOrErr =
       MemoryBuffer::getFile(Path);
   error(BufferOrErr, "error loading file '" + Path + "'");
   Buffer = std::move(BufferOrErr.get());
   CurrentActivity = ("loading file '" + Path + "'").str();
+  std::unique_ptr<LLVMContext> Context = llvm::make_unique<LLVMContext>();
+  Context->setDiagnosticHandler(diagnosticHandlerWithContext, nullptr, true);
   ErrorOr<std::unique_ptr<LTOModule>> Ret = LTOModule::createInLocalContext(
-      Buffer->getBufferStart(), Buffer->getBufferSize(), Options, Path);
+      std::move(Context), Buffer->getBufferStart(), Buffer->getBufferSize(),
+      Options, Path);
   CurrentActivity = "";
   return std::move(*Ret);
 }
 
 /// \brief List symbols in each IR file.
 ///
 /// The main point here is to provide lit-testable coverage for the LTOModule
 /// functionality that's exposed by the C API to list symbols.  Moreover, this
int main(int argc, char **argv) {
   if (ThinLTO) {
     createCombinedFunctionIndex();
     return 0;
   }
 
   unsigned BaseArg = 0;
 
   LLVMContext Context;
-  Context.setDiagnosticHandler(diagnosticHandlerWithContenxt, nullptr, true);
+  Context.setDiagnosticHandler(diagnosticHandlerWithContext, nullptr, true);
 
   LTOCodeGenerator CodeGen(Context);
 
   if (UseDiagnosticHandler)
     CodeGen.setDiagnosticHandler(handleDiagnostics, nullptr);
 
   CodeGen.setCodePICModel(RelocModel);
 

tools/lto/lto.cpp

if (!M)
     return nullptr;
   return wrap(M->release());
 }
 
 lto_module_t lto_module_create_in_local_context(const void *mem, size_t length,
                                                 const char *path) {
   lto_initialize();
   llvm::TargetOptions Options = InitTargetOptionsFromCodeGenFlags();
+
+  // Create a local context. Ownership will be transfered to LTOModule.
+  std::unique_ptr<LLVMContext> Context = llvm::make_unique<LLVMContext>();
+  Context->setDiagnosticHandler(diagnosticHandler, nullptr, true);
+
   ErrorOr<std::unique_ptr<LTOModule>> M =
-      LTOModule::createInLocalContext(mem, length, Options, path);
+      LTOModule::createInLocalContext(std::move(Context), mem, length, Options,
+                                      path);
   if (!M)
     return nullptr;
   return wrap(M->release());
 }
 
 lto_module_t lto_module_create_in_codegen_context(const void *mem,
                                                   size_t length,
                                                   const char *path,
                                                   lto_code_gen_t cg) {
   lto_initialize();
   llvm::TargetOptions Options = InitTargetOptionsFromCodeGenFlags();
-  ErrorOr<std::unique_ptr<LTOModule>> M = LTOModule::createInContext(
-      mem, length, Options, path, &unwrap(cg)->getContext());
+  ErrorOr<std::unique_ptr<LTOModule>> M = LTOModule::createFromBuffer(
+      unwrap(cg)->getContext(), mem, length, Options, path);
   return wrap(M->release());
 }
 
 void lto_module_dispose(lto_module_t mod) { delete unwrap(mod); }
 
 const char* lto_module_get_target_triple(lto_module_t mod) {
   return unwrap(mod)->getTargetTriple().c_str();
 }