[llvm] [BOLT] Fix --pad-funcs{, -before} state misinteraction (PR #121918)
Peter Waller via llvm-commits
llvm-commits at lists.llvm.org
Tue Jan 7 03:30:10 PST 2025
https://github.com/peterwaller-arm updated https://github.com/llvm/llvm-project/pull/121918
>From ebe879664912ffc8f02c434090f3b4068a255f9e Mon Sep 17 00:00:00 2001
From: Peter Waller <peter.waller at arm.com>
Date: Tue, 7 Jan 2025 10:10:50 +0000
Subject: [PATCH 1/2] Reapply "[BOLT] Add --pad-funcs-before=func:n (#117924)"
This reverts commit be21bd9bbf3bc906f9b98ac3de1fc88a4a8ac4b4.
---
bolt/lib/Core/BinaryEmitter.cpp | 53 ++++++++++++++++++++++------
bolt/lib/Passes/ReorderFunctions.cpp | 12 +++++--
bolt/test/AArch64/pad-before-funcs.s | 29 +++++++++++++++
3 files changed, 80 insertions(+), 14 deletions(-)
create mode 100644 bolt/test/AArch64/pad-before-funcs.s
diff --git a/bolt/lib/Core/BinaryEmitter.cpp b/bolt/lib/Core/BinaryEmitter.cpp
index 1744c1e5717224..5019cf31beee30 100644
--- a/bolt/lib/Core/BinaryEmitter.cpp
+++ b/bolt/lib/Core/BinaryEmitter.cpp
@@ -46,13 +46,17 @@ BreakFunctionNames("break-funcs",
cl::Hidden,
cl::cat(BoltCategory));
-static cl::list<std::string>
-FunctionPadSpec("pad-funcs",
- cl::CommaSeparated,
- cl::desc("list of functions to pad with amount of bytes"),
- cl::value_desc("func1:pad1,func2:pad2,func3:pad3,..."),
- cl::Hidden,
- cl::cat(BoltCategory));
+cl::list<std::string>
+ FunctionPadSpec("pad-funcs", cl::CommaSeparated,
+ cl::desc("list of functions to pad with amount of bytes"),
+ cl::value_desc("func1:pad1,func2:pad2,func3:pad3,..."),
+ cl::Hidden, cl::cat(BoltCategory));
+
+cl::list<std::string> FunctionPadBeforeSpec(
+ "pad-funcs-before", cl::CommaSeparated,
+ cl::desc("list of functions to pad with amount of bytes"),
+ cl::value_desc("func1:pad1,func2:pad2,func3:pad3,..."), cl::Hidden,
+ cl::cat(BoltCategory));
static cl::opt<bool> MarkFuncs(
"mark-funcs",
@@ -70,11 +74,12 @@ X86AlignBranchBoundaryHotOnly("x86-align-branch-boundary-hot-only",
cl::init(true),
cl::cat(BoltOptCategory));
-size_t padFunction(const BinaryFunction &Function) {
+size_t padFunction(const cl::list<std::string> &Spec,
+ const BinaryFunction &Function) {
static std::map<std::string, size_t> FunctionPadding;
- if (FunctionPadding.empty() && !FunctionPadSpec.empty()) {
- for (std::string &Spec : FunctionPadSpec) {
+ if (FunctionPadding.empty() && !Spec.empty()) {
+ for (const std::string &Spec : Spec) {
size_t N = Spec.find(':');
if (N == std::string::npos)
continue;
@@ -319,6 +324,32 @@ bool BinaryEmitter::emitFunction(BinaryFunction &Function,
Streamer.emitCodeAlignment(Function.getAlign(), &*BC.STI);
}
+ if (size_t Padding =
+ opts::padFunction(opts::FunctionPadBeforeSpec, Function)) {
+ // Handle padFuncsBefore after the above alignment logic but before
+ // symbol addresses are decided.
+ if (!BC.HasRelocations) {
+ BC.errs() << "BOLT-ERROR: -pad-before-funcs is not supported in "
+ << "non-relocation mode\n";
+ exit(1);
+ }
+
+ // Preserve Function.getMinAlign().
+ if (!isAligned(Function.getMinAlign(), Padding)) {
+ BC.errs() << "BOLT-ERROR: user-requested " << Padding
+ << " padding bytes before function " << Function
+ << " is not a multiple of the minimum function alignment ("
+ << Function.getMinAlign().value() << ").\n";
+ exit(1);
+ }
+
+ LLVM_DEBUG(dbgs() << "BOLT-DEBUG: padding before function " << Function
+ << " with " << Padding << " bytes\n");
+
+ // Since the padding is not executed, it can be null bytes.
+ Streamer.emitFill(Padding, 0);
+ }
+
MCContext &Context = Streamer.getContext();
const MCAsmInfo *MAI = Context.getAsmInfo();
@@ -373,7 +404,7 @@ bool BinaryEmitter::emitFunction(BinaryFunction &Function,
emitFunctionBody(Function, FF, /*EmitCodeOnly=*/false);
// Emit padding if requested.
- if (size_t Padding = opts::padFunction(Function)) {
+ if (size_t Padding = opts::padFunction(opts::FunctionPadSpec, Function)) {
LLVM_DEBUG(dbgs() << "BOLT-DEBUG: padding function " << Function << " with "
<< Padding << " bytes\n");
Streamer.emitFill(Padding, MAI->getTextAlignFillValue());
diff --git a/bolt/lib/Passes/ReorderFunctions.cpp b/bolt/lib/Passes/ReorderFunctions.cpp
index 1256d71342b13b..f8f6a01526dccf 100644
--- a/bolt/lib/Passes/ReorderFunctions.cpp
+++ b/bolt/lib/Passes/ReorderFunctions.cpp
@@ -28,7 +28,9 @@ extern cl::OptionCategory BoltOptCategory;
extern cl::opt<unsigned> Verbosity;
extern cl::opt<uint32_t> RandomSeed;
-extern size_t padFunction(const bolt::BinaryFunction &Function);
+extern size_t padFunction(const cl::list<std::string> &Spec,
+ const bolt::BinaryFunction &Function);
+extern cl::list<std::string> FunctionPadSpec, FunctionPadBeforeSpec;
extern cl::opt<bolt::ReorderFunctions::ReorderType> ReorderFunctions;
cl::opt<bolt::ReorderFunctions::ReorderType> ReorderFunctions(
@@ -304,8 +306,12 @@ Error ReorderFunctions::runOnFunctions(BinaryContext &BC) {
return false;
if (B->isIgnored())
return true;
- const size_t PadA = opts::padFunction(*A);
- const size_t PadB = opts::padFunction(*B);
+ const size_t PadA =
+ opts::padFunction(opts::FunctionPadSpec, *A) +
+ opts::padFunction(opts::FunctionPadBeforeSpec, *A);
+ const size_t PadB =
+ opts::padFunction(opts::FunctionPadSpec, *B) +
+ opts::padFunction(opts::FunctionPadBeforeSpec, *B);
if (!PadA || !PadB) {
if (PadA)
return true;
diff --git a/bolt/test/AArch64/pad-before-funcs.s b/bolt/test/AArch64/pad-before-funcs.s
new file mode 100644
index 00000000000000..3ce0ee5e383894
--- /dev/null
+++ b/bolt/test/AArch64/pad-before-funcs.s
@@ -0,0 +1,29 @@
+# Test checks that --pad-before-funcs is working as expected.
+# It should be able to introduce a configurable offset for the _start symbol.
+# It should reject requests which don't obey the code alignment requirement.
+
+# RUN: llvm-mc -filetype=obj -triple aarch64-unknown-unknown %s -o %t.o
+# RUN: %clang %cflags %t.o -o %t.exe -Wl,-q -Wl,--section-start=.text=0x4000
+# RUN: llvm-bolt %t.exe -o %t.bolt.0 --pad-funcs-before=_start:0
+# RUN: llvm-bolt %t.exe -o %t.bolt.4 --pad-funcs-before=_start:4
+# RUN: llvm-bolt %t.exe -o %t.bolt.8 --pad-funcs-before=_start:8
+
+# RUN: not llvm-bolt %t.exe -o %t.bolt.8 --pad-funcs-before=_start:1 2>&1 | FileCheck --check-prefix=CHECK-BAD-ALIGN %s
+
+# CHECK-BAD-ALIGN: user-requested 1 padding bytes before function _start(*2) is not a multiple of the minimum function alignment (4).
+
+# RUN: llvm-objdump --section=.text --disassemble %t.bolt.0 | FileCheck --check-prefix=CHECK-0 %s
+# RUN: llvm-objdump --section=.text --disassemble %t.bolt.4 | FileCheck --check-prefix=CHECK-4 %s
+# RUN: llvm-objdump --section=.text --disassemble %t.bolt.8 | FileCheck --check-prefix=CHECK-8 %s
+
+# Trigger relocation mode in bolt.
+.reloc 0, R_AARCH64_NONE
+
+.section .text
+.globl _start
+
+# CHECK-0: 0000000000400000 <_start>
+# CHECK-4: 0000000000400004 <_start>
+# CHECK-8: 0000000000400008 <_start>
+_start:
+ ret
>From 3b5d6262a68c044eda22215fc53bd4081c2f6373 Mon Sep 17 00:00:00 2001
From: Peter Waller <peter.waller at arm.com>
Date: Tue, 7 Jan 2025 10:48:22 +0000
Subject: [PATCH 2/2] [BOLT] Fix --pad-funcs{,-before} state misinteraction
When --pad-funcs-before was introduced, it introduced a bug whereby the
first one to get parsed could influence the other.
Ensure that each has its own state and test that they don't interact in
this manner.
---
bolt/lib/Core/BinaryEmitter.cpp | 23 +++++++++++++++--------
bolt/lib/Passes/ReorderFunctions.cpp | 15 ++++++---------
bolt/test/AArch64/pad-before-funcs.s | 21 ++++++++++++++++++++-
3 files changed, 41 insertions(+), 18 deletions(-)
diff --git a/bolt/lib/Core/BinaryEmitter.cpp b/bolt/lib/Core/BinaryEmitter.cpp
index 5019cf31beee30..1aad25242712f8 100644
--- a/bolt/lib/Core/BinaryEmitter.cpp
+++ b/bolt/lib/Core/BinaryEmitter.cpp
@@ -46,13 +46,13 @@ BreakFunctionNames("break-funcs",
cl::Hidden,
cl::cat(BoltCategory));
-cl::list<std::string>
+static cl::list<std::string>
FunctionPadSpec("pad-funcs", cl::CommaSeparated,
cl::desc("list of functions to pad with amount of bytes"),
cl::value_desc("func1:pad1,func2:pad2,func3:pad3,..."),
cl::Hidden, cl::cat(BoltCategory));
-cl::list<std::string> FunctionPadBeforeSpec(
+static cl::list<std::string> FunctionPadBeforeSpec(
"pad-funcs-before", cl::CommaSeparated,
cl::desc("list of functions to pad with amount of bytes"),
cl::value_desc("func1:pad1,func2:pad2,func3:pad3,..."), cl::Hidden,
@@ -74,10 +74,9 @@ X86AlignBranchBoundaryHotOnly("x86-align-branch-boundary-hot-only",
cl::init(true),
cl::cat(BoltOptCategory));
-size_t padFunction(const cl::list<std::string> &Spec,
+size_t padFunction(std::map<std::string, size_t> &FunctionPadding,
+ const cl::list<std::string> &Spec,
const BinaryFunction &Function) {
- static std::map<std::string, size_t> FunctionPadding;
-
if (FunctionPadding.empty() && !Spec.empty()) {
for (const std::string &Spec : Spec) {
size_t N = Spec.find(':');
@@ -99,6 +98,15 @@ size_t padFunction(const cl::list<std::string> &Spec,
return 0;
}
+size_t padFunctionBefore(const BinaryFunction &Function) {
+ static std::map<std::string, size_t> CacheFunctionPadding;
+ return padFunction(CacheFunctionPadding, FunctionPadBeforeSpec, Function);
+}
+size_t padFunctionAfter(const BinaryFunction &Function) {
+ static std::map<std::string, size_t> CacheFunctionPadding;
+ return padFunction(CacheFunctionPadding, FunctionPadSpec, Function);
+}
+
} // namespace opts
namespace {
@@ -324,8 +332,7 @@ bool BinaryEmitter::emitFunction(BinaryFunction &Function,
Streamer.emitCodeAlignment(Function.getAlign(), &*BC.STI);
}
- if (size_t Padding =
- opts::padFunction(opts::FunctionPadBeforeSpec, Function)) {
+ if (size_t Padding = opts::padFunctionBefore(Function)) {
// Handle padFuncsBefore after the above alignment logic but before
// symbol addresses are decided.
if (!BC.HasRelocations) {
@@ -404,7 +411,7 @@ bool BinaryEmitter::emitFunction(BinaryFunction &Function,
emitFunctionBody(Function, FF, /*EmitCodeOnly=*/false);
// Emit padding if requested.
- if (size_t Padding = opts::padFunction(opts::FunctionPadSpec, Function)) {
+ if (size_t Padding = opts::padFunctionAfter(Function)) {
LLVM_DEBUG(dbgs() << "BOLT-DEBUG: padding function " << Function << " with "
<< Padding << " bytes\n");
Streamer.emitFill(Padding, MAI->getTextAlignFillValue());
diff --git a/bolt/lib/Passes/ReorderFunctions.cpp b/bolt/lib/Passes/ReorderFunctions.cpp
index f8f6a01526dccf..35c5acfdecdb9d 100644
--- a/bolt/lib/Passes/ReorderFunctions.cpp
+++ b/bolt/lib/Passes/ReorderFunctions.cpp
@@ -28,9 +28,8 @@ extern cl::OptionCategory BoltOptCategory;
extern cl::opt<unsigned> Verbosity;
extern cl::opt<uint32_t> RandomSeed;
-extern size_t padFunction(const cl::list<std::string> &Spec,
- const bolt::BinaryFunction &Function);
-extern cl::list<std::string> FunctionPadSpec, FunctionPadBeforeSpec;
+extern size_t padFunctionBefore(const bolt::BinaryFunction &Function);
+extern size_t padFunctionAfter(const bolt::BinaryFunction &Function);
extern cl::opt<bolt::ReorderFunctions::ReorderType> ReorderFunctions;
cl::opt<bolt::ReorderFunctions::ReorderType> ReorderFunctions(
@@ -306,12 +305,10 @@ Error ReorderFunctions::runOnFunctions(BinaryContext &BC) {
return false;
if (B->isIgnored())
return true;
- const size_t PadA =
- opts::padFunction(opts::FunctionPadSpec, *A) +
- opts::padFunction(opts::FunctionPadBeforeSpec, *A);
- const size_t PadB =
- opts::padFunction(opts::FunctionPadSpec, *B) +
- opts::padFunction(opts::FunctionPadBeforeSpec, *B);
+ const size_t PadA = opts::padFunctionBefore(*A) +
+ opts::padFunctionAfter(*A);
+ const size_t PadB = opts::padFunctionBefore(*B) +
+ opts::padFunctionAfter(*B);
if (!PadA || !PadB) {
if (PadA)
return true;
diff --git a/bolt/test/AArch64/pad-before-funcs.s b/bolt/test/AArch64/pad-before-funcs.s
index 3ce0ee5e383894..f3e8a23ddfdda6 100644
--- a/bolt/test/AArch64/pad-before-funcs.s
+++ b/bolt/test/AArch64/pad-before-funcs.s
@@ -2,11 +2,18 @@
# It should be able to introduce a configurable offset for the _start symbol.
# It should reject requests which don't obey the code alignment requirement.
+# Tests check inserting padding before _start; and additionally a test where
+# padding is inserted after start. In each case, check that the following
+# symbol ends up in the expected place as well.
+
+
# RUN: llvm-mc -filetype=obj -triple aarch64-unknown-unknown %s -o %t.o
# RUN: %clang %cflags %t.o -o %t.exe -Wl,-q -Wl,--section-start=.text=0x4000
# RUN: llvm-bolt %t.exe -o %t.bolt.0 --pad-funcs-before=_start:0
# RUN: llvm-bolt %t.exe -o %t.bolt.4 --pad-funcs-before=_start:4
# RUN: llvm-bolt %t.exe -o %t.bolt.8 --pad-funcs-before=_start:8
+# RUN: llvm-bolt %t.exe -o %t.bolt.4.4 --pad-funcs-before=_start:4 --pad-funcs=_start:4
+# RUN: llvm-bolt %t.exe -o %t.bolt.4.8 --pad-funcs-before=_start:4 --pad-funcs=_start:8
# RUN: not llvm-bolt %t.exe -o %t.bolt.8 --pad-funcs-before=_start:1 2>&1 | FileCheck --check-prefix=CHECK-BAD-ALIGN %s
@@ -15,15 +22,27 @@
# RUN: llvm-objdump --section=.text --disassemble %t.bolt.0 | FileCheck --check-prefix=CHECK-0 %s
# RUN: llvm-objdump --section=.text --disassemble %t.bolt.4 | FileCheck --check-prefix=CHECK-4 %s
# RUN: llvm-objdump --section=.text --disassemble %t.bolt.8 | FileCheck --check-prefix=CHECK-8 %s
+# RUN: llvm-objdump --section=.text --disassemble %t.bolt.4.4 | FileCheck --check-prefix=CHECK-4-4 %s
+# RUN: llvm-objdump --section=.text --disassemble %t.bolt.4.8 | FileCheck --check-prefix=CHECK-4-8 %s
# Trigger relocation mode in bolt.
.reloc 0, R_AARCH64_NONE
.section .text
-.globl _start
# CHECK-0: 0000000000400000 <_start>
# CHECK-4: 0000000000400004 <_start>
+# CHECK-4-4: 0000000000400004 <_start>
# CHECK-8: 0000000000400008 <_start>
+.globl _start
_start:
ret
+
+# CHECK-0: 0000000000400004 <_subsequent>
+# CHECK-4: 0000000000400008 <_subsequent>
+# CHECK-4-4: 000000000040000c <_subsequent>
+# CHECK-4-8: 0000000000400010 <_subsequent>
+# CHECK-8: 000000000040000c <_subsequent>
+.globl _subsequent
+_subsequent:
+ ret
More information about the llvm-commits
mailing list