Merge branch 'main' into users/rovka/whole-wave-funcs

Author: rovka

.ci/compute_projects.py

@@ -77,6 +77,7 @@
 DEPENDENT_RUNTIMES_TO_TEST = {
     "clang": {"compiler-rt"},
     "clang-tools-extra": {"libc"},
+    "libc": {"libc"},
     ".ci": {"compiler-rt", "libc"},
 }
 DEPENDENT_RUNTIMES_TO_TEST_NEEDS_RECONFIG = {

.ci/compute_projects_test.py

@@ -187,7 +187,7 @@ def test_top_level_file(self):
         self.assertEqual(env_variables["runtimes_check_targets"], "")
         self.assertEqual(env_variables["runtimes_check_targets_needs_reconfig"], "")
 
-    def test_exclude_runtiems_in_projects(self):
+    def test_exclude_libcxx_in_projects(self):
         env_variables = compute_projects.get_env_variables(
             ["libcxx/CMakeLists.txt"], "Linux"
         )
@@ -197,6 +197,16 @@ def test_exclude_runtiems_in_projects(self):
         self.assertEqual(env_variables["runtimes_check_targets"], "")
         self.assertEqual(env_variables["runtimes_check_targets_needs_reconfig"], "")
 
+    def test_include_libc_in_runtimes(self):
+        env_variables = compute_projects.get_env_variables(
+            ["libc/CMakeLists.txt"], "Linux"
+        )
+        self.assertEqual(env_variables["projects_to_build"], "clang;lld")
+        self.assertEqual(env_variables["project_check_targets"], "")
+        self.assertEqual(env_variables["runtimes_to_build"], "libc")
+        self.assertEqual(env_variables["runtimes_check_targets"], "check-libc")
+        self.assertEqual(env_variables["runtimes_check_targets_needs_reconfig"], "")
+
     def test_exclude_docs(self):
         env_variables = compute_projects.get_env_variables(
             ["llvm/docs/CIBestPractices.rst"], "Linux"

bolt/include/bolt/Core/BinarySection.h

@@ -523,11 +523,6 @@ inline uint8_t *copyByteArray(const uint8_t *Data, uint64_t Size) {
   return Array;
 }
 
-inline uint8_t *copyByteArray(StringRef Buffer) {
-  return copyByteArray(reinterpret_cast<const uint8_t *>(Buffer.data()),
-                       Buffer.size());
-}
-
 inline uint8_t *copyByteArray(ArrayRef<char> Buffer) {
   return copyByteArray(reinterpret_cast<const uint8_t *>(Buffer.data()),
                        Buffer.size());

bolt/include/bolt/Profile/DataAggregator.h

@@ -374,6 +374,9 @@ class DataAggregator : public DataReader {
   /// Parse a single pair of binary full path and associated build-id
   std::optional<std::pair<StringRef, StringRef>> parseNameBuildIDPair();
 
+  /// Coordinate reading and parsing of perf.data file
+  void parsePerfData(BinaryContext &BC);
+
   /// Coordinate reading and parsing of pre-aggregated file
   ///
   /// The regular perf2bolt aggregation job is to read perf output directly.

bolt/lib/Passes/BinaryPasses.cpp

@@ -1765,27 +1765,26 @@ Error PrintProgramStats::runOnFunctions(BinaryContext &BC) {
 
   if (opts::ShowDensity) {
     double Density = 0.0;
-    // Sorted by the density in descending order.
-    llvm::stable_sort(FuncDensityList,
-                      [&](const std::pair<double, uint64_t> &A,
-                          const std::pair<double, uint64_t> &B) {
-                        if (A.first != B.first)
-                          return A.first > B.first;
-                        return A.second < B.second;
-                      });
+    llvm::sort(FuncDensityList);
 
     uint64_t AccumulatedSamples = 0;
-    uint32_t I = 0;
     assert(opts::ProfileDensityCutOffHot <= 1000000 &&
            "The cutoff value is greater than 1000000(100%)");
-    while (AccumulatedSamples <
-               TotalSampleCount *
-                   static_cast<float>(opts::ProfileDensityCutOffHot) /
-                   1000000 &&
-           I < FuncDensityList.size()) {
-      AccumulatedSamples += FuncDensityList[I].second;
-      Density = FuncDensityList[I].first;
-      I++;
+    // Subtract samples in zero-density functions (no fall-throughs) from
+    // TotalSampleCount (not used anywhere below).
+    for (const auto [CurDensity, CurSamples] : FuncDensityList) {
+      if (CurDensity != 0.0)
+        break;
+      TotalSampleCount -= CurSamples;
+    }
+    const uint64_t CutoffSampleCount =
+        1.f * TotalSampleCount * opts::ProfileDensityCutOffHot / 1000000;
+    // Process functions in decreasing density order
+    for (const auto [CurDensity, CurSamples] : llvm::reverse(FuncDensityList)) {
+      if (AccumulatedSamples >= CutoffSampleCount)
+        break;
+      AccumulatedSamples += CurSamples;
+      Density = CurDensity;
     }
     if (Density == 0.0) {
       BC.errs() << "BOLT-WARNING: the output profile is empty or the "

bolt/lib/Passes/PAuthGadgetScanner.cpp

@@ -82,6 +82,22 @@ namespace PAuthGadgetScanner {
   dbgs() << "\n";
 }
 
+// Iterates over BinaryFunction's instructions like a range-based for loop:
+//
+// iterateOverInstrs(BF, [&](MCInstReference Inst) {
+//   // loop body
+// });
+template <typename T> static void iterateOverInstrs(BinaryFunction &BF, T Fn) {
+  if (BF.hasCFG()) {
+    for (BinaryBasicBlock &BB : BF)
+      for (int64_t I = 0, E = BB.size(); I < E; ++I)
+        Fn(MCInstInBBReference(&BB, I));
+  } else {
+    for (auto I : BF.instrs())
+      Fn(MCInstInBFReference(&BF, I.first));
+  }
+}
+
 // This class represents mapping from a set of arbitrary physical registers to
 // consecutive array indexes.
 class TrackedRegisters {
@@ -342,6 +358,29 @@ class SrcSafetyAnalysis {
     return S;
   }
 
+  /// Computes a reasonably pessimistic estimation of the register state when
+  /// the previous instruction is not known for sure. Takes the set of registers
+  /// which are trusted at function entry and removes all registers that can be
+  /// clobbered inside this function.
+  SrcState computePessimisticState(BinaryFunction &BF) {
+    BitVector ClobberedRegs(NumRegs);
+    iterateOverInstrs(BF, [&](MCInstReference Inst) {
+      BC.MIB->getClobberedRegs(Inst, ClobberedRegs);
+
+      // If this is a call instruction, no register is safe anymore, unless
+      // it is a tail call. Ignore tail calls for the purpose of estimating the
+      // worst-case scenario, assuming no instructions are executed in the
+      // caller after this point anyway.
+      if (BC.MIB->isCall(Inst) && !BC.MIB->isTailCall(Inst))
+        ClobberedRegs.set();
+    });
+
+    SrcState S = createEntryState();
+    S.SafeToDerefRegs.reset(ClobberedRegs);
+    S.TrustedRegs.reset(ClobberedRegs);
+    return S;
+  }
+
   BitVector getClobberedRegs(const MCInst &Point) const {
     BitVector Clobbered(NumRegs);
     // Assume a call can clobber all registers, including callee-saved
@@ -545,6 +584,10 @@ class DataflowSrcSafetyAnalysis
   using SrcSafetyAnalysis::BC;
   using SrcSafetyAnalysis::computeNext;
 
+  // Pessimistic initial state for basic blocks without any predecessors
+  // (not needed for most functions, thus initialized lazily).
+  SrcState PessimisticState;
+
 public:
   DataflowSrcSafetyAnalysis(BinaryFunction &BF,
                             MCPlusBuilder::AllocatorIdTy AllocId,
@@ -585,6 +628,18 @@ class DataflowSrcSafetyAnalysis
     if (BB.isEntryPoint())
       return createEntryState();
 
+    // If a basic block without any predecessors is found in an optimized code,
+    // this likely means that some CFG edges were not detected. Pessimistically
+    // assume any register that can ever be clobbered in this function to be
+    // unsafe before this basic block.
+    // Warn about this fact in FunctionAnalysis::findUnsafeUses(), as it likely
+    // means imprecise CFG information.
+    if (BB.pred_empty()) {
+      if (PessimisticState.empty())
+        PessimisticState = computePessimisticState(*BB.getParent());
+      return PessimisticState;
+    }
+
     return SrcState();
   }
 
@@ -682,19 +737,14 @@ template <typename StateTy> class CFGUnawareAnalysis {
 //
 // Then, a function can be split into a number of disjoint contiguous sequences
 // of instructions without labels in between. These sequences can be processed
-// the same way basic blocks are processed by data-flow analysis, assuming
-// pessimistically that all registers are unsafe at the start of each sequence.
+// the same way basic blocks are processed by data-flow analysis, with the same
+// pessimistic estimation of the initial state at the start of each sequence
+// (except the first instruction of the function).
 class CFGUnawareSrcSafetyAnalysis : public SrcSafetyAnalysis,
                                     public CFGUnawareAnalysis<SrcState> {
   using SrcSafetyAnalysis::BC;
   BinaryFunction &BF;
 
-  /// Creates a state with all registers marked unsafe (not to be confused
-  /// with empty state).
-  SrcState createUnsafeState() const {
-    return SrcState(NumRegs, RegsToTrackInstsFor.getNumTrackedRegisters());
-  }
-
 public:
   CFGUnawareSrcSafetyAnalysis(BinaryFunction &BF,
                               MCPlusBuilder::AllocatorIdTy AllocId,
@@ -704,6 +754,7 @@ class CFGUnawareSrcSafetyAnalysis : public SrcSafetyAnalysis,
   }
 
   void run() override {
+    const SrcState DefaultState = computePessimisticState(BF);
     SrcState S = createEntryState();
     for (auto &I : BF.instrs()) {
       MCInst &Inst = I.second;
@@ -718,7 +769,7 @@ class CFGUnawareSrcSafetyAnalysis : public SrcSafetyAnalysis,
         LLVM_DEBUG({
           traceInst(BC, "Due to label, resetting the state before", Inst);
         });
-        S = createUnsafeState();
+        S = DefaultState;
       }
 
       // Attach the state *before* this instruction executes.
@@ -1344,17 +1395,6 @@ shouldReportAuthOracle(const BinaryContext &BC, const MCInstReference &Inst,
   return make_gadget_report(AuthOracleKind, Inst, *AuthReg);
 }
 
-template <typename T> static void iterateOverInstrs(BinaryFunction &BF, T Fn) {
-  if (BF.hasCFG()) {
-    for (BinaryBasicBlock &BB : BF)
-      for (int64_t I = 0, E = BB.size(); I < E; ++I)
-        Fn(MCInstInBBReference(&BB, I));
-  } else {
-    for (auto I : BF.instrs())
-      Fn(MCInstInBFReference(&BF, I.first));
-  }
-}
-
 static SmallVector<MCPhysReg>
 collectRegsToTrack(ArrayRef<PartialReport<MCPhysReg>> Reports) {
   SmallSet<MCPhysReg, 4> RegsToTrack;
@@ -1375,17 +1415,60 @@ void FunctionAnalysisContext::findUnsafeUses(
     BF.dump();
   });
 
+  bool UnreachableBBReported = false;
+  if (BF.hasCFG()) {
+    // Warn on basic blocks being unreachable according to BOLT (at most once
+    // per BinaryFunction), as this likely means the CFG reconstructed by BOLT
+    // is imprecise. A basic block can be
+    // * reachable from an entry basic block - a hopefully correct non-empty
+    //   state is propagated to that basic block sooner or later. All basic
+    //   blocks are expected to belong to this category under normal conditions.
+    // * reachable from a "directly unreachable" BB (a basic block that has no
+    //   direct predecessors and this is not because it is an entry BB) - *some*
+    //   non-empty state is propagated to this basic block sooner or later, as
+    //   the initial state of directly unreachable basic blocks is
+    //   pessimistically initialized to "all registers are unsafe"
+    //   - a warning can be printed for the "directly unreachable" basic block
+    // * neither reachable from an entry nor from a "directly unreachable" BB
+    //   (such as if this BB is in an isolated loop of basic blocks) - the final
+    //   state is computed to be empty for this basic block
+    //   - a warning can be printed for this basic block
+    for (BinaryBasicBlock &BB : BF) {
+      MCInst *FirstInst = BB.getFirstNonPseudoInstr();
+      // Skip empty basic block early for simplicity.
+      if (!FirstInst)
+        continue;
+
+      bool IsDirectlyUnreachable = BB.pred_empty() && !BB.isEntryPoint();
+      bool HasNoStateComputed = Analysis->getStateBefore(*FirstInst).empty();
+      if (!IsDirectlyUnreachable && !HasNoStateComputed)
+        continue;
+
+      // Arbitrarily attach the report to the first instruction of BB.
+      // This is printed as "[message] in function [name], basic block ...,
+      // at address ..." when the issue is reported to the user.
+      Reports.push_back(make_generic_report(
+          MCInstReference::get(FirstInst, BF),
+          "Warning: possibly imprecise CFG, the analysis quality may be "
+          "degraded in this function. According to BOLT, unreachable code is "
+          "found" /* in function [name]... */));
+      UnreachableBBReported = true;
+      break; // One warning per function.
+    }
+  }
+  // FIXME: Warn the user about imprecise analysis when the function has no CFG
+  //        information at all.
+
   iterateOverInstrs(BF, [&](MCInstReference Inst) {
     if (BC.MIB->isCFI(Inst))
       return;
 
     const SrcState &S = Analysis->getStateBefore(Inst);
-
-    // If non-empty state was never propagated from the entry basic block
-    // to Inst, assume it to be unreachable and report a warning.
     if (S.empty()) {
-      Reports.push_back(
-          make_generic_report(Inst, "Warning: unreachable instruction found"));
+      LLVM_DEBUG(
+          { traceInst(BC, "Instruction has no state, skipping", Inst); });
+      assert(UnreachableBBReported && "Should be reported at least once");
+      (void)UnreachableBBReported;
       return;
     }
 

bolt/lib/Profile/DataAggregator.cpp

@@ -466,9 +466,7 @@ int DataAggregator::prepareToParse(StringRef Name, PerfProcessInfo &Process,
   return PI.ReturnCode;
 }
 
-Error DataAggregator::preprocessProfile(BinaryContext &BC) {
-  this->BC = &BC;
-
+void DataAggregator::parsePerfData(BinaryContext &BC) {
   auto ErrorCallback = [](int ReturnCode, StringRef ErrBuf) {
     errs() << "PERF-ERROR: return code " << ReturnCode << "\n" << ErrBuf;
     exit(1);
@@ -481,11 +479,6 @@ Error DataAggregator::preprocessProfile(BinaryContext &BC) {
       ErrorCallback(ReturnCode, ErrBuf);
   };
 
-  if (opts::ReadPreAggregated) {
-    parsePreAggregated();
-    goto heatmap;
-  }
-
   if (std::optional<StringRef> FileBuildID = BC.getFileBuildID()) {
     outs() << "BOLT-INFO: binary build-id is:     " << *FileBuildID << "\n";
     processFileBuildID(*FileBuildID);
@@ -534,22 +527,28 @@ Error DataAggregator::preprocessProfile(BinaryContext &BC) {
              << '\n';
 
   deleteTempFiles();
+}
 
-heatmap:
-  // Sort parsed traces for faster processing.
-  llvm::sort(Traces, llvm::less_first());
+Error DataAggregator::preprocessProfile(BinaryContext &BC) {
+  this->BC = &BC;
 
-  if (!opts::HeatmapMode)
-    return Error::success();
+  if (opts::ReadPreAggregated) {
+    parsePreAggregated();
+  } else {
+    parsePerfData(BC);
+  }
 
-  if (std::error_code EC = printLBRHeatMap())
-    return errorCodeToError(EC);
+  // Sort parsed traces for faster processing.
+  llvm::sort(Traces, llvm::less_first());
 
-  if (opts::HeatmapMode == opts::HeatmapModeKind::HM_Optional)
-    return Error::success();
+  if (opts::HeatmapMode) {
+    if (std::error_code EC = printLBRHeatMap())
+      return errorCodeToError(EC);
+    if (opts::HeatmapMode == opts::HeatmapModeKind::HM_Exclusive)
+      exit(0);
+  }
 
-  assert(opts::HeatmapMode == opts::HeatmapModeKind::HM_Exclusive);
-  exit(0);
+  return Error::success();
 }
 
 Error DataAggregator::readProfile(BinaryContext &BC) {