[BOLT] Support computed goto and allow map addrs inside functions #120267
Conversation
…ic relocations and allow getNewFunctionOrDataAddress to map addrs inside functions. By adding addresses referenced by dynamic relocations as entry points, this patch fixes an issue where bolt fails on code using computing goto's. This also fixes a mapping issue with the bugfix from this PR: llvm#117766.
Rin18
requested review from
aaupov,
maksfb,
rafaelauler,
ayermolo and
dcci
as code owners
|
@llvm/pr-subscribers-bolt Author: Rin Dobrescu (Rin18) ChangesBy adding addresses referenced by dynamic relocations as entry points, this patch fixes an issue where bolt fails on code using computing goto's. This also fixes a mapping issue with the bugfix from this PR: #117766. Full diff: https://github.com/llvm/llvm-project/pull/120267.diff 2 Files Affected:
diff --git a/bolt/lib/Rewrite/RewriteInstance.cpp b/bolt/lib/Rewrite/RewriteInstance.cpp
index 4329235d470497..55fcd6b6e782c4 100644
--- a/bolt/lib/Rewrite/RewriteInstance.cpp
+++ b/bolt/lib/Rewrite/RewriteInstance.cpp
@@ -2439,6 +2439,14 @@ void RewriteInstance::readDynamicRelocations(const SectionRef &Section,
if (Symbol)
SymbolIndex[Symbol] = getRelocationSymbol(InputFile, Rel);
+ const uint64_t SymAddress = SymbolAddress + Addend;
+ BinaryFunction *Func = BC->getBinaryFunctionContainingAddress(SymAddress);
+ if(Func){
+ const uint64_t FunctionOffset = SymAddress - Func->getAddress();
+ if(FunctionOffset)
+ Func->addEntryPointAtOffset(FunctionOffset);
+ }
+
BC->addDynamicRelocation(Rel.getOffset(), Symbol, RType, Addend);
}
}
@@ -5599,7 +5607,7 @@ uint64_t RewriteInstance::getNewFunctionOrDataAddress(uint64_t OldAddress) {
for (const BinaryBasicBlock &BB : *BF)
if (BB.isEntryPoint() &&
(BF->getAddress() + BB.getOffset()) == OldAddress)
- return BF->getOutputAddress() + BB.getOffset();
+ return BB.getOutputStartAddress();
}
BC->errs() << "BOLT-ERROR: unable to get new address corresponding to "
"input address 0x"
diff --git a/bolt/test/AArch64/computed-goto.s b/bolt/test/AArch64/computed-goto.s
new file mode 100644
index 00000000000000..043f9a8e37e6b0
--- /dev/null
+++ b/bolt/test/AArch64/computed-goto.s
@@ -0,0 +1,39 @@
+# RUN: llvm-mc -filetype=obj -triple aarch64-unknown-unknown %s -o %t.o
+# RUN: %clang %cflags %t.o -o %t.exe -Wl,-q
+# RUN: llvm-bolt %t.exe -o %t.bolt 2>&1 | FileCheck %s
+
+## Before bolt could handle mapping addresses within moved functions, it
+## would bail out with an error of the form:
+## BOLT-ERROR: unable to get new address corresponding to input address 0x10390 in function main. Consider adding this function to --skip-funcs=...
+## These addresses arise if computed GOTO is in use.
+## Check that bolt does not emit any error.
+
+# CHECK-NOT: BOLT-ERROR
+
+.globl main
+.p2align 2
+.type main,@function
+main:
+.cfi_startproc
+ adrp x8, .L__const.main.ptrs+8
+ add x8, x8, :lo12:.L__const.main.ptrs+8
+ ldr x9, [x8], #8
+ br x9
+
+.Label0: // Block address taken
+ ldr x9, [x8], #8
+ br x9
+
+.Label1: // Block address taken
+ mov w0, #42
+ ret
+
+.Lfunc_end0:
+.size main, .Lfunc_end0-main
+.cfi_endproc
+ .type .L__const.main.ptrs,@object
+ .section .data.rel.ro,"aw",@progbits
+ .p2align 3, 0x0
+.L__const.main.ptrs:
+ .xword .Label0
+ .xword .Label1
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
Rin18
changed the title
Thanks for pointing this out, I'll change the title and summary. |
|
@Rin18 FYI, this patch might help to improve the usage of BOLT in Python as discussed in a related Python issue, see the comment and discussion in: python/cpython#124948 (comment) Maybe it is a good test case for this MR, too. |
Thanks for bringing this to my attention, I'll need to look into the mentioned issue and see if this patch impacts it. |
Great, please also notice some downstream work on the python side which works around the current BOLT issues and might provide some hints: astral-sh/python-build-standalone#463 Ideally, there should be no need to use |
I've taken a look at the link to the issue you've mentioned. This patch provides support for computed goto's. Once the PR is merged, it should get rid of the need to provide the |
- Updated BOLT flags; unfortunately three functions still need to get skipped even with a patched LLVM with llvm/llvm-project#120267
|
I've added another test that is target independent. Is there anything else needed to approve this patch? |
bolt/lib/Rewrite/RewriteInstance.cpp
Outdated
| if (Func && !Func->isInConstantIsland(SymAddress)) { | ||
| if (const uint64_t SymOffset = SymAddress - Func->getAddress()) | ||
| Func->addEntryPointAtOffset(SymOffset); | ||
| } |
There was a problem hiding this comment.
To clarify my original comment, I'd like us to detect cases that are not supported and fail during build time. E.g. if the relocation references foo + 0x20 and the size of foo is 0x10, then we should report an error. Likewise if the offset points to a constant islands. I don't think at the moment we will update the reference correctly and hence should issue an error.
The vendored patches were produced from the latest versions of the following PRs: * llvm/llvm-project#114990 * llvm/llvm-project#120267 The first improves codegen for computed gotos. There was a regression in LLVM 19 causing a ~10% performance drop in CPython. The second enables BOLT to work with computed gotos. This enables BOLT to accomplish more on CPython.
The vendored patches were produced from the latest versions of the following PRs: * llvm/llvm-project#114990 * llvm/llvm-project#120267 The first improves codegen for computed gotos. There was a regression in LLVM 19 causing a ~10% performance drop in CPython. The second enables BOLT to work with computed gotos. This enables BOLT to accomplish more on CPython.
The vendored patches were produced from the latest versions of the following PRs: * llvm/llvm-project#114990 * llvm/llvm-project#120267 The first improves codegen for computed gotos. There was a regression in LLVM 19 causing a ~10% performance drop in CPython. The second enables BOLT to work with computed gotos. This enables BOLT to accomplish more on CPython.
The vendored patches were produced from the latest versions of the following PRs: * llvm/llvm-project#114990 * llvm/llvm-project#120267 The first improves codegen for computed gotos. There was a regression in LLVM 19 causing a ~10% performance drop in CPython. The second enables BOLT to work with computed gotos. This enables BOLT to accomplish more on CPython.
The vendored patches were produced from the latest versions of the following PRs: * llvm/llvm-project#114990 * llvm/llvm-project#120267 The first improves codegen for computed gotos. There was a regression in LLVM 19 causing a ~10% performance drop in CPython. The second enables BOLT to work with computed gotos. This enables BOLT to accomplish more on CPython.
bolt/lib/Rewrite/RewriteInstance.cpp
Outdated
| if (const uint64_t ReferenceOffset = | ||
| ReferencedAddress - Func->getAddress()) { | ||
| Func->addEntryPointAtOffset(ReferenceOffset); | ||
| } else if (ReferencedAddress < Func->getAddress()) { |
There was a problem hiding this comment.
Since Func is discovered using getBinaryFunctionContainingAddress(ReferencedAddress), we should never hit this condition. You can use ReferencedOffset unconditionally.
There was a problem hiding this comment.
I think we can hit the case where the contained address is the same as the function address, leading ReferenceOffset to be 0. But the else if condition is redundant you're right. I have removed it.
bolt/lib/Rewrite/RewriteInstance.cpp
Outdated
| } | ||
| } else { | ||
| BC->errs() << "BOLT-ERROR: referenced address at 0x" | ||
| << ReferencedAddress |
There was a problem hiding this comment.
| << ReferencedAddress | |
| << Twine::utohexstr(ReferencedAddress) |
There was a problem hiding this comment.
please address the comment regarding the hex print before committing
I apologise, I thought I addressed that in my previous commit. It is in hex print now. Thanks for the review!
|
Test @Rin18 , can you reproduce this? A full log is available at: https://download.copr.fedorainfracloud.org/results/@fedora-llvm-team/llvm-snapshots-big-merge-20250321/fedora-rawhide-aarch64/08800037-llvm/builder-live.log.gz |
@tuliom yes I am able to reproduce this after pulling all the changes from main. Looks like something changed while I was working on the patch and extra entry points for dynamic relocations are added even in the NON-PIE case. I'll have to look into which commit adds this problem. |
Yes, I've checked the NO-PIE test without the changes in my PR and it fails. Thanks, I'll remove the test. |
This test was added in PR: llvm#120267. The -no-pie case in the above mentioned test needs to be removed as subsequent changes have caused it to fail.
This test was added in PR: #120267. The -no-pie case in the above mentioned test needs to be removed as subsequent changes have caused it to fail.
This test was added in PR: llvm/llvm-project#120267. The -no-pie case in the above mentioned test needs to be removed as subsequent changes have caused it to fail.
Create entry points for addresses referenced by dynamic relocations and allow getNewFunctionOrDataAddress to map addrs inside functions. By adding addresses referenced by dynamic relocations as entry points, this patch fixes an issue where bolt fails on code using computing goto's. This also fixes a mapping issue with the bugfix from this PR: #117766.