Address the comments

Author: atrosinenko

bolt/include/bolt/Passes/PAuthGadgetScanner.h

@@ -206,9 +206,10 @@ namespace PAuthGadgetScanner {
 // * 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.
-// * each instruction is checked to be 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 BriefReport<ReqT>.
+// * 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
@@ -218,7 +219,7 @@ namespace PAuthGadgetScanner {
 //
 // 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.
-// BriefReport<T>::RequestedDetails, on the other hand, is replaced with
+// 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
@@ -267,7 +268,7 @@ struct GenericDiagnostic : public Diagnostic {
                               const BinaryContext &BC) const override;
 };
 
-/// An information about an issue collected on the slower, detailed,
+/// Extra information about an issue collected on the slower, detailed,
 /// run of the analysis.
 class ExtraInfo {
 public:
@@ -290,9 +291,9 @@ class ClobberingInfo : public ExtraInfo {
 /// 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 BriefReport {
-  BriefReport(std::shared_ptr<Diagnostic> Issue,
-              const std::optional<T> RequestedDetails)
+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;
@@ -322,12 +323,12 @@ class FunctionAnalysisContext {
 
   bool PacRetGadgetsOnly;
 
-  void findUnsafeUses(SmallVector<BriefReport<MCPhysReg>> &Reports);
-  void augmentUnsafeUseReports(ArrayRef<BriefReport<MCPhysReg>> Reports);
+  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<BriefReport<MCPhysReg>> &Reports);
+  void handleSimpleReports(SmallVector<PartialReport<MCPhysReg>> &Reports);
 
 public:
   FunctionAnalysisContext(BinaryFunction &BF,

bolt/lib/Passes/PAuthGadgetScanner.cpp

@@ -713,21 +713,23 @@ SrcSafetyAnalysis::create(BinaryFunction &BF,
                                                        RegsToTrackInstsFor);
 }
 
-static BriefReport<MCPhysReg> make_generic_report(MCInstReference Location,
-                                                  StringRef Text) {
+// This function could return PartialReport<T>, but currently T is always
+// MCPhysReg, even though it is an implementation detail.
+static PartialReport<MCPhysReg> make_generic_report(MCInstReference Location,
+                                                    StringRef Text) {
   auto Report = std::make_shared<GenericDiagnostic>(Location, Text);
-  return BriefReport<MCPhysReg>(Report, std::nullopt);
+  return PartialReport<MCPhysReg>(Report, std::nullopt);
 }
 
 template <typename T>
-static BriefReport<T> make_report(const GadgetKind &Kind,
-                                  MCInstReference Location,
-                                  T RequestedDetails) {
+static PartialReport<T> make_gadget_report(const GadgetKind &Kind,
+                                           MCInstReference Location,
+                                           T RequestedDetails) {
   auto Report = std::make_shared<GadgetDiagnostic>(Kind, Location);
-  return BriefReport<T>(Report, RequestedDetails);
+  return PartialReport<T>(Report, RequestedDetails);
 }
 
-static std::optional<BriefReport<MCPhysReg>>
+static std::optional<PartialReport<MCPhysReg>>
 shouldReportReturnGadget(const BinaryContext &BC, const MCInstReference &Inst,
                          const SrcState &S) {
   static const GadgetKind RetKind("non-protected ret found");
@@ -752,10 +754,10 @@ shouldReportReturnGadget(const BinaryContext &BC, const MCInstReference &Inst,
   if (S.SafeToDerefRegs[RetReg])
     return std::nullopt;
 
-  return make_report(RetKind, Inst, RetReg);
+  return make_gadget_report(RetKind, Inst, RetReg);
 }
 
-static std::optional<BriefReport<MCPhysReg>>
+static std::optional<PartialReport<MCPhysReg>>
 shouldReportCallGadget(const BinaryContext &BC, const MCInstReference &Inst,
                        const SrcState &S) {
   static const GadgetKind CallKind("non-protected call found");
@@ -777,10 +779,10 @@ shouldReportCallGadget(const BinaryContext &BC, const MCInstReference &Inst,
   if (S.SafeToDerefRegs[DestReg])
     return std::nullopt;
 
-  return make_report(CallKind, Inst, DestReg);
+  return make_gadget_report(CallKind, Inst, DestReg);
 }
 
-static std::optional<BriefReport<MCPhysReg>>
+static std::optional<PartialReport<MCPhysReg>>
 shouldReportSigningOracle(const BinaryContext &BC, const MCInstReference &Inst,
                           const SrcState &S) {
   static const GadgetKind SigningOracleKind("signing oracle found");
@@ -797,7 +799,7 @@ shouldReportSigningOracle(const BinaryContext &BC, const MCInstReference &Inst,
   if (S.TrustedRegs[SignedReg])
     return std::nullopt;
 
-  return make_report(SigningOracleKind, Inst, SignedReg);
+  return make_gadget_report(SigningOracleKind, Inst, SignedReg);
 }
 
 template <typename T> static void iterateOverInstrs(BinaryFunction &BF, T Fn) {
@@ -812,7 +814,7 @@ template <typename T> static void iterateOverInstrs(BinaryFunction &BF, T Fn) {
 }
 
 static SmallVector<MCPhysReg>
-collectRegsToTrack(ArrayRef<BriefReport<MCPhysReg>> Reports) {
+collectRegsToTrack(ArrayRef<PartialReport<MCPhysReg>> Reports) {
   SmallSet<MCPhysReg, 4> RegsToTrack;
   for (auto Report : Reports)
     if (Report.RequestedDetails)
@@ -822,7 +824,7 @@ collectRegsToTrack(ArrayRef<BriefReport<MCPhysReg>> Reports) {
 }
 
 void FunctionAnalysisContext::findUnsafeUses(
-    SmallVector<BriefReport<MCPhysReg>> &Reports) {
+    SmallVector<PartialReport<MCPhysReg>> &Reports) {
   auto Analysis = SrcSafetyAnalysis::create(BF, AllocatorId, {});
   LLVM_DEBUG({ dbgs() << "Running src register safety analysis...\n"; });
   Analysis->run();
@@ -856,7 +858,7 @@ void FunctionAnalysisContext::findUnsafeUses(
 }
 
 void FunctionAnalysisContext::augmentUnsafeUseReports(
-    ArrayRef<BriefReport<MCPhysReg>> Reports) {
+    ArrayRef<PartialReport<MCPhysReg>> Reports) {
   SmallVector<MCPhysReg> RegsToTrack = collectRegsToTrack(Reports);
   // Re-compute the analysis with register tracking.
   auto Analysis = SrcSafetyAnalysis::create(BF, AllocatorId, RegsToTrack);
@@ -882,7 +884,7 @@ void FunctionAnalysisContext::augmentUnsafeUseReports(
 }
 
 void FunctionAnalysisContext::handleSimpleReports(
-    SmallVector<BriefReport<MCPhysReg>> &Reports) {
+    SmallVector<PartialReport<MCPhysReg>> &Reports) {
   // Before re-running the detailed analysis, process the reports which do not
   // need any additional details to be attached.
   for (auto &Report : Reports) {
@@ -899,7 +901,7 @@ void FunctionAnalysisContext::run() {
     BF.dump();
   });
 
-  SmallVector<BriefReport<MCPhysReg>> UnsafeUses;
+  SmallVector<PartialReport<MCPhysReg>> UnsafeUses;
   findUnsafeUses(UnsafeUses);
   handleSimpleReports(UnsafeUses);
   if (!UnsafeUses.empty())