Merge branch 'main' into statusline-format-data-race

Author: JDevlieghere

.github/workflows/containers/github-action-ci-windows/Dockerfile

@@ -85,7 +85,7 @@ RUN powershell -Command \
 RUN git config --system core.longpaths true & \
     git config --global core.autocrlf false
 
-ARG RUNNER_VERSION=2.324.0
+ARG RUNNER_VERSION=2.325.0
 ENV RUNNER_VERSION=$RUNNER_VERSION
 
 RUN powershell -Command \

.github/workflows/containers/github-action-ci/Dockerfile

@@ -86,7 +86,7 @@ WORKDIR /home/gha
 
 FROM ci-container as ci-container-agent
 
-ENV GITHUB_RUNNER_VERSION=2.324.0
+ENV GITHUB_RUNNER_VERSION=2.325.0
 
 RUN mkdir actions-runner && \
     cd actions-runner && \

bolt/include/bolt/Core/BinaryFunction.h

@@ -2348,6 +2348,7 @@ class BinaryFunction {
       releaseCFG();
       CurrentState = State::Emitted;
     }
+    clearList(Relocations);
   }
 
   /// Process LSDA information for the function.

bolt/include/bolt/Profile/DataAggregator.h

@@ -370,33 +370,46 @@ class DataAggregator : public DataReader {
   /// memory.
   ///
   /// File format syntax:
-  /// {B|F|f|T} [<start_id>:]<start_offset> [<end_id>:]<end_offset> [<ft_end>]
-  ///       <count> [<mispred_count>]
+  /// E <event>
+  /// S <start> <count>
+  /// T <start> <end> <ft_end> <count>
+  /// B <start> <end> <count> <mispred_count>
+  /// [Ff] <start> <end> <count>
   ///
-  /// B - indicates an aggregated branch
-  /// F - an aggregated fall-through
+  /// where <start>, <end>, <ft_end> have the format [<id>:]<offset>
+  ///
+  /// E - name of the sampling event used for subsequent entries
+  /// S - indicates an aggregated basic sample at <start>
+  /// B - indicates an aggregated branch from <start> to <end>
+  /// F - an aggregated fall-through from <start> to <end>
   /// f - an aggregated fall-through with external origin - used to disambiguate
   ///       between a return hitting a basic block head and a regular internal
   ///       jump to the block
-  /// T - an aggregated trace: branch with a fall-through (from, to, ft_end)
-  ///
-  /// <start_id> - build id of the object containing the start address. We can
-  /// skip it for the main binary and use "X" for an unknown object. This will
-  /// save some space and facilitate human parsing.
-  ///
-  /// <start_offset> - hex offset from the object base load address (0 for the
-  /// main executable unless it's PIE) to the start address.
+  /// T - an aggregated trace: branch from <start> to <end> with a fall-through
+  ///       to <ft_end>
   ///
-  /// <end_id>, <end_offset> - same for the end address.
+  /// <id> - build id of the object containing the address. We can skip it for
+  /// the main binary and use "X" for an unknown object. This will save some
+  /// space and facilitate human parsing.
   ///
-  /// <ft_end> - same for the fallthrough_end address.
+  /// <offset> - hex offset from the object base load address (0 for the
+  /// main executable unless it's PIE) to the address.
   ///
-  /// <count> - total aggregated count of the branch or a fall-through.
+  /// <count> - total aggregated count.
   ///
   /// <mispred_count> - the number of times the branch was mispredicted.
-  /// Omitted for fall-throughs.
   ///
   /// Example:
+  /// Basic samples profile:
+  /// E cycles
+  /// S 41be50 3
+  /// E br_inst_retired.near_taken
+  /// S 41be60 6
+  ///
+  /// Trace profile combining branches and fall-throughs:
+  /// T 4b196f 4b19e0 4b19ef 2
+  ///
+  /// Legacy branch profile with separate branches and fall-throughs:
   /// F 41be50 41be50 3
   /// F 41be90 41be90 4
   /// B 4b1942 39b57f0 3 0

bolt/lib/Core/BinaryContext.cpp

@@ -1032,10 +1032,8 @@ void BinaryContext::adjustCodePadding() {
 
     if (!hasValidCodePadding(BF)) {
       if (HasRelocations) {
-        if (opts::Verbosity >= 1) {
-          this->outs() << "BOLT-INFO: function " << BF
-                       << " has invalid padding. Ignoring the function.\n";
-        }
+        this->errs() << "BOLT-WARNING: function " << BF
+                     << " has invalid padding. Ignoring the function\n";
         BF.setIgnored();
       } else {
         BF.setMaxSize(BF.getSize());

bolt/lib/Core/BinaryFunction.cpp

@@ -1526,8 +1526,6 @@ Error BinaryFunction::disassemble() {
   if (uint64_t Offset = getFirstInstructionOffset())
     Labels[Offset] = BC.Ctx->createNamedTempSymbol();
 
-  clearList(Relocations);
-
   if (!IsSimple) {
     clearList(Instructions);
     return createNonFatalBOLTError("");
@@ -3244,16 +3242,26 @@ void BinaryFunction::setTrapOnEntry() {
 }
 
 void BinaryFunction::setIgnored() {
+  IsIgnored = true;
+
   if (opts::processAllFunctions()) {
     // We can accept ignored functions before they've been disassembled.
-    // In that case, they would still get disassembled and emited, but not
+    // In that case, they would still get disassembled and emitted, but not
     // optimized.
-    assert(CurrentState == State::Empty &&
-           "cannot ignore non-empty functions in current mode");
-    IsIgnored = true;
+    if (CurrentState != State::Empty) {
+      BC.errs() << "BOLT-ERROR: cannot ignore non-empty function " << *this
+                << " in current mode\n";
+      exit(1);
+    }
     return;
   }
 
+  IsSimple = false;
+  LLVM_DEBUG(dbgs() << "Ignoring " << getPrintName() << '\n');
+
+  if (CurrentState == State::Empty)
+    return;
+
   clearDisasmState();
 
   // Clear CFG state too.
@@ -3273,9 +3281,11 @@ void BinaryFunction::setIgnored() {
 
   CurrentState = State::Empty;
 
-  IsIgnored = true;
-  IsSimple = false;
-  LLVM_DEBUG(dbgs() << "Ignoring " << getPrintName() << '\n');
+  // Fix external references in the original function body.
+  if (BC.HasRelocations) {
+    LLVM_DEBUG(dbgs() << "Scanning refs in " << *this << '\n');
+    scanExternalRefs();
+  }
 }
 
 void BinaryFunction::duplicateConstantIslands() {
@@ -3764,7 +3774,6 @@ void BinaryFunction::postProcessBranches() {
 
 MCSymbol *BinaryFunction::addEntryPointAtOffset(uint64_t Offset) {
   assert(Offset && "cannot add primary entry point");
-  assert(CurrentState == State::Empty || CurrentState == State::Disassembled);
 
   const uint64_t EntryPointAddress = getAddress() + Offset;
   MCSymbol *LocalSymbol = getOrCreateLocalLabel(EntryPointAddress);
@@ -3773,6 +3782,8 @@ MCSymbol *BinaryFunction::addEntryPointAtOffset(uint64_t Offset) {
   if (EntrySymbol)
     return EntrySymbol;
 
+  assert(CurrentState == State::Empty || CurrentState == State::Disassembled);
+
   if (BinaryData *EntryBD = BC.getBinaryDataAtAddress(EntryPointAddress)) {
     EntrySymbol = EntryBD->getSymbol();
   } else {

bolt/lib/Profile/DataAggregator.cpp

@@ -1204,77 +1204,106 @@ ErrorOr<Location> DataAggregator::parseLocationOrOffset() {
 }
 
 std::error_code DataAggregator::parseAggregatedLBREntry() {
-  while (checkAndConsumeFS()) {
-  }
+  enum AggregatedLBREntry : char {
+    INVALID = 0,
+    EVENT_NAME,        // E
+    TRACE,             // T
+    SAMPLE,            // S
+    BRANCH,            // B
+    FT,                // F
+    FT_EXTERNAL_ORIGIN // f
+  } Type = INVALID;
+
+  // The number of fields to parse, set based on Type.
+  int AddrNum = 0;
+  int CounterNum = 0;
+  // Storage for parsed fields.
+  StringRef EventName;
+  std::optional<Location> Addr[3];
+  int64_t Counters[2];
+
+  while (Type == INVALID || Type == EVENT_NAME) {
+    while (checkAndConsumeFS()) {
+    }
+    ErrorOr<StringRef> StrOrErr =
+        parseString(FieldSeparator, Type == EVENT_NAME);
+    if (std::error_code EC = StrOrErr.getError())
+      return EC;
+    StringRef Str = StrOrErr.get();
 
-  ErrorOr<StringRef> TypeOrErr = parseString(FieldSeparator);
-  if (std::error_code EC = TypeOrErr.getError())
-    return EC;
-  enum AggregatedLBREntry { TRACE, BRANCH, FT, FT_EXTERNAL_ORIGIN, INVALID };
-  auto Type = StringSwitch<AggregatedLBREntry>(TypeOrErr.get())
-                  .Case("T", TRACE)
-                  .Case("B", BRANCH)
-                  .Case("F", FT)
-                  .Case("f", FT_EXTERNAL_ORIGIN)
-                  .Default(INVALID);
-  if (Type == INVALID) {
-    reportError("expected T, B, F or f");
-    return make_error_code(llvm::errc::io_error);
-  }
+    if (Type == EVENT_NAME) {
+      EventName = Str;
+      break;
+    }
 
-  while (checkAndConsumeFS()) {
-  }
-  ErrorOr<Location> From = parseLocationOrOffset();
-  if (std::error_code EC = From.getError())
-    return EC;
+    Type = StringSwitch<AggregatedLBREntry>(Str)
+               .Case("T", TRACE)
+               .Case("S", SAMPLE)
+               .Case("E", EVENT_NAME)
+               .Case("B", BRANCH)
+               .Case("F", FT)
+               .Case("f", FT_EXTERNAL_ORIGIN)
+               .Default(INVALID);
+
+    if (Type == INVALID) {
+      reportError("expected T, S, E, B, F or f");
+      return make_error_code(llvm::errc::io_error);
+    }
 
-  while (checkAndConsumeFS()) {
+    using SSI = StringSwitch<int>;
+    AddrNum = SSI(Str).Case("T", 3).Case("S", 1).Case("E", 0).Default(2);
+    CounterNum = SSI(Str).Case("B", 2).Case("E", 0).Default(1);
   }
-  ErrorOr<Location> To = parseLocationOrOffset();
-  if (std::error_code EC = To.getError())
-    return EC;
 
-  ErrorOr<Location> TraceFtEnd = std::error_code();
-  if (Type == AggregatedLBREntry::TRACE) {
+  for (int I = 0; I < AddrNum; ++I) {
     while (checkAndConsumeFS()) {
     }
-    TraceFtEnd = parseLocationOrOffset();
-    if (std::error_code EC = TraceFtEnd.getError())
+    ErrorOr<Location> AddrOrErr = parseLocationOrOffset();
+    if (std::error_code EC = AddrOrErr.getError())
       return EC;
+    Addr[I] = AddrOrErr.get();
   }
 
-  while (checkAndConsumeFS()) {
-  }
-  ErrorOr<int64_t> Frequency =
-      parseNumberField(FieldSeparator, Type != AggregatedLBREntry::BRANCH);
-  if (std::error_code EC = Frequency.getError())
-    return EC;
-
-  uint64_t Mispreds = 0;
-  if (Type == AggregatedLBREntry::BRANCH) {
+  for (int I = 0; I < CounterNum; ++I) {
     while (checkAndConsumeFS()) {
     }
-    ErrorOr<int64_t> MispredsOrErr = parseNumberField(FieldSeparator, true);
-    if (std::error_code EC = MispredsOrErr.getError())
+    ErrorOr<int64_t> CountOrErr =
+        parseNumberField(FieldSeparator, I + 1 == CounterNum);
+    if (std::error_code EC = CountOrErr.getError())
       return EC;
-    Mispreds = static_cast<uint64_t>(MispredsOrErr.get());
+    Counters[I] = CountOrErr.get();
   }
 
   if (!checkAndConsumeNewLine()) {
     reportError("expected end of line");
     return make_error_code(llvm::errc::io_error);
   }
 
-  BinaryFunction *FromFunc = getBinaryFunctionContainingAddress(From->Offset);
-  BinaryFunction *ToFunc = getBinaryFunctionContainingAddress(To->Offset);
+  if (Type == EVENT_NAME) {
+    EventNames.insert(EventName);
+    return std::error_code();
+  }
 
-  for (BinaryFunction *BF : {FromFunc, ToFunc})
-    if (BF)
-      BF->setHasProfileAvailable();
+  const uint64_t FromOffset = Addr[0]->Offset;
+  BinaryFunction *FromFunc = getBinaryFunctionContainingAddress(FromOffset);
+  if (FromFunc)
+    FromFunc->setHasProfileAvailable();
+
+  int64_t Count = Counters[0];
+  int64_t Mispreds = Counters[1];
+
+  if (Type == SAMPLE) {
+    BasicSamples[FromOffset] += Count;
+    NumTotalSamples += Count;
+    return std::error_code();
+  }
 
-  uint64_t Count = static_cast<uint64_t>(Frequency.get());
+  const uint64_t ToOffset = Addr[1]->Offset;
+  BinaryFunction *ToFunc = getBinaryFunctionContainingAddress(ToOffset);
+  if (ToFunc)
+    ToFunc->setHasProfileAvailable();
 
-  Trace Trace(From->Offset, To->Offset);
+  Trace Trace(FromOffset, ToOffset);
   // Taken trace
   if (Type == TRACE || Type == BRANCH) {
     TakenBranchInfo &Info = BranchLBRs[Trace];
@@ -1285,8 +1314,9 @@ std::error_code DataAggregator::parseAggregatedLBREntry() {
   }
   // Construct fallthrough part of the trace
   if (Type == TRACE) {
-    Trace.From = To->Offset;
-    Trace.To = TraceFtEnd->Offset;
+    const uint64_t TraceFtEndOffset = Addr[2]->Offset;
+    Trace.From = ToOffset;
+    Trace.To = TraceFtEndOffset;
     Type = FromFunc == ToFunc ? FT : FT_EXTERNAL_ORIGIN;
   }
   // Add fallthrough trace

bolt/lib/Rewrite/RewriteInstance.cpp

@@ -3446,6 +3446,7 @@ void RewriteInstance::disassembleFunctions() {
         BC->outs() << "BOLT-INFO: could not disassemble function " << Function
                    << ". Will ignore.\n";
       // Forcefully ignore the function.
+      Function.scanExternalRefs();
       Function.setIgnored();
     });
 

bolt/test/AArch64/patch-ignored.s

@@ -0,0 +1,33 @@
+## Check that llvm-bolt patches functions that are getting ignored after their
+## CFG was constructed.
+
+# RUN: %clang %cflags %s -o %t.exe -Wl,-q
+# RUN: llvm-bolt %t.exe -o %t.bolt --force-patch 2>&1 | FileCheck %s
+# RUN: llvm-objdump -d %t.bolt | FileCheck %s --check-prefix=CHECK-OBJDUMP
+
+  .text
+
+## The function is too small to be patched and BOLT is forced to ignore it under
+## --force-patch. Check that the reference to _start is updated.
+# CHECK: BOLT-WARNING: failed to patch entries in unpatchable
+	.globl unpatchable
+  .type unpatchable, %function
+unpatchable:
+  .cfi_startproc
+# CHECK-OBJDUMP:      <unpatchable>:
+# CHECK-OBJDUMP-NEXT: bl {{.*}} <_start>
+  bl _start
+  ret
+  .cfi_endproc
+  .size unpatchable, .-unpatchable
+
+  .globl _start
+  .type _start, %function
+_start:
+  .cfi_startproc
+  cmp  x0, 1
+  b.eq  .L0
+.L0:
+  ret  x30
+  .cfi_endproc
+  .size _start, .-_start