[BOLT] Gadget scanner: refactor issue reporting #135662
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Apr 14, 2025
This was referenced Apr 14, 2025
atrosinenko
requested review from
aaupov,
asl,
ayermolo,
dcci,
jacobbramley,
kbeyls,
maksfb,
rafaelauler and
yota9
|
@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]
|
atrosinenko
force-pushed
the
users/atrosinenko/bolt-gs-refactor-reports
branch
from
c82cab5 to
ec437a4
Compare
atrosinenko
force-pushed
the
users/atrosinenko/bolt-gs-use-better-types
branch
from
51373db to
d57dc48
Compare
atrosinenko
force-pushed
the
users/atrosinenko/bolt-gs-use-better-types
branch
from
d57dc48 to
cf7d310
Compare
atrosinenko
force-pushed
the
users/atrosinenko/bolt-gs-refactor-reports
branch
from
bbec9f8 to
36c3cb9
Compare
atrosinenko
force-pushed
the
users/atrosinenko/bolt-gs-use-better-types
branch
from
cf7d310 to
d9af9e8
Compare
atrosinenko
force-pushed
the
users/atrosinenko/bolt-gs-refactor-reports
branch
from
36c3cb9 to
4c957e1
Compare
kbeyls
reviewed
Apr 25, 2025
kbeyls
reviewed
Apr 29, 2025
atrosinenko
force-pushed
the
users/atrosinenko/bolt-gs-use-better-types
branch
from
554bcfd to
a5b966d
Compare
atrosinenko
force-pushed
the
users/atrosinenko/bolt-gs-refactor-reports
branch
from
1a4a3a2 to
9144462
Compare
This was referenced Apr 30, 2025
atrosinenko
force-pushed
the
users/atrosinenko/bolt-gs-refactor-reports
branch
from
9144462 to
cab66b6
Compare
atrosinenko
force-pushed
the
users/atrosinenko/bolt-gs-use-better-types
branch
2 times, most recently
from
5e2fd61 to
eb984d9
Compare
atrosinenko
force-pushed
the
users/atrosinenko/bolt-gs-refactor-reports
branch
2 times, most recently
from
3bc553d to
91b4c58
Compare
atrosinenko
force-pushed
the
users/atrosinenko/bolt-gs-use-better-types
branch
from
eb984d9 to
62b27f9
Compare
Base automatically changed from
users/atrosinenko/bolt-gs-use-better-types
to
main
May 20, 2025 11:36
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.
atrosinenko
force-pushed
the
users/atrosinenko/bolt-gs-refactor-reports
branch
from
91b4c58 to
aeb987a
Compare
|
I updated the PR description to reflect the improvements made throughout the review. I believe this patch is ready for review again, as there are no more unresponded comments and its predecessor PRs were finally unblocked and merged. |
kbeyls
approved these changes
May 22, 2025
This was referenced May 27, 2025
sivan-shani
pushed a commit
to sivan-shani/llvm-project
that referenced
this pull request
Jun 3, 2025
Remove `getAffectedRegisters` and `setOverwritingInstrs` methods from the base `Report` class. Instead, rename the `Report` class to `Diagnostic` and make it always represent the brief version of the report, which is kept unchanged since initially found. Throughout its life-cycle, an instance of `Diagnostic` is first wrapped into `PartialReport<ReqT>` together with an optional request for extra details. Then, on the second run of the analysis, it is re-wrapped into `FinalReport` together with the requested detailed information.
ajaden-codes
pushed a commit
to Jaddyen/llvm-project
that referenced
this pull request
Jun 6, 2025
Remove `getAffectedRegisters` and `setOverwritingInstrs` methods from the base `Report` class. Instead, rename the `Report` class to `Diagnostic` and make it always represent the brief version of the report, which is kept unchanged since initially found. Throughout its life-cycle, an instance of `Diagnostic` is first wrapped into `PartialReport<ReqT>` together with an optional request for extra details. Then, on the second run of the analysis, it is re-wrapped into `FinalReport` together with the requested detailed information.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Remove
getAffectedRegistersandsetOverwritingInstrsmethods from the baseReportclass. Instead, rename theReportclass toDiagnosticand make it always represent the brief version of the report, which is kept unchanged since initially found. Throughout its life-cycle, an instance ofDiagnosticis first wrapped intoPartialReport<ReqT>together with an optional request for extra details. Then, on the second run of the analysis, it is re-wrapped intoFinalReporttogether with the requested detailed information.