Skip to content

[BOLT] Gadget scanner: streamline issue reporting #131896

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

atrosinenko
Copy link
Contributor

In preparation for adding more gadget kinds to detect, streamline
issue reporting.

Rename classes representing issue reports. In particular, rename
Annotation base class to Report, as it has nothing to do with
"annotations" in MCPlus terms anymore. Remove references to "return
instructions" from variable names and report messages, use generic
terms instead. Rename NonPacProtectedRetAnalysis to PAuthGadgetScanner.

Remove GeneralDiagnostic as a separate class, make GenericReport
(former GenDiag) store std::string Text directly. Remove unused
operator= and operator== methods, as Reports are created on the
heap and referenced via shared_ptrs.

Introduce GadgetKind class - currently, it only wraps a const char *
description to display to the user. This description is intended to be
a per-gadget-kind constant (or a few hard-coded constants), so no need
to store it to std::string field in each report instance. To handle
both free-form GenericReports and statically-allocated messages
without unnecessary overhead, move printing of the report header to the
base class (and take the message argument as a StringRef).

@llvmbot
Copy link
Member

llvmbot commented Mar 18, 2025

@llvm/pr-subscribers-bolt

Author: Anatoly Trosinenko (atrosinenko)

Changes

In preparation for adding more gadget kinds to detect, streamline
issue reporting.

Rename classes representing issue reports. In particular, rename
Annotation base class to Report, as it has nothing to do with
"annotations" in MCPlus terms anymore. Remove references to "return
instructions" from variable names and report messages, use generic
terms instead. Rename NonPacProtectedRetAnalysis to PAuthGadgetScanner.

Remove GeneralDiagnostic as a separate class, make GenericReport
(former GenDiag) store std::string Text directly. Remove unused
operator= and operator== methods, as Reports are created on the
heap and referenced via shared_ptrs.

Introduce GadgetKind class - currently, it only wraps a const char *
description to display to the user. This description is intended to be
a per-gadget-kind constant (or a few hard-coded constants), so no need
to store it to std::string field in each report instance. To handle
both free-form GenericReports and statically-allocated messages
without unnecessary overhead, move printing of the report header to the
base class (and take the message argument as a StringRef).


Patch is 35.14 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/131896.diff

6 Files Affected:

  • (renamed) bolt/include/bolt/Passes/PAuthGadgetScanner.h (+43-47)
  • (modified) bolt/lib/Passes/CMakeLists.txt (+1-1)
  • (renamed) bolt/lib/Passes/PAuthGadgetScanner.cpp (+43-36)
  • (modified) bolt/lib/Rewrite/RewriteInstance.cpp (+2-3)
  • (modified) bolt/test/binary-analysis/AArch64/gs-pacret-autiasp.s (+66-66)
  • (modified) bolt/test/binary-analysis/AArch64/gs-pacret-multi-bb.s (+6-6)
diff --git a/bolt/include/bolt/Passes/NonPacProtectedRetAnalysis.h b/bolt/include/bolt/Passes/PAuthGadgetScanner.h
similarity index 77%
rename from bolt/include/bolt/Passes/NonPacProtectedRetAnalysis.h
rename to bolt/include/bolt/Passes/PAuthGadgetScanner.h
index 3fde41d7858b7..2d8109f8ca43b 100644
--- a/bolt/include/bolt/Passes/NonPacProtectedRetAnalysis.h
+++ b/bolt/include/bolt/Passes/PAuthGadgetScanner.h
@@ -1,4 +1,4 @@
-//===- bolt/Passes/NonPacProtectedRetAnalysis.h -----------------*- C++ -*-===//
+//===- bolt/Passes/PAuthGadgetScanner.h -------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
@@ -6,8 +6,8 @@
 //
 //===----------------------------------------------------------------------===//
 
-#ifndef BOLT_PASSES_NONPACPROTECTEDRETANALYSIS_H
-#define BOLT_PASSES_NONPACPROTECTEDRETANALYSIS_H
+#ifndef BOLT_PASSES_PAUTHGADGETSCANNER_H
+#define BOLT_PASSES_PAUTHGADGETSCANNER_H
 
 #include "bolt/Core/BinaryContext.h"
 #include "bolt/Core/BinaryFunction.h"
