From be21bd9bbf3bc906f9b98ac3de1fc88a4a8ac4b4 Mon Sep 17 00:00:00 2001 From: Amir Ayupov Date: Mon, 6 Jan 2025 12:52:44 -0800 Subject: [PATCH] Revert "[BOLT] Add --pad-funcs-before=func:n (#117924)" 14dcf8214f9c66172d17c1cfaec6aec0030748e0 introduced a subtle bug with the static `FunctionPadding` map. If either `opts::FunctionPadSpec` or `opts::FunctionPadBeforeSpec` are set, the map is going to be populated with the respective spec in the first invocation of `BinaryEmitter::emitFunction`. The subsequent invocations will pick up the padding from the map irrespective of whether `opts::FunctionPadSpec` or `opts::FunctionPadBeforeSpec` is passed as a parameter. This breaks an internal test, hence reverting the patch. --- bolt/lib/Core/BinaryEmitter.cpp | 53 ++++++---------------------- bolt/lib/Passes/ReorderFunctions.cpp | 12 ++----- bolt/test/AArch64/pad-before-funcs.s | 29 --------------- 3 files changed, 14 insertions(+), 80 deletions(-) delete mode 100644 bolt/test/AArch64/pad-before-funcs.s diff --git a/bolt/lib/Core/BinaryEmitter.cpp b/bolt/lib/Core/BinaryEmitter.cpp index 5019cf31beee..1744c1e57172 100644 --- a/bolt/lib/Core/BinaryEmitter.cpp +++ b/bolt/lib/Core/BinaryEmitter.cpp @@ -46,17 +46,13 @@ BreakFunctionNames("break-funcs", cl::Hidden, cl::cat(BoltCategory)); -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)); - -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::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)); static cl::opt MarkFuncs( "mark-funcs", @@ -74,12 +70,11 @@ X86AlignBranchBoundaryHotOnly("x86-align-branch-boundary-hot-only", cl::init(true), cl::cat(BoltOptCategory)); -size_t padFunction(const cl::list &Spec, - const BinaryFunction &Function) { +size_t padFunction(const BinaryFunction &Function) { static std::map FunctionPadding; - if (FunctionPadding.empty() && !Spec.empty()) { - for (const std::string &Spec : Spec) { + if (FunctionPadding.empty() && !FunctionPadSpec.empty()) { + for (std::string &Spec : FunctionPadSpec) { size_t N = Spec.find(':'); if (N == std::string::npos) continue; @@ -324,32 +319,6 @@ 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(); @@ -404,7 +373,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::padFunction(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 f8f6a01526dc..1256d71342b1 100644 --- a/bolt/lib/Passes/ReorderFunctions.cpp +++ b/bolt/lib/Passes/ReorderFunctions.cpp @@ -28,9 +28,7 @@ extern cl::OptionCategory BoltOptCategory; extern cl::opt Verbosity; extern cl::opt RandomSeed; -extern size_t padFunction(const cl::list &Spec, - const bolt::BinaryFunction &Function); -extern cl::list FunctionPadSpec, FunctionPadBeforeSpec; +extern size_t padFunction(const bolt::BinaryFunction &Function); extern cl::opt ReorderFunctions; cl::opt ReorderFunctions( @@ -306,12 +304,8 @@ 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::padFunction(*A); + const size_t PadB = opts::padFunction(*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 deleted file mode 100644 index 3ce0ee5e3838..000000000000 --- a/bolt/test/AArch64/pad-before-funcs.s +++ /dev/null @@ -1,29 +0,0 @@ -# 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