review feedback

Created using spr 1.3.6

Author: cachemeifyoucan

.ci/generate_test_report_lib.py

@@ -12,6 +12,65 @@
     "https://github.com/llvm/llvm-project/issues and add the "
     "`infrastructure` label."
 )
+# The maximum number of lines to pull from a ninja failure.
+NINJA_LOG_SIZE_THRESHOLD = 500
+
+
+def _parse_ninja_log(ninja_log: list[str]) -> list[tuple[str, str]]:
+    """Parses an individual ninja log."""
+    failures = []
+    index = 0
+    while index < len(ninja_log):
+        while index < len(ninja_log) and not ninja_log[index].startswith("FAILED:"):
+            index += 1
+        if index == len(ninja_log):
+            # We hit the end of the log without finding a build failure, go to
+            # the next log.
+            return failures
+        # We are trying to parse cases like the following:
+        #
+        # [4/5] test/4.stamp
+        # FAILED: touch test/4.stamp
+        # touch test/4.stamp
+        #
+        # index will point to the line that starts with Failed:. The progress
+        # indicator is the line before this ([4/5] test/4.stamp) and contains a pretty
+        # printed version of the target being built (test/4.stamp). We use this line
+        # and remove the progress information to get a succinct name for the target.
+        failing_action = ninja_log[index - 1].split("] ")[1]
+        failure_log = []
+        while (
+            index < len(ninja_log)
+            and not ninja_log[index].startswith("[")
+            and not ninja_log[index].startswith("ninja: build stopped:")
+            and len(failure_log) < NINJA_LOG_SIZE_THRESHOLD
+        ):
+            failure_log.append(ninja_log[index])
+            index += 1
+        failures.append((failing_action, "\n".join(failure_log)))
+    return failures
+
+
+def find_failure_in_ninja_logs(ninja_logs: list[list[str]]) -> list[tuple[str, str]]:
+    """Extracts failure messages from ninja output.
+
+    This function takes stdout/stderr from ninja in the form of a list of files
+    represented as a list of lines. This function then returns tuples containing
+    the name of the target and the error message.
+
+    Args:
+      ninja_logs: A list of files in the form of a list of lines representing the log
+        files captured from ninja.
+
+    Returns:
+      A list of tuples. The first string is the name of the target that failed. The
+      second string is the error message.
+    """
+    failures = []
+    for ninja_log in ninja_logs:
+        log_failures = _parse_ninja_log(ninja_log)
+        failures.extend(log_failures)
+    return failures
 
 
 # Set size_limit to limit the byte size of the report. The default is 1MB as this

.ci/generate_test_report_lib_test.py

@@ -19,6 +19,111 @@ def junit_from_xml(xml):
 
 
 class TestReports(unittest.TestCase):
