- Notifications
You must be signed in to change notification settings - Fork16.2k
[BOLT] Support computed goto and allow map addrs inside functions#120267
[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 computinggoto's. This also fixes a mapping issue with the bugfix from thisPR:llvm#117766.
llvmbot commentedDec 17, 2024
@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.cppindex 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.snew file mode 100644index 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 |
github-actionsbot commentedDec 17, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
✅ With the latest revision this PR passed the C/C++ code formatter. |
aaupov left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Looks good overall. The PR isn't limited to AArch64, so please drop that from the title. Please make the title more concise, e.g. "Support computed goto", expanding what is changed in the summary.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Asher8118 commentedDec 18, 2024
Thanks for pointing this out, I'll change the title and summary. |
ms178 commentedJan 1, 2025
@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. |
Asher8118 commentedJan 2, 2025
Thanks for bringing this to my attention, I'll need to look into the mentioned issue and see if this patch impacts it. |
ms178 commentedJan 2, 2025
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 |
Asher8118 commentedJan 3, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 withllvm/llvm-project#120267
Asher8118 commentedJan 20, 2025
I've added another test that is target independent. Is there anything else needed to approve this patch? |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
The vendored patches were produced from the latest versions of thefollowing PRs:*llvm/llvm-project#114990*llvm/llvm-project#120267The first improves codegen for computed gotos. There was aregression in LLVM 19 causing a ~10% performance drop in CPython.The second enables BOLT to work with computed gotos. This enablesBOLT to accomplish more on CPython.
The vendored patches were produced from the latest versions of thefollowing PRs:*llvm/llvm-project#114990*llvm/llvm-project#120267The first improves codegen for computed gotos. There was aregression in LLVM 19 causing a ~10% performance drop in CPython.The second enables BOLT to work with computed gotos. This enablesBOLT to accomplish more on CPython.
The vendored patches were produced from the latest versions of thefollowing PRs:*llvm/llvm-project#114990*llvm/llvm-project#120267The first improves codegen for computed gotos. There was aregression in LLVM 19 causing a ~10% performance drop in CPython.The second enables BOLT to work with computed gotos. This enablesBOLT to accomplish more on CPython.
The vendored patches were produced from the latest versions of thefollowing PRs:*llvm/llvm-project#114990*llvm/llvm-project#120267The first improves codegen for computed gotos. There was aregression in LLVM 19 causing a ~10% performance drop in CPython.The second enables BOLT to work with computed gotos. This enablesBOLT to accomplish more on CPython.
The vendored patches were produced from the latest versions of thefollowing PRs:*llvm/llvm-project#114990*llvm/llvm-project#120267The first improves codegen for computed gotos. There was aregression in LLVM 19 causing a ~10% performance drop in CPython.The second enables BOLT to work with computed gotos. This enablesBOLT to accomplish more on CPython.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
SinceFunc is discovered usinggetBinaryFunctionContainingAddress(ReferencedAddress), we should never hit this condition. You can useReferencedOffset unconditionally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think we can hit the case where the contained address is the same as the function address, leadingReferenceOffset to be 0. But the else if condition is redundant you're right. I have removed it.
Uh oh!
There was an error while loading.Please reload this page.
maksfb left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Looks good, but please address the comment regarding the hex print before committing. Thanks!
Uh oh!
There was an error while loading.Please reload this page.
bolt/lib/Rewrite/RewriteInstance.cpp Outdated
| } | ||
| } else { | ||
| BC->errs() << "BOLT-ERROR: referenced address at 0x" | ||
| << ReferencedAddress |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| << ReferencedAddress | |
| <<Twine::utohexstr(ReferencedAddress) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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!
3bba268 intollvm:mainUh oh!
There was an error while loading.Please reload this page.
tuliom commentedMar 21, 2025
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 |
Asher8118 commentedMar 21, 2025
@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. |
Asher8118 commentedMar 25, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Asher8118 commentedMar 25, 2025
maksfb commentedMar 25, 2025
Asher8118 commentedMar 26, 2025
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 removedas subsequent changes have caused it to fail.
This test was added in PR:#120267. The -no-pie case inthe above mentioned test needs to be removed as subsequent changes havecaused it to fail.
This test was added in PR:llvm/llvm-project#120267. The -no-pie case inthe above mentioned test needs to be removed as subsequent changes havecaused it to fail.
The python and postgres tests ensure that,followingllvm/llvm-project#120267,bolt can successfully run on those binaries without any errorsand without the need for the --skip-funcs flag. The rustc andnginx tests only check that bolt can successfully run with no errorson those binaries.
Uh oh!
There was an error while loading.Please reload this page.
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.