jtstogel
diff --git a/‎.ci/generate_test_report_lib.py
Lines changed: 14 additions & 1 deletion b/‎.ci/generate_test_report_lib.py
Lines changed: 14 additions & 1 deletion
diff --git a/‎.ci/generate_test_report_lib_test.py
Lines changed: 24 additions & 9 deletions b/‎.ci/generate_test_report_lib_test.py
Lines changed: 24 additions & 9 deletions
diff --git a/‎.ci/metrics/metrics.py
Lines changed: 3 additions & 0 deletions b/‎.ci/metrics/metrics.py
Lines changed: 3 additions & 0 deletions
diff --git a/‎bolt/CMakeLists.txt
Lines changed: 4 additions & 0 deletions b/‎bolt/CMakeLists.txt
Lines changed: 4 additions & 0 deletions
diff --git a/‎bolt/include/bolt/Passes/PAuthGadgetScanner.h
Lines changed: 109 additions & 40 deletions b/‎bolt/include/bolt/Passes/PAuthGadgetScanner.h
Lines changed: 109 additions & 40 deletions
diff --git a/‎bolt/lib/Core/BinaryContext.cpp
Lines changed: 1 addition & 1 deletion b/‎bolt/lib/Core/BinaryContext.cpp
Lines changed: 1 addition & 1 deletion
diff --git a/‎bolt/lib/Core/BinaryFunction.cpp
Lines changed: 1 addition & 6 deletions b/‎bolt/lib/Core/BinaryFunction.cpp
Lines changed: 1 addition & 6 deletions
@@ -92,7 +92,9 @@ def plural(num_tests):
             ]
         )
     elif failures:
-        report.extend(["", "## Failed Tests", "(click on a test name to see its output)"])
+        report.extend(
+            ["", "## Failed Tests", "(click on a test name to see its output)"]
+        )
 
         for testsuite_name, failures in failures.items():
             report.extend(["", f"### {testsuite_name}"])
@@ -120,6 +122,17 @@ def plural(num_tests):
             ]
         )
 
+    if failures or return_code != 0:
+        report.extend(
+            [
+                "",
+                "If these failures are unrelated to your changes (for example "
+                "tests are broken or flaky at HEAD), please open an issue at "
+                "https://github.com/llvm/llvm-project/issues and add the "
+                "`infrastructure` label.",
+            ]
+        )
+
     report = "\n".join(report)
     if len(report.encode("utf-8")) > size_limit:
         return generate_report(
 
@@ -109,7 +109,9 @@ def test_no_failures_build_failed(self):
 
               All tests passed but another part of the build **failed**.
 
-              [Download](https://buildkite.com/organizations/organization_slug/pipelines/pipeline_slug/builds/build_number/jobs/job_id/download.txt) the build's log file to see the details."""
+              [Download](https://buildkite.com/organizations/organization_slug/pipelines/pipeline_slug/builds/build_number/jobs/job_id/download.txt) the build's log file to see the details.
+              
+              If these failures are unrelated to your changes (for example tests are broken or flaky at HEAD), please open an issue at https://github.com/llvm/llvm-project/issues and add the `infrastructure` label."""
                 ),
                 "error",
             ),
@@ -169,7 +171,9 @@ def test_report_single_file_single_testsuite(self):
           ```
           Other output goes here
           ```
-          </details>"""
+          </details>
+          
+          If these failures are unrelated to your changes (for example tests are broken or flaky at HEAD), please open an issue at https://github.com/llvm/llvm-project/issues and add the `infrastructure` label."""
                 ),
                 "error",
             ),
@@ -203,7 +207,9 @@ def test_report_single_file_single_testsuite(self):
         ```
         DEF/test_2 output goes here
         ```
-        </details>"""
+        </details>
+        
+        If these failures are unrelated to your changes (for example tests are broken or flaky at HEAD), please open an issue at https://github.com/llvm/llvm-project/issues and add the `infrastructure` label."""
         ),
         "error",
     )
@@ -311,7 +317,9 @@ def test_report_dont_list_failures(self):
 
           * 1 test failed
 
-          Failed tests and their output was too large to report. Download the build's log file to see the details."""
+          Failed tests and their output was too large to report. Download the build's log file to see the details.
+          
+          If these failures are unrelated to your changes (for example tests are broken or flaky at HEAD), please open an issue at https://github.com/llvm/llvm-project/issues and add the `infrastructure` label."""
                 ),
                 "error",
             ),
@@ -352,13 +360,16 @@ def test_report_dont_list_failures_link_to_log(self):
 
           * 1 test failed
 
