[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:28:52 PST 2025
https://github.com/peterwaller-arm created https://github.com/llvm/llvm-project/pull/121918
- **Reapply "[BOLT] Add --pad-funcs-before=func:n (#117924)"**
- **[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.
Fixed by having a function (and static state) for each of before/after.
>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 7c3321bf20b42c0ba22235380f7228b6568e508a 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 | 13 ++++++-------
bolt/test/AArch64/pad-before-funcs.s | 21 ++++++++++++++++++++-
3 files changed, 41 insertions(+), 16 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..42de1bcb34240d 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(
@@ -307,11 +306,11 @@ Error ReorderFunctions::runOnFunctions(BinaryContext &BC) {
if (B->isIgnored())
return true;
const size_t PadA =
- opts::padFunction(opts::FunctionPadSpec, *A) +
- opts::padFunction(opts::FunctionPadBeforeSpec, *A);
+ opts::padFunctionBefore(*A) +
+ opts::padFunctionAfter(*A);
const size_t PadB =
- opts::padFunction(opts::FunctionPadSpec, *B) +
- opts::padFunction(opts::FunctionPadBeforeSpec, *B);
+ 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