NoQ updated this revision to Diff 189948.
NoQ added a comment.
Found a bug! The lambda has to manually make sure that the bug report actually
does have something to do with the checker. The attached testcase shows that we
definitely need to avoid attaching a MIG checker note to a core.DivideZero bug
report. I stuffed a check into the lambda to demonstrate what sort of logic do
we need to avoid this problem:
if (&BR.getBugType() != &BT)
return "";
But of course i'd much rather automate that because most checkers will need
this sort of logic and because it looks really ugly and is very easy to forget.
My best idea so far, for the next patch, is to store a pointer to the `Checker`
object in both the note and the bug report and automatically compare them in
`TagVisitor` before even trying to emit a note. Such check should probably be
optional, because otherwise only checkers would be able to use note tags, while
i still want the Core to be able to use it as well.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D58367/new/
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/Checkers/MIGChecker.cpp
clang/lib/StaticAnalyzer/Core/BugReporter.cpp
clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
clang/test/Analysis/mig.mm
Index: clang/test/Analysis/mig.mm
===================================================================
--- clang/test/Analysis/mig.mm
+++ clang/test/Analysis/mig.mm
@@ -91,6 +91,14 @@
// expected-note@-1{{MIG callback fails with error after deallocating argument value. This is a use-after-free vulnerability because the caller will try to deallocate it again}}
}
+MIG_SERVER_ROUTINE
+kern_return_t no_unrelated_notes(mach_port_name_t port, vm_address_t address, vm_size_t size) {
+ vm_deallocate(port, address, size); // no-note
+ 1 / 0; // expected-warning{{Division by zero}}
+ // expected-note@-1{{Division by zero}}
+ return KERN_SUCCESS;
+}
+
// Make sure we find the bug when the object is destroyed within an
// automatic destructor.
MIG_SERVER_ROUTINE
Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -2496,6 +2496,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->generateMessage(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/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
@@ -80,43 +80,10 @@
checkReturnAux(RS, C);
}
- class Visitor : public BugReporterVisitor {
- public:
- void Profile(llvm::FoldingSetNodeID &ID) const {
- static int X = 0;
- ID.AddPointer(&X);
- }
-
- std::shared_ptr<PathDiagnosticPiece> VisitNode(const ExplodedNode *N,
- BugReporterContext &BRC, BugReport &R);
- };
};
} // end anonymous namespace
-// FIXME: It's a 'const ParmVarDecl *' but there's no ready-made GDM traits
-// specialization for this sort of types.
-REGISTER_TRAIT_WITH_PROGRAMSTATE(ReleasedParameter, const void *)
-
-std::shared_ptr<PathDiagnosticPiece>
-MIGChecker::Visitor::VisitNode(const ExplodedNode *N, BugReporterContext &BRC,
- BugReport &R) {
- const auto *NewPVD = static_cast<const ParmVarDecl *>(
- N->getState()->get<ReleasedParameter>());
- const auto *OldPVD = static_cast<const ParmVarDecl *>(
- N->getFirstPred()->getState()->get<ReleasedParameter>());
- if (OldPVD == NewPVD)
- return nullptr;
-
- assert(NewPVD && "What is deallocated cannot be un-deallocated!");
- SmallString<64> Str;
- llvm::raw_svector_ostream OS(Str);
- OS << "Value passed through parameter '" << NewPVD->getName()
- << "' is deallocated";
-
- PathDiagnosticLocation Loc =
- PathDiagnosticLocation::create(N->getLocation(), BRC.getSourceManager());
- return std::make_shared<PathDiagnosticEventPiece>(Loc, OS.str());
-}
+REGISTER_TRAIT_WITH_PROGRAMSTATE(ReleasedParameter, bool)
static const ParmVarDecl *getOriginParam(SVal V, CheckerContext &C) {
SymbolRef Sym = V.getAsSymbol();
@@ -195,7 +162,16 @@
if (!PVD)
return;
- C.addTransition(C.getState()->set<ReleasedParameter>(PVD));
+ const NoteTag *T = C.getNoteTag([this, PVD](BugReport &BR) -> std::string {
+ if (&BR.getBugType() != &BT)
+ return "";
+ SmallString<64> Str;
+ llvm::raw_svector_ostream OS(Str);
+ OS << "Value passed through parameter '" << PVD->getName()
+ << "\' is deallocated";
+ return OS.str();
+ });
+ C.addTransition(C.getState()->set<ReleasedParameter>(true), T);
}
// Returns true if V can potentially represent a "successful" kern_return_t.
@@ -260,7 +236,6 @@
R->addRange(RS->getSourceRange());
bugreporter::trackExpressionValue(N, RS->getRetValue(), *R, false);
- R->addVisitor(llvm::make_unique<Visitor>());
C.emitReport(std::move(R));
}
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,24 @@
Eng.getBugReporter().emitReport(std::move(R));
}
+
+ /// Produce a program point tag that displays an additional path note
+ /// to the user. This is a lightweight 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().makeNoteTag(std::move(Cb));
+ }
+
+ /// A shorthand version of getNoteTag that doesn't require you to accept
+ /// the BugReporterContext arguments when you don't need it.
+ const NoteTag *getNoteTag(std::function<std::string(BugReport &)> &&Cb) {
+ return getNoteTag(
+ [Cb](BugReporterContext &, BugReport &BR) { return Cb(BR); });
+ }
+
+
/// 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,74 @@
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;
+ mutable Optional<std::string> MemoizedMessage;
+
+ NoteTag(Callback &&Cb) : ProgramPointTag(&Kind), Cb(std::move(Cb)) {}
+
+public:
+ static bool classof(const ProgramPointTag *T) {
+ return T->getTagKind() == &Kind;
+ }
+
+ Optional<std::string> generateMessage(BugReporterContext &BRC,
+ BugReport &R) const {
+ if (!MemoizedMessage)
+ MemoizedMessage = Cb(BRC, R);
+ // Empty string is converted to lack of note.
+ // This simplifies the checker API.
+ if (MemoizedMessage->empty())
+ return None;
+
+ return *MemoizedMessage;
+ }
+
+ StringRef getTagDescription() const override {
+ if (MemoizedMessage)
+ return *MemoizedMessage;
+ else
+ return "Untriggered Note Tag";
+ }
+
+ // Manage memory for NoteTag objects.
+ class Factory {
+ std::vector<std::unique_ptr<NoteTag>> Tags;
+
+ public:
+ const NoteTag *makeNoteTag(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
