[llvm-branch-commits] [llvm] [BOLT] Gadget scanner: refactor issue reporting (PR #135662)

[Kristof Beyls via llvm-branch-commits]

================
@@ -198,73 +198,147 @@ 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.
+// * 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>.
+// * 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.
+// BriefReport<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;
 };
 
+/// An information about an issue collected on the slower, detailed,
----------------
kbeyls wrote:

"An information" sounds a bit strange to me. Maybe better to say "Extra information" here?

https://github.com/llvm/llvm-project/pull/135662