[llvm] [BOLT] Be less strict about Remember-Restore CFI mismatches in input binary (PR #134051)
via llvm-commits
llvm-commits at lists.llvm.org
Wed Apr 2 01:34:09 PDT 2025
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-bolt
Author: Gergely Bálint (bgergely0)
<details>
<summary>Changes</summary>
## Problem
There are several open issues related to `annotateCFIState` failing, because of incorrect Remember-Restore CFI sequences in the input binary
- https://github.com/llvm/llvm-project/issues/133501
- https://github.com/llvm/llvm-project/issues/131790
- https://github.com/llvm/llvm-project/issues/124157
- https://github.com/llvm/llvm-project/issues/61971
There is a stack in `annotateCFIState` to which we push the state, when encountering a `RememberState` CFI, and popping from it, when encountering a `RestoreState` CFI.
There are two assertion that can fail: one is trying to pop from an empty stack, the other is at the end of the function, where we check if the stack is empty at the end.
It has previously been realized that some compilers can generate incorrect code, and [one of these assertions were converted into a warning](https://github.com/llvm/llvm-project/commit/f83a89c1b1ce78cfac1de1c72a03b234d2a844b6#diff-ca39122670f58da421af7031c33576d77078d2b81f48aa9baccd945724edb891) and this is already in main. However, the other is still an assertion, causing many users to not be able to run BOLT on certain binaries.
## Workaround
I believe we can relax the other check similarly. In this patch, I convert `annotateCFIState` from returning a `void` to `bool`, and if BOLT cannot process the CFIs correctly, I mark the function `Ignored`. After this, we need to check if the function `isIgnored` in several passes.
This way, the function with incorrect CFIs is not optimized, but the rest of the input binary is.
I am not entirely convinced that the issue is only with codegen (so the input binary), and not partly in BOLT. Because of this, I view my patch as a workaround, instead of a proper solution.
Feel free to use the patch!
---
Full diff: https://github.com/llvm/llvm-project/pull/134051.diff
12 Files Affected:
- (modified) bolt/include/bolt/Core/BinaryFunction.h (+1-1)
- (modified) bolt/lib/Core/BinaryFunction.cpp (+17-4)
- (modified) bolt/lib/Passes/ADRRelaxationPass.cpp (+1-1)
- (modified) bolt/lib/Passes/Aligner.cpp (+7-1)
- (modified) bolt/lib/Passes/BinaryPasses.cpp (+8-2)
- (modified) bolt/lib/Passes/FixRelaxationPass.cpp (+2)
- (modified) bolt/lib/Passes/Instrumentation.cpp (+1-1)
- (modified) bolt/lib/Passes/LongJmp.cpp (+4)
- (modified) bolt/lib/Passes/MCF.cpp (+2)
- (modified) bolt/lib/Passes/ValidateInternalCalls.cpp (+2)
- (modified) bolt/lib/Passes/VeneerElimination.cpp (+3-4)
- (modified) bolt/lib/Rewrite/RewriteInstance.cpp (+3)
``````````diff
diff --git a/bolt/include/bolt/Core/BinaryFunction.h b/bolt/include/bolt/Core/BinaryFunction.h
index d3d11f8c5fb73..753e8ef183a40 100644
--- a/bolt/include/bolt/Core/BinaryFunction.h
+++ b/bolt/include/bolt/Core/BinaryFunction.h
@@ -453,7 +453,7 @@ class BinaryFunction {
/// Annotate each basic block entry with its current CFI state. This is
/// run right after the construction of CFG while basic blocks are in their
/// original order.
- void annotateCFIState();
+ bool annotateCFIState();
/// Associate DW_CFA_GNU_args_size info with invoke instructions
/// (call instructions with non-empty landing pad).
diff --git a/bolt/lib/Core/BinaryFunction.cpp b/bolt/lib/Core/BinaryFunction.cpp
index d1b293ada5fdc..638b2e89e8853 100644
--- a/bolt/lib/Core/BinaryFunction.cpp
+++ b/bolt/lib/Core/BinaryFunction.cpp
@@ -2426,7 +2426,13 @@ Error BinaryFunction::buildCFG(MCPlusBuilder::AllocatorIdTy AllocatorId) {
recomputeLandingPads();
// Assign CFI information to each BB entry.
- annotateCFIState();
+ bool SuccessfulAnnotation = annotateCFIState();
+ if (!SuccessfulAnnotation) {
+ BC.errs() << "BOLT-WARNING: failed to annotate CFI state for "
+ << this->getPrintName() << "\n";
+ setIgnored();
+ return createNonFatalBOLTError("");
+ }
// Annotate invoke instructions with GNU_args_size data.
propagateGnuArgsSizeInfo(AllocatorId);
@@ -2443,7 +2449,11 @@ Error BinaryFunction::buildCFG(MCPlusBuilder::AllocatorIdTy AllocatorId) {
Layout.updateLayoutIndices();
- normalizeCFIState();
+ if (SuccessfulAnnotation)
+ normalizeCFIState();
+ else
+ BC.errs() << "BOLT-WARNING: cannot normalize CFI State for "
+ << this->getPrintName() << "\n";
// Clean-up memory taken by intermediate structures.
//
@@ -2618,7 +2628,7 @@ uint64_t BinaryFunction::getFunctionScore() const {
return FunctionScore;
}
-void BinaryFunction::annotateCFIState() {
+bool BinaryFunction::annotateCFIState() {
assert(CurrentState == State::Disassembled && "unexpected function state");
assert(!BasicBlocks.empty() && "basic block list should not be empty");
@@ -2654,7 +2664,9 @@ void BinaryFunction::annotateCFIState() {
EffectiveState = State;
break;
case MCCFIInstruction::OpRestoreState:
- assert(!StateStack.empty() && "corrupt CFI stack");
+ if (StateStack.empty()) {
+ return false;
+ }
EffectiveState = StateStack.top();
StateStack.pop();
break;
@@ -2673,6 +2685,7 @@ void BinaryFunction::annotateCFIState() {
BC.errs() << "BOLT-WARNING: non-empty CFI stack at the end of " << *this
<< '\n';
}
+ return StateStack.empty();
}
namespace {
diff --git a/bolt/lib/Passes/ADRRelaxationPass.cpp b/bolt/lib/Passes/ADRRelaxationPass.cpp
index 4b37a061ac12d..39e6b9529999b 100644
--- a/bolt/lib/Passes/ADRRelaxationPass.cpp
+++ b/bolt/lib/Passes/ADRRelaxationPass.cpp
@@ -36,7 +36,7 @@ namespace bolt {
static bool PassFailed = false;
void ADRRelaxationPass::runOnFunction(BinaryFunction &BF) {
- if (PassFailed)
+ if (PassFailed || BF.isIgnored())
return;
BinaryContext &BC = BF.getBinaryContext();
diff --git a/bolt/lib/Passes/Aligner.cpp b/bolt/lib/Passes/Aligner.cpp
index c3ddedaaa1466..431d469fb9737 100644
--- a/bolt/lib/Passes/Aligner.cpp
+++ b/bolt/lib/Passes/Aligner.cpp
@@ -62,6 +62,8 @@ namespace bolt {
// Align function to the specified byte-boundary (typically, 64) offsetting
// the fuction by not more than the corresponding value
static void alignMaxBytes(BinaryFunction &Function) {
+ if (Function.isIgnored())
+ return;
Function.setAlignment(opts::AlignFunctions);
Function.setMaxAlignmentBytes(opts::AlignFunctionsMaxBytes);
Function.setMaxColdAlignmentBytes(opts::AlignFunctionsMaxBytes);
@@ -73,6 +75,9 @@ static void alignMaxBytes(BinaryFunction &Function) {
// -- the specified number of bytes
static void alignCompact(BinaryFunction &Function,
const MCCodeEmitter *Emitter) {
+
+ if (Function.isIgnored())
+ return;
const BinaryContext &BC = Function.getBinaryContext();
size_t HotSize = 0;
size_t ColdSize = 0;
@@ -97,7 +102,8 @@ static void alignCompact(BinaryFunction &Function,
void AlignerPass::alignBlocks(BinaryFunction &Function,
const MCCodeEmitter *Emitter) {
- if (!Function.hasValidProfile() || !Function.isSimple())
+ if (Function.isIgnored() || !Function.hasValidProfile() ||
+ !Function.isSimple())
return;
const BinaryContext &BC = Function.getBinaryContext();
diff --git a/bolt/lib/Passes/BinaryPasses.cpp b/bolt/lib/Passes/BinaryPasses.cpp
index d8628c62d8654..e4c6aaaa00916 100644
--- a/bolt/lib/Passes/BinaryPasses.cpp
+++ b/bolt/lib/Passes/BinaryPasses.cpp
@@ -1810,10 +1810,14 @@ Error PrintProgramStats::runOnFunctions(BinaryContext &BC) {
}
Error InstructionLowering::runOnFunctions(BinaryContext &BC) {
- for (auto &BFI : BC.getBinaryFunctions())
- for (BinaryBasicBlock &BB : BFI.second)
+ for (auto &BFI : BC.getBinaryFunctions()) {
+ BinaryFunction &BF = BFI.second;
+ if (BF.isIgnored())
+ continue;
+ for (BinaryBasicBlock &BB : BF)
for (MCInst &Instruction : BB)
BC.MIB->lowerTailCall(Instruction);
+ }
return Error::success();
}
@@ -2029,6 +2033,8 @@ Error SpecializeMemcpy1::runOnFunctions(BinaryContext &BC) {
}
void RemoveNops::runOnFunction(BinaryFunction &BF) {
+ if (BF.isIgnored())
+ return;
const BinaryContext &BC = BF.getBinaryContext();
for (BinaryBasicBlock &BB : BF) {
for (int64_t I = BB.size() - 1; I >= 0; --I) {
diff --git a/bolt/lib/Passes/FixRelaxationPass.cpp b/bolt/lib/Passes/FixRelaxationPass.cpp
index 7c970e464a94e..108749f1129ed 100644
--- a/bolt/lib/Passes/FixRelaxationPass.cpp
+++ b/bolt/lib/Passes/FixRelaxationPass.cpp
@@ -24,6 +24,8 @@ namespace bolt {
// target of ADD is another symbol. When found change ADRP symbol reference to
// the ADDs one.
void FixRelaxations::runOnFunction(BinaryFunction &BF) {
+ if (BF.isIgnored())
+ return;
BinaryContext &BC = BF.getBinaryContext();
for (BinaryBasicBlock &BB : BF) {
for (auto II = BB.begin(); II != BB.end(); ++II) {
diff --git a/bolt/lib/Passes/Instrumentation.cpp b/bolt/lib/Passes/Instrumentation.cpp
index fbf889279f1c0..e11a176399277 100644
--- a/bolt/lib/Passes/Instrumentation.cpp
+++ b/bolt/lib/Passes/Instrumentation.cpp
@@ -368,7 +368,7 @@ bool Instrumentation::instrumentOneTarget(
void Instrumentation::instrumentFunction(BinaryFunction &Function,
MCPlusBuilder::AllocatorIdTy AllocId) {
- if (Function.hasUnknownControlFlow())
+ if (Function.hasUnknownControlFlow() || Function.isIgnored())
return;
BinaryContext &BC = Function.getBinaryContext();
diff --git a/bolt/lib/Passes/LongJmp.cpp b/bolt/lib/Passes/LongJmp.cpp
index e6bd417705e6f..b8674f60ce969 100644
--- a/bolt/lib/Passes/LongJmp.cpp
+++ b/bolt/lib/Passes/LongJmp.cpp
@@ -557,6 +557,10 @@ Error LongJmpPass::relax(BinaryFunction &Func, bool &Modified) {
const BinaryContext &BC = Func.getBinaryContext();
assert(BC.isAArch64() && "Unsupported arch");
+
+ if (Func.isIgnored())
+ return Error::success();
+
constexpr int InsnSize = 4; // AArch64
std::vector<std::pair<BinaryBasicBlock *, std::unique_ptr<BinaryBasicBlock>>>
Insertions;
diff --git a/bolt/lib/Passes/MCF.cpp b/bolt/lib/Passes/MCF.cpp
index 77dea7369140e..c378803469661 100644
--- a/bolt/lib/Passes/MCF.cpp
+++ b/bolt/lib/Passes/MCF.cpp
@@ -435,6 +435,8 @@ void equalizeBBCounts(DataflowInfoManager &Info, BinaryFunction &BF) {
}
void EstimateEdgeCounts::runOnFunction(BinaryFunction &BF) {
+ if (BF.isIgnored())
+ return;
EdgeWeightMap PredEdgeWeights;
EdgeWeightMap SuccEdgeWeights;
if (!opts::IterativeGuess) {
diff --git a/bolt/lib/Passes/ValidateInternalCalls.cpp b/bolt/lib/Passes/ValidateInternalCalls.cpp
index bdab895b6ac2e..a5ee058fb9fe5 100644
--- a/bolt/lib/Passes/ValidateInternalCalls.cpp
+++ b/bolt/lib/Passes/ValidateInternalCalls.cpp
@@ -306,6 +306,8 @@ Error ValidateInternalCalls::runOnFunctions(BinaryContext &BC) {
std::set<BinaryFunction *> NeedsValidation;
for (auto &BFI : BC.getBinaryFunctions()) {
BinaryFunction &Function = BFI.second;
+ if (Function.isIgnored())
+ continue;
for (BinaryBasicBlock &BB : Function) {
for (MCInst &Inst : BB) {
if (getInternalCallTarget(Function, Inst)) {
diff --git a/bolt/lib/Passes/VeneerElimination.cpp b/bolt/lib/Passes/VeneerElimination.cpp
index 99d0ffeca8cc2..ead875de15ab1 100644
--- a/bolt/lib/Passes/VeneerElimination.cpp
+++ b/bolt/lib/Passes/VeneerElimination.cpp
@@ -40,10 +40,7 @@ Error VeneerElimination::runOnFunctions(BinaryContext &BC) {
std::unordered_map<const MCSymbol *, const MCSymbol *> VeneerDestinations;
uint64_t NumEliminatedVeneers = 0;
for (BinaryFunction &BF : llvm::make_second_range(BC.getBinaryFunctions())) {
- if (!isPossibleVeneer(BF))
- continue;
-
- if (BF.isIgnored())
+ if (BF.isIgnored() || !isPossibleVeneer(BF))
continue;
MCInst &FirstInstruction = *(BF.begin()->begin());
@@ -84,6 +81,8 @@ Error VeneerElimination::runOnFunctions(BinaryContext &BC) {
uint64_t VeneerCallers = 0;
for (BinaryFunction &BF : llvm::make_second_range(BC.getBinaryFunctions())) {
+ if (BF.isIgnored())
+ continue;
for (BinaryBasicBlock &BB : BF) {
for (MCInst &Instr : BB) {
if (!BC.MIB->isCall(Instr) || BC.MIB->isIndirectCall(Instr))
diff --git a/bolt/lib/Rewrite/RewriteInstance.cpp b/bolt/lib/Rewrite/RewriteInstance.cpp
index f204aa3eb8a38..4f46a8dcb13da 100644
--- a/bolt/lib/Rewrite/RewriteInstance.cpp
+++ b/bolt/lib/Rewrite/RewriteInstance.cpp
@@ -3503,6 +3503,9 @@ void RewriteInstance::postProcessFunctions() {
if (Function.empty())
continue;
+ if (Function.isIgnored())
+ continue;
+
Function.postProcessCFG();
if (opts::PrintAll || opts::PrintCFG)
``````````
</details>
https://github.com/llvm/llvm-project/pull/134051
More information about the llvm-commits
mailing list