Skip to content

Commit

Permalink
Revert "[BOLT] Add --pad-funcs-before=func:n (llvm#117924)"
Browse files Browse the repository at this point in the history
14dcf82 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.
  • Loading branch information
aaupov committed Jan 6, 2025
1 parent fbcf3cb commit be21bd9
Show file tree
Hide file tree
Showing 3 changed files with 14 additions and 80 deletions.
53 changes: 11 additions & 42 deletions bolt/lib/Core/BinaryEmitter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -46,17 +46,13 @@ BreakFunctionNames("break-funcs",
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::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));

static cl::opt<bool> MarkFuncs(
"mark-funcs",
Expand All @@ -74,12 +70,11 @@ X86AlignBranchBoundaryHotOnly("x86-align-branch-boundary-hot-only",
cl::init(true),
cl::cat(BoltOptCategory));

size_t padFunction(const cl::list<std::string> &Spec,
const BinaryFunction &Function) {
size_t padFunction(const BinaryFunction &Function) {
static std::map<std::string, size_t> 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;
Expand Down Expand Up @@ -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();

Expand Down Expand Up @@ -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());
Expand Down
12 changes: 3 additions & 9 deletions bolt/lib/Passes/ReorderFunctions.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,7 @@ 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 padFunction(const bolt::BinaryFunction &Function);

extern cl::opt<bolt::ReorderFunctions::ReorderType> ReorderFunctions;
cl::opt<bolt::ReorderFunctions::ReorderType> ReorderFunctions(
Expand Down Expand Up @@ -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;
Expand Down
29 changes: 0 additions & 29 deletions bolt/test/AArch64/pad-before-funcs.s

This file was deleted.

0 comments on commit be21bd9

Please sign in to comment.