[BOLT] Be less strict about Remember-Restore CFI mismatches in input binary #134051
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- instead setIgnored() on failing functions
- check isIgnored() in several Passes to skip
previously ignored functions
bgergely0
requested review from
aaupov,
ayermolo,
dcci,
maksfb,
rafaelauler and
yota9
as code owners
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
Member
|
@llvm/pr-subscribers-bolt Author: Gergely Bálint (bgergely0) ChangesProblemThere are several open issues related to
There is a stack in It has previously been realized that some compilers can generate incorrect code, and one of these assertions were converted into a warning 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. WorkaroundI believe we can relax the other check similarly. In this patch, I convert 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:
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)
|
Contributor
Author
|
The proper fix is merged in #144348. This workaround is obsolete now. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
There are several open issues related to
annotateCFIStatefailing, because of incorrect Remember-Restore CFI sequences in the input binaryThere is a stack in
annotateCFIStateto which we push the state, when encountering aRememberStateCFI, and popping from it, when encountering aRestoreStateCFI.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 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
annotateCFIStatefrom returning avoidtobool, and if BOLT cannot process the CFIs correctly, I mark the functionIgnored. After this, we need to check if the functionisIgnoredin 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!