Skip to content

[BOLT] Support computed goto and allow map addrs inside functions #120267

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 13 commits into from
Mar 19, 2025
9 changes: 8 additions & 1 deletion bolt/lib/Rewrite/RewriteInstance.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2439,6 +2439,13 @@ 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 && !Func->isInConstantIsland(SymAddress)) {
if (const uint64_t SymOffset = SymAddress - Func->getAddress())
Func->addEntryPointAtOffset(SymOffset);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add more checks? E.g., what happens if Symbol points to a function and SymAddress falls into another function? What if the reference is in the constant island? It's better to fail at build time.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll look into adding more checks.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what happens if Symbol points to a function and SymAddress falls into another function?

I've added another check for this.

What if the reference is in the constant island?

My understanding is that  SymAddress is the relocation reference here. In which case, there already is the !Func->isInConstantIsland(SymAddress) check which should make sure it's not in the constant island. Is there some other reference that you're referring to here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the clarification, I have now added BOLT-ERRORS for those checks. Let me know if there is anything I've missed.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@maksfb does this look good to you? Are there any further comments that should be addressed for this PR to be ready?


BC->addDynamicRelocation(Rel.getOffset(), Symbol, RType, Addend);
}
}
Expand Down Expand Up @@ -5599,7 +5606,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"
Expand Down
67 changes: 67 additions & 0 deletions bolt/test/AArch64/computed-goto.s
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
// This test checks that BOLT creates entry points for addresses
// referenced by dynamic relocations.
// The test also checks that BOLT can map addresses inside functions.

// Checks for error and entry points.
# 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
# RUN: llvm-bolt %t.exe -o %t.bolt --print-cfg | FileCheck --check-prefix=CHECK-ENTRYS %s

// Checks for dynamic relocations.
# RUN: llvm-readelf -dr %t.bolt > %t.out.txt
# RUN: llvm-objdump -j .rela.dyn -d %t.bolt >> %t.out.txt
# RUN: FileCheck --check-prefix=CHECK-RELOCS %s --input-file=%t.out.txt

// 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

// Check that there are dynamic relocations.
# CHECK-RELOCS: Dynamic section at offset {{.*}} contains {{.*}} entries:
# CHECK-RELOCS: Relocation section '.rela.dyn' at offset {{.*}} contains {{.*}} entries

// Check that dynamic relocations were updated
# CHECK-RELOCS: [[#%x,OFF:]] [[#%x,INFO_DYN:]] R_AARCH64_RELATIVE [[#%x,ADDR:]]
# CHECK-RELOCS-NEXT: [[#OFF + 8]] {{0*}}[[#INFO_DYN]] R_AARCH64_RELATIVE [[#ADDR + 8]]
# CHECK-RELOCS: [[#ADDR]] <unknown>
# CHECK-RELOCS: [[#ADDR + 8]] <unknown>

// Check that BOLT registers extra entry points for dynamic relocations.
# CHECK-ENTRYS: Binary Function "main" after building cfg {
# CHECK-ENTRYS: IsMultiEntry: 1
# CHECK-ENTRYS: .Ltmp0 {{.*}}
# CHECK-ENTRYS-NEXT: Secondary Entry Point: {{.*}}
# CHECK-ENTRYS: .Ltmp1 {{.*}}
# CHECK-ENTRYS-NEXT: Secondary Entry Point: {{.*}}

.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
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
int main(int argc, char *argv[]) {
static const void *T1[] = { &&L1, &&L2 };
static const void *T2[] = { &&L2, &&L3 };
static const void *T1[] = {&&L1, &&L2};
static const void *T2[] = {&&L2, &&L3};

const void **T = (argc > 1) ? T1 : T2;

Expand Down
16 changes: 0 additions & 16 deletions bolt/test/X86/indirect-goto-pie.test

This file was deleted.

2 changes: 1 addition & 1 deletion bolt/test/X86/indirect-goto.test
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
## Check llvm-bolt processes binaries compiled from sources that use indirect goto.
RUN: %clang %cflags -no-pie %S/Inputs/indirect_goto.c -Wl,-q -o %t
RUN: %clang %cflags -no-pie %S/../Inputs/indirect_goto.c -Wl,-q -o %t
RUN: llvm-bolt %t -o %t.null --relocs=1 --print-cfg --print-only=main \
RUN: --strict \
RUN: 2>&1 | FileCheck %s
Expand Down
13 changes: 13 additions & 0 deletions bolt/test/dynamic-relocs.test
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// This test checks that BOLT creates entry points for addresses
// referenced by dynamic relocations.

RUN: %clang %cflags %S/Inputs/indirect_goto.c -o %t.exe -Wl,-q
RUN: llvm-bolt %t.exe -o %t.bolt --print-cfg | FileCheck %s

// Check that bolt does not emit any error.
CHECK-NOT: BOLT-ERROR

// Check that BOLT registers extra entry points for dynamic relocations.
CHECK: Binary Function "main" after building cfg {
CHECK: IsMultiEntry: 1
CHECK: Secondary Entry Points : {{.*}}
Loading