[llvm] [BOLT][AArch64] Provide createDummyReturnFunction (PR #96626)
Paschalis Mpeis via llvm-commits
llvm-commits at lists.llvm.org
Fri Jul 12 01:54:04 PDT 2024
https://github.com/paschalis-mpeis updated https://github.com/llvm/llvm-project/pull/96626
>From a2e6301fabe271c67070c484ce63eaa1c48dfdc1 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 1/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.s | 24 ++++++++++++++++++++++++
bolt/test/AArch64/dummy-return.test | 6 ++++++
bolt/test/Inputs/main.c | 3 +++
3 files changed, 33 insertions(+)
create mode 100644 bolt/test/AArch64/dummy-return.s
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.s b/bolt/test/AArch64/dummy-return.s
new file mode 100644
index 0000000000000..ba6892da50849
--- /dev/null
+++ b/bolt/test/AArch64/dummy-return.s
@@ -0,0 +1,24 @@
+# REQUIRES: system-linux
+
+# RUN: llvm-mc -filetype=obj -triple aarch64-unknown-unknown %s -o %t.o
+# RUN: %clang %cflags %t.o -o %t.exe -Wl,-q -static
+# RUN: not --crash llvm-bolt -instrument -instrumentation-sleep-time=1 %t.exe \
+# RUN: -o %t.instr 2>&1 | FileCheck %s
+
+# CHECK: not implemented
+
+ .text
+ .align 4
+ .global _start
+ .type _start, %function
+_start:
+ bl foo
+ ret
+ .size _start, .-_start
+
+ .global foo
+ .type foo, %function
+foo:
+ mov w0, wzr
+ ret
+ .size foo, .-foo
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 8e7dad8df29d61b5993b6b60c428c9f3d4049493 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 2/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 1a2327df1d323..e52b690e44ded 100644
--- a/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp
+++ b/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp
@@ -1644,6 +1644,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
>From 0c3bc045b78757f3a65408808821a5a083953f67 Mon Sep 17 00:00:00 2001
From: Paschalis Mpeis <Paschalis.Mpeis at arm.com>
Date: Fri, 5 Jul 2024 09:23:56 +0100
Subject: [PATCH 3/7] Refactored, improved test, and addressed reviewers.
---
bolt/include/bolt/Core/MCPlusBuilder.h | 5 ++++-
bolt/lib/Passes/Instrumentation.cpp | 2 +-
bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp | 2 +-
bolt/lib/Target/X86/X86MCPlusBuilder.cpp | 2 +-
bolt/test/AArch64/dummy-return.test | 9 ++++++---
bolt/test/Inputs/main.c | 2 +-
6 files changed, 14 insertions(+), 8 deletions(-)
diff --git a/bolt/include/bolt/Core/MCPlusBuilder.h b/bolt/include/bolt/Core/MCPlusBuilder.h
index ab07f07e49845..e0a5bd986f585 100644
--- a/bolt/include/bolt/Core/MCPlusBuilder.h
+++ b/bolt/include/bolt/Core/MCPlusBuilder.h
@@ -2041,7 +2041,10 @@ class MCPlusBuilder {
return InstructionListType();
}
- virtual InstructionListType createDummyReturnFunction(MCContext *Ctx) const {
+ /// Returns a function body that contains only a return instruction. An
+ /// example usage is a workaround for the '__bolt_fini_trampoline' of
+ // Instrumentation.
+ virtual InstructionListType createDummyReturn(MCContext *Ctx) const {
llvm_unreachable("not implemented");
return InstructionListType();
}
diff --git a/bolt/lib/Passes/Instrumentation.cpp b/bolt/lib/Passes/Instrumentation.cpp
index e824a42d82696..0af8a873790b1 100644
--- a/bolt/lib/Passes/Instrumentation.cpp
+++ b/bolt/lib/Passes/Instrumentation.cpp
@@ -754,7 +754,7 @@ void Instrumentation::createAuxiliaryFunctions(BinaryContext &BC) {
// with unknown symbol in runtime library. E.g. for static PIE
// executable
createSimpleFunction("__bolt_fini_trampoline",
- BC.MIB->createDummyReturnFunction(BC.Ctx.get()));
+ BC.MIB->createDummyReturn(BC.Ctx.get()));
}
}
}
diff --git a/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp b/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp
index e52b690e44ded..ed7d641f40e3d 100644
--- a/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp
+++ b/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp
@@ -1644,7 +1644,7 @@ class AArch64MCPlusBuilder : public MCPlusBuilder {
return Instrs;
}
- InstructionListType createDummyReturnFunction(MCContext *Ctx) const override {
+ InstructionListType createDummyReturn(MCContext *Ctx) const override {
InstructionListType Insts(1);
createReturn(Insts[0]);
return Insts;
diff --git a/bolt/lib/Target/X86/X86MCPlusBuilder.cpp b/bolt/lib/Target/X86/X86MCPlusBuilder.cpp
index 37136f4a5c551..8945ad17b657d 100644
--- a/bolt/lib/Target/X86/X86MCPlusBuilder.cpp
+++ b/bolt/lib/Target/X86/X86MCPlusBuilder.cpp
@@ -3241,7 +3241,7 @@ class X86MCPlusBuilder : public MCPlusBuilder {
return Insts;
}
- InstructionListType createDummyReturnFunction(MCContext *Ctx) const override {
+ InstructionListType createDummyReturn(MCContext *Ctx) const override {
InstructionListType Insts(1);
createReturn(Insts[0]);
return Insts;
diff --git a/bolt/test/AArch64/dummy-return.test b/bolt/test/AArch64/dummy-return.test
index 76f12cf9d4f09..d7bc1f82d641e 100644
--- a/bolt/test/AArch64/dummy-return.test
+++ b/bolt/test/AArch64/dummy-return.test
@@ -1,10 +1,13 @@
-// Tests that AArch64 is able to instrument static binaries in relocation mode.
+// Tests that AArch64 is able to instrument static binaries in relocation mode,
+// by checking that '__bolt_fini_trampoline' is generated and contains a 'ret'
+// instruction.
REQUIRES: system-linux
RUN: %clang %p/../Inputs/main.c -o %t -Wl,-q -static
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
+RUN: llvm-objdump --disassemble-symbols=__bolt_fini_trampoline %t.instr -D | FileCheck %s -check-prefix=CHECK-ASM
CHECK: BOLT-INFO: output linked against instrumentation runtime library
-CHECK-SYM: __bolt_fini_trampoline
\ No newline at end of file
+CHECK-ASM: <__bolt_fini_trampoline>:
+CHECK-ASM-NEXT: ret
diff --git a/bolt/test/Inputs/main.c b/bolt/test/Inputs/main.c
index c05bedd98dba0..7bfe92adb92d3 100644
--- a/bolt/test/Inputs/main.c
+++ b/bolt/test/Inputs/main.c
@@ -1,3 +1,3 @@
-// dummy function just for emitting relocations to the linker.
+// dummy function just for emitting relocations to the linker.
int foo() { return 0; }
int main(int argc, char **argv) { return foo() + 1; }
>From 542e46efcdd3674910686e7fa567343b1269705b Mon Sep 17 00:00:00 2001
From: Paschalis Mpeis <Paschalis.Mpeis at arm.com>
Date: Wed, 10 Jul 2024 07:44:37 +0100
Subject: [PATCH 4/7] Addressing reviewers (1): Uplifted 'createDummyReturn' to
base class
---
bolt/include/bolt/Core/MCPlusBuilder.h | 5 +----
bolt/lib/Core/MCPlusBuilder.cpp | 6 ++++++
bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp | 6 ------
bolt/lib/Target/X86/X86MCPlusBuilder.cpp | 6 ------
4 files changed, 7 insertions(+), 16 deletions(-)
diff --git a/bolt/include/bolt/Core/MCPlusBuilder.h b/bolt/include/bolt/Core/MCPlusBuilder.h
index e0a5bd986f585..b1d8d636a9f43 100644
--- a/bolt/include/bolt/Core/MCPlusBuilder.h
+++ b/bolt/include/bolt/Core/MCPlusBuilder.h
@@ -2044,10 +2044,7 @@ class MCPlusBuilder {
/// Returns a function body that contains only a return instruction. An
/// example usage is a workaround for the '__bolt_fini_trampoline' of
// Instrumentation.
- virtual InstructionListType createDummyReturn(MCContext *Ctx) const {
- llvm_unreachable("not implemented");
- return InstructionListType();
- }
+ virtual InstructionListType createDummyReturn(MCContext *Ctx) const;
/// This method takes an indirect call instruction and splits it up into an
/// equivalent set of instructions that use direct calls for target
diff --git a/bolt/lib/Core/MCPlusBuilder.cpp b/bolt/lib/Core/MCPlusBuilder.cpp
index 7ff7a2288451c..d9281bf2e991f 100644
--- a/bolt/lib/Core/MCPlusBuilder.cpp
+++ b/bolt/lib/Core/MCPlusBuilder.cpp
@@ -147,6 +147,12 @@ bool MCPlusBuilder::isTailCall(const MCInst &Inst) const {
return false;
}
+InstructionListType MCPlusBuilder::createDummyReturn(MCContext *Ctx) const {
+ InstructionListType Insts(1);
+ createReturn(Insts[0]);
+ return Insts;
+}
+
std::optional<MCLandingPad> MCPlusBuilder::getEHInfo(const MCInst &Inst) const {
if (!isCall(Inst))
return std::nullopt;
diff --git a/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp b/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp
index ed7d641f40e3d..1a2327df1d323 100644
--- a/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp
+++ b/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp
@@ -1644,12 +1644,6 @@ class AArch64MCPlusBuilder : public MCPlusBuilder {
return Instrs;
}
- InstructionListType createDummyReturn(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/lib/Target/X86/X86MCPlusBuilder.cpp b/bolt/lib/Target/X86/X86MCPlusBuilder.cpp
index 8945ad17b657d..e46c42533031c 100644
--- a/bolt/lib/Target/X86/X86MCPlusBuilder.cpp
+++ b/bolt/lib/Target/X86/X86MCPlusBuilder.cpp
@@ -3241,12 +3241,6 @@ class X86MCPlusBuilder : public MCPlusBuilder {
return Insts;
}
- InstructionListType createDummyReturn(MCContext *Ctx) const override {
- InstructionListType Insts(1);
- createReturn(Insts[0]);
- return Insts;
- }
-
BlocksVectorTy indirectCallPromotion(
const MCInst &CallInst,
const std::vector<std::pair<MCSymbol *, uint64_t>> &Targets,
>From 1a0542a012e8802ed0357732a5a1ec41f6cb8f54 Mon Sep 17 00:00:00 2001
From: Paschalis Mpeis <Paschalis.Mpeis at arm.com>
Date: Thu, 11 Jul 2024 09:14:35 +0100
Subject: [PATCH 5/7] Addressing reviewers (2)
---
bolt/include/bolt/Core/MCPlusBuilder.h | 6 +++++-
bolt/lib/Core/MCPlusBuilder.cpp | 6 ------
bolt/lib/Passes/Instrumentation.cpp | 2 +-
3 files changed, 6 insertions(+), 8 deletions(-)
diff --git a/bolt/include/bolt/Core/MCPlusBuilder.h b/bolt/include/bolt/Core/MCPlusBuilder.h
index b1d8d636a9f43..885d627f7b64f 100644
--- a/bolt/include/bolt/Core/MCPlusBuilder.h
+++ b/bolt/include/bolt/Core/MCPlusBuilder.h
@@ -2044,7 +2044,11 @@ class MCPlusBuilder {
/// Returns a function body that contains only a return instruction. An
/// example usage is a workaround for the '__bolt_fini_trampoline' of
// Instrumentation.
- virtual InstructionListType createDummyReturn(MCContext *Ctx) const;
+ virtual InstructionListType createDummyReturnFunction(MCContext *Ctx) const {
+ InstructionListType Insts(1);
+ createReturn(Insts[0]);
+ return Insts;
+ }
/// This method takes an indirect call instruction and splits it up into an
/// equivalent set of instructions that use direct calls for target
diff --git a/bolt/lib/Core/MCPlusBuilder.cpp b/bolt/lib/Core/MCPlusBuilder.cpp
index d9281bf2e991f..7ff7a2288451c 100644
--- a/bolt/lib/Core/MCPlusBuilder.cpp
+++ b/bolt/lib/Core/MCPlusBuilder.cpp
@@ -147,12 +147,6 @@ bool MCPlusBuilder::isTailCall(const MCInst &Inst) const {
return false;
}
-InstructionListType MCPlusBuilder::createDummyReturn(MCContext *Ctx) const {
- InstructionListType Insts(1);
- createReturn(Insts[0]);
- return Insts;
-}
-
std::optional<MCLandingPad> MCPlusBuilder::getEHInfo(const MCInst &Inst) const {
if (!isCall(Inst))
return std::nullopt;
diff --git a/bolt/lib/Passes/Instrumentation.cpp b/bolt/lib/Passes/Instrumentation.cpp
index 0af8a873790b1..e824a42d82696 100644
--- a/bolt/lib/Passes/Instrumentation.cpp
+++ b/bolt/lib/Passes/Instrumentation.cpp
@@ -754,7 +754,7 @@ void Instrumentation::createAuxiliaryFunctions(BinaryContext &BC) {
// with unknown symbol in runtime library. E.g. for static PIE
// executable
createSimpleFunction("__bolt_fini_trampoline",
- BC.MIB->createDummyReturn(BC.Ctx.get()));
+ BC.MIB->createDummyReturnFunction(BC.Ctx.get()));
}
}
}
>From 8351110d590553bbbb6330d729e98c3d4cf3a384 Mon Sep 17 00:00:00 2001
From: Paschalis Mpeis <paschalis.mpeis at arm.com>
Date: Fri, 12 Jul 2024 08:07:55 +0100
Subject: [PATCH 6/7] Update bolt/test/AArch64/dummy-return.test
Co-authored-by: Amir Ayupov <aaupov at fb.com>
---
bolt/test/AArch64/dummy-return.test | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/bolt/test/AArch64/dummy-return.test b/bolt/test/AArch64/dummy-return.test
index d7bc1f82d641e..8af6e83159326 100644
--- a/bolt/test/AArch64/dummy-return.test
+++ b/bolt/test/AArch64/dummy-return.test
@@ -4,7 +4,7 @@
REQUIRES: system-linux
-RUN: %clang %p/../Inputs/main.c -o %t -Wl,-q -static
+RUN: %clang %cflags %p/../Inputs/main.c -o %t -Wl,-q -static
RUN: llvm-bolt -instrument -instrumentation-sleep-time=1 %t -o %t.instr 2>&1 | FileCheck %s
RUN: llvm-objdump --disassemble-symbols=__bolt_fini_trampoline %t.instr -D | FileCheck %s -check-prefix=CHECK-ASM
>From fa4ded334094933b222e6115e1e443c570094a19 Mon Sep 17 00:00:00 2001
From: Paschalis Mpeis <Paschalis.Mpeis at arm.com>
Date: Fri, 12 Jul 2024 09:31:43 +0100
Subject: [PATCH 7/7] Converted test to assembly for '%cflags' compatibility.
The '-static' option does not work well with '-nostartfiles -nostdlib'
that comes with '%cflags', but the latter is needed for better buildbot
compatibility.
I converted the test to assembly and dropped the previous c test.
Dropped dummy-return.test and main.c Input file.
---
bolt/test/AArch64/dummy-return.s | 8 ++++++--
bolt/test/AArch64/dummy-return.test | 13 -------------
bolt/test/Inputs/main.c | 3 ---
3 files changed, 6 insertions(+), 18 deletions(-)
delete mode 100644 bolt/test/AArch64/dummy-return.test
delete mode 100644 bolt/test/Inputs/main.c
diff --git a/bolt/test/AArch64/dummy-return.s b/bolt/test/AArch64/dummy-return.s
index ba6892da50849..8bea90cbe6fc8 100644
--- a/bolt/test/AArch64/dummy-return.s
+++ b/bolt/test/AArch64/dummy-return.s
@@ -2,10 +2,14 @@
# RUN: llvm-mc -filetype=obj -triple aarch64-unknown-unknown %s -o %t.o
# RUN: %clang %cflags %t.o -o %t.exe -Wl,-q -static
-# RUN: not --crash llvm-bolt -instrument -instrumentation-sleep-time=1 %t.exe \
+# RUN: llvm-bolt -instrument -instrumentation-sleep-time=1 %t.exe \
# RUN: -o %t.instr 2>&1 | FileCheck %s
+# RUN: llvm-objdump --disassemble-symbols=__bolt_fini_trampoline %t.instr -D \
+# RUN: | FileCheck %s -check-prefix=CHECK-ASM
-# CHECK: not implemented
+# CHECK: BOLT-INFO: output linked against instrumentation runtime library
+# CHECK-ASM: <__bolt_fini_trampoline>:
+# CHECK-ASM-NEXT: ret
.text
.align 4
diff --git a/bolt/test/AArch64/dummy-return.test b/bolt/test/AArch64/dummy-return.test
deleted file mode 100644
index 8af6e83159326..0000000000000
--- a/bolt/test/AArch64/dummy-return.test
+++ /dev/null
@@ -1,13 +0,0 @@
-// Tests that AArch64 is able to instrument static binaries in relocation mode,
-// by checking that '__bolt_fini_trampoline' is generated and contains a 'ret'
-// instruction.
-
-REQUIRES: system-linux
-
-RUN: %clang %cflags %p/../Inputs/main.c -o %t -Wl,-q -static
-RUN: llvm-bolt -instrument -instrumentation-sleep-time=1 %t -o %t.instr 2>&1 | FileCheck %s
-RUN: llvm-objdump --disassemble-symbols=__bolt_fini_trampoline %t.instr -D | FileCheck %s -check-prefix=CHECK-ASM
-
-CHECK: BOLT-INFO: output linked against instrumentation runtime library
-CHECK-ASM: <__bolt_fini_trampoline>:
-CHECK-ASM-NEXT: ret
diff --git a/bolt/test/Inputs/main.c b/bolt/test/Inputs/main.c
deleted file mode 100644
index 7bfe92adb92d3..0000000000000
--- a/bolt/test/Inputs/main.c
+++ /dev/null
@@ -1,3 +0,0 @@
-// dummy function just for emitting relocations to the linker.
-int foo() { return 0; }
-int main(int argc, char **argv) { return foo() + 1; }
More information about the llvm-commits
mailing list