Skip to content

Commit 13d60ce

Browse files
aaupovrafaelauler
aaupov
and
rafaelauler
authored
[BOLT][NFC] Propagate BOLTErrors from Core, RewriteInstance, and passes (2/2) (#81523)
As part of the effort to refactor old error handling code that would directly call exit(1), in this patch continue the migration on libCore, libRewrite and libPasses to use the new BOLTError class whenever a failure occurs. Test Plan: NFC Co-authored-by: Rafael Auler <[email protected]>
1 parent fa7dd49 commit 13d60ce

20 files changed

+242
-147
lines changed

Diff for: ‎bolt/include/bolt/Core/BinaryFunction.h

+11-10
Original file line numberDiff line numberDiff line change
@@ -1910,12 +1910,11 @@ class BinaryFunction {
19101910

19111911
/// Support dynamic relocations in constant islands, which may happen if
19121912
/// binary is linked with -z notext option.
1913-
void markIslandDynamicRelocationAtAddress(uint64_t Address) {
1914-
if (!isInConstantIsland(Address)) {
1915-
errs() << "BOLT-ERROR: dynamic relocation found for text section at 0x"
1916-
<< Twine::utohexstr(Address) << "\n";
1917-
exit(1);
1918-
}
1913+
Error markIslandDynamicRelocationAtAddress(uint64_t Address) {
1914+
if (!isInConstantIsland(Address))
1915+
return createFatalBOLTError(
1916+
Twine("dynamic relocation found for text section at 0x") +
1917+
Twine::utohexstr(Address) + Twine("\n"));
19191918

19201919
// Mark island to have dynamic relocation
19211920
Islands->HasDynamicRelocations = true;
@@ -1924,6 +1923,7 @@ class BinaryFunction {
19241923
// move binary data during updateOutputValues, making us emit
19251924
// dynamic relocation with the right offset value.
19261925
getOrCreateIslandAccess(Address);
1926+
return Error::success();
19271927
}
19281928

19291929
bool hasDynamicRelocationAtIsland() const {
@@ -2054,9 +2054,10 @@ class BinaryFunction {
20542054
/// state to State:Disassembled.
20552055
///
20562056
/// Returns false if disassembly failed.
2057-
bool disassemble();
2057+
Error disassemble();
20582058

2059-
void handlePCRelOperand(MCInst &Instruction, uint64_t Address, uint64_t Size);
2059+
Error handlePCRelOperand(MCInst &Instruction, uint64_t Address,
2060+
uint64_t Size);
20602061

20612062
MCSymbol *handleExternalReference(MCInst &Instruction, uint64_t Size,
20622063
uint64_t Offset, uint64_t TargetAddress,
@@ -2100,7 +2101,7 @@ class BinaryFunction {
21002101
///
21012102
/// Returns true on success and update the current function state to
21022103
/// State::CFG. Returns false if CFG cannot be built.
2103-
bool buildCFG(MCPlusBuilder::AllocatorIdTy);
2104+
Error buildCFG(MCPlusBuilder::AllocatorIdTy);
21042105

21052106
/// Perform post-processing of the CFG.
21062107
void postProcessCFG();
@@ -2217,7 +2218,7 @@ class BinaryFunction {
22172218
}
22182219

22192220
/// Process LSDA information for the function.
2220-
void parseLSDA(ArrayRef<uint8_t> LSDAData, uint64_t LSDAAddress);
2221+
Error parseLSDA(ArrayRef<uint8_t> LSDAData, uint64_t LSDAAddress);
22212222

22222223
/// Update exception handling ranges for the function.
22232224
void updateEHRanges();

Diff for: ‎bolt/include/bolt/Core/BinarySection.h

+2-2
Original file line numberDiff line numberDiff line change
@@ -112,7 +112,7 @@ class BinarySection {
112112
static StringRef getName(SectionRef Section) {
113113
return cantFail(Section.getName());
114114
}
115-
static StringRef getContents(SectionRef Section) {
115+
static StringRef getContentsOrQuit(SectionRef Section) {
116116
if (Section.getObject()->isELF() &&
117117
ELFSectionRef(Section).getType() == ELF::SHT_NOBITS)
118118
return StringRef();
@@ -159,7 +159,7 @@ class BinarySection {
159159

160160
BinarySection(BinaryContext &BC, SectionRef Section)
161161
: BC(BC), Name(getName(Section)), Section(Section),
162-
Contents(getContents(Section)), Address(Section.getAddress()),
162+
Contents(getContentsOrQuit(Section)), Address(Section.getAddress()),
163163
Size(Section.getSize()), Alignment(Section.getAlignment().value()),
164164
OutputName(Name), SectionNumber(++Count) {
165165
if (isELF()) {

Diff for: ‎bolt/include/bolt/Passes/FrameOptimizer.h

+2-2
Original file line numberDiff line numberDiff line change
@@ -98,8 +98,8 @@ class FrameOptimizerPass : public BinaryFunctionPass {
9898
void removeUnusedStores(const FrameAnalysis &FA, BinaryFunction &BF);
9999

100100
/// Perform shrinkwrapping step
101-
void performShrinkWrapping(const RegAnalysis &RA, const FrameAnalysis &FA,
102-
BinaryContext &BC);
101+
Error performShrinkWrapping(const RegAnalysis &RA, const FrameAnalysis &FA,
102+
BinaryContext &BC);
103103

104104
public:
105105
explicit FrameOptimizerPass(const cl::opt<bool> &PrintPass)

Diff for: ‎bolt/include/bolt/Passes/LongJmp.h

+2-2
Original file line numberDiff line numberDiff line change
@@ -131,14 +131,14 @@ class LongJmpPass : public BinaryFunctionPass {
131131
uint64_t DotAddress) const;
132132

133133
/// Expand the range of the stub in StubBB if necessary
134-
bool relaxStub(BinaryBasicBlock &StubBB);
134+
Error relaxStub(BinaryBasicBlock &StubBB, bool &Modified);
135135

136136
/// Helper to resolve a symbol address according to our tentative layout
137137
uint64_t getSymbolAddress(const BinaryContext &BC, const MCSymbol *Target,
138138
const BinaryBasicBlock *TgtBB) const;
139139

140140
/// Relax function by adding necessary stubs or relaxing existing stubs
141-
bool relax(BinaryFunction &BF);
141+
Error relax(BinaryFunction &BF, bool &Modified);
142142

143143
public:
144144
/// BinaryPass public interface

Diff for: ‎bolt/include/bolt/Passes/ReorderFunctions.h

+1-1
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@ class ReorderFunctions : public BinaryFunctionPass {
4444
const char *getName() const override { return "reorder-functions"; }
4545
Error runOnFunctions(BinaryContext &BC) override;
4646

47-
static std::vector<std::string> readFunctionOrderFile();
47+
static Error readFunctionOrderFile(std::vector<std::string> &FunctionNames);
4848
};
4949

5050
} // namespace bolt

Diff for: ‎bolt/include/bolt/Passes/ShrinkWrapping.h

+13-11
Original file line numberDiff line numberDiff line change
@@ -467,8 +467,9 @@ class ShrinkWrapping {
467467
/// If \p CreatePushOrPop is true, create a push/pop instead. Current SP/FP
468468
/// values, as determined by StackPointerTracking, should be informed via
469469
/// \p SPVal and \p FPVal in order to emit the correct offset form SP/FP.
470-
MCInst createStackAccess(int SPVal, int FPVal, const FrameIndexEntry &FIE,
471-
bool CreatePushOrPop);
470+
Expected<MCInst> createStackAccess(int SPVal, int FPVal,
471+
const FrameIndexEntry &FIE,
472+
bool CreatePushOrPop);
472473

473474
/// Update the CFI referenced by \p Inst with \p NewOffset, if the CFI has
474475
/// an offset.
@@ -484,22 +485,23 @@ class ShrinkWrapping {
484485
/// InsertionPoint for other instructions that need to be inserted at the same
485486
/// original location, since this insertion may have invalidated the previous
486487
/// location.
487-
BBIterTy processInsertion(BBIterTy InsertionPoint, BinaryBasicBlock *CurBB,
488-
const WorklistItem &Item, int64_t SPVal,
489-
int64_t FPVal);
488+
Expected<BBIterTy> processInsertion(BBIterTy InsertionPoint,
489+
BinaryBasicBlock *CurBB,
490+
const WorklistItem &Item, int64_t SPVal,
491+
int64_t FPVal);
490492

491493
/// Auxiliary function to processInsertions(), helping perform all the
492494
/// insertion tasks in the todo list associated with a single insertion point.
493495
/// Return true if at least one insertion was performed.
494-
BBIterTy processInsertionsList(BBIterTy InsertionPoint,
495-
BinaryBasicBlock *CurBB,
496-
std::vector<WorklistItem> &TodoList,
497-
int64_t SPVal, int64_t FPVal);
496+
Expected<BBIterTy> processInsertionsList(BBIterTy InsertionPoint,
497+
BinaryBasicBlock *CurBB,
498+
std::vector<WorklistItem> &TodoList,
499+
int64_t SPVal, int64_t FPVal);
498500

499501
/// Apply all insertion todo tasks regarding insertion of new stores/loads or
500502
/// push/pops at annotated points. Return false if the entire function had
501503
/// no todo tasks annotation and this pass has nothing to do.
502-
bool processInsertions();
504+
Expected<bool> processInsertions();
503505

504506
/// Apply all deletion todo tasks (or tasks to change a push/pop to a memory
505507
/// access no-op)
@@ -519,7 +521,7 @@ class ShrinkWrapping {
519521
BC.MIB->removeAnnotation(Inst, getAnnotationIndex());
520522
}
521523

522-
bool perform(bool HotOnly = false);
524+
Expected<bool> perform(bool HotOnly = false);
523525

524526
static void printStats();
525527
};

Diff for: ‎bolt/include/bolt/Rewrite/BinaryPassManager.h

+2-2
Original file line numberDiff line numberDiff line change
@@ -46,10 +46,10 @@ class BinaryFunctionPassManager {
4646
}
4747

4848
/// Run all registered passes in the order they were added.
49-
void runPasses();
49+
Error runPasses();
5050

5151
/// Runs all enabled implemented passes on all functions.
52-
static void runAllPasses(BinaryContext &BC);
52+
static Error runAllPasses(BinaryContext &BC);
5353
};
5454

5555
} // namespace bolt

Diff for: ‎bolt/lib/Core/BinaryFunction.cpp

+47-24
Original file line numberDiff line numberDiff line change
@@ -1021,24 +1021,25 @@ bool BinaryFunction::isZeroPaddingAt(uint64_t Offset) const {
10211021
return true;
10221022
}
10231023

1024-
void BinaryFunction::handlePCRelOperand(MCInst &Instruction, uint64_t Address,
1025-
uint64_t Size) {
1024+
Error BinaryFunction::handlePCRelOperand(MCInst &Instruction, uint64_t Address,
1025+
uint64_t Size) {
10261026
auto &MIB = BC.MIB;
10271027
uint64_t TargetAddress = 0;
10281028
if (!MIB->evaluateMemOperandTarget(Instruction, TargetAddress, Address,
10291029
Size)) {
1030-
errs() << "BOLT-ERROR: PC-relative operand can't be evaluated:\n";
1031-
BC.InstPrinter->printInst(&Instruction, 0, "", *BC.STI, errs());
1032-
errs() << '\n';
1033-
Instruction.dump_pretty(errs(), BC.InstPrinter.get());
1034-
errs() << '\n';
1035-
errs() << "BOLT-ERROR: cannot handle PC-relative operand at 0x"
1036-
<< Twine::utohexstr(Address) << ". Skipping function " << *this
1037-
<< ".\n";
1030+
std::string Msg;
1031+
raw_string_ostream SS(Msg);
1032+
SS << "BOLT-ERROR: PC-relative operand can't be evaluated:\n";
1033+
BC.InstPrinter->printInst(&Instruction, 0, "", *BC.STI, SS);
1034+
SS << '\n';
1035+
Instruction.dump_pretty(SS, BC.InstPrinter.get());
1036+
SS << '\n';
1037+
SS << "BOLT-ERROR: cannot handle PC-relative operand at 0x"
1038+
<< Twine::utohexstr(Address) << ". Skipping function " << *this << ".\n";
10381039
if (BC.HasRelocations)
1039-
exit(1);
1040+
return createFatalBOLTError(Msg);
10401041
IsSimple = false;
1041-
return;
1042+
return createNonFatalBOLTError(Msg);
10421043
}
10431044
if (TargetAddress == 0 && opts::Verbosity >= 1) {
10441045
outs() << "BOLT-INFO: PC-relative operand is zero in function " << *this
@@ -1054,6 +1055,7 @@ void BinaryFunction::handlePCRelOperand(MCInst &Instruction, uint64_t Address,
10541055
Instruction, TargetSymbol, static_cast<int64_t>(TargetOffset), &*BC.Ctx);
10551056
(void)ReplaceSuccess;
10561057
assert(ReplaceSuccess && "Failed to replace mem operand with symbol+off.");
1058+
return Error::success();
10571059
}
10581060

10591061
MCSymbol *BinaryFunction::handleExternalReference(MCInst &Instruction,
@@ -1164,7 +1166,7 @@ void BinaryFunction::handleAArch64IndirectCall(MCInst &Instruction,
11641166
}
11651167
}
11661168

1167-
bool BinaryFunction::disassemble() {
1169+
Error BinaryFunction::disassemble() {
11681170
NamedRegionTimer T("disassemble", "Disassemble function", "buildfuncs",
11691171
"Build Binary Functions", opts::TimeBuild);
11701172
ErrorOr<ArrayRef<uint8_t>> ErrorOrFunctionData = getData();
@@ -1332,8 +1334,19 @@ bool BinaryFunction::disassemble() {
13321334
if (MIB->isIndirectBranch(Instruction))
13331335
handleIndirectBranch(Instruction, Size, Offset);
13341336
// Indirect call. We only need to fix it if the operand is RIP-relative.
1335-
if (IsSimple && MIB->hasPCRelOperand(Instruction))
1336-
handlePCRelOperand(Instruction, AbsoluteInstrAddr, Size);
1337+
if (IsSimple && MIB->hasPCRelOperand(Instruction)) {
1338+
if (auto NewE = handleErrors(
1339+
handlePCRelOperand(Instruction, AbsoluteInstrAddr, Size),
1340+
[&](const BOLTError &E) -> Error {
1341+
if (E.isFatal())
1342+
return Error(std::make_unique<BOLTError>(std::move(E)));
1343+
if (!E.getMessage().empty())
1344+
E.log(errs());
1345+
return Error::success();
1346+
})) {
1347+
return Error(std::move(NewE));
1348+
}
1349+
}
13371350

13381351
if (BC.isAArch64())
13391352
handleAArch64IndirectCall(Instruction, Offset);
@@ -1372,8 +1385,18 @@ bool BinaryFunction::disassemble() {
13721385
UsedReloc = true;
13731386
}
13741387

1375-
if (!BC.isRISCV() && MIB->hasPCRelOperand(Instruction) && !UsedReloc)
1376-
handlePCRelOperand(Instruction, AbsoluteInstrAddr, Size);
1388+
if (!BC.isRISCV() && MIB->hasPCRelOperand(Instruction) && !UsedReloc) {
1389+
if (auto NewE = handleErrors(
1390+
handlePCRelOperand(Instruction, AbsoluteInstrAddr, Size),
1391+
[&](const BOLTError &E) -> Error {
1392+
if (E.isFatal())
1393+
return Error(std::make_unique<BOLTError>(std::move(E)));
1394+
if (!E.getMessage().empty())
1395+
E.log(errs());
1396+
return Error::success();
1397+
}))
1398+
return Error(std::move(NewE));
1399+
}
13771400
}
13781401

13791402
add_instruction:
@@ -1413,12 +1436,12 @@ bool BinaryFunction::disassemble() {
14131436

14141437
if (!IsSimple) {
14151438
clearList(Instructions);
1416-
return false;
1439+
return createNonFatalBOLTError("");
14171440
}
14181441

14191442
updateState(State::Disassembled);
14201443

1421-
return true;
1444+
return Error::success();
14221445
}
14231446

14241447
bool BinaryFunction::scanExternalRefs() {
@@ -1946,17 +1969,17 @@ void BinaryFunction::recomputeLandingPads() {
19461969
}
19471970
}
19481971

1949-
bool BinaryFunction::buildCFG(MCPlusBuilder::AllocatorIdTy AllocatorId) {
1972+
Error BinaryFunction::buildCFG(MCPlusBuilder::AllocatorIdTy AllocatorId) {
19501973
auto &MIB = BC.MIB;
19511974

19521975
if (!isSimple()) {
19531976
assert(!BC.HasRelocations &&
19541977
"cannot process file with non-simple function in relocs mode");
1955-
return false;
1978+
return createNonFatalBOLTError("");
19561979
}
19571980

19581981
if (CurrentState != State::Disassembled)
1959-
return false;
1982+
return createNonFatalBOLTError("");
19601983

19611984
assert(BasicBlocks.empty() && "basic block list should be empty");
19621985
assert((Labels.find(getFirstInstructionOffset()) != Labels.end()) &&
@@ -2093,7 +2116,7 @@ bool BinaryFunction::buildCFG(MCPlusBuilder::AllocatorIdTy AllocatorId) {
20932116

20942117
if (BasicBlocks.empty()) {
20952118
setSimple(false);
2096-
return false;
2119+
return createNonFatalBOLTError("");
20972120
}
20982121

20992122
// Intermediate dump.
@@ -2204,7 +2227,7 @@ bool BinaryFunction::buildCFG(MCPlusBuilder::AllocatorIdTy AllocatorId) {
22042227
clearList(ExternallyReferencedOffsets);
22052228
clearList(UnknownIndirectBranchOffsets);
22062229

2207-
return true;
2230+
return Error::success();
22082231
}
22092232

22102233
void BinaryFunction::postProcessCFG() {

Diff for: ‎bolt/lib/Core/BinarySection.cpp

+1-1
Original file line numberDiff line numberDiff line change
@@ -198,7 +198,7 @@ BinarySection::~BinarySection() {
198198

199199
if (!isAllocatable() && !hasValidSectionID() &&
200200
(!hasSectionRef() ||
201-
OutputContents.data() != getContents(Section).data())) {
201+
OutputContents.data() != getContentsOrQuit(Section).data())) {
202202
delete[] getOutputData();
203203
}
204204
}

Diff for: ‎bolt/lib/Core/Exceptions.cpp

+6-5
Original file line numberDiff line numberDiff line change
@@ -98,12 +98,12 @@ namespace bolt {
9898
// site table will be the same size as GCC uses uleb encodings for PC offsets.
9999
//
100100
// Note: some functions have LSDA entries with 0 call site entries.
101-
void BinaryFunction::parseLSDA(ArrayRef<uint8_t> LSDASectionData,
102-
uint64_t LSDASectionAddress) {
101+
Error BinaryFunction::parseLSDA(ArrayRef<uint8_t> LSDASectionData,
102+
uint64_t LSDASectionAddress) {
103103
assert(CurrentState == State::Disassembled && "unexpected function state");
104104

105105
if (!getLSDAAddress())
106-
return;
106+
return Error::success();
107107

108108
DWARFDataExtractor Data(
109109
StringRef(reinterpret_cast<const char *>(LSDASectionData.data()),
@@ -121,7 +121,7 @@ void BinaryFunction::parseLSDA(ArrayRef<uint8_t> LSDASectionData,
121121
if (!MaybeLPStart) {
122122
errs() << "BOLT-ERROR: unsupported LPStartEncoding: "
123123
<< (unsigned)LPStartEncoding << '\n';
124-
exit(1);
124+
return createFatalBOLTError("");
125125
}
126126
LPStart = *MaybeLPStart;
127127
}
@@ -209,7 +209,7 @@ void BinaryFunction::parseLSDA(ArrayRef<uint8_t> LSDASectionData,
209209
"BOLT-ERROR: cannot have landing pads in different functions");
210210
setHasIndirectTargetToSplitFragment(true);
211211
BC.addFragmentsToSkip(this);
212-
return;
212+
return Error::success();
213213
}
214214

215215
const uint64_t LPOffset = LandingPad - getAddress();
@@ -354,6 +354,7 @@ void BinaryFunction::parseLSDA(ArrayRef<uint8_t> LSDASectionData,
354354
LSDATypeIndexTable =
355355
LSDASectionData.slice(TypeIndexTableStart, MaxTypeIndexTableOffset);
356356
}
357+
return Error::success();
357358
}
358359

359360
void BinaryFunction::updateEHRanges() {

Diff for: ‎bolt/lib/Passes/BinaryPasses.cpp

+3-3
Original file line numberDiff line numberDiff line change
@@ -1456,9 +1456,9 @@ Error PrintProgramStats::runOnFunctions(BinaryContext &BC) {
14561456
<< format(" (%.1f%% of all stale)", PctStaleBlocksWithEqualIcount)
14571457
<< " have matching icount.\n";
14581458
if (PctStale > opts::StaleThreshold) {
1459-
errs() << "BOLT-ERROR: stale functions exceed specified threshold of "
1460-
<< opts::StaleThreshold << "%. Exiting.\n";
1461-
exit(1);
1459+
return createFatalBOLTError(
1460+
Twine("BOLT-ERROR: stale functions exceed specified threshold of ") +
1461+
Twine(opts::StaleThreshold.getValue()) + Twine("%. Exiting.\n"));
14621462
}
14631463
}
14641464
if (NumInferredFunctions) {

0 commit comments

Comments
 (0)