[BOLT][NFC] Report errors from createBinaryContext and RewriteInstance ctor

Refactor createBinaryContext and RewriteInstance/MachORewriteInstance
constructors to report an error in a library and fuzzer-friendly way instead of
returning a nullptr or exiting.

Reviewed By: rafauler

Differential Revision: https://reviews.llvm.org/D119658

10 files changed, 132 insertions, 59 deletions

diff --git a/bolt/include/bolt/Core/BinaryContext.h b/bolt/include/bolt/Core/BinaryContext.h
index aff770112be1..ce246d51281a 100644
--- a/bolt/include/bolt/Core/BinaryContext.h
+++ b/bolt/include/bolt/Core/BinaryContext.h
@@ -211,7 +211,7 @@ class BinaryContext {
   std::map<unsigned, DwarfLineTable> DwarfLineTablesCUMap;
 
 public:
-  static std::unique_ptr<BinaryContext>
+  static Expected<std::unique_ptr<BinaryContext>>
   createBinaryContext(const ObjectFile *File, bool IsPIC,
                       std::unique_ptr<DWARFContext> DwCtx);
 
diff --git a/bolt/include/bolt/Rewrite/MachORewriteInstance.h b/bolt/include/bolt/Rewrite/MachORewriteInstance.h
index 81a6331b6462..0d3b72d5eac8 100644
--- a/bolt/include/bolt/Rewrite/MachORewriteInstance.h
+++ b/bolt/include/bolt/Rewrite/MachORewriteInstance.h
@@ -65,7 +65,15 @@ class MachORewriteInstance {
   void rewriteFile();
 
 public:
-  MachORewriteInstance(object::MachOObjectFile *InputFile, StringRef ToolPath);
+  // This constructor has complex initialization that can fail during
+  // construction. Constructors can’t return errors, so clients must test \p Err
+  // after the object is constructed. Use createMachORewriteInstance instead.
+  MachORewriteInstance(object::MachOObjectFile *InputFile, StringRef ToolPath,
+                       Error &Err);
+
+  static Expected<std::unique_ptr<MachORewriteInstance>>
+  createMachORewriteInstance(object::MachOObjectFile *InputFile,
+                             StringRef ToolPath);
   ~MachORewriteInstance();
 
   Error setProfile(StringRef FileName);
diff --git a/bolt/include/bolt/Rewrite/RewriteInstance.h b/bolt/include/bolt/Rewrite/RewriteInstance.h
index e0cb8b1fd631..6c8d91042e0a 100644
--- a/bolt/include/bolt/Rewrite/RewriteInstance.h
+++ b/bolt/include/bolt/Rewrite/RewriteInstance.h
@@ -41,8 +41,15 @@ class ProfileReaderBase;
 /// events.
 class RewriteInstance {
 public:
+  // This constructor has complex initialization that can fail during
+  // construction. Constructors can’t return errors, so clients must test \p Err
+  // after the object is constructed. Use createRewriteInstance instead.
   RewriteInstance(llvm::object::ELFObjectFileBase *File, const int Argc,
-                  const char *const *Argv, StringRef ToolPath);
+                  const char *const *Argv, StringRef ToolPath, Error &Err);
+
+  static Expected<std::unique_ptr<RewriteInstance>>
+  createRewriteInstance(llvm::object::ELFObjectFileBase *File, const int Argc,
+                        const char *const *Argv, StringRef ToolPath);
   ~RewriteInstance();
 
   /// Assign profile from \p Filename to this instance.
diff --git a/bolt/lib/Core/BinaryContext.cpp b/bolt/lib/Core/BinaryContext.cpp
index 36745580217e..36092e3a945f 100644
--- a/bolt/lib/Core/BinaryContext.cpp
+++ b/bolt/lib/Core/BinaryContext.cpp
@@ -33,6 +33,7 @@
 #include "llvm/MC/MCSubtargetInfo.h"
 #include "llvm/MC/MCSymbol.h"
 #include "llvm/Support/CommandLine.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Support/Regex.h"
 #include <algorithm>
 #include <functional>
@@ -115,7 +116,7 @@ BinaryContext::~BinaryContext() {
 
 /// Create BinaryContext for a given architecture \p ArchName and
 /// triple \p TripleName.
-std::unique_ptr<BinaryContext>
+Expected<std::unique_ptr<BinaryContext>>
 BinaryContext::createBinaryContext(const ObjectFile *File, bool IsPIC,
                                    std::unique_ptr<DWARFContext> DwCtx) {
   StringRef ArchName = "";
@@ -131,8 +132,8 @@ BinaryContext::createBinaryContext(const ObjectFile *File, bool IsPIC,
                   "+fullfp16,+spe,+fuse-aes,+rcpc";
     break;
   default:
-    errs() << "BOLT-ERROR: Unrecognized machine in ELF file.\n";
-    return nullptr;
+    return createStringError(std::errc::not_supported,
+                             "BOLT-ERROR: Unrecognized machine in ELF file");
   }
 
   auto TheTriple = std::make_unique<Triple>(File->makeTriple());
@@ -141,39 +142,37 @@ BinaryContext::createBinaryContext(const ObjectFile *File, bool IsPIC,
   std::string Error;
   const Target *TheTarget =
       TargetRegistry::lookupTarget(std::string(ArchName), *TheTriple, Error);
-  if (!TheTarget) {
-    errs() << "BOLT-ERROR: " << Error;
-    return nullptr;
-  }
+  if (!TheTarget)
+    return createStringError(make_error_code(std::errc::not_supported),
+                             Twine("BOLT-ERROR: ", Error));
 
   std::unique_ptr<const MCRegisterInfo> MRI(
       TheTarget->createMCRegInfo(TripleName));
-  if (!MRI) {
-    errs() << "BOLT-ERROR: no register info for target " << TripleName << "\n";
-    return nullptr;
-  }
+  if (!MRI)
+    return createStringError(
+        make_error_code(std::errc::not_supported),
+        Twine("BOLT-ERROR: no register info for target ", TripleName));
 
   // Set up disassembler.
   std::unique_ptr<const MCAsmInfo> AsmInfo(
       TheTarget->createMCAsmInfo(*MRI, TripleName, MCTargetOptions()));
-  if (!AsmInfo) {
-    errs() << "BOLT-ERROR: no assembly info for target " << TripleName << "\n";
-    return nullptr;
-  }
+  if (!AsmInfo)
+    return createStringError(
+        make_error_code(std::errc::not_supported),
+        Twine("BOLT-ERROR: no assembly info for target ", TripleName));
 
   std::unique_ptr<const MCSubtargetInfo> STI(
       TheTarget->createMCSubtargetInfo(TripleName, "", FeaturesStr));
-  if (!STI) {
-    errs() << "BOLT-ERROR: no subtarget info for target " << TripleName << "\n";
-    return nullptr;
-  }
+  if (!STI)
+    return createStringError(
+        make_error_code(std::errc::not_supported),
+        Twine("BOLT-ERROR: no subtarget info for target ", TripleName));
 
   std::unique_ptr<const MCInstrInfo> MII(TheTarget->createMCInstrInfo());
-  if (!MII) {
-    errs() << "BOLT-ERROR: no instruction info for target " << TripleName
-           << "\n";
-    return nullptr;
-  }
+  if (!MII)
+    return createStringError(
+        make_error_code(std::errc::not_supported),
+        Twine("BOLT-ERROR: no instruction info for target ", TripleName));
 
   std::unique_ptr<MCContext> Ctx(
       new MCContext(*TheTriple, AsmInfo.get(), MRI.get(), STI.get()));
@@ -198,28 +197,27 @@ BinaryContext::createBinaryContext(const ObjectFile *File, bool IsPIC,
   std::unique_ptr<MCDisassembler> DisAsm(
       TheTarget->createMCDisassembler(*STI, *Ctx));
 
-  if (!DisAsm) {
-    errs() << "BOLT-ERROR: no disassembler for target " << TripleName << "\n";
-    return nullptr;
-  }
+  if (!DisAsm)
+    return createStringError(
+        make_error_code(std::errc::not_supported),
+        Twine("BOLT-ERROR: no disassembler info for target ", TripleName));
 
   std::unique_ptr<const MCInstrAnalysis> MIA(
       TheTarget->createMCInstrAnalysis(MII.get()));
-  if (!MIA) {
-    errs() << "BOLT-ERROR: failed to create instruction analysis for target"
-           << TripleName << "\n";
-    return nullptr;
-  }
+  if (!MIA)
+    return createStringError(
+        make_error_code(std::errc::not_supported),
+        Twine("BOLT-ERROR: failed to create instruction analysis for target ",
+              TripleName));
 
   int AsmPrinterVariant = AsmInfo->getAssemblerDialect();
   std::unique_ptr<MCInstPrinter> InstructionPrinter(
       TheTarget->createMCInstPrinter(*TheTriple, AsmPrinterVariant, *AsmInfo,
                                      *MII, *MRI));
-  if (!InstructionPrinter) {
-    errs() << "BOLT-ERROR: no instruction printer for target " << TripleName
-           << '\n';
-    return nullptr;
-  }
+  if (!InstructionPrinter)
+    return createStringError(
+        make_error_code(std::errc::not_supported),
+        Twine("BOLT-ERROR: no instruction printer for target ", TripleName));
   InstructionPrinter->setPrintImmHex(true);
 
   std::unique_ptr<MCCodeEmitter> MCE(
diff --git a/bolt/lib/Rewrite/DWARFRewriter.cpp b/bolt/lib/Rewrite/DWARFRewriter.cpp
index 071759a436f0..40d43bf858f8 100644
--- a/bolt/lib/Rewrite/DWARFRewriter.cpp
+++ b/bolt/lib/Rewrite/DWARFRewriter.cpp
@@ -30,6 +30,7 @@
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/Endian.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/ThreadPool.h"
 #include "llvm/Support/ToolOutputFile.h"
@@ -863,11 +864,11 @@ namespace {
 
 std::unique_ptr<BinaryContext>
 createDwarfOnlyBC(const object::ObjectFile &File) {
-  return BinaryContext::createBinaryContext(
+  return cantFail(BinaryContext::createBinaryContext(
       &File, false,
       DWARFContext::create(File, DWARFContext::ProcessDebugRelocations::Ignore,
                            nullptr, "", WithColor::defaultErrorHandler,
-                           WithColor::defaultWarningHandler));
+                           WithColor::defaultWarningHandler)));
 }
 
 StringMap<KnownSectionsEntry>
diff --git a/bolt/lib/Rewrite/MachORewriteInstance.cpp b/bolt/lib/Rewrite/MachORewriteInstance.cpp
index 207f6c070fa8..00fe8cf9fb5f 100644
--- a/bolt/lib/Rewrite/MachORewriteInstance.cpp
+++ b/bolt/lib/Rewrite/MachORewriteInstance.cpp
@@ -24,6 +24,7 @@
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/ToolOutputFile.h"
+#include <memory>
 
 namespace opts {
 
@@ -82,11 +83,28 @@ MCPlusBuilder *createMCPlusBuilder(const Triple::ArchType Arch,
 
 #define DEBUG_TYPE "bolt"
 
+Expected<std::unique_ptr<MachORewriteInstance>>
+MachORewriteInstance::createMachORewriteInstance(
+    object::MachOObjectFile *InputFile, StringRef ToolPath) {
+  Error Err = Error::success();
+  auto MachORI =
+      std::make_unique<MachORewriteInstance>(InputFile, ToolPath, Err);
+  if (Err)
+    return std::move(Err);
+  return MachORI;
+}
+
 MachORewriteInstance::MachORewriteInstance(object::MachOObjectFile *InputFile,
-                                           StringRef ToolPath)
-    : InputFile(InputFile), ToolPath(ToolPath),
-      BC(BinaryContext::createBinaryContext(InputFile, /* IsPIC */ true,
-                                            DWARFContext::create(*InputFile))) {
+                                           StringRef ToolPath, Error &Err)
+    : InputFile(InputFile), ToolPath(ToolPath) {
+  ErrorAsOutParameter EAO(&Err);
+  auto BCOrErr = BinaryContext::createBinaryContext(
+      InputFile, /* IsPIC */ true, DWARFContext::create(*InputFile));
+  if (Error E = BCOrErr.takeError()) {
+    Err = std::move(E);
+    return;
+  }
+  BC = std::move(BCOrErr.get());
   BC->initializeTarget(std::unique_ptr<MCPlusBuilder>(createMCPlusBuilder(
       BC->TheTriple->getArch(), BC->MIA.get(), BC->MII.get(), BC->MRI.get())));
   if (opts::Instrument)
diff --git a/bolt/lib/Rewrite/RewriteInstance.cpp b/bolt/lib/Rewrite/RewriteInstance.cpp
index b07f849b6fe9..2671df8ebc31 100644
--- a/bolt/lib/Rewrite/RewriteInstance.cpp
+++ b/bolt/lib/Rewrite/RewriteInstance.cpp
@@ -47,6 +47,7 @@
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/DataExtractor.h"
 #include "llvm/Support/Errc.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/LEB128.h"
 #include "llvm/Support/ManagedStatic.h"
@@ -55,6 +56,7 @@
 #include "llvm/Support/raw_ostream.h"
 #include <algorithm>
 #include <fstream>
+#include <memory>
 #include <system_error>
 
 #undef  DEBUG_TYPE
@@ -353,14 +355,28 @@ bool refersToReorderedSection(ErrorOr<BinarySection &> Section) {
 
 } // anonymous namespace
 
+Expected<std::unique_ptr<RewriteInstance>>
+RewriteInstance::createRewriteInstance(ELFObjectFileBase *File, const int Argc,
+                                       const char *const *Argv,
+                                       StringRef ToolPath) {
+  Error Err = Error::success();
+  auto RI = std::make_unique<RewriteInstance>(File, Argc, Argv, ToolPath, Err);
+  if (Err)
+    return std::move(Err);
+  return RI;
+}
+
 RewriteInstance::RewriteInstance(ELFObjectFileBase *File, const int Argc,
-                                 const char *const *Argv, StringRef ToolPath)
+                                 const char *const *Argv, StringRef ToolPath,
+                                 Error &Err)
     : InputFile(File), Argc(Argc), Argv(Argv), ToolPath(ToolPath),
       SHStrTab(StringTableBuilder::ELF) {
+  ErrorAsOutParameter EAO(&Err);
   auto ELF64LEFile = dyn_cast<ELF64LEObjectFile>(InputFile);
   if (!ELF64LEFile) {
-    errs() << "BOLT-ERROR: only 64-bit LE ELF binaries are supported\n";
-    exit(1);
+    Err = createStringError(errc::not_supported,
+                            "Only 64-bit LE ELF binaries are supported");
+    return;
   }
 
   bool IsPIC = false;
@@ -371,13 +387,17 @@ RewriteInstance::RewriteInstance(ELFObjectFileBase *File, const int Argc,
     IsPIC = true;
   }
 
-  BC = BinaryContext::createBinaryContext(
+  auto BCOrErr = BinaryContext::createBinaryContext(
       File, IsPIC,
       DWARFContext::create(*File, DWARFContext::ProcessDebugRelocations::Ignore,
                            nullptr, opts::DWPPathName,
                            WithColor::defaultErrorHandler,
                            WithColor::defaultWarningHandler));
-
+  if (Error E = BCOrErr.takeError()) {
+    Err = std::move(E);
+    return;
+  }
+  BC = std::move(BCOrErr.get());
   BC->initializeTarget(std::unique_ptr<MCPlusBuilder>(createMCPlusBuilder(
       BC->TheTriple->getArch(), BC->MIA.get(), BC->MII.get(), BC->MRI.get())));
 
diff --git a/bolt/tools/driver/llvm-bolt.cpp b/bolt/tools/driver/llvm-bolt.cpp
index 0e522891229d..2c8d1d28f840 100644
--- a/bolt/tools/driver/llvm-bolt.cpp
+++ b/bolt/tools/driver/llvm-bolt.cpp
@@ -216,7 +216,11 @@ int main(int argc, char **argv) {
     Binary &Binary = *BinaryOrErr.get().getBinary();
 
     if (auto *e = dyn_cast<ELFObjectFileBase>(&Binary)) {
-      RewriteInstance RI(e, argc, argv, ToolPath);
+      auto RIOrErr =
+          RewriteInstance::createRewriteInstance(e, argc, argv, ToolPath);
+      if (Error E = RIOrErr.takeError())
+        report_error(opts::InputFilename, std::move(E));
+      RewriteInstance &RI = *RIOrErr.get();
       if (!opts::PerfData.empty()) {
         if (!opts::AggregateOnly) {
           errs() << ToolName
@@ -239,7 +243,11 @@ int main(int argc, char **argv) {
 
       RI.run();
     } else if (auto *O = dyn_cast<MachOObjectFile>(&Binary)) {
-      MachORewriteInstance MachORI(O, ToolPath);
+      auto MachORIOrErr =
+          MachORewriteInstance::createMachORewriteInstance(O, ToolPath);
+      if (Error E = MachORIOrErr.takeError())
+        report_error(opts::InputFilename, std::move(E));
+      MachORewriteInstance &MachORI = *MachORIOrErr.get();
 
       if (!opts::InputDataFilename.empty())
         if (Error E = MachORI.setProfile(opts::InputDataFilename))
@@ -266,10 +274,18 @@ int main(int argc, char **argv) {
   Binary &Binary2 = *BinaryOrErr2.get().getBinary();
   if (auto *ELFObj1 = dyn_cast<ELFObjectFileBase>(&Binary1)) {
     if (auto *ELFObj2 = dyn_cast<ELFObjectFileBase>(&Binary2)) {
-      RewriteInstance RI1(ELFObj1, argc, argv, ToolPath);
+      auto RI1OrErr =
+          RewriteInstance::createRewriteInstance(ELFObj1, argc, argv, ToolPath);
+      if (Error E = RI1OrErr.takeError())
+        report_error(opts::InputFilename, std::move(E));
+      RewriteInstance &RI1 = *RI1OrErr.get();
       if (Error E = RI1.setProfile(opts::InputDataFilename))
         report_error(opts::InputDataFilename, std::move(E));
-      RewriteInstance RI2(ELFObj2, argc, argv, ToolPath);
+      auto RI2OrErr =
+          RewriteInstance::createRewriteInstance(ELFObj2, argc, argv, ToolPath);
+      if (Error E = RI2OrErr.takeError())
+        report_error(opts::InputFilename2, std::move(E));
+      RewriteInstance &RI2 = *RI2OrErr.get();
       if (Error E = RI2.setProfile(opts::InputDataFilename2))
         report_error(opts::InputDataFilename2, std::move(E));
       outs() << "BOLT-DIFF: *** Analyzing binary 1: " << opts::InputFilename
diff --git a/bolt/tools/heatmap/heatmap.cpp b/bolt/tools/heatmap/heatmap.cpp
index 887a120de6cf..0ab6a4fa52b7 100644
--- a/bolt/tools/heatmap/heatmap.cpp
+++ b/bolt/tools/heatmap/heatmap.cpp
@@ -85,7 +85,12 @@ int main(int argc, char **argv) {
   Binary &Binary = *BinaryOrErr.get().getBinary();
 
   if (auto *e = dyn_cast<ELFObjectFileBase>(&Binary)) {
-    RewriteInstance RI(e, argc, argv, ToolPath);
+    auto RIOrErr =
+        RewriteInstance::createRewriteInstance(e, argc, argv, ToolPath);
+    if (Error E = RIOrErr.takeError())
+      report_error("RewriteInstance", std::move(E));
+
+    RewriteInstance &RI = *RIOrErr.get();
     if (Error E = RI.setProfile(opts::PerfData))
       report_error(opts::PerfData, std::move(E));
 
diff --git a/bolt/unittests/Core/MCPlusBuilder.cpp b/bolt/unittests/Core/MCPlusBuilder.cpp
index ec881bf4ead1..2158f652c071 100644
--- a/bolt/unittests/Core/MCPlusBuilder.cpp
+++ b/bolt/unittests/Core/MCPlusBuilder.cpp
@@ -47,8 +47,8 @@ protected:
   }
 
   void initializeBolt() {
-    BC = BinaryContext::createBinaryContext(
-        ObjFile.get(), true, DWARFContext::create(*ObjFile.get()));
+    BC = cantFail(BinaryContext::createBinaryContext(
+        ObjFile.get(), true, DWARFContext::create(*ObjFile.get())));
     ASSERT_FALSE(!BC);
     BC->initializeTarget(std::unique_ptr<MCPlusBuilder>(createMCPlusBuilder(
         GetParam(), BC->MIA.get(), BC->MII.get(), BC->MRI.get())));