-          Failed tests and their output was too large to report. [Download](https://buildkite.com/organizations/organization_slug/pipelines/pipeline_slug/builds/build_number/jobs/job_id/download.txt) the build's log file to see the details."""
+          Failed tests and their output was too large to report. [Download](https://buildkite.com/organizations/organization_slug/pipelines/pipeline_slug/builds/build_number/jobs/job_id/download.txt) the build's log file to see the details.
+          
+          If these failures are unrelated to your changes (for example tests are broken or flaky at HEAD), please open an issue at https://github.com/llvm/llvm-project/issues and add the `infrastructure` label."""
                 ),
                 "error",
             ),
         )
 
     def test_report_size_limit(self):
+        test_output = "f" * 1000
         self.assertEqual(
             generate_test_report_lib.generate_report(
                 "Foo",
@@ -371,14 +382,16 @@ def test_report_size_limit(self):
           <testsuites time="0.02">
           <testsuite name="Bar" tests="1" failures="1" skipped="0" time="0.02">
           <testcase classname="Bar/test_1" name="test_1" time="0.02">
-            <failure><![CDATA[Some long output goes here...]]></failure>
+            <failure><![CDATA[{output}]]></failure>
           </testcase>
           </testsuite>
-          </testsuites>"""
+          </testsuites>""".format(
+                                output=test_output
+                            )
                         )
                     )
                 ],
-                size_limit=128,
+                size_limit=512,
             ),
             (
                 dedent(
@@ -387,7 +400,9 @@ def test_report_size_limit(self):
 
           * 1 test failed
 
-          Failed tests and their output was too large to report. Download the build's log file to see the details."""
+          Failed tests and their output was too large to report. Download the build's log file to see the details.
+          
+          If these failures are unrelated to your changes (for example tests are broken or flaky at HEAD), please open an issue at https://github.com/llvm/llvm-project/issues and add the `infrastructure` label."""
                 ),
                 "error",
             ),
 
@@ -67,6 +67,7 @@
 # remain small.
 BUILDKITE_GRAPHQL_BUILDS_PER_PAGE = 50
 
+
 @dataclass
 class JobMetrics:
     job_name: str
@@ -77,6 +78,7 @@ class JobMetrics:
     workflow_id: int
     workflow_name: str
 
+
 @dataclass
 class GaugeMetric:
     name: str
