[BOLT] Fix references in ignored functions in CFG state #140678
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
When we call setIgnored() on functions that already have CFG built, these functions are not going to get emitted and we risk missing external function references being updated. To mitigate the potential issues, run scanExternalRefs() on such functions to create patches/relocations. Since scanExternalRefs() relies on function relocations, we have to preserve relocations until the function is emitted. As a result, the memory overhead without debug info update could reach up to 2%.
maksfb
requested review from
aaupov,
rafaelauler,
ayermolo and
yota9
as code owners
|
@llvm/pr-subscribers-bolt Author: Maksim Panchenko (maksfb) ChangesWhen we call setIgnored() on functions that already have CFG built, these functions are not going to get emitted and we risk missing external function references being updated. To mitigate the potential issues, run scanExternalRefs() on such functions to create patches/relocations. Since scanExternalRefs() relies on function relocations, we have to preserve relocations until the function is emitted. As a result, the memory overhead without debug info update could reach up to 2%. Full diff: https://github.com/llvm/llvm-project/pull/140678.diff 6 Files Affected:
diff --git a/bolt/include/bolt/Core/BinaryFunction.h b/bolt/include/bolt/Core/BinaryFunction.h
index 6f3b5923d3ef4..a8624f285c1e9 100644
--- a/bolt/include/bolt/Core/BinaryFunction.h
+++ b/bolt/include/bolt/Core/BinaryFunction.h
@@ -2335,6 +2335,7 @@ class BinaryFunction {
releaseCFG();
CurrentState = State::Emitted;
}
+ clearList(Relocations);
}
/// Process LSDA information for the function.
diff --git a/bolt/lib/Core/BinaryContext.cpp b/bolt/lib/Core/BinaryContext.cpp
index 82ede09c29ddf..f070abb6fadd1 100644
--- a/bolt/lib/Core/BinaryContext.cpp
+++ b/bolt/lib/Core/BinaryContext.cpp
@@ -1032,10 +1032,8 @@ void BinaryContext::adjustCodePadding() {
if (!hasValidCodePadding(BF)) {
if (HasRelocations) {
- if (opts::Verbosity >= 1) {
- this->outs() << "BOLT-INFO: function " << BF
- << " has invalid padding. Ignoring the function.\n";
- }
+ this->errs() << "BOLT-WARNING: function " << BF
+ << " has invalid padding. Ignoring the function\n";
BF.setIgnored();
} else {
BF.setMaxSize(BF.getSize());
diff --git a/bolt/lib/Core/BinaryFunction.cpp b/bolt/lib/Core/BinaryFunction.cpp
index 851fa36a6b4b7..f887f8ee6a3ae 100644
--- a/bolt/lib/Core/BinaryFunction.cpp
+++ b/bolt/lib/Core/BinaryFunction.cpp
@@ -1531,8 +1531,6 @@ Error BinaryFunction::disassemble() {
if (uint64_t Offset = getFirstInstructionOffset())
Labels[Offset] = BC.Ctx->createNamedTempSymbol();
- clearList(Relocations);
-
if (!IsSimple) {
clearList(Instructions);
return createNonFatalBOLTError("");
@@ -3237,16 +3235,26 @@ void BinaryFunction::setTrapOnEntry() {
}
void BinaryFunction::setIgnored() {
+ IsIgnored = true;
+
if (opts::processAllFunctions()) {
// We can accept ignored functions before they've been disassembled.
- // In that case, they would still get disassembled and emited, but not
+ // In that case, they would still get disassembled and emitted, but not
// optimized.
- assert(CurrentState == State::Empty &&
- "cannot ignore non-empty functions in current mode");
- IsIgnored = true;
+ if (CurrentState != State::Empty) {
+ BC.errs() << "BOLT-ERROR: cannot ignore non-empty function " << *this
+ << " in current mode\n";
+ exit(1);
+ }
return;
}
+ IsSimple = false;
+ LLVM_DEBUG(dbgs() << "Ignoring " << getPrintName() << '\n');
+
+ if (CurrentState == State::Empty)
+ return;
+
clearDisasmState();
// Clear CFG state too.
@@ -3266,9 +3274,11 @@ void BinaryFunction::setIgnored() {
CurrentState = State::Empty;
- IsIgnored = true;
- IsSimple = false;
- LLVM_DEBUG(dbgs() << "Ignoring " << getPrintName() << '\n');
+ // Fix external references in the original function body.
+ if (BC.HasRelocations) {
+ LLVM_DEBUG(dbgs() << "Scanning refs in " << *this << '\n');
+ scanExternalRefs();
+ }
}
void BinaryFunction::duplicateConstantIslands() {
@@ -3755,7 +3765,6 @@ void BinaryFunction::postProcessBranches() {
MCSymbol *BinaryFunction::addEntryPointAtOffset(uint64_t Offset) {
assert(Offset && "cannot add primary entry point");
- assert(CurrentState == State::Empty || CurrentState == State::Disassembled);
const uint64_t EntryPointAddress = getAddress() + Offset;
MCSymbol *LocalSymbol = getOrCreateLocalLabel(EntryPointAddress);
@@ -3764,6 +3773,8 @@ MCSymbol *BinaryFunction::addEntryPointAtOffset(uint64_t Offset) {
if (EntrySymbol)
return EntrySymbol;
+ assert(CurrentState == State::Empty || CurrentState == State::Disassembled);
+
if (BinaryData *EntryBD = BC.getBinaryDataAtAddress(EntryPointAddress)) {
EntrySymbol = EntryBD->getSymbol();
} else {
diff --git a/bolt/lib/Rewrite/RewriteInstance.cpp b/bolt/lib/Rewrite/RewriteInstance.cpp
index ad062ea3622d1..4ca407b11d655 100644
--- a/bolt/lib/Rewrite/RewriteInstance.cpp
+++ b/bolt/lib/Rewrite/RewriteInstance.cpp
@@ -3433,6 +3433,7 @@ void RewriteInstance::disassembleFunctions() {
BC->outs() << "BOLT-INFO: could not disassemble function " << Function
<< ". Will ignore.\n";
// Forcefully ignore the function.
+ Function.scanExternalRefs();
Function.setIgnored();
});
diff --git a/bolt/test/AArch64/patch-ignored.s b/bolt/test/AArch64/patch-ignored.s
new file mode 100644
index 0000000000000..377d4dacea606
--- /dev/null
+++ b/bolt/test/AArch64/patch-ignored.s
@@ -0,0 +1,33 @@
+## Check that llvm-bolt patches functions that are getting ignored after their
+## CFG was constructed.
+
+# RUN: %clang %cflags %s -o %t.exe -Wl,-q
+# RUN: llvm-bolt %t.exe -o %t.bolt --force-patch 2>&1 | FileCheck %s
+# RUN: llvm-objdump -d %t.bolt | FileCheck %s --check-prefix=CHECK-OBJDUMP
+
+ .text
+
+## The function is too small to be patched and BOLT is forced to ignore it under
+## --force-patch. Check that the reference to _start is updated.
+# CHECK: BOLT-WARNING: failed to patch entries in unpatchable
+ .globl unpatchable
+ .type unpatchable, %function
+unpatchable:
+ .cfi_startproc
+# CHECK-OBJDUMP: <unpatchable>:
+# CHECK-OBJDUMP-NEXT: bl {{.*}} <_start>
+ bl _start
+ ret
+ .cfi_endproc
+ .size unpatchable, .-unpatchable
+
+ .globl _start
+ .type _start, %function
+_start:
+ .cfi_startproc
+ cmp x0, 1
+ b.eq .L0
+.L0:
+ ret x30
+ .cfi_endproc
+ .size _start, .-_start
diff --git a/bolt/test/X86/patch-ignored.s b/bolt/test/X86/patch-ignored.s
new file mode 100644
index 0000000000000..4f98b51ca1ca0
--- /dev/null
+++ b/bolt/test/X86/patch-ignored.s
@@ -0,0 +1,44 @@
+## Check that llvm-bolt patches functions that are getting ignored after their
+## CFG was constructed.
+
+# RUN: %clang %cflags %s -o %t.exe -Wl,-q
+# RUN: llvm-bolt %t.exe -o %t.bolt 2>&1 | FileCheck %s
+# RUN: llvm-objdump -d %t.bolt | FileCheck %s --check-prefix=CHECK-OBJDUMP
+
+ .text
+
+## After the CFG is built, the following function will be marked as ignored
+## due to the presence of the internal call.
+# CHECK: BOLT-WARNING: will skip the following function
+# CHECK-NEXT: internal_call
+ .globl internal_call
+ .type internal_call, %function
+internal_call:
+ .cfi_startproc
+# CHECK-OBJDUMP: <internal_call>:
+ call .L1
+ jmp .L2
+# CHECK-OBJDUMP: jmp
+.L1:
+ jmp _start
+# CHECK-OBJDUMP: jmp
+# CHECK-OBJDUMP-SAME: <_start>
+ ret
+.L2:
+ jmp _start
+# CHECK-OBJDUMP: jmp
+# CHECK-OBJDUMP-SAME: <_start>
+ .cfi_endproc
+ .size internal_call, .-internal_call
+
+ .globl _start
+ .type _start, %function
+_start:
+ .cfi_startproc
+ cmpq %rdi, 1
+ jne .L0
+ movq %rdi, %rax
+.L0:
+ ret
+ .cfi_endproc
+ .size _start, .-_start
|
aaupov
approved these changes
May 20, 2025
paschalis-mpeis
approved these changes
May 20, 2025
| @@ -3764,6 +3773,8 @@ MCSymbol *BinaryFunction::addEntryPointAtOffset(uint64_t Offset) { | |||
| if (EntrySymbol) | |||
| return EntrySymbol; | |||
|
|
|||
| assert(CurrentState == State::Empty || CurrentState == State::Disassembled); | |||
There was a problem hiding this comment.
Current tests aren't affected by moving this hunk. Does it make some difference?
There was a problem hiding this comment.
One of our internal tests failed. The newly ignored function had a call to a secondary entry point that caused the earlier assertion to fire.
| @@ -3433,6 +3433,7 @@ void RewriteInstance::disassembleFunctions() { | |||
| BC->outs() << "BOLT-INFO: could not disassemble function " << Function | |||
| << ". Will ignore.\n"; | |||
| // Forcefully ignore the function. | |||
| Function.scanExternalRefs(); | |||
There was a problem hiding this comment.
Note that scanExternalRefs is now called inside Function.setIgnored body. It never runs twice though, due to the code structure.
I wonder if it would be clearer to perform all scanExternalRefs calls within BF.setIgnored at the relevant return points.
Maybe it's also worth adding tests for these cases using the flags --skip-funcs=unpatchable and --strict?
There was a problem hiding this comment.
Good point. Let me think about it.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
When we call setIgnored() on functions that already have CFG built, these functions are not going to get emitted and we risk missing external function references being updated.
To mitigate the potential issues, run scanExternalRefs() on such functions to create patches/relocations.
Since scanExternalRefs() relies on function relocations, we have to preserve relocations until the function is emitted. As a result, the memory overhead without debug info update could reach up to 2%.