[BOLT] Set cold sections alignment explicitly

[yota9]

Summary

The cold text section alignment is set using the maximum alignment value
passed to the emitCodeAlignment. In order to calculate tentetive layout
right we will set the alignment of such sections to the maximum possible
function alignment explicitly.

[yota9]

Set the min section alignment to AlignFunctions

[yota9]

nit

[yota9]

@rafaelauler Gentle ping :)

[rafauler]

bolt/lib/Core/BinaryEmitter.cpp

294–296

Why is this necessary? Is it possible to write a testcase that shows where we are getting the layout wrong?

[yota9]

bolt/lib/Core/BinaryEmitter.cpp

294–296

As I said cold text section alignment is set using the maximum alignment value
passed to the emitCodeAlignment (see just under of these new lines). The thing is that when we calculate tentative layout in LongJmp currently we don't take into account that there will be the alignment gap between text and text.cold (see new line I've added there). In order to take cold text alignment in to the account we need to know the biggest alignment value we will pass to the emitCodeAlignment. So the simple idea here is to align the cold section ( which we didn't explicitly aligned before ) with the biggest possible value (opts::AlignFunctions). This way we are guaranteed that it will be the maximum possible value that might be passed to the emitCodeAlignment and now we can easily calculate tentative layout more precise.
I will try to create the simple test soon, I hope it won't be difficult.

[yota9]

bolt/lib/Core/BinaryEmitter.cpp

294–296

UPD: I've tried to think of the test, but as for aarch64 we now can trigger only the bl case. To do this we should write something, that will have between the call and callee ~128mb, to trigger the error after cold section will be created. We can use "rept" in asm, but it will be very slow (both codegen and bolt) and quite a big binary will be created. So I just don't see the reasonable and easy way to create such test currently..

[rafauler]

bolt/lib/Passes/LongJmp.cpp

301

From what I understand, the real problem though is here, right?

We align with the minimum value, but we could be aligning more aggressively depending on "max align bytes", correct?

Also, this is done for every function. So if there is an alignment difference created from MinAlign vs. real alignment, I imagine this difference will increase as we emit more and more functions? What confused me then is why are we aligning only the very first function (via section alignment, since all functions are emitted to the same section). Wouldn't be better to replicate the logic to compute the correct alignment here? (Use a formula that retrieves how many bytes need to align and check it against max align bytes). The goal of LongJmp is to replicate exactly what binary emitter is doing (I know this is unfortunate and bug prone, but I don't think there is a better way to have access to the exact offsets of the future layout of functions without calling the assembler and emitting everything)

[yota9]

bolt/lib/Passes/LongJmp.cpp

301

The thing is that we don't align the first function here, we align the section.
On function emittion we call

Streamer.emitCodeAlignment

for each function.

That function calls emitValueToAlignment located in MCObjectStreamer.cpp that contains the following lines:

if (ByteAlignment > CurSec->getAlignment())
  CurSec->setAlignment(Align(ByteAlignment));

So depending on what we passed in emitValueToAlignment we might change the section alignment and (of course) the alignment of the very first function. I don't want to overcomplicate the code trying to first to calculate the section alignment that the functions alignment & etc (this should also be done for golang for example, so for every pass that will calculate the layout), so the easiest and the most straightforward way I see is to align the cold section using the maximum possible value explicitly. As for the space-wasting this is very minor, since the ByteAlignment is limited by uint16_t value (64kb) and most of the time we are speaking about just a ~64 bytes. But such a solution will significantly simplify the calculations for such passes :)

[rafauler]

Oh, I see what you mean now and what's being fixed. Thanks for explaining!
I noticed that what I said about considering "max align bytes" is already being done, so the only thing missing was the section alignment that happens implicitly via a call to "emitCodeAlignment".

bolt/include/bolt/Utils/CommandLineOpts.h

 extern llvm::cl::OptionCategory BoltOptCategory;
 extern llvm::cl::OptionCategory BoltRelocCategory;
 extern llvm::cl::OptionCategory BoltOutputCategory;
 extern llvm::cl::OptionCategory AggregatorCategory;
 extern llvm::cl::OptionCategory BoltInstrCategory;
 extern llvm::cl::OptionCategory HeatmapCategory;
 
 extern llvm::cl::opt<unsigned> AlignText;
+extern llvm::cl::opt<unsigned> AlignFunctions;
 extern llvm::cl::opt<bool> AggregateOnly;
 extern llvm::cl::opt<unsigned> BucketsPerLine;
 extern llvm::cl::opt<bool> DiffOnly;
 extern llvm::cl::opt<bool> EnableBAT;
 extern llvm::cl::opt<bool> RemoveSymtab;
 extern llvm::cl::opt<unsigned> ExecutionCountThreshold;
 extern llvm::cl::opt<unsigned> HeatmapBlock;
 extern llvm::cl::opt<unsigned long long> HeatmapMaxAddress;

bolt/lib/Core/BinaryEmitter.cpp

