NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a_sidorin, rnkovacs,
mikhail.ramalho, Szelethus, baloghadamsoftware.
Herald added subscribers: cfe-commits, jdoerfert, dkrupp, donat.nagy,
a.sidorin, szepet.
Herald added a project: clang.
As i mentioned in D58067#1393674 <https://reviews.llvm.org/D58067#1393674>, i
dislike the way bug reporter visitors have to reverse-engineer the checker's
behavior instead of asking the checker what happened directly.
The approach suggested here is to allow the checker to take notes of what
happens as it adds transitions, and then when the report is thrown, reconstruct
path messages from these notes.
Because for the sake of future-proofness such messages should probably be
allowed to be expensive to construct (even though i'm not aware of any specific
examples of expensive-to-construct messages), the checker notes are defined to
be lambdas that simply capture the necessary information but don't process it
until a report is actually emitted. As a useful side effect, this allows the
message to depend on the `BugReport` object itself, which is completely
essential because most checkers won't emit path messages for every state update
they make. For instance, when MallocChecker diagnoses a use-after-free, it only
emits the "memory is freed" path message for the symbol that was accessed after
free, but not for other symbols that were allocated or deallocated along the
path. With lambda notes, we can check if the symbol is marked as "interesting"
in the BugReport before emitting the path message. In the future we may want to
extend the BugReport object to carry arbitrary Checker-specific data that the
checker can take advantage of within its note lambdas.
Checker notes from which path messages are constructed are implemented as
`ProgramPointTags` of special sub-kind: `NoteTag`. Then a special visitor is
added to every report in order to scan the report for those tags and invoke the
lambdas.
Here's how it looks in the checker in my next patch that actually makes use of
the new functionality:
const NoteTag *T = C.getNoteTag([PVD]() -> std::string {
SmallString<64> Str;
llvm::raw_svector_ostream OS(Str);
OS << "Deallocating object passed through parameter '" << PVD->getName() <<
'\'';
return OS.str();
});
C.addTransition(C.getState()->set<ReleasedParameter>(true), T);
And there's no need to write all of this anymore:
class MyVisitor: public BugReporterVisitor {
/*Manual captures...*/
public:
MyVisitor(/*Manual captures...*/) { ... }
void Profile(llvm::FoldingSetNodeID &ID) {
// The usual static int business.
// And mention all manual captures.
// Or maybe almost all, depends.
}
std::shared_ptr<PathDiagnosticPiece> VisitNode(const ExplodedNode *N,
BugReporterContext &BRC,
BugReport &BR) override {
// The usual GDM update reverse-engineering idiom.
if (N->getState()->get<MyTrait>(...) !=
N->getFirstPred()->get<MyTrait>(...)) {
// Then the same message generating code.
// Then a bunch of boilerplate to generate the piece itself:
const Stmt *S = PathDiagnosticLocation::getStmt(N);
if (!S)
return nullptr;
PathDiagnosticLocation Loc = PathDiagnosticLocation::create(S);
return std::make_shared<PathDiagnosticEventPiece>(Loc, OS.str());
}
}
};
...
BR.addVisitor(MyVisitor(/*Manual captures...*/));
Repository:
rC Clang
https://reviews.llvm.org/D58367
Files:
clang/include/clang/Analysis/ProgramPoint.h
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h
clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
clang/lib/StaticAnalyzer/Core/BugReporter.cpp
clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -2461,6 +2461,30 @@
return nullptr;
}
+int NoteTag::Kind = 0;
+
+void TagVisitor::Profile(llvm::FoldingSetNodeID &ID) const {
+ static int Tag = 0;
+ ID.AddPointer(&Tag);
+}
+
+std::shared_ptr<PathDiagnosticPiece>
+TagVisitor::VisitNode(const ExplodedNode *N, BugReporterContext &BRC,
+ BugReport &R) {
+ ProgramPoint PP = N->getLocation();
+ const NoteTag *T = dyn_cast_or_null<NoteTag>(PP.getTag());
+ if (!T)
+ return nullptr;
+
+ if (Optional<std::string> Msg = T->getNote(BRC, R)) {
+ PathDiagnosticLocation Loc =
+ PathDiagnosticLocation::create(PP, BRC.getSourceManager());
+ return std::make_shared<PathDiagnosticEventPiece>(Loc, *Msg);
+ }
+
+ return nullptr;
+}
+
void FalsePositiveRefutationBRVisitor::Profile(
llvm::FoldingSetNodeID &ID) const {
static int Tag = 0;
Index: clang/lib/StaticAnalyzer/Core/BugReporter.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Core/BugReporter.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporter.cpp
@@ -2612,6 +2612,7 @@
R->addVisitor(llvm::make_unique<NilReceiverBRVisitor>());
R->addVisitor(llvm::make_unique<ConditionBRVisitor>());
R->addVisitor(llvm::make_unique<CXXSelfAssignmentBRVisitor>());
+ R->addVisitor(llvm::make_unique<TagVisitor>());
BugReporterContext BRC(Reporter, ErrorGraph.BackMap);
Index: clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
===================================================================
--- clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
+++ clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
@@ -22,6 +22,7 @@
#include "clang/Analysis/ProgramPoint.h"
#include "clang/Basic/LLVM.h"
#include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/FunctionSummary.h"
@@ -155,6 +156,8 @@
/// The flag, which specifies the mode of inlining for the engine.
InliningModes HowToInline;
+ NoteTag::Factory NoteTags;
+
public:
ExprEngine(cross_tu::CrossTranslationUnitContext &CTU, AnalysisManager &mgr,
SetOfConstDecls *VisitedCalleesIn,
@@ -396,6 +399,8 @@
SymbolManager &getSymbolManager() { return SymMgr; }
MemRegionManager &getRegionManager() { return MRMgr; }
+ NoteTag::Factory &getNoteTags() { return NoteTags; }
+
// Functions for external checking of whether we have unfinished work
bool wasBlocksExhausted() const { return Engine.wasBlocksExhausted(); }
Index: clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h
===================================================================
--- clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h
+++ clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h
@@ -219,6 +219,21 @@
Eng.getBugReporter().emitReport(std::move(R));
}
+ /// Produce a program point tag that displays an additional path note
+ /// to the user. This is a lightweirght alternative to the
+ /// BugReporterVisitor mechanism: instead of visiting the bug report
+ /// node-by-node to restore the sequence of events that led to discovering
+ /// a bug, you can add notes as you add your transitions.
+ const NoteTag *getNoteTag(NoteTag::Callback &&Cb) {
+ return Eng.getNoteTags().getNoteTag(std::move(Cb));
+ }
+
+ /// A shorthand version of getNoteTag that doesn't require you to accept
+ /// the BugReporterContext/BugReport arguments when you don't need them.
+ const NoteTag *getNoteTag(std::function<std::string()> &&Cb) {
+ return getNoteTag([Cb](BugReporterContext &, BugReport &) { return Cb(); });
+ }
+
/// Returns the word that should be used to refer to the declaration
/// in the report.
StringRef getDeclDescription(const Decl *D);
Index: clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
===================================================================
--- clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
+++ clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
@@ -14,6 +14,7 @@
#ifndef LLVM_CLANG_STATICANALYZER_CORE_BUGREPORTER_BUGREPORTERVISITORS_H
#define LLVM_CLANG_STATICANALYZER_CORE_BUGREPORTER_BUGREPORTERVISITORS_H
+#include "clang/Analysis/ProgramPoint.h"
#include "clang/Basic/LLVM.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/SVals.h"
@@ -342,6 +343,66 @@
BugReport &BR) override;
};
+/// The tag upon which the TagVisitor reacts. Add these in order to display
+/// additional PathDiagnosticEventPieces along the path.
+class NoteTag : public ProgramPointTag {
+public:
+ typedef std::function<std::string(BugReporterContext &, BugReport &)>
+ Callback;
+
+private:
+ static int Kind;
+
+ const Callback Cb;
+ NoteTag(Callback &&Cb) : ProgramPointTag(&Kind), Cb(std::move(Cb)) {}
+
+public:
+ static bool classof(const ProgramPointTag *T) {
+ return T->getTagKind() == &Kind;
+ }
+
+ Optional<std::string> getNote(BugReporterContext &BRC, BugReport &R) const {
+ std::string Note = Cb(BRC, R);
+ // Empty string is converted to lack of note. This simplifies the API
+ // by not forcing checker developers to write "Optional" every time
+ // they define a tag and have to write down the callback's return type.
+ if (Note.size() == 0)
+ return None;
+ return Note;
+ }
+
+ StringRef getTagDescription() const override {
+ return "Note Tag";
+ }
+
+ // Manage memory for NoteTag objects.
+ class Factory {
+ std::vector<std::unique_ptr<NoteTag>> Tags;
+
+ public:
+ const NoteTag *getNoteTag(Callback &&Cb) {
+ // We cannot use make_unique because we cannot access the private
+ // constructor from inside it.
+ std::unique_ptr<NoteTag> T(new NoteTag(std::move(Cb)));
+ Tags.push_back(std::move(T));
+ return Tags.back().get();
+ }
+ };
+
+ friend class Factory;
+ friend class TagVisitor;
+};
+
+/// The visitor detects NoteTags and displays the event notes they contain.
+class TagVisitor : public BugReporterVisitor {
+public:
+ void Profile(llvm::FoldingSetNodeID &ID) const override;
+
+ std::shared_ptr<PathDiagnosticPiece> VisitNode(const ExplodedNode *N,
+ BugReporterContext &BRC,
+ BugReport &R) override;
+};
+
namespace bugreporter {
/// Attempts to add visitors to track expression value back to its point of
Index: clang/include/clang/Analysis/ProgramPoint.h
===================================================================
--- clang/include/clang/Analysis/ProgramPoint.h
+++ clang/include/clang/Analysis/ProgramPoint.h
@@ -42,12 +42,11 @@
virtual ~ProgramPointTag();
virtual StringRef getTagDescription() const = 0;
-protected:
/// Used to implement 'isKind' in subclasses.
- const void *getTagKind() { return TagKind; }
+ const void *getTagKind() const { return TagKind; }
private:
- const void *TagKind;
+ const void *const TagKind;
};
class SimpleProgramPointTag : public ProgramPointTag {
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