@@ -258,6 +260,7 @@ def buildkite_get_metrics(
 
     return output, incomplete_now
 
+
 def github_get_metrics(
     github_repo: github.Repository, last_workflows_seen_as_completed: set[int]
 ) -> tuple[list[JobMetrics], int]:
 
@@ -46,6 +46,10 @@ if(BOLT_BUILT_STANDALONE)
   set(LLVM_RUNTIME_OUTPUT_INTDIR ${CMAKE_BINARY_DIR}/${CMAKE_CFG_INTDIR}/bin)
   set(LLVM_LIBRARY_OUTPUT_INTDIR ${CMAKE_BINARY_DIR}/${CMAKE_CFG_INTDIR}/lib${LLVM_LIBDIR_SUFFIX})
 
+  separate_arguments(LLVM_DEFINITIONS_LIST NATIVE_COMMAND ${LLVM_DEFINITIONS})
+  add_definitions(${LLVM_DEFINITIONS_LIST})
+  list(APPEND CMAKE_REQUIRED_DEFINITIONS ${LLVM_DEFINITIONS_LIST})
+
   include(AddLLVM)
   include(TableGen)
   include_directories(${LLVM_INCLUDE_DIRS})
 
@@ -196,73 +196,148 @@ raw_ostream &operator<<(raw_ostream &OS, const MCInstReference &);
 
 namespace PAuthGadgetScanner {
 
+// The report classes are designed to be used in an immutable manner.
+// When an issue report is constructed in multiple steps, an attempt is made
+// to distinguish intermediate and final results at the type level.
+//
+// Here is an overview of issue life-cycle:
+// * an analysis (SrcSafetyAnalysis at now, DstSafetyAnalysis will be added
+//   later to support the detection of authentication oracles) computes register
+//   state for each instruction in the function.
+// * for each instruction, it is checked whether it is a gadget of some kind,
+//   taking the computed state into account. If a gadget is found, its kind
+//   and location are stored into a subclass of Diagnostic wrapped into
+//   PartialReport<ReqT>.
+// * if any issue is to be reported for the function, the same analysis is
+//   re-run to collect extra information to provide to the user. Which extra
+//   information can be requested depends on the particular analysis (for
+//   example, SrcSafetyAnalysis is able to compute the set of instructions
+//   clobbering the particular register, thus ReqT is MCPhysReg). At this stage,
+//   `FinalReport`s are created.
+//
+// Here, the subclasses of Diagnostic store the pieces of information which
+// are kept unchanged since they are collected on the first run of the analysis.
+// PartialReport<T>::RequestedDetails, on the other hand, is replaced with
+// FinalReport::Details computed by the second run of the analysis.
+
 /// Description of a gadget kind that can be detected. Intended to be
-/// statically allocated to be attached to reports by reference.
+/// statically allocated and attached to reports by reference.
 class GadgetKind {
   const char *Description;
 
 public:
+  /// Wraps a description string which must be a string literal.
   GadgetKind(const char *Description) : Description(Description) {}
 
   StringRef getDescription() const { return Description; }
 };
 
-/// Base report located at some instruction, without any additional information.
-struct Report {
+/// Basic diagnostic information, which is kept unchanged since it is collected
+/// on the first run of the analysis.
+struct Diagnostic {
   MCInstReference Location;
 
-  Report(MCInstReference Location) : Location(Location) {}
-  virtual ~Report() {}
+  Diagnostic(MCInstReference Location) : Location(Location) {}
+  virtual ~Diagnostic() {}
 
   virtual void generateReport(raw_ostream &OS,
                               const BinaryContext &BC) const = 0;
 
-  // The two methods below are called by Analysis::computeDetailedInfo when
-  // iterating over the reports.
-  virtual ArrayRef<MCPhysReg> getAffectedRegisters() const { return {}; }
-  virtual void setOverwritingInstrs(ArrayRef<MCInstReference> Instrs) {}
-
   void printBasicInfo(raw_ostream &OS, const BinaryContext &BC,
                       StringRef IssueKind) const;
 };
 
-struct GadgetReport : public Report {
+struct GadgetDiagnostic : public Diagnostic {
   // The particular kind of gadget that is detected.
   const GadgetKind &Kind;
-  // The set of registers related to this gadget report (possibly empty).
-  SmallVector<MCPhysReg, 1> AffectedRegisters;
-  // The instructions that clobber the affected registers.
-  // There is no one-to-one correspondence with AffectedRegisters: for example,
-  // the same register can be overwritten by different instructions in different
-  // preceding basic blocks.
-  SmallVector<MCInstReference> OverwritingInstrs;
-
-  GadgetReport(const GadgetKind &Kind, MCInstReference Location,
-               MCPhysReg AffectedRegister)
-      : Report(Location), Kind(Kind), AffectedRegisters({AffectedRegister}) {}
 
-  void generateReport(raw_ostream &OS, const BinaryContext &BC) const override;
+  GadgetDiagnostic(const GadgetKind &Kind, MCInstReference Location)
+      : Diagnostic(Location), Kind(Kind) {}
 
-  ArrayRef<MCPhysReg> getAffectedRegisters() const override {
-    return AffectedRegisters;
-  }
-
-  void setOverwritingInstrs(ArrayRef<MCInstReference> Instrs) override {
-    OverwritingInstrs.assign(Instrs.begin(), Instrs.end());
-  }
+  void generateReport(raw_ostream &OS, const BinaryContext &BC) const override;
 };
 
 /// Report with a free-form message attached.
-struct GenericReport : public Report {
+struct GenericDiagnostic : public Diagnostic {
   std::string Text;
-  GenericReport(MCInstReference Location, StringRef Text)
-      : Report(Location), Text(Text) {}
+  GenericDiagnostic(MCInstReference Location, StringRef Text)
+      : Diagnostic(Location), Text(Text) {}
   virtual void generateReport(raw_ostream &OS,
                               const BinaryContext &BC) const override;
 };
 
+/// Extra information about an issue collected on the slower, detailed,
+/// run of the analysis.
+class ExtraInfo {
+public:
+  virtual void print(raw_ostream &OS, const MCInstReference Location) const = 0;
+
+  virtual ~ExtraInfo() {}
+};
+
+class ClobberingInfo : public ExtraInfo {
+  SmallVector<MCInstReference> ClobberingInstrs;
+
+public:
+  ClobberingInfo(ArrayRef<MCInstReference> Instrs) : ClobberingInstrs(Instrs) {}
+
+  void print(raw_ostream &OS, const MCInstReference Location) const override;
+};
+
+/// A brief version of a report that can be further augmented with the details.
+///
+/// A half-baked report produced on the first run of the analysis. An extra,
+/// analysis-specific information may be requested to be collected on the
+/// second run.
+template <typename T> struct PartialReport {
+  PartialReport(std::shared_ptr<Diagnostic> Issue,
+                const std::optional<T> RequestedDetails)
+      : Issue(Issue), RequestedDetails(RequestedDetails) {}
+
+  std::shared_ptr<Diagnostic> Issue;
+  std::optional<T> RequestedDetails;
+};
+
+/// A final version of the report.
+struct FinalReport {
+  FinalReport(std::shared_ptr<Diagnostic> Issue,
+              std::shared_ptr<ExtraInfo> Details)
+      : Issue(Issue), Details(Details) {}
+
+  std::shared_ptr<Diagnostic> Issue;
+  std::shared_ptr<ExtraInfo> Details;
+};
+
 struct FunctionAnalysisResult {
-  std::vector<std::shared_ptr<Report>> Diagnostics;
+  std::vector<FinalReport> Diagnostics;
+};
+
+/// A helper class storing per-function context to be instantiated by Analysis.
+class FunctionAnalysisContext {
+  BinaryContext &BC;
+  BinaryFunction &BF;
+  MCPlusBuilder::AllocatorIdTy AllocatorId;
+  FunctionAnalysisResult Result;
+
+  bool PacRetGadgetsOnly;
+
+  void findUnsafeUses(SmallVector<PartialReport<MCPhysReg>> &Reports);
+  void augmentUnsafeUseReports(ArrayRef<PartialReport<MCPhysReg>> Reports);
+
+  /// Process the reports which do not have to be augmented, and remove them
+  /// from Reports.
+  void handleSimpleReports(SmallVector<PartialReport<MCPhysReg>> &Reports);
+
+public:
+  FunctionAnalysisContext(BinaryFunction &BF,
+                          MCPlusBuilder::AllocatorIdTy AllocatorId,
+                          bool PacRetGadgetsOnly)
+      : BC(BF.getBinaryContext()), BF(BF), AllocatorId(AllocatorId),
+        PacRetGadgetsOnly(PacRetGadgetsOnly) {}
+
+  void run();
+
+  const FunctionAnalysisResult &getResult() const { return Result; }
 };
 
 class Analysis : public BinaryFunctionPass {
@@ -271,12 +346,6 @@ class Analysis : public BinaryFunctionPass {
 
   void runOnFunction(BinaryFunction &Function,
                      MCPlusBuilder::AllocatorIdTy AllocatorId);
-  FunctionAnalysisResult findGadgets(BinaryFunction &BF,
-                                     MCPlusBuilder::AllocatorIdTy AllocatorId);
-
-  void computeDetailedInfo(BinaryFunction &BF,
-                           MCPlusBuilder::AllocatorIdTy AllocatorId,
-                           FunctionAnalysisResult &Result);
 
   std::map<const BinaryFunction *, FunctionAnalysisResult> AnalysisResults;
   std::mutex AnalysisResultsMutex;
 
@@ -201,7 +201,7 @@ Expected<std::unique_ptr<BinaryContext>> BinaryContext::createBinaryContext(
 
   std::string Error;
   const Target *TheTarget =
-      TargetRegistry::lookupTarget(std::string(ArchName), TheTriple, Error);
+      TargetRegistry::lookupTarget(ArchName, TheTriple, Error);
   if (!TheTarget)
     return createStringError(make_error_code(std::errc::not_supported),
                              Twine("BOLT-ERROR: ", Error));
 
@@ -166,12 +166,7 @@ bool shouldPrint(const BinaryFunction &Function) {
   }
 
   std::optional<StringRef> Origin = Function.getOriginSectionName();
-  if (Origin && llvm::any_of(opts::PrintOnly, [&](const std::string &Name) {
-        return Name == *Origin;
-      }))
-    return true;
-
-  return false;
+  return Origin && llvm::is_contained(opts::PrintOnly, *Origin);
 }
 
 } // namespace opts
Original file line number	Diff line number	Diff line change
`@@ -166,12 +166,7 @@ bool shouldPrint(const BinaryFunction &Function) {`
`166`	`166`	`}`
`167`	`167`
`168`	`168`	`std::optional<StringRef> Origin = Function.getOriginSectionName();`
`169`		`- if (Origin && llvm::any_of(opts::PrintOnly, [&](const std::string &Name) {`
`170`		`- return Name == *Origin;`
`171`		`- }))`
`172`		`- return true;`
`173`		`-`
`174`		`- return false;`
	`169`	`+ return Origin && llvm::is_contained(opts::PrintOnly, *Origin);`
`175`	`170`	`}`
`176`	`171`
`177`	`172`	`} // namespace opts`