bool BinaryEmitter::emitFunction(BinaryFunction &Function, bool EmitColdPart) {
   MCSection *Section =
       BC.getCodeSection(EmitColdPart ? Function.getColdCodeSectionName()
                                      : Function.getCodeSectionName());
   Streamer.SwitchSection(Section);
   Section->setHasInstructions(true);
   BC.Ctx->addGenDwarfSection(Section);
 
   if (BC.HasRelocations) {
+    // Set section alignment to the maximum possible object alignment.
+    // We need this to support LongJmp and other passes that calculates
+    // tentative layout.
+    if (Section->getAlignment() == 1)
+      Section->setAlignment(Align(opts::AlignFunctions));
+
     Streamer.emitCodeAlignment(BinaryFunction::MinAlign, &*BC.STI);
     uint16_t MaxAlignBytes = EmitColdPart ? Function.getMaxColdAlignmentBytes()
                                           : Function.getMaxAlignmentBytes();
     if (MaxAlignBytes > 0)
       Streamer.emitCodeAlignment(Function.getAlignment(), &*BC.STI,
                                  MaxAlignBytes);
   } else {
     Streamer.emitCodeAlignment(Function.getAlignment(), &*BC.STI);

bolt/lib/Passes/Aligner.cpp

 using namespace llvm;
 
 namespace opts {
 
 extern cl::OptionCategory BoltOptCategory;
 
 extern cl::opt<bool> AlignBlocks;
 extern cl::opt<bool> PreserveBlocksAlignment;
+extern cl::opt<unsigned> AlignFunctions;
 
 cl::opt<unsigned>
 AlignBlocksMinSize("align-blocks-min-size",
   cl::desc("minimal size of the basic block that should be aligned"),
   cl::init(0),
   cl::ZeroOrMore,
   cl::Hidden,
   cl::cat(BoltOptCategory));
 
 cl::opt<unsigned>
 AlignBlocksThreshold("align-blocks-threshold",
   cl::desc("align only blocks with frequency larger than containing function "
            "execution frequency specified in percent. E.g. 1000 means aligning "
            "blocks that are 10 times more frequently executed than the "
            "containing function."),
   cl::init(800),
   cl::ZeroOrMore,
   cl::Hidden,
   cl::cat(BoltOptCategory));
 
 cl::opt<unsigned>
-AlignFunctions("align-functions",
-  cl::desc("align functions at a given value (relocation mode)"),
-  cl::init(64),
-  cl::ZeroOrMore,
-  cl::cat(BoltOptCategory));
-
-cl::opt<unsigned>
 AlignFunctionsMaxBytes("align-functions-max-bytes",
   cl::desc("maximum number of bytes to use to align functions"),
   cl::init(32),
   cl::ZeroOrMore,
   cl::cat(BoltOptCategory));
 
 cl::opt<unsigned>
 BlockAlignment("block-alignment",

bolt/lib/Passes/LongJmp.cpp

 //===- bolt/Passes/LongJmp.cpp --------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // This file implements the LongJmpPass class.
 //
 //===----------------------------------------------------------------------===//
 
 #include "bolt/Passes/LongJmp.h"
-#include "llvm/Support/Alignment.h"
 
 #define DEBUG_TYPE "longjmp"
 
 using namespace llvm;
 
 namespace opts {
 extern cl::OptionCategory BoltOptCategory;
-
+extern cl::opt<unsigned> AlignFunctions;
 extern cl::opt<bool> UseOldText;
 extern cl::opt<bool> HotFunctionsAtEnd;
 
 static cl::opt<bool>
 GroupStubs("group-stubs",
   cl::desc("share stubs across functions"),
   cl::init(true),
   cl::ZeroOrMore,
if (Cold || BB->isCold()) {
       HotDot += BC.computeCodeSize(BB->begin(), BB->end());
     }
   }
 }
 
 uint64_t LongJmpPass::tentativeLayoutRelocColdPart(
     const BinaryContext &BC, std::vector<BinaryFunction *> &SortedFunctions,
     uint64_t DotAddress) {
+  DotAddress = alignTo(DotAddress, llvm::Align(opts::AlignFunctions));
   for (BinaryFunction *Func : SortedFunctions) {
     if (!Func->isSplit())
       continue;
     DotAddress = alignTo(DotAddress, BinaryFunction::MinAlign);
     uint64_t Pad =
         offsetToAlignment(DotAddress, llvm::Align(Func->getAlignment()));
     if (Pad <= Func->getMaxColdAlignmentBytes())
       DotAddress += Pad;
     ColdAddresses[Func] = DotAddress;
     LLVM_DEBUG(dbgs() << Func->getPrintName() << " cold tentative: "
                       << Twine::utohexstr(DotAddress) << "\n");
     DotAddress += Func->estimateColdSize();

bolt/lib/Utils/CommandLineOpts.cpp

 
 cl::opt<unsigned>
 AlignText("align-text",
   cl::desc("alignment of .text section"),
   cl::ZeroOrMore,
   cl::Hidden,
   cl::cat(BoltCategory));
 
+cl::opt<unsigned> AlignFunctions(
+    "align-functions",
+    cl::desc("align functions at a given value (relocation mode)"),
+    cl::init(64), cl::ZeroOrMore, cl::cat(BoltOptCategory));
+
 cl::opt<bool>
 AggregateOnly("aggregate-only",
   cl::desc("exit after writing aggregated data file"),
   cl::Hidden,
   cl::cat(AggregatorCategory));
 
 cl::opt<unsigned>
     BucketsPerLine("line-size",