+    def test_find_failure_ninja_logs(self):
+        failures = generate_test_report_lib.find_failure_in_ninja_logs(
+            [
+                [
+                    "[1/5] test/1.stamp",
+                    "[2/5] test/2.stamp",
+                    "[3/5] test/3.stamp",
+                    "[4/5] test/4.stamp",
+                    "FAILED: touch test/4.stamp",
+                    "Wow! This system is really broken!",
+                    "[5/5] test/5.stamp",
+                ],
+            ]
+        )
+        self.assertEqual(len(failures), 1)
+        self.assertEqual(
+            failures[0],
+            (
+                "test/4.stamp",
+                dedent(
+                    """\
+                    FAILED: touch test/4.stamp
+                    Wow! This system is really broken!"""
+                ),
+            ),
+        )
+
+    def test_no_failure_ninja_log(self):
+        failures = generate_test_report_lib.find_failure_in_ninja_logs(
+            [
+                [
+                    "[1/3] test/1.stamp",
+                    "[2/3] test/2.stamp",
+                    "[3/3] test/3.stamp",
+                ]
+            ]
+        )
+        self.assertEqual(failures, [])
+
+    def test_ninja_log_end(self):
+        failures = generate_test_report_lib.find_failure_in_ninja_logs(
+            [
+                [
+                    "[1/3] test/1.stamp",
+                    "[2/3] test/2.stamp",
+                    "[3/3] test/3.stamp",
+                    "FAILED: touch test/3.stamp",
+                    "Wow! This system is really broken!",
+                    "ninja: build stopped: subcommand failed.",
+                ]
+            ]
+        )
+        self.assertEqual(len(failures), 1)
+        self.assertEqual(
+            failures[0],
+            (
+                "test/3.stamp",
+                dedent(
+                    """\
+                    FAILED: touch test/3.stamp
+                    Wow! This system is really broken!"""
+                ),
+            ),
+        )
+
+    def test_ninja_log_multiple_failures(self):
+        failures = generate_test_report_lib.find_failure_in_ninja_logs(
+            [
+                [
+                    "[1/5] test/1.stamp",
+                    "[2/5] test/2.stamp",
+                    "FAILED: touch test/2.stamp",
+                    "Wow! This system is really broken!",
+                    "[3/5] test/3.stamp",
+                    "[4/5] test/4.stamp",
+                    "FAILED: touch test/4.stamp",
+                    "Wow! This system is maybe broken!",
+                    "[5/5] test/5.stamp",
+                ]
+            ]
+        )
+        self.assertEqual(len(failures), 2)
+        self.assertEqual(
+            failures[0],
+            (
+                "test/2.stamp",
+                dedent(
+                    """\
+                    FAILED: touch test/2.stamp
+                    Wow! This system is really broken!"""
+                ),
+            ),
+        )
+        self.assertEqual(
+            failures[1],
+            (
+                "test/4.stamp",
+                dedent(
+                    """\
+                    FAILED: touch test/4.stamp
+                    Wow! This system is maybe broken!"""
+                ),
+            ),
+        )
+
     def test_title_only(self):
         self.assertEqual(
             generate_test_report_lib.generate_report("Foo", 0, []),

.github/workflows/premerge.yaml

@@ -35,9 +35,6 @@ jobs:
         with:
           fetch-depth: 2
       - name: Build and Test
-        # Mark the job as a success even if the step fails so that people do
-        # not get notified while the new premerge pipeline is in an
-        # experimental state.
         run: |
           git config --global --add safe.directory '*'
 
@@ -109,9 +106,6 @@ jobs:
           echo "windows-projects=${projects_to_build}" >> $GITHUB_OUTPUT
           echo "windows-check-targets=${project_check_targets}" >> $GITHUB_OUTPUT
       - name: Build and Test
-        # Mark the job as a success even if the step fails so that people do
-        # not get notified while the new premerge pipeline is in an
-        # experimental state.
         if: ${{ steps.vars.outputs.windows-projects != '' }}
         shell: cmd
         run: |

bolt/include/bolt/Profile/DataAggregator.h

@@ -502,9 +502,6 @@ class DataAggregator : public DataReader {
   /// entries).
   void imputeFallThroughs();
 
-  /// Register profiled functions for lite mode.
-  void registerProfiledFunctions();
-
   /// Debugging dump methods
   void dump() const;
   void dump(const PerfBranchSample &Sample) const;

bolt/lib/Passes/FrameOptimizer.cpp

@@ -224,6 +224,11 @@ Error FrameOptimizerPass::runOnFunctions(BinaryContext &BC) {
   if (opts::FrameOptimization == FOP_NONE)
     return Error::success();
 
+  if (!BC.isX86()) {
+    BC.errs() << "BOLT-ERROR: " << getName() << " is supported only on X86\n";
+    exit(1);
+  }
+
   std::unique_ptr<BinaryFunctionCallGraph> CG;
   std::unique_ptr<FrameAnalysis> FA;
   std::unique_ptr<RegAnalysis> RA;

bolt/lib/Profile/DataAggregator.cpp

@@ -581,26 +581,6 @@ void DataAggregator::imputeFallThroughs() {
     outs() << "BOLT-INFO: imputed " << InferredTraces << " traces\n";
 }
 
-void DataAggregator::registerProfiledFunctions() {
-  DenseSet<uint64_t> Addrs;
-  for (const auto &Trace : llvm::make_first_range(Traces)) {
-    if (Trace.Branch != Trace::FT_ONLY &&
-        Trace.Branch != Trace::FT_EXTERNAL_ORIGIN)
-      Addrs.insert(Trace.Branch);
-    Addrs.insert(Trace.From);
-  }
-
-  for (const auto [PC, _] : BasicSamples)
-    Addrs.insert(PC);
-
-  for (const PerfMemSample &MemSample : MemSamples)
-    Addrs.insert(MemSample.PC);
-
-  for (const uint64_t Addr : Addrs)
-    if (BinaryFunction *Func = getBinaryFunctionContainingAddress(Addr))
-      Func->setHasProfileAvailable();
-}
-
 Error DataAggregator::preprocessProfile(BinaryContext &BC) {
   this->BC = &BC;
 
@@ -623,7 +603,6 @@ Error DataAggregator::preprocessProfile(BinaryContext &BC) {
       exit(0);
   }
 
-  registerProfiledFunctions();
   return Error::success();
 }
 
@@ -1368,6 +1347,10 @@ std::error_code DataAggregator::parseAggregatedLBREntry() {
   }
 
   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];
 
@@ -1378,6 +1361,11 @@ std::error_code DataAggregator::parseAggregatedLBREntry() {
     return std::error_code();
   }
 
+  const uint64_t ToOffset = Addr[1]->Offset;
+  BinaryFunction *ToFunc = getBinaryFunctionContainingAddress(ToOffset);
+  if (ToFunc)
+    ToFunc->setHasProfileAvailable();
+
   /// For fall-through types, adjust locations to match Trace container.
   if (Type == FT || Type == FT_EXTERNAL_ORIGIN || Type == FT_EXTERNAL_RETURN) {
     Addr[2] = Location(Addr[1]->Offset); // Trace To
@@ -1625,6 +1613,9 @@ std::error_code DataAggregator::parseBranchEvents() {
   Traces.reserve(TraceMap.size());
   for (const auto &[Trace, Info] : TraceMap) {
     Traces.emplace_back(Trace, Info);
+    for (const uint64_t Addr : {Trace.Branch, Trace.From})
+      if (BinaryFunction *BF = getBinaryFunctionContainingAddress(Addr))
+        BF->setHasProfileAvailable();
   }
   clear(TraceMap);
 
@@ -1685,6 +1676,9 @@ std::error_code DataAggregator::parseBasicEvents() {
       continue;
     ++NumTotalSamples;
 
+    if (BinaryFunction *BF = getBinaryFunctionContainingAddress(Sample->PC))
+      BF->setHasProfileAvailable();
+
     ++BasicSamples[Sample->PC];
     EventNames.insert(Sample->EventName);
   }
@@ -1722,6 +1716,9 @@ std::error_code DataAggregator::parseMemEvents() {
     if (std::error_code EC = Sample.getError())
       return EC;
 
+    if (BinaryFunction *BF = getBinaryFunctionContainingAddress(Sample->PC))
+      BF->setHasProfileAvailable();
+
     MemSamples.emplace_back(std::move(Sample.get()));
   }
 

bolt/test/AArch64/unsupported-passes.test

@@ -0,0 +1,8 @@
+// Checks that non-fully supported passes on AArch64 are handled appropriately.
+
+// REQUIRES: system-linux,asserts,target=aarch64{{.*}}
+
+RUN: %clang %cflags %p/../Inputs/hello.c -o %t -Wl,-q
+RUN: not llvm-bolt %t -o %t.bolt --frame-opt=all 2>&1 | FileCheck %s
+
+CHECK: BOLT-ERROR: frame-optimizer is supported only on X86

clang-tools-extra/clang-tidy/bugprone/NarrowingConversionsCheck.cpp

@@ -555,15 +555,22 @@ bool NarrowingConversionsCheck::handleConditionalOperator(
     // We have an expression like so: `output = cond ? lhs : rhs`
     // From the point of view of narrowing conversion we treat it as two
     // expressions `output = lhs` and `output = rhs`.
-    handleBinaryOperator(Context, CO->getLHS()->getExprLoc(), Lhs,
-                         *CO->getLHS());
-    handleBinaryOperator(Context, CO->getRHS()->getExprLoc(), Lhs,
-                         *CO->getRHS());
+    handleConditionalOperatorArgument(Context, Lhs, CO->getLHS());
+    handleConditionalOperatorArgument(Context, Lhs, CO->getRHS());
     return true;
   }
   return false;
 }
 
+void NarrowingConversionsCheck::handleConditionalOperatorArgument(
+    const ASTContext &Context, const Expr &Lhs, const Expr *Arg) {
+  if (const auto *ICE = llvm::dyn_cast<ImplicitCastExpr>(Arg))
+    if (!Arg->getIntegerConstantExpr(Context))
+      Arg = ICE->getSubExpr();
+
+  handleBinaryOperator(Context, Arg->getExprLoc(), Lhs, *Arg);
+}
+
 void NarrowingConversionsCheck::handleImplicitCast(
     const ASTContext &Context, const ImplicitCastExpr &Cast) {
   if (Cast.getExprLoc().isMacroID())

clang-tools-extra/clang-tidy/bugprone/NarrowingConversionsCheck.h

@@ -85,6 +85,8 @@ class NarrowingConversionsCheck : public ClangTidyCheck {
   bool handleConditionalOperator(const ASTContext &Context, const Expr &Lhs,
                                  const Expr &Rhs);
 
+  void handleConditionalOperatorArgument(const ASTContext &Context,
+                                         const Expr &Lhs, const Expr *Arg);
   void handleImplicitCast(const ASTContext &Context,
                           const ImplicitCastExpr &Cast);
 

clang-tools-extra/clangd/CMakeLists.txt

@@ -108,6 +108,7 @@ add_clang_library(clangDaemon STATIC
   SemanticHighlighting.cpp
   SemanticSelection.cpp
   SourceCode.cpp
+  SymbolDocumentation.cpp
   SystemIncludeExtractor.cpp
   TidyProvider.cpp
   TUScheduler.cpp