[llvm] [BOLT][AArch64] Implemented createDummyReturnFunction. (PR #96626)
Paschalis Mpeis via llvm-commits
llvm-commits at lists.llvm.org
Tue Jun 25 05:39:29 PDT 2024
https://github.com/paschalis-mpeis created https://github.com/llvm/llvm-project/pull/96626
On AArch64, this method is needed when trying to instrument a static
binary.
Sample commands:
```bash
clang -Wl,-q test.c -static -o out
llvm-bolt -instrument -instrumentation-sleep-time=5 out -o out.instr
```
# (STACKED / DRAFT PR)
>From 3c4f4f3810ae1e21ff707d78d923a61aa0cea89c Mon Sep 17 00:00:00 2001
From: Adam Kallai <kadam at inf.u-szeged.hu>
Date: Thu, 29 Feb 2024 09:45:39 +0100
Subject: [PATCH 1/7] [BOLT][AArch64] Fixes assertion errors occurred when
perf2bolt was executed
BOLT only checks for the most common indirect branch pattern during
the branch analyzation. Needs to extend the current logic with other cases
which slightly differs from the expected one.
This instruction sequences comes from libc,
it occurs when the binary is static.
As a workaround mark it as UNKNOWN branch and add support for them
in a follow up PR later.
Fixes: #83114
---
.../Target/AArch64/AArch64MCPlusBuilder.cpp | 29 ++++++++++++++--
bolt/test/AArch64/test-indirect-branch.c | 33 +++++++++++++++++++
2 files changed, 60 insertions(+), 2 deletions(-)
create mode 100644 bolt/test/AArch64/test-indirect-branch.c
diff --git a/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp b/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp
index a74eda8e4a566..0758ad491a518 100644
--- a/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp
+++ b/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp
@@ -706,8 +706,20 @@ class AArch64MCPlusBuilder : public MCPlusBuilder {
unsigned ShiftVal = AArch64_AM::getArithShiftValue(OperandExtension);
AArch64_AM::ShiftExtendType ExtendType =
AArch64_AM::getArithExtendType(OperandExtension);
- if (ShiftVal != 2)
- llvm_unreachable("Failed to match indirect branch! (fragment 2)");
+ if (ShiftVal != 2) {
+ // TODO: handle that case where ShiftVal != 2.
+ // The following code sequence below has no shift amount,
+ // the range could be 0 to 4.
+ // The pattern comes from libc, it occurs when the binary is static.
+ // adr x6, 0x219fb0 <sigall_set+0x88>
+ // add x6, x6, x14, lsl #2
+ // ldr w7, [x6]
+ // add x6, x6, w7, sxtw => no shift amount
+ // br x6
+ errs() << "BOLT-WARNING: "
+ "Failed to match indirect branch: ShiftVAL != 2 \n";
+ return false;
+ }
if (ExtendType == AArch64_AM::SXTB)
ScaleValue = 1LL;
@@ -752,6 +764,19 @@ class AArch64MCPlusBuilder : public MCPlusBuilder {
return true;
}
+ if (DefJTBaseAdd->getOpcode() == AArch64::ADR) {
+ // TODO: handle that case when we do not have adrp/add pair.
+ // It also occurs when the binary is static.
+ // adr x13, 0x215a18 <_nl_value_type_LC_COLLATE+0x50>
+ // ldrh w13, [x13, w12, uxtw #1]
+ // adr x12, 0x247b30 <__gettextparse+0x5b0>
+ // add x13, x12, w13, sxth #2
+ // br x13
+ errs() << "BOLT-WARNING: Failed to match indirect branch: "
+ "nop/adr instead of adrp/add \n";
+ return false;
+ }
+
assert(DefJTBaseAdd->getOpcode() == AArch64::ADDXri &&
"Failed to match jump table base address pattern! (1)");
diff --git a/bolt/test/AArch64/test-indirect-branch.c b/bolt/test/AArch64/test-indirect-branch.c
new file mode 100644
index 0000000000000..97bd8265feb34
--- /dev/null
+++ b/bolt/test/AArch64/test-indirect-branch.c
@@ -0,0 +1,33 @@
+// Test how BOLT handles indirect branch sequence of instructions in
+// AArch64MCPlus builder.
+// This test checks that case when we have no shift amount after add
+// instruction. This pattern comes from libc, so needs to build '-static'
+// binary to reproduce the issue easily.
+//
+// adr x6, 0x219fb0 <sigall_set+0x88>
+// add x6, x6, x14, lsl #2
+// ldr w7, [x6]
+// add x6, x6, w7, sxtw => no shift amount
+// br x6
+// It also tests another case when we use '-fuse-ld=lld' along with '-static'
+// which produces the following sequence of intsructions:
+//
+// nop => nop/adr instead of adrp/add
+// adr x13, 0x215a18 <_nl_value_type_LC_COLLATE+0x50>
+// ldrh w13, [x13, w12, uxtw #1]
+// adr x12, 0x247b30 <__gettextparse+0x5b0>
+// add x13, x12, w13, sxth #2
+// br x13
+
+// clang-format off
+
+// REQUIRES: system-linux
+// RUN: %clang %s -o %t.exe -Wl,-q -static -fuse-ld=lld \
+// RUN: --target=aarch64-unknown-linux-gnu
+// RUN: llvm-bolt %t.exe -o %t.bolt --print-cfg \
+// RUN: -v=1 2>&1 | FileCheck --match-full-lines %s
+
+// CHECK: BOLT-WARNING: Failed to match indirect branch: nop/adr instead of adrp/add
+// CHECK: BOLT-WARNING: Failed to match indirect branch: ShiftVAL != 2
+
+int main() { return 42; }
>From c24be4640227cbaf59a8a8ed4ef929bb8750e20b Mon Sep 17 00:00:00 2001
From: Adam Kallai <kadam at inf.u-szeged.hu>
Date: Wed, 12 Jun 2024 15:17:00 +0200
Subject: [PATCH 2/7] Enable this test only on AArch64 target
---
bolt/test/AArch64/test-indirect-branch.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/bolt/test/AArch64/test-indirect-branch.c b/bolt/test/AArch64/test-indirect-branch.c
index 97bd8265feb34..8c8a15af72dbb 100644
--- a/bolt/test/AArch64/test-indirect-branch.c
+++ b/bolt/test/AArch64/test-indirect-branch.c
@@ -21,7 +21,7 @@
// clang-format off
-// REQUIRES: system-linux
+// REQUIRES: system-linux,target=aarch64{{.*}}
// RUN: %clang %s -o %t.exe -Wl,-q -static -fuse-ld=lld \
// RUN: --target=aarch64-unknown-linux-gnu
// RUN: llvm-bolt %t.exe -o %t.bolt --print-cfg \
>From 920aad45e4813f4c528b7ac4d64b08c562826f29 Mon Sep 17 00:00:00 2001
From: Adam Kallai <kadam at inf.u-szeged.hu>
Date: Tue, 18 Jun 2024 17:28:20 +0200
Subject: [PATCH 3/7] Added assembly test
---
bolt/test/AArch64/test-indirect-branch.c | 33 -------
bolt/test/AArch64/test-indirect-branch.s | 107 +++++++++++++++++++++++
2 files changed, 107 insertions(+), 33 deletions(-)
delete mode 100644 bolt/test/AArch64/test-indirect-branch.c
create mode 100644 bolt/test/AArch64/test-indirect-branch.s
diff --git a/bolt/test/AArch64/test-indirect-branch.c b/bolt/test/AArch64/test-indirect-branch.c
deleted file mode 100644
index 8c8a15af72dbb..0000000000000
--- a/bolt/test/AArch64/test-indirect-branch.c
+++ /dev/null
@@ -1,33 +0,0 @@
-// Test how BOLT handles indirect branch sequence of instructions in
-// AArch64MCPlus builder.
-// This test checks that case when we have no shift amount after add
-// instruction. This pattern comes from libc, so needs to build '-static'
-// binary to reproduce the issue easily.
-//
-// adr x6, 0x219fb0 <sigall_set+0x88>
-// add x6, x6, x14, lsl #2
-// ldr w7, [x6]
-// add x6, x6, w7, sxtw => no shift amount
-// br x6
-// It also tests another case when we use '-fuse-ld=lld' along with '-static'
-// which produces the following sequence of intsructions:
-//
-// nop => nop/adr instead of adrp/add
-// adr x13, 0x215a18 <_nl_value_type_LC_COLLATE+0x50>
-// ldrh w13, [x13, w12, uxtw #1]
-// adr x12, 0x247b30 <__gettextparse+0x5b0>
-// add x13, x12, w13, sxth #2
-// br x13
-
-// clang-format off
-
-// REQUIRES: system-linux,target=aarch64{{.*}}
-// RUN: %clang %s -o %t.exe -Wl,-q -static -fuse-ld=lld \
-// RUN: --target=aarch64-unknown-linux-gnu
-// RUN: llvm-bolt %t.exe -o %t.bolt --print-cfg \
-// RUN: -v=1 2>&1 | FileCheck --match-full-lines %s
-
-// CHECK: BOLT-WARNING: Failed to match indirect branch: nop/adr instead of adrp/add
-// CHECK: BOLT-WARNING: Failed to match indirect branch: ShiftVAL != 2
-
-int main() { return 42; }
diff --git a/bolt/test/AArch64/test-indirect-branch.s b/bolt/test/AArch64/test-indirect-branch.s
new file mode 100644
index 0000000000000..2b38037ba4c2b
--- /dev/null
+++ b/bolt/test/AArch64/test-indirect-branch.s
@@ -0,0 +1,107 @@
+// Test how BOLT handles indirect branch sequence of instructions in
+// AArch64MCPlus builder.
+// This test checks that case when we have no shift amount after add
+// instruction. This pattern comes from libc, so needs to build '-static'
+// binary to reproduce the issue easily.
+//
+// adr x6, 0x219fb0 <sigall_set+0x88>
+// add x6, x6, x14, lsl #2
+// ldr w7, [x6]
+// add x6, x6, w7, sxtw => no shift amount
+// br x6
+// It also tests another case when we use '-fuse-ld=lld' along with '-static'
+// which produces the following sequence of intsructions:
+//
+// nop => nop/adr instead of adrp/add
+// adr x13, 0x215a18 <_nl_value_type_LC_COLLATE+0x50>
+// ldrh w13, [x13, w12, uxtw #1]
+// adr x12, 0x247b30 <__gettextparse+0x5b0>
+// add x13, x12, w13, sxth #2
+// br x13
+
+// clang-format off
+
+// REQUIRES: system-linux
+// RUN: llvm-mc -filetype=obj -triple aarch64-unknown-unknown %s -o %t.o
+// RUN: %clang %cflags --target=aarch64-unknown-linux %t.o -o %t.exe -Wl,-q
+// RUN: llvm-bolt %t.exe -o %t.bolt --print-cfg \
+// RUN: -v=1 2>&1 | FileCheck %s
+
+// CHECK: BOLT-WARNING: Failed to match indirect branch: nop/adr instead of adrp/add
+// CHECK: BOLT-WARNING: Failed to match indirect branch: ShiftVAL != 2
+
+
+ .section .text
+ .align 4
+ .globl _start
+ .type _start, %function
+_start:
+ bl bar
+ bl end
+ mov x0, #4
+ mov w8, #93
+ svc #0
+
+bar:
+ mov w1, #3
+ cmp x1, #0
+ b.eq end
+ nop
+ adr x3, jump_table
+ ldrh w3, [x3, x1, lsl #1]
+ adr x1, .case0
+ add x3, x1, w3, sxth #2
+ br x3
+.case0:
+ mov w0, #1
+ ret
+.case1:
+ mov w0, #2
+ ret
+.case3:
+ mov w0, #3
+ ret
+.case4:
+ nop
+ mov x1, #0
+ adr x3, datatable
+ add x3, x3, x1, lsl #2
+ ldr w2, [x3]
+ add x3, x3, w2, sxtw
+ br x3
+ nop
+ mov w0, #4
+ ret
+.case7:
+ mov w0, #4
+ ret
+
+foo1:
+ ret
+
+foo2:
+ add w0, w0, #3
+ ret
+
+foo3:
+ add w0, w0, #3
+ ret
+
+end:
+ add x0, x0, #99
+ ret
+
+ .section .rodata,"a", at progbits
+jump_table:
+ .hword (.case0-.case0)>>2
+ .hword (.case1-.case0)>>2
+ .hword (.case3-.case0)>>2
+ .hword (.case4-.case0)>>2
+ .hword (.case7-.case0)>>2
+
+
+datatable:
+ .word foo1-datatable
+ .word foo2-datatable
+ .word foo3-datatable
+ .word 20
>From 2107f0c2bf61ced0e39ac8f09c28ea995ef9d026 Mon Sep 17 00:00:00 2001
From: Adam Kallai <kadam at inf.u-szeged.hu>
Date: Tue, 18 Jun 2024 17:40:02 +0200
Subject: [PATCH 4/7] Fix the comment
---
bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp b/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp
index 0758ad491a518..c5af2d4d56888 100644
--- a/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp
+++ b/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp
@@ -707,7 +707,7 @@ class AArch64MCPlusBuilder : public MCPlusBuilder {
AArch64_AM::ShiftExtendType ExtendType =
AArch64_AM::getArithExtendType(OperandExtension);
if (ShiftVal != 2) {
- // TODO: handle that case where ShiftVal != 2.
+ // TODO: Handle the patten where ShiftVal != 2.
// The following code sequence below has no shift amount,
// the range could be 0 to 4.
// The pattern comes from libc, it occurs when the binary is static.
@@ -765,7 +765,7 @@ class AArch64MCPlusBuilder : public MCPlusBuilder {
}
if (DefJTBaseAdd->getOpcode() == AArch64::ADR) {
- // TODO: handle that case when we do not have adrp/add pair.
+ // TODO: Handle the pattern where there is no adrp/add pair.
// It also occurs when the binary is static.
// adr x13, 0x215a18 <_nl_value_type_LC_COLLATE+0x50>
// ldrh w13, [x13, w12, uxtw #1]
>From 67133dd752562ff409eb4e522f944bbb78f2acd6 Mon Sep 17 00:00:00 2001
From: Adam Kallai <kadam at inf.u-szeged.hu>
Date: Wed, 19 Jun 2024 16:50:52 +0200
Subject: [PATCH 5/7] Added a detailed description into the test
---
bolt/test/AArch64/test-indirect-branch.s | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/bolt/test/AArch64/test-indirect-branch.s b/bolt/test/AArch64/test-indirect-branch.s
index 2b38037ba4c2b..cb9e325654774 100644
--- a/bolt/test/AArch64/test-indirect-branch.s
+++ b/bolt/test/AArch64/test-indirect-branch.s
@@ -1,16 +1,19 @@
// Test how BOLT handles indirect branch sequence of instructions in
// AArch64MCPlus builder.
-// This test checks that case when we have no shift amount after add
-// instruction. This pattern comes from libc, so needs to build '-static'
-// binary to reproduce the issue easily.
+// This test checks the pattern where there is no shift amount after add
+// instruction. The pattern come from libc, it can be reproduced with
+// a 'static' built binary.
//
// adr x6, 0x219fb0 <sigall_set+0x88>
// add x6, x6, x14, lsl #2
// ldr w7, [x6]
// add x6, x6, w7, sxtw => no shift amount
// br x6
-// It also tests another case when we use '-fuse-ld=lld' along with '-static'
-// which produces the following sequence of intsructions:
+//
+// It also tests another case where there is no adrp/add pair.
+// The pattern also come from libc, and it only represents in the binary
+// if the lld linker is used to create the static binary.
+// It doesn't occur with GCC ld linker.
//
// nop => nop/adr instead of adrp/add
// adr x13, 0x215a18 <_nl_value_type_LC_COLLATE+0x50>
>From a0796e6f1ee3d7c0ba30daec6bb11421814e077c Mon Sep 17 00:00:00 2001
From: Paschalis Mpeis <Paschalis.Mpeis at arm.com>
Date: Tue, 25 Jun 2024 12:52:40 +0100
Subject: [PATCH 6/7] [BOLT][AArch64] Instrumentation for static binaries
doesn't work in reloc mode.
On AArch64, when trying to instrument a static binary that has relocation
data BOLT would crash as it misses `createDummyReturnFunction` function.
---
bolt/test/AArch64/dummy-return.test | 6 ++++++
bolt/test/Inputs/main.c | 3 +++
2 files changed, 9 insertions(+)
create mode 100644 bolt/test/AArch64/dummy-return.test
create mode 100644 bolt/test/Inputs/main.c
diff --git a/bolt/test/AArch64/dummy-return.test b/bolt/test/AArch64/dummy-return.test
new file mode 100644
index 0000000000000..ff8422f33bf84
--- /dev/null
+++ b/bolt/test/AArch64/dummy-return.test
@@ -0,0 +1,6 @@
+REQUIRES: system-linux
+
+RUN: %clang %p/../Inputs/main.c -o %t -Wl,-q -static
+RUN: not llvm-bolt -instrument -instrumentation-sleep-time=1 %t -o %t.instr 2>&1 | FileCheck %s
+
+CHECK: not implemented
\ No newline at end of file
diff --git a/bolt/test/Inputs/main.c b/bolt/test/Inputs/main.c
new file mode 100644
index 0000000000000..c05bedd98dba0
--- /dev/null
+++ b/bolt/test/Inputs/main.c
@@ -0,0 +1,3 @@
+// dummy function just for emitting relocations to the linker.
+int foo() { return 0; }
+int main(int argc, char **argv) { return foo() + 1; }
>From 7719e07d74a98f580836d3e32f6133e994be4f98 Mon Sep 17 00:00:00 2001
From: Paschalis Mpeis <Paschalis.Mpeis at arm.com>
Date: Tue, 25 Jun 2024 12:57:37 +0100
Subject: [PATCH 7/7] [BOLT][AArch64] Implemented createDummyReturnFunction.
On AArch64, this method is needed when trying to instrument a static
binary.
Sample commands:
```bash
clang -Wl,-q test.c -static -o out
llvm-bolt -instrument -instrumentation-sleep-time=5 out -o out.instr
```
---
bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp | 6 ++++++
bolt/test/AArch64/dummy-return.test | 8 ++++++--
2 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp b/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp
index c5af2d4d56888..4388b796252dd 100644
--- a/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp
+++ b/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp
@@ -1607,6 +1607,12 @@ class AArch64MCPlusBuilder : public MCPlusBuilder {
return Instrs;
}
+ InstructionListType createDummyReturnFunction(MCContext *Ctx) const override {
+ InstructionListType Insts(1);
+ createReturn(Insts[0]);
+ return Insts;
+ }
+
std::vector<MCInst> createSymbolTrampoline(const MCSymbol *TgtSym,
MCContext *Ctx) override {
std::vector<MCInst> Insts;
diff --git a/bolt/test/AArch64/dummy-return.test b/bolt/test/AArch64/dummy-return.test
index ff8422f33bf84..76f12cf9d4f09 100644
--- a/bolt/test/AArch64/dummy-return.test
+++ b/bolt/test/AArch64/dummy-return.test
@@ -1,6 +1,10 @@
+// Tests that AArch64 is able to instrument static binaries in relocation mode.
+
REQUIRES: system-linux
RUN: %clang %p/../Inputs/main.c -o %t -Wl,-q -static
-RUN: not llvm-bolt -instrument -instrumentation-sleep-time=1 %t -o %t.instr 2>&1 | FileCheck %s
+RUN: llvm-bolt -instrument -instrumentation-sleep-time=1 %t -o %t.instr 2>&1 | FileCheck %s
+RUN: llvm-nm -n %t.instr | FileCheck %s -check-prefix=CHECK-SYM
-CHECK: not implemented
\ No newline at end of file
+CHECK: BOLT-INFO: output linked against instrumentation runtime library
+CHECK-SYM: __bolt_fini_trampoline
\ No newline at end of file
More information about the llvm-commits
mailing list