[BOLT] Gadget scanner: refactor issue reporting #135662
Conversation
|
@llvm/pr-subscribers-bolt Author: Anatoly Trosinenko (atrosinenko) ChangesRemove Patch is 21.59 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/135662.diff 3 Files Affected:
diff --git a/bolt/include/bolt/Passes/PAuthGadgetScanner.h b/bolt/include/bolt/Passes/PAuthGadgetScanner.h
index 3e39b64e59e0f..3b6c1f6af94a0 100644
--- a/bolt/include/bolt/Passes/PAuthGadgetScanner.h
+++ b/bolt/include/bolt/Passes/PAuthGadgetScanner.h
@@ -219,11 +219,6 @@ struct Report {
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 const ArrayRef<MCPhysReg> getAffectedRegisters() const { return {}; }
- virtual void setOverwritingInstrs(const ArrayRef<MCInstReference> Instrs) {}
-
void printBasicInfo(raw_ostream &OS, const BinaryContext &BC,
StringRef IssueKind) const;
};
@@ -231,27 +226,11 @@ struct Report {
struct GadgetReport : public Report {
// 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;
- const ArrayRef<MCPhysReg> getAffectedRegisters() const override {
- return AffectedRegisters;
- }
+ GadgetReport(const GadgetKind &Kind, MCInstReference Location)
+ : Report(Location), Kind(Kind) {}
- void setOverwritingInstrs(const 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.
@@ -263,8 +242,75 @@ struct GenericReport : public Report {
const BinaryContext &BC) const override;
};
+/// An information about an issue collected on the slower, detailed,
+/// run of an 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(const 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.
+///
+/// It is common for a particular type of gadget detector to be tied to some
+/// specific kind of analysis. If an issue is returned by that detector, it may
+/// be further augmented with the detailed info in an analysis-specific way,
+/// or just be left as-is (f.e. if a free-form warning was reported).
+template <typename T> struct BriefReport {
+ BriefReport(std::shared_ptr<Report> Issue,
+ const std::optional<T> RequestedDetails)
+ : Issue(Issue), RequestedDetails(RequestedDetails) {}
+
+ std::shared_ptr<Report> Issue;
+ std::optional<T> RequestedDetails;
+};
+
+/// A detailed version of a report.
+struct DetailedReport {
+ DetailedReport(std::shared_ptr<Report> Issue,
+ std::shared_ptr<ExtraInfo> Details)
+ : Issue(Issue), Details(Details) {}
+
+ std::shared_ptr<Report> Issue;
+ std::shared_ptr<ExtraInfo> Details;
+};
+
struct FunctionAnalysisResult {
- std::vector<std::shared_ptr<Report>> Diagnostics;
+ std::vector<DetailedReport> Diagnostics;
+};
+
+/// A helper class storing per-function context to be instantiated by Analysis.
+class FunctionAnalysis {
+ BinaryContext &BC;
+ BinaryFunction &BF;
+ MCPlusBuilder::AllocatorIdTy AllocatorId;
+ FunctionAnalysisResult Result;
+
+ bool PacRetGadgetsOnly;
+
+ void findUnsafeUses(SmallVector<BriefReport<MCPhysReg>> &Reports);
+ void augmentUnsafeUseReports(const ArrayRef<BriefReport<MCPhysReg>> Reports);
+
+public:
+ FunctionAnalysis(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 {
@@ -273,12 +319,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;
diff --git a/bolt/lib/Passes/PAuthGadgetScanner.cpp b/bolt/lib/Passes/PAuthGadgetScanner.cpp
index ed89471cbb8d3..b3081f034e8ee 100644
--- a/bolt/lib/Passes/PAuthGadgetScanner.cpp
+++ b/bolt/lib/Passes/PAuthGadgetScanner.cpp
@@ -518,18 +518,13 @@ class SrcSafetyAnalysis {
public:
std::vector<MCInstReference>
getLastClobberingInsts(const MCInst &Inst, BinaryFunction &BF,
- const ArrayRef<MCPhysReg> UsedDirtyRegs) const {
- if (RegsToTrackInstsFor.empty())
+ std::optional<MCPhysReg> ClobberedReg) const {
+ if (!ClobberedReg || RegsToTrackInstsFor.empty())
return {};
const SrcState &S = getStateBefore(Inst);
- // Due to aliasing registers, multiple registers may have been tracked.
- std::set<const MCInst *> LastWritingInsts;
- for (MCPhysReg TrackedReg : UsedDirtyRegs) {
- for (const MCInst *Inst : lastWritingInsts(S, TrackedReg))
- LastWritingInsts.insert(Inst);
- }
+
std::vector<MCInstReference> Result;
- for (const MCInst *Inst : LastWritingInsts) {
+ for (const MCInst *Inst : lastWritingInsts(S, *ClobberedReg)) {
MCInstReference Ref = MCInstReference::get(Inst, BF);
assert(Ref && "Expected Inst to be found");
Result.push_back(Ref);
@@ -717,16 +712,30 @@ SrcSafetyAnalysis::create(BinaryFunction &BF,
RegsToTrackInstsFor);
}
-static std::shared_ptr<Report>
+static BriefReport<MCPhysReg> make_generic_report(MCInstReference Location,
+ StringRef Text) {
+ auto Report = std::make_shared<GenericReport>(Location, Text);
+ return BriefReport<MCPhysReg>(Report, std::nullopt);
+}
+
+template <typename T>
+static BriefReport<T> make_report(const GadgetKind &Kind,
+ MCInstReference Location,
+ T RequestedDetails) {
+ auto Report = std::make_shared<GadgetReport>(Kind, Location);
+ return BriefReport<T>(Report, RequestedDetails);
+}
+
+static std::optional<BriefReport<MCPhysReg>>
shouldReportReturnGadget(const BinaryContext &BC, const MCInstReference &Inst,
const SrcState &S) {
static const GadgetKind RetKind("non-protected ret found");
if (!BC.MIB->isReturn(Inst))
- return nullptr;
+ return std::nullopt;
ErrorOr<MCPhysReg> MaybeRetReg = BC.MIB->getRegUsedAsRetDest(Inst);
if (MaybeRetReg.getError()) {
- return std::make_shared<GenericReport>(
+ return make_generic_report(
Inst, "Warning: pac-ret analysis could not analyze this return "
"instruction");
}
@@ -737,26 +746,26 @@ shouldReportReturnGadget(const BinaryContext &BC, const MCInstReference &Inst,
traceReg(BC, "Authenticated reg", BC.MIB->getAuthenticatedReg(Inst));
});
if (BC.MIB->isAuthenticationOfReg(Inst, RetReg))
- return nullptr;
+ return std::nullopt;
LLVM_DEBUG({ traceRegMask(BC, "SafeToDerefRegs", S.SafeToDerefRegs); });
if (S.SafeToDerefRegs[RetReg])
- return nullptr;
+ return std::nullopt;
- return std::make_shared<GadgetReport>(RetKind, Inst, RetReg);
+ return make_report(RetKind, Inst, RetReg);
}
-static std::shared_ptr<Report>
+static std::optional<BriefReport<MCPhysReg>>
shouldReportCallGadget(const BinaryContext &BC, const MCInstReference &Inst,
const SrcState &S) {
static const GadgetKind CallKind("non-protected call found");
if (!BC.MIB->isIndirectCall(Inst) && !BC.MIB->isIndirectBranch(Inst))
- return nullptr;
+ return std::nullopt;
bool IsAuthenticated = false;
MCPhysReg DestReg =
BC.MIB->getRegUsedAsIndirectBranchDest(Inst, IsAuthenticated);
if (IsAuthenticated)
- return nullptr;
+ return std::nullopt;
assert(DestReg != BC.MIB->getNoRegister());
LLVM_DEBUG({
@@ -765,19 +774,19 @@ shouldReportCallGadget(const BinaryContext &BC, const MCInstReference &Inst,
traceRegMask(BC, "SafeToDerefRegs", S.SafeToDerefRegs);
});
if (S.SafeToDerefRegs[DestReg])
- return nullptr;
+ return std::nullopt;
- return std::make_shared<GadgetReport>(CallKind, Inst, DestReg);
+ return make_report(CallKind, Inst, DestReg);
}
-static std::shared_ptr<Report>
+static std::optional<BriefReport<MCPhysReg>>
shouldReportSigningOracle(const BinaryContext &BC, const MCInstReference &Inst,
const SrcState &S) {
static const GadgetKind SigningOracleKind("signing oracle found");
MCPhysReg SignedReg = BC.MIB->getSignedReg(Inst);
if (SignedReg == BC.MIB->getNoRegister())
- return nullptr;
+ return std::nullopt;
LLVM_DEBUG({
traceInst(BC, "Found sign inst", Inst);
@@ -785,9 +794,9 @@ shouldReportSigningOracle(const BinaryContext &BC, const MCInstReference &Inst,
traceRegMask(BC, "TrustedRegs", S.TrustedRegs);
});
if (S.TrustedRegs[SignedReg])
- return nullptr;
+ return std::nullopt;
- return std::make_shared<GadgetReport>(SigningOracleKind, Inst, SignedReg);
+ return make_report(SigningOracleKind, Inst, SignedReg);
}
template <typename T> static void iterateOverInstrs(BinaryFunction &BF, T Fn) {
@@ -801,11 +810,18 @@ template <typename T> static void iterateOverInstrs(BinaryFunction &BF, T Fn) {
}
}
-FunctionAnalysisResult
-Analysis::findGadgets(BinaryFunction &BF,
- MCPlusBuilder::AllocatorIdTy AllocatorId) {
- FunctionAnalysisResult Result;
+static SmallVector<MCPhysReg>
+collectRegsToTrack(const ArrayRef<BriefReport<MCPhysReg>> Reports) {
+ SmallSet<MCPhysReg, 4> RegsToTrack;
+ for (auto Report : Reports)
+ if (Report.RequestedDetails)
+ RegsToTrack.insert(*Report.RequestedDetails);
+
+ return SmallVector<MCPhysReg>(RegsToTrack.begin(), RegsToTrack.end());
+}
+void FunctionAnalysis::findUnsafeUses(
+ SmallVector<BriefReport<MCPhysReg>> &Reports) {
auto Analysis = SrcSafetyAnalysis::create(BF, AllocatorId, {});
LLVM_DEBUG({ dbgs() << "Running src register safety analysis...\n"; });
Analysis->run();
@@ -814,45 +830,35 @@ Analysis::findGadgets(BinaryFunction &BF,
BF.dump();
});
- BinaryContext &BC = BF.getBinaryContext();
iterateOverInstrs(BF, [&](MCInstReference Inst) {
const SrcState &S = Analysis->getStateBefore(Inst);
// If non-empty state was never propagated from the entry basic block
// to Inst, assume it to be unreachable and report a warning.
if (S.empty()) {
- Result.Diagnostics.push_back(std::make_shared<GenericReport>(
- Inst, "Warning: unreachable instruction found"));
+ Reports.push_back(
+ make_generic_report(Inst, "Warning: unreachable instruction found"));
return;
}
if (auto Report = shouldReportReturnGadget(BC, Inst, S))
- Result.Diagnostics.push_back(Report);
+ Reports.push_back(*Report);
if (PacRetGadgetsOnly)
return;
if (auto Report = shouldReportCallGadget(BC, Inst, S))
- Result.Diagnostics.push_back(Report);
+ Reports.push_back(*Report);
if (auto Report = shouldReportSigningOracle(BC, Inst, S))
- Result.Diagnostics.push_back(Report);
+ Reports.push_back(*Report);
});
- return Result;
}
-void Analysis::computeDetailedInfo(BinaryFunction &BF,
- MCPlusBuilder::AllocatorIdTy AllocatorId,
- FunctionAnalysisResult &Result) {
- BinaryContext &BC = BF.getBinaryContext();
-
- // Collect the affected registers across all gadgets found in this function.
- SmallSet<MCPhysReg, 4> RegsToTrack;
- for (auto Report : Result.Diagnostics)
- RegsToTrack.insert_range(Report->getAffectedRegisters());
- std::vector<MCPhysReg> RegsToTrackVec(RegsToTrack.begin(), RegsToTrack.end());
-
+void FunctionAnalysis::augmentUnsafeUseReports(
+ const ArrayRef<BriefReport<MCPhysReg>> Reports) {
+ SmallVector<MCPhysReg> RegsToTrack = collectRegsToTrack(Reports);
// Re-compute the analysis with register tracking.
- auto Analysis = SrcSafetyAnalysis::create(BF, AllocatorId, RegsToTrackVec);
+ auto Analysis = SrcSafetyAnalysis::create(BF, AllocatorId, RegsToTrack);
LLVM_DEBUG(
{ dbgs() << "\nRunning detailed src register safety analysis...\n"; });
Analysis->run();
@@ -862,32 +868,37 @@ void Analysis::computeDetailedInfo(BinaryFunction &BF,
});
// Augment gadget reports.
- for (auto Report : Result.Diagnostics) {
- LLVM_DEBUG(
- { traceInst(BC, "Attaching clobbering info to", Report->Location); });
- (void)BC;
- Report->setOverwritingInstrs(Analysis->getLastClobberingInsts(
- Report->Location, BF, Report->getAffectedRegisters()));
+ for (auto Report : Reports) {
+ MCInstReference Location = Report.Issue->Location;
+ LLVM_DEBUG({ traceInst(BC, "Attaching clobbering info to", Location); });
+ auto DetailedInfo =
+ std::make_shared<ClobberingInfo>(Analysis->getLastClobberingInsts(
+ Location, BF, Report.RequestedDetails));
+ Result.Diagnostics.emplace_back(Report.Issue, DetailedInfo);
}
}
-void Analysis::runOnFunction(BinaryFunction &BF,
- MCPlusBuilder::AllocatorIdTy AllocatorId) {
+void FunctionAnalysis::run() {
LLVM_DEBUG({
- dbgs() << "Analyzing in function " << BF.getPrintName() << ", AllocatorId "
- << AllocatorId << "\n";
+ dbgs() << "Analyzing function " << BF.getPrintName()
+ << ", AllocatorId = " << AllocatorId << "\n";
BF.dump();
});
- FunctionAnalysisResult FAR = findGadgets(BF, AllocatorId);
- if (FAR.Diagnostics.empty())
- return;
+ SmallVector<BriefReport<MCPhysReg>> UnsafeUses;
+ findUnsafeUses(UnsafeUses);
+ if (!UnsafeUses.empty())
+ augmentUnsafeUseReports(UnsafeUses);
+}
- // Redo the analysis, but now also track which instructions last wrote
- // to any of the registers in RetRegsWithGadgets, so that better
- // diagnostics can be produced.
+void Analysis::runOnFunction(BinaryFunction &BF,
+ MCPlusBuilder::AllocatorIdTy AllocatorId) {
+ FunctionAnalysis FA(BF, AllocatorId, PacRetGadgetsOnly);
+ FA.run();
- computeDetailedInfo(BF, AllocatorId, FAR);
+ const FunctionAnalysisResult &FAR = FA.getResult();
+ if (FAR.Diagnostics.empty())
+ return;
// `runOnFunction` is typically getting called from multiple threads in
// parallel. Therefore, use a lock to avoid data races when storing the
@@ -915,16 +926,14 @@ static void printBB(const BinaryContext &BC, const BinaryBasicBlock *BB,
}
}
-static void reportFoundGadgetInSingleBBSingleOverwInst(
- raw_ostream &OS, const BinaryContext &BC, const MCInstReference OverwInst,
+static void reportFoundGadgetInSingleBBSingleRelatedInst(
+ raw_ostream &OS, const BinaryContext &BC, const MCInstReference RelatedInst,
const MCInstReference Location) {
BinaryBasicBlock *BB = Location.getBasicBlock();
- assert(OverwInst.ParentKind == MCInstReference::BasicBlockParent);
+ assert(RelatedInst.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 < Location.U.BBRef.BBIndex);
+ MCInstInBBReference RelatedInstBB = RelatedInst.U.BBRef;
+ if (BB == RelatedInstBB.BB) {
OS << " This happens in the following basic block:\n";
printBB(BC, BB);
}
@@ -947,31 +956,42 @@ void Report::printBasicInfo(raw_ostream &OS, const BinaryContext &BC,
void GadgetReport::generateReport(raw_ostream &OS,
const BinaryContext &BC) const {
printBasicInfo(OS, BC, Kind.getDescription());
+}
+
+static void printRelatedInstrs(raw_ostream &OS, const MCInstReference Location,
+ const ArrayRef<MCInstReference> RelatedInstrs) {
+ const BinaryFunction &BF = *Location.getFunction();
+ const BinaryContext &BC = BF.getBinaryContext();
- 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.
- SmallVector<MCInstReference> OI = OverwritingInstrs;
- llvm::sort(OI, [](const MCInstReference &A, const MCInstReference &B) {
+ SmallVector<MCInstReference> RI(RelatedInstrs);
+ llvm::sort(RI, [](const MCInstReference &A, const MCInstReference &B) {
return A.getAddress() < B.getAddress();
});
- for (unsigned I = 0; I < OI.size(); ++I) {
- MCInstReference InstRef = OI[I];
+ for (unsigned I = 0; I < RI.size(); ++I) {
+ MCInstReference InstRef = RI[I];
OS << " " << (I + 1) << ". ";
- BC.printInstruction(OS, InstRef, InstRef.getAddress(), BF);
+ BC.printInstruction(OS, InstRef, InstRef.getAddress(), &BF);
};
- if (OverwritingInstrs.size() == 1) {
- const MCInstReference OverwInst = OverwritingInstrs[0];
+ if (RelatedInstrs.size() == 1) {
+ const MCInstReference RelatedInst = RelatedInstrs[0];
// Printing the details for the MCInstReference::FunctionParent case
// is not implemented not to overcomplicate the code, as most functions
// are expected to have CFG information.
- if (OverwInst.ParentKind == MCInstReference::BasicBlockParent)
- reportFoundGadgetInSingleBBSingleOverwInst(OS, BC, OverwInst, Location);
+ if (RelatedInst.ParentKind == MCInstReference::BasicBlockParent)
+ reportFoundGadgetInSingleBBSingleRelatedInst(OS, BC, RelatedInst,
+ Location);
}
}
+void ClobberingInfo::print(raw_ostream &OS,
+ const MCInstReference Location) const {
+ OS << " The " << ClobberingInstrs.size()
+ << " instructions that write to the affected registers after any "
+ "authentication are:\n";
+ printRelatedInstrs(OS, Location, ClobberingInstrs);
+}
+
void GenericReport::generateReport(raw_ostream &OS,
const BinaryContext &BC) const {
printBasicInfo(OS, BC, Text);
@@ -991,11 +1011,15 @@ Error Analysis::runOnFunctions(BinaryContext &BC) {
BC, ParallelUtilities::SchedulingPolicy::SP_INST_LINEAR, WorkFun,
SkipFunc, "PAuthGadgetScanner");
- for (BinaryFunction *BF : BC.getAllBinaryFunctions())
- if (AnalysisResults.count(BF) > 0) {
- for (const std::shared_ptr<Report> &R : AnalysisResults[BF].Diagnostics)
- R->generateReport(outs(), BC);
+ for (BinaryFunction *BF : BC.getAllBinaryFunctions()) {
+ if (!AnalysisResults.count(BF))
+ continue;
+ for (const DetailedReport &R : AnalysisResults[BF].Diagnostics) {
+ R.Issue->generateReport(outs(), BC);
+ if (R.Details)
+ R.Details->print(outs(), R.Issue->Location);
}
+ }
return Error::success();
}
diff --git a/bolt/test/binary-analysis/AArch64/gs-pauth-debug-output.s b/bolt/test/binary-analysis/AArch64/gs-pauth-debug-out...
[truncated]
|
c82cab5 to
ec437a4
Compare
51373db to
d57dc48
Compare
d57dc48 to
cf7d310
Compare
bbec9f8 to
36c3cb9
Compare
cf7d310 to
d9af9e8
Compare
36c3cb9 to
4c957e1
Compare
| BriefReport(std::shared_ptr<Report> Issue, | ||
| const std::optional<T> RequestedDetails) | ||
| : Issue(Issue), RequestedDetails(RequestedDetails) {} |
There was a problem hiding this comment.
Reading through the code, I find it a bit confusing to understand what all the different "report" classes actually mean.
With this patch, we'll have Report, GadgetReport, GenericReport, BriefReport and DetailedReport.
If I understand the code correctly, Report, GadgetReport and GenericReport are similar, but BriefReport and DetailedReport really are something somewhat different.
Would it be better to have a different name than Report to distinguish the 2 different classes of things represented here?
I'm not sure what the best possible name is for BriefReport and DetailedReport. I didn't fully understand the code so far, but is my impression correct that BriefReports main reason for existing is to be able to record RequestedDetails, so that it can record what kind of more detailed information should be added later to the report?
Thinking about better names, maybe renaming Report, GadgetReport and GenericReport to BaseDiagnostic, GadgetDiagnositic and GenericDiagnostic is an improvement, as it clarifies these are things that the tool will actually report to the user of the tool?
Then, maybe BriefReport is a class representing a diagnostic that is still under construction?
If my guess here is correct would a name like DiagnosticUnderConstruction be more self-describing?
Assuming all of my guesses above are correct, I'm also not sure if there is a need for a DetailedReport class? Couldn't ExtraInfo just be added to the BaseDiagnostic or GadgetDiagnostic classes, which would eliminate the need of one class, and as a result make the diagnostic reporting engine a little bit less complex?
There was a problem hiding this comment.
If I understand the code correctly,
Report,GadgetReportandGenericReportare similar, butBriefReportandDetailedReportreally are something somewhat different.
Yes, these are two groups of classes with misleading common "report" suffix. I like the idea of replacing Report with Diagnostic in Report, GadgetReport and GenericReport, thanks! This way, the "report" suffix is left for the last two classes which are closer to the issues reported to the user.
I'm not sure DiagnosticUnderConstruction is better than BriefReport, but DetailedReport can definitely be renamed to FinalReport for clarity.
Assuming all of my guesses above are correct, I'm also not sure if there is a need for a
DetailedReportclass? Couldn'tExtraInfojust be added to theBaseDiagnosticorGadgetDiagnosticclasses, which would eliminate the need of one class, and as a result make the diagnostic reporting engine a little bit less complex?
After thinking a bit more, I think the updated structure still makes sense, but requires a more detailed explanation. Thank you for pointing this out!
| /// A helper class storing per-function context to be instantiated by Analysis. | ||
| class FunctionAnalysis { | ||
| BinaryContext &BC; |
There was a problem hiding this comment.
I'm linearly reading through this class, so maybe my comment turns out to not make sense after I've read the rest of the patch. But as is, it seems to me that this class represents the context for a function analysis, and the results of the function analysis, rather than the analysis itself?
If that's correct, maybe there is a better name then FunctionAnalysis for it? Maybe FunctionAnalysisContext? Or something else?
There was a problem hiding this comment.
Sounds reasonable, thanks!
d9af9e8 to
554bcfd
Compare
4c957e1 to
21f15ce
Compare
| // * 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 |
There was a problem hiding this comment.
nitpick: maybe "For each instruction, it is checked whether it is a gadget of some kind"?
| // re-run to collect extra information to provide to the user. Which extra | ||
| // information can be requested depends on the particular analysis (for |
There was a problem hiding this comment.
nitpick: maybe it's simpler and still correct to just phrase this as "Which extra information depends on the particular analysis"?
There was a problem hiding this comment.
Hmm, the longer version sounds clearer to me, but I'm not sure...
| // computed state into account. If a gadget is found, its kind and location | ||
| // are stored into a subclass of Diagnostic wrapped into BriefReport<ReqT>. |
There was a problem hiding this comment.
Reading it like this in the documentation, it seems to me that a name like PartialReport would slightly better describe what it is than BriefReport. WDYT?
There was a problem hiding this comment.
Sounds reasonable, thanks!
| virtual void generateReport(raw_ostream &OS, | ||
| const BinaryContext &BC) const override; | ||
| }; | ||
|
|
||
| /// An information about an issue collected on the slower, detailed, |
There was a problem hiding this comment.
"An information" sounds a bit strange to me. Maybe better to say "Extra information" here?
| static BriefReport<MCPhysReg> make_generic_report(MCInstReference Location, | ||
| StringRef Text) { | ||
| auto Report = std::make_shared<GenericDiagnostic>(Location, Text); | ||
| return BriefReport<MCPhysReg>(Report, std::nullopt); | ||
| } | ||
|
|
||
| template <typename T> | ||
| static BriefReport<T> make_report(const GadgetKind &Kind, | ||
| MCInstReference Location, |
There was a problem hiding this comment.
If the first function is called make_generic_report, would it be more consistent to call the second function make_gadget_report?
I started wondering if it'd be possible to unify the two functions into a more general templated function, something like
template <typename DiagT, typename DiagParam, typename ReqDetailsT>
static BriefReport<T> make_brief_report(MCInstReference Location, DiagParam P, ReqDetailsT ReqDetails) {
auto Report = std::make_shared<DiagT>(Location, P);
return BriefReport<T>(Report, ReqDetails);
}
and then it gets called like
make_brief_report<GenericDiagnostic, StringRef, MCPhysReg>(Location, Text, std::nullopt) or
make_brief_report<GadgetDiagnostic, GadgetKind, RequestedDetailsT>(Location, Kind, RequestedDetails)
(sorry, haven't spent time thinking about reducing the number of explicit template parameters).
I'm guessing you might have thought about this and come to the conclusion that having two make_report functions is better?
There was a problem hiding this comment.
If the first function is called
make_generic_report, would it be more consistent to call the second functionmake_gadget_report?
This definitely makes sense.
I'm guessing you might have thought about this and come to the conclusion that having two
make_reportfunctions is better?
Not sure two separate functions are absolutely necessary, but I would like to get rid of template parameters when calling the function when possible (similar to calling std::make_pair vs. instantiating std::pair directly). Maybe is would be reasonable to add an explicit type parameter to make_generic_report instead of assuming MCPhysReg, but added a brief explanation instead for now...
554bcfd to
a5b966d
Compare
1a4a3a2 to
9144462
Compare
Remove `getAffectedRegisters` and `setOverwritingInstrs` methods from the base `Report` class. Instead, make `Report` always represent the brief version of the report. When an issue is detected on the first run of the analysis, return an optional request for extra details to attach to the report on the second run.
9144462 to
cab66b6
Compare
a5b966d to
5e2fd61
Compare
Remove
getAffectedRegistersandsetOverwritingInstrsmethods fromthe base
Reportclass. Instead, makeReportalways represent thebrief version of the report. When an issue is detected on the first run
of the analysis, return an optional request for extra details to attach
to the report on the second run.