[BOLT][NFC] Propagate BOLTErrors from Core, RewriteInstance, and passes (2/2)

bolt/include/bolt/Core/BinaryContext.h

@@ -145,6 +145,29 @@ class FilterIterator {
   }
 };
 
+/// BOLT-exclusive errors generated in core BOLT libraries, optionally holding a
+/// string message and whether it is fatal or not. In case it is fatal and if
+/// BOLT is running as a standalone process, the process might be killed as soon
+/// as the error is checked.
+class BOLTError : public ErrorInfo<BOLTError> {
+public:
+  static char ID;
+
+  BOLTError(bool IsFatal, const Twine &S = Twine());
+  void log(raw_ostream &OS) const override;
+  bool isFatal() const { return IsFatal; }
+
+  const std::string &getMessage() const { return Msg; }
+  std::error_code convertToErrorCode() const override;
+
+private:
+  bool IsFatal;
+  std::string Msg;
+};
+
+Error createNonFatalBOLTError(const Twine &S);
+Error createFatalBOLTError(const Twine &S);
+
 class BinaryContext {
   BinaryContext() = delete;
 

bolt/include/bolt/Core/BinaryFunction.h

@@ -1910,12 +1910,11 @@ class BinaryFunction {
 
   /// Support dynamic relocations in constant islands, which may happen if
   /// binary is linked with -z notext option.
-  void markIslandDynamicRelocationAtAddress(uint64_t Address) {
-    if (!isInConstantIsland(Address)) {
-      errs() << "BOLT-ERROR: dynamic relocation found for text section at 0x"
-             << Twine::utohexstr(Address) << "\n";
-      exit(1);
-    }
+  Error markIslandDynamicRelocationAtAddress(uint64_t Address) {
+    if (!isInConstantIsland(Address))
+      return createFatalBOLTError(
+          Twine("dynamic relocation found for text section at 0x") +
+          Twine::utohexstr(Address) + Twine("\n"));
 
     // Mark island to have dynamic relocation
     Islands->HasDynamicRelocations = true;
@@ -1924,6 +1923,7 @@ class BinaryFunction {
     // move binary data during updateOutputValues, making us emit
     // dynamic relocation with the right offset value.
     getOrCreateIslandAccess(Address);
+    return Error::success();
   }
 
   bool hasDynamicRelocationAtIsland() const {
@@ -2054,9 +2054,10 @@ class BinaryFunction {
   /// state to State:Disassembled.
   ///
   /// Returns false if disassembly failed.
-  bool disassemble();
+  Error disassemble();
 
-  void handlePCRelOperand(MCInst &Instruction, uint64_t Address, uint64_t Size);
+  Error handlePCRelOperand(MCInst &Instruction, uint64_t Address,
+                           uint64_t Size);
 
   MCSymbol *handleExternalReference(MCInst &Instruction, uint64_t Size,
                                     uint64_t Offset, uint64_t TargetAddress,
@@ -2100,7 +2101,7 @@ class BinaryFunction {
   ///
   /// Returns true on success and update the current function state to
   /// State::CFG. Returns false if CFG cannot be built.
-  bool buildCFG(MCPlusBuilder::AllocatorIdTy);
+  Error buildCFG(MCPlusBuilder::AllocatorIdTy);
 
   /// Perform post-processing of the CFG.
   void postProcessCFG();
@@ -2217,7 +2218,7 @@ class BinaryFunction {
   }
 
   /// Process LSDA information for the function.
-  void parseLSDA(ArrayRef<uint8_t> LSDAData, uint64_t LSDAAddress);
+  Error parseLSDA(ArrayRef<uint8_t> LSDAData, uint64_t LSDAAddress);
 
   /// Update exception handling ranges for the function.
   void updateEHRanges();

bolt/include/bolt/Core/BinarySection.h

@@ -112,7 +112,7 @@ class BinarySection {
   static StringRef getName(SectionRef Section) {
     return cantFail(Section.getName());
   }
-  static StringRef getContents(SectionRef Section) {
+  static StringRef getContentsOrQuit(SectionRef Section) {
     if (Section.getObject()->isELF() &&
         ELFSectionRef(Section).getType() == ELF::SHT_NOBITS)
       return StringRef();
@@ -159,7 +159,7 @@ class BinarySection {
 
   BinarySection(BinaryContext &BC, SectionRef Section)
       : BC(BC), Name(getName(Section)), Section(Section),
-        Contents(getContents(Section)), Address(Section.getAddress()),
+        Contents(getContentsOrQuit(Section)), Address(Section.getAddress()),
         Size(Section.getSize()), Alignment(Section.getAlignment().value()),
         OutputName(Name), SectionNumber(++Count) {
     if (isELF()) {

bolt/include/bolt/Passes/FrameOptimizer.h

@@ -98,8 +98,8 @@ class FrameOptimizerPass : public BinaryFunctionPass {
   void removeUnusedStores(const FrameAnalysis &FA, BinaryFunction &BF);
 
   /// Perform shrinkwrapping step
-  void performShrinkWrapping(const RegAnalysis &RA, const FrameAnalysis &FA,
-                             BinaryContext &BC);
+  Error performShrinkWrapping(const RegAnalysis &RA, const FrameAnalysis &FA,
+                              BinaryContext &BC);
 
 public:
   explicit FrameOptimizerPass(const cl::opt<bool> &PrintPass)

bolt/include/bolt/Passes/LongJmp.h

@@ -131,14 +131,14 @@ class LongJmpPass : public BinaryFunctionPass {
                  uint64_t DotAddress) const;
 
   /// Expand the range of the stub in StubBB if necessary
-  bool relaxStub(BinaryBasicBlock &StubBB);
+  Error relaxStub(BinaryBasicBlock &StubBB, bool &Modified);
 
   /// Helper to resolve a symbol address according to our tentative layout
   uint64_t getSymbolAddress(const BinaryContext &BC, const MCSymbol *Target,
                             const BinaryBasicBlock *TgtBB) const;
 
   /// Relax function by adding necessary stubs or relaxing existing stubs
-  bool relax(BinaryFunction &BF);
+  Error relax(BinaryFunction &BF, bool &Modified);
 
 public:
   /// BinaryPass public interface

bolt/include/bolt/Passes/ReorderFunctions.h

@@ -44,7 +44,7 @@ class ReorderFunctions : public BinaryFunctionPass {
   const char *getName() const override { return "reorder-functions"; }
   Error runOnFunctions(BinaryContext &BC) override;
 
-  static std::vector<std::string> readFunctionOrderFile();
+  static Error readFunctionOrderFile(std::vector<std::string> &FunctionNames);
 };
 
 } // namespace bolt

bolt/include/bolt/Passes/ShrinkWrapping.h

@@ -467,8 +467,9 @@ class ShrinkWrapping {
   /// If \p CreatePushOrPop is true, create a push/pop instead. Current SP/FP
   /// values, as determined by StackPointerTracking, should be informed via
   /// \p SPVal and \p FPVal in order to emit the correct offset form SP/FP.
-  MCInst createStackAccess(int SPVal, int FPVal, const FrameIndexEntry &FIE,
-                           bool CreatePushOrPop);
+  Expected<MCInst> createStackAccess(int SPVal, int FPVal,
+                                     const FrameIndexEntry &FIE,
+                                     bool CreatePushOrPop);
 
   /// Update the CFI referenced by \p Inst with \p NewOffset, if the CFI has
   /// an offset.
@@ -484,22 +485,23 @@ class ShrinkWrapping {
   /// InsertionPoint for other instructions that need to be inserted at the same
   /// original location, since this insertion may have invalidated the previous
   /// location.
-  BBIterTy processInsertion(BBIterTy InsertionPoint, BinaryBasicBlock *CurBB,
-                            const WorklistItem &Item, int64_t SPVal,
-                            int64_t FPVal);
+  Expected<BBIterTy> processInsertion(BBIterTy InsertionPoint,
+                                      BinaryBasicBlock *CurBB,
+                                      const WorklistItem &Item, int64_t SPVal,
+                                      int64_t FPVal);
 
   /// Auxiliary function to processInsertions(), helping perform all the
   /// insertion tasks in the todo list associated with a single insertion point.
   /// Return true if at least one insertion was performed.
-  BBIterTy processInsertionsList(BBIterTy InsertionPoint,
-                                 BinaryBasicBlock *CurBB,
-                                 std::vector<WorklistItem> &TodoList,
-                                 int64_t SPVal, int64_t FPVal);
+  Expected<BBIterTy> processInsertionsList(BBIterTy InsertionPoint,
+                                           BinaryBasicBlock *CurBB,
+                                           std::vector<WorklistItem> &TodoList,
+                                           int64_t SPVal, int64_t FPVal);
 
   /// Apply all insertion todo tasks regarding insertion of new stores/loads or
   /// push/pops at annotated points. Return false if the entire function had
   /// no todo tasks annotation and this pass has nothing to do.
-  bool processInsertions();
+  Expected<bool> processInsertions();
 
   /// Apply all deletion todo tasks (or tasks to change a push/pop to a memory
   /// access no-op)
@@ -519,7 +521,7 @@ class ShrinkWrapping {
         BC.MIB->removeAnnotation(Inst, getAnnotationIndex());
   }
 
-  bool perform(bool HotOnly = false);
+  Expected<bool> perform(bool HotOnly = false);
 
   static void printStats();
 };

bolt/include/bolt/Rewrite/BinaryPassManager.h

@@ -46,10 +46,10 @@ class BinaryFunctionPassManager {
   }
 
   /// Run all registered passes in the order they were added.
-  void runPasses();
+  Error runPasses();
 
   /// Runs all enabled implemented passes on all functions.
-  static void runAllPasses(BinaryContext &BC);
+  static Error runAllPasses(BinaryContext &BC);
 };
 
 } // namespace bolt

bolt/lib/Core/BinaryContext.cpp

@@ -83,6 +83,37 @@ cl::opt<std::string> CompDirOverride(
 namespace llvm {
 namespace bolt {
 
+char BOLTError::ID = 0;
+
+BOLTError::BOLTError(bool IsFatal, const Twine &S)
+    : IsFatal(IsFatal), Msg(S.str()) {}
+
+void BOLTError::log(raw_ostream &OS) const {
+  if (IsFatal)
+    OS << "FATAL ";
+  StringRef ErrMsg = StringRef(Msg);
+  // Prepend our error prefix if it is missing
+  if (ErrMsg.empty()) {
+    OS << "BOLT-ERROR\n";
+  } else {
+    if (!ErrMsg.starts_with("BOLT-ERROR"))
+      OS << "BOLT-ERROR: ";
+    OS << ErrMsg << "\n";
+  }
+}
+
+std::error_code BOLTError::convertToErrorCode() const {
+  return inconvertibleErrorCode();
+}
+
+Error createNonFatalBOLTError(const Twine &S) {
+  return make_error<BOLTError>(/*IsFatal*/ false, S);
+}
+
+Error createFatalBOLTError(const Twine &S) {
+  return make_error<BOLTError>(/*IsFatal*/ true, S);
+}
+
 BinaryContext::BinaryContext(std::unique_ptr<MCContext> Ctx,
                              std::unique_ptr<DWARFContext> DwCtx,
                              std::unique_ptr<Triple> TheTriple,

bolt/lib/Core/BinaryFunction.cpp

@@ -1021,24 +1021,25 @@ bool BinaryFunction::isZeroPaddingAt(uint64_t Offset) const {
   return true;
 }
 
-void BinaryFunction::handlePCRelOperand(MCInst &Instruction, uint64_t Address,
-                                        uint64_t Size) {
+Error BinaryFunction::handlePCRelOperand(MCInst &Instruction, uint64_t Address,
+                                         uint64_t Size) {
   auto &MIB = BC.MIB;
   uint64_t TargetAddress = 0;
   if (!MIB->evaluateMemOperandTarget(Instruction, TargetAddress, Address,
                                      Size)) {
-    errs() << "BOLT-ERROR: PC-relative operand can't be evaluated:\n";
-    BC.InstPrinter->printInst(&Instruction, 0, "", *BC.STI, errs());
-    errs() << '\n';
-    Instruction.dump_pretty(errs(), BC.InstPrinter.get());
-    errs() << '\n';
-    errs() << "BOLT-ERROR: cannot handle PC-relative operand at 0x"
-           << Twine::utohexstr(Address) << ". Skipping function " << *this
-           << ".\n";
+    std::string Msg;
+    raw_string_ostream SS(Msg);
+    SS << "BOLT-ERROR: PC-relative operand can't be evaluated:\n";
+    BC.InstPrinter->printInst(&Instruction, 0, "", *BC.STI, SS);
+    SS << '\n';
+    Instruction.dump_pretty(SS, BC.InstPrinter.get());
+    SS << '\n';
+    SS << "BOLT-ERROR: cannot handle PC-relative operand at 0x"
+       << Twine::utohexstr(Address) << ". Skipping function " << *this << ".\n";
     if (BC.HasRelocations)
-      exit(1);
+      return createFatalBOLTError(Msg);
     IsSimple = false;
-    return;
+    return createNonFatalBOLTError(Msg);
   }
   if (TargetAddress == 0 && opts::Verbosity >= 1) {
     outs() << "BOLT-INFO: PC-relative operand is zero in function " << *this
@@ -1054,6 +1055,7 @@ void BinaryFunction::handlePCRelOperand(MCInst &Instruction, uint64_t Address,
       Instruction, TargetSymbol, static_cast<int64_t>(TargetOffset), &*BC.Ctx);
   (void)ReplaceSuccess;
   assert(ReplaceSuccess && "Failed to replace mem operand with symbol+off.");
+  return Error::success();
 }
 
 MCSymbol *BinaryFunction::handleExternalReference(MCInst &Instruction,
@@ -1164,7 +1166,7 @@ void BinaryFunction::handleAArch64IndirectCall(MCInst &Instruction,
   }
 }
 
-bool BinaryFunction::disassemble() {
+Error BinaryFunction::disassemble() {
   NamedRegionTimer T("disassemble", "Disassemble function", "buildfuncs",
                      "Build Binary Functions", opts::TimeBuild);
   ErrorOr<ArrayRef<uint8_t>> ErrorOrFunctionData = getData();
@@ -1332,8 +1334,19 @@ bool BinaryFunction::disassemble() {
         if (MIB->isIndirectBranch(Instruction))
           handleIndirectBranch(Instruction, Size, Offset);
         // Indirect call. We only need to fix it if the operand is RIP-relative.
-        if (IsSimple && MIB->hasPCRelOperand(Instruction))
-          handlePCRelOperand(Instruction, AbsoluteInstrAddr, Size);
+        if (IsSimple && MIB->hasPCRelOperand(Instruction)) {
+          if (auto NewE = handleErrors(
+                  handlePCRelOperand(Instruction, AbsoluteInstrAddr, Size),
+                  [&](const BOLTError &E) -> Error {
+                    if (E.isFatal())
+                      return Error(std::make_unique<BOLTError>(std::move(E)));
+                    if (!E.getMessage().empty())
+                      E.log(errs());
+                    return Error::success();
+                  })) {
+            return Error(std::move(NewE));
+          }
+        }
 
         if (BC.isAArch64())
           handleAArch64IndirectCall(Instruction, Offset);
@@ -1372,8 +1385,18 @@ bool BinaryFunction::disassemble() {
         UsedReloc = true;
       }
 
-      if (!BC.isRISCV() && MIB->hasPCRelOperand(Instruction) && !UsedReloc)
-        handlePCRelOperand(Instruction, AbsoluteInstrAddr, Size);
+      if (!BC.isRISCV() && MIB->hasPCRelOperand(Instruction) && !UsedReloc) {
+        if (auto NewE = handleErrors(
+                handlePCRelOperand(Instruction, AbsoluteInstrAddr, Size),
+                [&](const BOLTError &E) -> Error {
+                  if (E.isFatal())
+                    return Error(std::make_unique<BOLTError>(std::move(E)));
+                  if (!E.getMessage().empty())
+                    E.log(errs());
+                  return Error::success();
+                }))
+          return Error(std::move(NewE));
+      }
     }
 
 add_instruction:
@@ -1413,12 +1436,12 @@ bool BinaryFunction::disassemble() {
 
   if (!IsSimple) {
     clearList(Instructions);
-    return false;
+    return createNonFatalBOLTError("");
   }
 
   updateState(State::Disassembled);
 
-  return true;
+  return Error::success();
 }
 
 bool BinaryFunction::scanExternalRefs() {
@@ -1946,17 +1969,17 @@ void BinaryFunction::recomputeLandingPads() {
   }
 }
 
-bool BinaryFunction::buildCFG(MCPlusBuilder::AllocatorIdTy AllocatorId) {
+Error BinaryFunction::buildCFG(MCPlusBuilder::AllocatorIdTy AllocatorId) {
   auto &MIB = BC.MIB;
 
   if (!isSimple()) {
     assert(!BC.HasRelocations &&
            "cannot process file with non-simple function in relocs mode");
-    return false;
+    return createNonFatalBOLTError("");
   }
 
   if (CurrentState != State::Disassembled)
-    return false;
+    return createNonFatalBOLTError("");
 
   assert(BasicBlocks.empty() && "basic block list should be empty");
   assert((Labels.find(getFirstInstructionOffset()) != Labels.end()) &&
@@ -2093,7 +2116,7 @@ bool BinaryFunction::buildCFG(MCPlusBuilder::AllocatorIdTy AllocatorId) {
 
   if (BasicBlocks.empty()) {
     setSimple(false);
-    return false;
+    return createNonFatalBOLTError("");
   }
 
   // Intermediate dump.
@@ -2204,7 +2227,7 @@ bool BinaryFunction::buildCFG(MCPlusBuilder::AllocatorIdTy AllocatorId) {
   clearList(ExternallyReferencedOffsets);
   clearList(UnknownIndirectBranchOffsets);
 
-  return true;
+  return Error::success();
 }
 
 void BinaryFunction::postProcessCFG() {