@@ -173,63 +173,59 @@ struct MCInstReference {
 
 raw_ostream &operator<<(raw_ostream &OS, const MCInstReference &);
 
-struct GeneralDiagnostic {
-  std::string Text;
-  GeneralDiagnostic(const std::string &Text) : Text(Text) {}
-  bool operator==(const GeneralDiagnostic &RHS) const {
-    return Text == RHS.Text;
-  }
+namespace PAuthGadgetScanner {
+
+class PacRetAnalysis;
+struct State;
+
+/// Description of a gadget kind that can be detected. Intended to be
+/// statically allocated to be attached to reports by reference.
+class GadgetKind {
+  const char *Description;
+
+public:
+  GadgetKind(const char *Description) : Description(Description) {}
+
+  const StringRef getDescription() const { return Description; }
 };
 
-raw_ostream &operator<<(raw_ostream &OS, const GeneralDiagnostic &Diag);
+/// Base report located at some instruction, without any additional information.
+struct Report {
+  MCInstReference Location;
+
+  Report(MCInstReference Location) : Location(Location) {}
+  virtual ~Report() {}
 
-namespace NonPacProtectedRetAnalysis {
-struct Annotation {
-  MCInstReference RetInst;
-  Annotation(MCInstReference RetInst) : RetInst(RetInst) {}
-  virtual bool operator==(const Annotation &RHS) const {
-    return RetInst == RHS.RetInst;
-  }
-  Annotation &operator=(const Annotation &Other) {
-    if (this == &Other)
-      return *this;
-    RetInst = Other.RetInst;
-    return *this;
-  }
-  virtual ~Annotation() {}
   virtual void generateReport(raw_ostream &OS,
                               const BinaryContext &BC) const = 0;
+
+  void printBasicInfo(raw_ostream &OS, const BinaryContext &BC,
+                      StringRef IssueKind) const;
 };
 
-struct Gadget : public Annotation {
-  std::vector<MCInstReference> OverwritingRetRegInst;
-  virtual bool operator==(const Gadget &RHS) const {
-    return Annotation::operator==(RHS) &&
-           OverwritingRetRegInst == RHS.OverwritingRetRegInst;
-  }
-  Gadget(MCInstReference RetInst,
-         const std::vector<MCInstReference> &OverwritingRetRegInst)
-      : Annotation(RetInst), OverwritingRetRegInst(OverwritingRetRegInst) {}
-  virtual void generateReport(raw_ostream &OS,
-                              const BinaryContext &BC) const override;
+struct GadgetReport : public Report {
+  const GadgetKind &Kind;
+  std::vector<MCInstReference> OverwritingInstrs;
+
+  GadgetReport(const GadgetKind &Kind, MCInstReference Location,
+               std::vector<MCInstReference> OverwritingInstrs)
+      : Report(Location), Kind(Kind), OverwritingInstrs(OverwritingInstrs) {}
+
+  void generateReport(raw_ostream &OS, const BinaryContext &BC) const override;
 };
 
-struct GenDiag : public Annotation {
-  GeneralDiagnostic Diag;
-  virtual bool operator==(const GenDiag &RHS) const {
-    return Annotation::operator==(RHS) && Diag == RHS.Diag;
-  }
-  GenDiag(MCInstReference RetInst, const std::string &Text)
-      : Annotation(RetInst), Diag(Text) {}
+/// Report with a free-form message attached.
+struct GenericReport : public Report {
+  std::string Text;
+  GenericReport(MCInstReference Location, const std::string &Text)
+      : Report(Location), Text(Text) {}
   virtual void generateReport(raw_ostream &OS,
                               const BinaryContext &BC) const override;
 };
 
-class PacRetAnalysis;
-
 struct FunctionAnalysisResult {
   SmallSet<MCPhysReg, 1> RegistersAffected;
-  std::vector<std::shared_ptr<Annotation>> Diagnostics;
+  std::vector<std::shared_ptr<Report>> Diagnostics;
 };
 
 class Analysis : public BinaryFunctionPass {
@@ -245,13 +241,13 @@ class Analysis : public BinaryFunctionPass {
 public:
   explicit Analysis() : BinaryFunctionPass(false) {}
 
-  const char *getName() const override { return "non-pac-protected-rets"; }
+  const char *getName() const override { return "pauth-gadget-scanner"; }
 
   /// Pass entry point
   Error runOnFunctions(BinaryContext &BC) override;
 };
 
-} // namespace NonPacProtectedRetAnalysis
+} // namespace PAuthGadgetScanner
 } // namespace bolt
 } // namespace llvm
 
diff --git a/bolt/lib/Passes/CMakeLists.txt b/bolt/lib/Passes/CMakeLists.txt
index 3864255a09ebe..77d2bb9c2bcb5 100644
--- a/bolt/lib/Passes/CMakeLists.txt
+++ b/bolt/lib/Passes/CMakeLists.txt
@@ -23,8 +23,8 @@ add_llvm_library(LLVMBOLTPasses
   LoopInversionPass.cpp
   LivenessAnalysis.cpp
   MCF.cpp
-  NonPacProtectedRetAnalysis.cpp
   PatchEntries.cpp
+  PAuthGadgetScanner.cpp
   PettisAndHansen.cpp
   PLTCall.cpp
   ProfileQualityStats.cpp
diff --git a/bolt/lib/Passes/NonPacProtectedRetAnalysis.cpp b/bolt/lib/Passes/PAuthGadgetScanner.cpp
similarity index 90%
rename from bolt/lib/Passes/NonPacProtectedRetAnalysis.cpp
rename to bolt/lib/Passes/PAuthGadgetScanner.cpp
index 77a16379a14b9..813fa43c85dc5 100644
--- a/bolt/lib/Passes/NonPacProtectedRetAnalysis.cpp
+++ b/bolt/lib/Passes/PAuthGadgetScanner.cpp
@@ -1,4 +1,4 @@
-//===- bolt/Passes/NonPacProtectedRetAnalysis.cpp -------------------------===//
+//===- bolt/Passes/PAuthGadgetScanner.cpp ---------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
@@ -11,7 +11,7 @@
 //
 //===----------------------------------------------------------------------===//
 
-#include "bolt/Passes/NonPacProtectedRetAnalysis.h"
+#include "bolt/Passes/PAuthGadgetScanner.h"
 #include "bolt/Core/ParallelUtilities.h"
 #include "bolt/Passes/DataflowAnalysis.h"
 #include "llvm/ADT/STLExtras.h"
@@ -20,7 +20,7 @@
 #include "llvm/Support/Format.h"
 #include <memory>
 
-#define DEBUG_TYPE "bolt-nonpacprotectedret"
+#define DEBUG_TYPE "bolt-pauth-scanner"
 
 namespace llvm {
 namespace bolt {
@@ -57,7 +57,7 @@ raw_ostream &operator<<(raw_ostream &OS, const MCInstReference &Ref) {
   llvm_unreachable("");
 }
 
-namespace NonPacProtectedRetAnalysis {
+namespace PAuthGadgetScanner {
 
 static void traceInst(const BinaryContext &BC, StringRef Label,
                       const MCInst &MI) {
@@ -395,7 +395,7 @@ Analysis::computeDfState(PacRetAnalysis &PRA, BinaryFunction &BF,
       if (BC.MIB->isReturn(Inst)) {
         ErrorOr<MCPhysReg> MaybeRetReg = BC.MIB->getRegUsedAsRetDest(Inst);
         if (MaybeRetReg.getError()) {
-          Result.Diagnostics.push_back(std::make_shared<GenDiag>(
+          Result.Diagnostics.push_back(std::make_shared<GenericReport>(
               MCInstInBBReference(&BB, I),
               "Warning: pac-ret analysis could not analyze this return "
               "instruction"));
@@ -416,9 +416,10 @@ Analysis::computeDfState(PacRetAnalysis &PRA, BinaryFunction &BF,
         LLVM_DEBUG(
             { traceRegMask(BC, "Intersection with RetReg", UsedDirtyRegs); });
         if (UsedDirtyRegs.any()) {
+          static const GadgetKind RetKind("non-protected ret found");
           // This return instruction needs to be reported
-          Result.Diagnostics.push_back(std::make_shared<Gadget>(
-              MCInstInBBReference(&BB, I),
+          Result.Diagnostics.push_back(std::make_shared<GadgetReport>(
+              RetKind, MCInstInBBReference(&BB, I),
               PRA.getLastClobberingInsts(Inst, BF, UsedDirtyRegs)));
           for (MCPhysReg RetRegWithGadget : UsedDirtyRegs.set_bits())
             Result.RegistersAffected.insert(RetRegWithGadget);
@@ -480,28 +481,43 @@ static void printBB(const BinaryContext &BC, const BinaryBasicBlock *BB,
 
 static void reportFoundGadgetInSingleBBSingleOverwInst(
     raw_ostream &OS, const BinaryContext &BC, const MCInstReference OverwInst,
-    const MCInstReference RetInst) {
-  BinaryBasicBlock *BB = RetInst.getBasicBlock();
+    const MCInstReference Location) {
+  BinaryBasicBlock *BB = Location.getBasicBlock();
   assert(OverwInst.ParentKind == MCInstReference::BasicBlockParent);
-  assert(RetInst.ParentKind == MCInstReference::BasicBlockParent);
+  assert(Location.ParentKind == MCInstReference::BasicBlockParent);
   MCInstInBBReference OverwInstBB = OverwInst.U.BBRef;
   if (BB == OverwInstBB.BB) {
     // overwriting inst and ret instruction are in the same basic block.
-    assert(OverwInstBB.BBIndex < RetInst.U.BBRef.BBIndex);
+    assert(OverwInstBB.BBIndex < Location.U.BBRef.BBIndex);
     OS << "  This happens in the following basic block:\n";
     printBB(BC, BB);
   }
 }
 
-void Gadget::generateReport(raw_ostream &OS, const BinaryContext &BC) const {
-  GenDiag(RetInst, "non-protected ret found").generateReport(OS, BC);
+void Report::printBasicInfo(raw_ostream &OS, const BinaryContext &BC,
+                            StringRef IssueKind) const {
+  BinaryFunction *BF = Location.getFunction();
+  BinaryBasicBlock *BB = Location.getBasicBlock();
+
+  OS << "\nGS-PAUTH: " << IssueKind;
+  OS << " in function " << BF->getPrintName();
+  if (BB)
+    OS << ", basic block " << BB->getName();
+  OS << ", at address " << llvm::format("%x", Location.getAddress()) << "\n";
+  OS << "  The instruction is ";
+  BC.printInstruction(OS, Location, Location.getAddress(), BF);
+}
 
-  BinaryFunction *BF = RetInst.getFunction();
-  OS << "  The " << OverwritingRetRegInst.size()
-     << " instructions that write to the return register after any "
+void GadgetReport::generateReport(raw_ostream &OS,
+                                  const BinaryContext &BC) const {
+  printBasicInfo(OS, BC, Kind.getDescription());
+
+  BinaryFunction *BF = Location.getFunction();
+  OS << "  The " << OverwritingInstrs.size()
+     << " instructions that write to the affected registers after any "
         "authentication are:\n";
   // Sort by address to ensure output is deterministic.
-  std::vector<MCInstReference> ORRI = OverwritingRetRegInst;
+  std::vector<MCInstReference> ORRI = OverwritingInstrs;
   llvm::sort(ORRI, [](const MCInstReference &A, const MCInstReference &B) {
     return A.getAddress() < B.getAddress();
   });
@@ -510,24 +526,16 @@ void Gadget::generateReport(raw_ostream &OS, const BinaryContext &BC) const {
     OS << "  " << (I + 1) << ". ";
     BC.printInstruction(OS, InstRef, InstRef.getAddress(), BF);
   };
-  if (OverwritingRetRegInst.size() == 1) {
-    const MCInstReference OverwInst = OverwritingRetRegInst[0];
+  if (OverwritingInstrs.size() == 1) {
+    const MCInstReference OverwInst = OverwritingInstrs[0];
     assert(OverwInst.ParentKind == MCInstReference::BasicBlockParent);
-    reportFoundGadgetInSingleBBSingleOverwInst(OS, BC, OverwInst, RetInst);
+    reportFoundGadgetInSingleBBSingleOverwInst(OS, BC, OverwInst, Location);
   }
 }
 
-void GenDiag::generateReport(raw_ostream &OS, const BinaryContext &BC) const {
-  BinaryFunction *BF = RetInst.getFunction();
-  BinaryBasicBlock *BB = RetInst.getBasicBlock();
-
-  OS << "\nGS-PACRET: " << Diag.Text;
-  OS << " in function " << BF->getPrintName();
-  if (BB)
-    OS << ", basic block " << BB->getName();
-  OS << ", at address " << llvm::format("%x", RetInst.getAddress()) << "\n";
-  OS << "  The return instruction is ";
-  BC.printInstruction(OS, RetInst, RetInst.getAddress(), BF);
+void GenericReport::generateReport(raw_ostream &OS,
+                                   const BinaryContext &BC) const {
+  printBasicInfo(OS, BC, Text);
 }
 
 Error Analysis::runOnFunctions(BinaryContext &BC) {
@@ -542,17 +550,16 @@ Error Analysis::runOnFunctions(BinaryContext &BC) {
 
   ParallelUtilities::runOnEachFunctionWithUniqueAllocId(
       BC, ParallelUtilities::SchedulingPolicy::SP_INST_LINEAR, WorkFun,
-      SkipFunc, "NonPacProtectedRetAnalysis");
+      SkipFunc, "PAuthGadgetScanner");
 
   for (BinaryFunction *BF : BC.getAllBinaryFunctions())
     if (AnalysisResults.count(BF) > 0) {
-      for (const std::shared_ptr<Annotation> &A :
-           AnalysisResults[BF].Diagnostics)
-        A->generateReport(outs(), BC);
+      for (const std::shared_ptr<Report> &R : AnalysisResults[BF].Diagnostics)
+        R->generateReport(outs(), BC);
     }
   return Error::success();
 }
 
-} // namespace NonPacProtectedRetAnalysis
+} // namespace PAuthGadgetScanner
 } // namespace bolt
 } // namespace llvm
diff --git a/bolt/lib/Rewrite/RewriteInstance.cpp b/bolt/lib/Rewrite/RewriteInstance.cpp
index 560b30c6c676c..f69601e388a54 100644
--- a/bolt/lib/Rewrite/RewriteInstance.cpp
+++ b/bolt/lib/Rewrite/RewriteInstance.cpp
@@ -20,7 +20,7 @@
 #include "bolt/Passes/BinaryPasses.h"
 #include "bolt/Passes/CacheMetrics.h"
 #include "bolt/Passes/IdenticalCodeFolding.h"
-#include "bolt/Passes/NonPacProtectedRetAnalysis.h"
+#include "bolt/Passes/PAuthGadgetScanner.h"
 #include "bolt/Passes/ReorderFunctions.h"
 #include "bolt/Profile/BoltAddressTranslation.h"
 #include "bolt/Profile/DataAggregator.h"
@@ -3520,8 +3520,7 @@ void RewriteInstance::runBinaryAnalyses() {
     opts::GadgetScannersToRun.addValue(GSK::GS_ALL);
   for (GSK ScannerToRun : opts::GadgetScannersToRun) {
     if (ScannerToRun == GSK::GS_PACRET || ScannerToRun == GSK::GS_ALL)
-      Manager.registerPass(
-          std::make_unique<NonPacProtectedRetAnalysis::Analysis>());
+      Manager.registerPass(std::make_unique<PAuthGadgetScanner::Analysis>());
   }
 
   BC->logBOLTErrorsAndQuitOnFatal(Manager.runPasses());
diff --git a/bolt/test/binary-analysis/AArch64/gs-pacret-autiasp.s b/bolt/test/binary-analysis/AArch64/gs-pacret-autiasp.s
index cb81a63289f43..0d263199b376f 100644
--- a/bolt/test/binary-analysis/AArch64/gs-pacret-autiasp.s
+++ b/bolt/test/binary-analysis/AArch64/gs-pacret-autiasp.s
@@ -13,9 +13,9 @@ f1:
         add     x0, x0, #3
         ldp     x29, x30, [sp], #16
         // autiasp
-// CHECK-LABEL: GS-PACRET: non-protected ret found in function f1, basic block {{[0-9a-zA-Z.]+}}, at address
-// CHECK-NEXT:    The return instruction is     {{[0-9a-f]+}}:       ret
-// CHECK-NEXT:    The 1 instructions that write to the return register after any authentication are:
+// CHECK-LABEL: GS-PAUTH: non-protected ret found in function f1, basic block {{[0-9a-zA-Z.]+}}, at address
+// CHECK-NEXT:    The instruction is     {{[0-9a-f]+}}:       ret
+// CHECK-NEXT:    The 1 instructions that write to the affected registers after any authentication are:
 // CHECK-NEXT:    1. {{[0-9a-f]+}}: ldp     x29, x30, [sp], #0x10
 // CHECK-NEXT:  This happens in the following basic block:
 // CHECK-NEXT: {{[0-9a-f]+}}:   add     x0, x0, #0x3
@@ -35,9 +35,9 @@ f_intermediate_overwrite1:
         add     x0, x0, #3
         autiasp
         ldp     x29, x30, [sp], #16
-// CHECK-LABEL: GS-PACRET: non-protected ret found in function f_intermediate_overwrite1, basic block {{[0-9a-zA-Z.]+}}
-// CHECK-NEXT:    The return instruction is     {{[0-9a-f]+}}:       ret
-// CHECK-NEXT:    The 1 instructions that write to the return register after any authentication are:
+// CHECK-LABEL: GS-PAUTH: non-protected ret found in function f_intermediate_overwrite1, basic block {{[0-9a-zA-Z.]+}}
+// CHECK-NEXT:    The instruction is     {{[0-9a-f]+}}:       ret
+// CHECK-NEXT:    The 1 instructions that write to the affected registers after any authentication are:
 // CHECK-NEXT:    1. {{[0-9a-f]+}}: ldp     x29, x30, [sp], #0x10
 // CHECK-NEXT:  This happens in the following basic block:
 // CHECK-NEXT: {{[0-9a-f]+}}:   add     x0, x0, #0x3
@@ -58,9 +58,9 @@ f_intermediate_overwrite2:
         ldp     x29, x30, [sp], #16
         autiasp
         mov     x30, x0
-// CHECK-LABEL: GS-PACRET: non-protected ret found in function f_intermediate_overwrite2, basic block {{[0-9a-zA-Z.]+}}, at address
-// CHECK-NEXT:    The return instruction is     {{[0-9a-f]+}}:       ret
-// CHECK-NEXT:    The 1 instructions that write to the return register after any authentication are:
+// CHECK-LABEL: GS-PAUTH: non-protected ret found in function f_intermediate_overwrite2, basic block {{[0-9a-zA-Z.]+}}, at address
+// CHECK-NEXT:    The instruction is     {{[0-9a-f]+}}:       ret
+// CHECK-NEXT:    The 1 instructions that write to the affected registers after any authentication are:
 // CHECK-NEXT:    1. {{[0-9a-f]+}}: mov     x30, x0
 // CHECK-NEXT:  This happens in the following basic block:
 // CHECK-NEXT: {{[0-9a-f]+}}:   add     x0, x0, #0x3
@@ -97,9 +97,9 @@ f_intermediate_overwrite3:
         ldp     x29, x30, [sp], #16
         autiasp
         mov     w30, w0
-// CHECK-LABEL: GS-PACRET: non-protected ret found in function f_intermediate_overwrite3, basic block {{[0-9a-zA-Z.]+}}, at address
-// CHECK-NEXT:    The return instruction is     {{[0-9a-f]+}}:       ret
-// CHECK-NEXT:    The 1 instructions that write to the return register after any authentication are:
+// CHECK-LABEL: GS-PAUTH: non-protected ret found in function f_intermediate_overwrite3, basic block {{[0-9a-zA-Z.]+}}, at address
+// CHECK-NEXT:    The instruction is     {{[0-9a-f]+}}:       ret
+// CHECK-NEXT:    The 1 instructions that write to the affected registers after any authentication are:
 // CHECK-NEXT:    1. {{[0-9a-f]+}}: mov     w30, w0
 // CHECK-NEXT:  This happens in the following basic block:
 // CHECK-NEXT: {{[0-9a-f]+}}:   add     x0, x0, #0x3
@@ -121,9 +121,9 @@ f_nonx30_ret:
         ldp     x29, x30, [sp], #16
         mov     x16, x30
         autiasp
-// CHECK-LABEL: GS-PACRET: non-protected ret found in function f_nonx30_ret, basic block {{[0-9a-zA-Z.]+}}, at address
-// CHECK-NEXT:    The return instruction is     {{[0-9a-f]+}}:       ret     x16
-// CHECK-NEXT:    The 1 instructions that write to the return register after any authentication are:
+// CHECK-LABEL: GS-PAUTH: non-protected ret found in function f_nonx30_ret, basic block {{[0-9a-zA-Z.]+}}, at address
+// CHECK-NEXT:    The instruction is     {{[0-9a-f]+}}:       ret     x16
+// CHECK-NEXT:    The 1 instructions that write to the affected registers after any authentication are:
 // CHECK-NEXT:    1. {{[0-9a-f]+}}: mov     x16, x30
 // CHECK-NEXT:  This happens in the following basic block:
 // CHECK-NEXT: {{[0-9a-f]+}}:   add     x0, x0, #0x3
@@ -202,9 +202,9 @@ f_nonx30_ret_non_auted:
         .type   f_callclobbered_x30,@function
 f_callclobbered_x30:
         bl      g
-// CHECK-LABEL: GS-PACRET: non-protected ret found in function f_callclobbered_x30, basic block {{[0-9a-zA-Z.]+}}, at address
-// CHECK-NEXT:    The return instruction is     {{[0-9a-f]+}}:       ret
-// CHECK-NEXT:    The 1 instructions that write to the return register after any authentication are:
+// CHECK-LABEL: GS-PAUTH: non-protected ret found in function f_callclobbered_x30, basic block {{[0-9a-zA-Z.]+}}, at address
+// CHECK-NEXT:    The instruction is     {{[0-9a-f]+}}:       ret
+// CHECK-NEXT:    The 1 instructions that write to the affected registers after any authentication are:
 // CHECK-NEXT:    1. {{[0-9a-f]+}}: bl
         ret
         .size f_callclobbered_x30, .-f_callclobbered_x30
@@ -213,9 +213,9 @@ f_callclobbered_x30:
         .type   f_callclobbered_calleesaved,@function
 f_callclobbered_calleesaved:
         bl      g
-// CHECK-LABEL: GS-PACRET: non-protected ret found in function f_callclobbered_calleesaved, basic block {{[0-9a-zA-Z.]+}}, at address
-// CHECK-NEXT:    The return instruction is     {{[0-9a-f]+}}:       ret x19
-// CHECK-NEXT:    The 1 instructions that write to the return register after any authentication are:
+// CHECK-LABEL: GS-PAUTH: non-protected ret found in function f_callclobbered_calleesaved, basic block {{[0-9a-zA-Z.]+}}, at address
+// CHECK-NEXT:    The instruction is ...
[truncated]

Copy link
Collaborator

@kbeyls kbeyls left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM overall, I just have one very minor nitpick...

@atrosinenko atrosinenko force-pushed the users/atrosinenko/bolt-gs-streamline-issue-reporting branch from cf9baa5 to fa3befd Compare March 20, 2025 13:34
Base automatically changed from users/atrosinenko/bolt-gs-factor-out-utility-code to main March 20, 2025 16:35
@atrosinenko atrosinenko force-pushed the users/atrosinenko/bolt-gs-streamline-issue-reporting branch from fa3befd to dcbdeb3 Compare March 20, 2025 16:39
In preparation for adding more gadget kinds to detect, streamline
issue reporting.

Rename classes representing issue reports. In particular, rename
`Annotation` base class to `Report`, as it has nothing to do with
"annotations" in `MCPlus` terms anymore. Remove references to "return
instructions" from variable names and report messages, use generic
terms instead. Rename NonPacProtectedRetAnalysis to PAuthGadgetScanner.

Remove `GeneralDiagnostic` as a separate class, make `GenericReport`
(former `GenDiag`) store `std::string Text` directly. Remove unused
`operator=` and `operator==` methods, as `Report`s are created on the
heap and referenced via `shared_ptr`s.

Introduce `GadgetKind` class - currently, it only wraps a `const char *`
description to display to the user. This description is intended to be
a per-gadget-kind constant (or a few hard-coded constants), so no need
to store it to `std::string` field in each report instance. To handle
both free-form `GenericReport`s and statically-allocated messages
without unnecessary overhead, move printing of the report header to the
base class (and take the message argument as a `StringRef`).
@atrosinenko atrosinenko force-pushed the users/atrosinenko/bolt-gs-streamline-issue-reporting branch from dcbdeb3 to af6f92b Compare March 20, 2025 18:15
@atrosinenko atrosinenko merged commit 0355716 into main Mar 21, 2025
10 checks passed
@atrosinenko atrosinenko deleted the users/atrosinenko/bolt-gs-streamline-issue-reporting branch March 21, 2025 08:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants