From aa9cc721e58f086ba6a3f9711fefdb61a184f786 Mon Sep 17 00:00:00 2001 From: Peter Waller Date: Tue, 7 Jan 2025 17:25:04 +0000 Subject: [PATCH] Reapply "[BOLT] Add --pad-funcs-before=func:n (#117924)" (#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 by testing how the `_subsequent` symbol moves when both arguments are supplied with different padding values. Fixed by having a function (and static state) for each of before/after. --- bolt/lib/Core/BinaryEmitter.cpp | 62 ++++++++++++++++++++++------ bolt/lib/Passes/ReorderFunctions.cpp | 9 ++-- bolt/test/AArch64/pad-before-funcs.s | 48 +++++++++++++++++++++ 3 files changed, 104 insertions(+), 15 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..1aad25242712f8 100644 --- a/bolt/lib/Core/BinaryEmitter.cpp +++ b/bolt/lib/Core/BinaryEmitter.cpp @@ -47,12 +47,16 @@ BreakFunctionNames("break-funcs", cl::cat(BoltCategory)); static cl::list -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)); + 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)); + +static cl::list 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 MarkFuncs( "mark-funcs", @@ -70,11 +74,11 @@ X86AlignBranchBoundaryHotOnly("x86-align-branch-boundary-hot-only", cl::init(true), cl::cat(BoltOptCategory)); -size_t padFunction(const BinaryFunction &Function) { - static std::map FunctionPadding; - - if (FunctionPadding.empty() && !FunctionPadSpec.empty()) { - for (std::string &Spec : FunctionPadSpec) { +size_t padFunction(std::map &FunctionPadding, + const cl::list &Spec, + const BinaryFunction &Function) { + if (FunctionPadding.empty() && !Spec.empty()) { + for (const std::string &Spec : Spec) { size_t N = Spec.find(':'); if (N == std::string::npos) continue; @@ -94,6 +98,15 @@ size_t padFunction(const BinaryFunction &Function) { return 0; } +size_t padFunctionBefore(const BinaryFunction &Function) { + static std::map CacheFunctionPadding; + return padFunction(CacheFunctionPadding, FunctionPadBeforeSpec, Function); +} +size_t padFunctionAfter(const BinaryFunction &Function) { + static std::map CacheFunctionPadding; + return padFunction(CacheFunctionPadding, FunctionPadSpec, Function); +} + } // namespace opts namespace { @@ -319,6 +332,31 @@ bool BinaryEmitter::emitFunction(BinaryFunction &Function, Streamer.emitCodeAlignment(Function.getAlign(), &*BC.STI); } + if (size_t Padding = opts::padFunctionBefore(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 +411,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::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 1256d71342b13b..35c5acfdecdb9d 100644 --- a/bolt/lib/Passes/ReorderFunctions.cpp +++ b/bolt/lib/Passes/ReorderFunctions.cpp @@ -28,7 +28,8 @@ extern cl::OptionCategory BoltOptCategory; extern cl::opt Verbosity; extern cl::opt RandomSeed; -extern size_t padFunction(const bolt::BinaryFunction &Function); +extern size_t padFunctionBefore(const bolt::BinaryFunction &Function); +extern size_t padFunctionAfter(const bolt::BinaryFunction &Function); extern cl::opt ReorderFunctions; cl::opt ReorderFunctions( @@ -304,8 +305,10 @@ 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::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 new file mode 100644 index 00000000000000..f3e8a23ddfdda6 --- /dev/null +++ b/bolt/test/AArch64/pad-before-funcs.s @@ -0,0 +1,48 @@ +# 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. + +# 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 + +# 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 +# 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 + +# 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