Skip to content

[BOLT] Add --pad-funcs-before=func:n #117924

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Dec 11, 2024
Merged

[BOLT] Add --pad-funcs-before=func:n #117924

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 42 additions & 11 deletions bolt/lib/Core/BinaryEmitter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's an issue here with the static 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 one of our internal tests so I'm going to revert this patch.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for spotting and the revert. Fix is at #121918.


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;
Expand Down Expand Up @@ -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();

Expand Down Expand Up @@ -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());
Expand Down
12 changes: 9 additions & 3 deletions bolt/lib/Passes/ReorderFunctions.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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;
Expand Down
29 changes: 29 additions & 0 deletions bolt/test/AArch64/pad-before-funcs.s
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
Loading