r176756 - [analyzer] Make Suppress IDC checker aware that it might not start from the same node it was registered at
Anna Zaks
ganna at apple.com
Fri Mar 8 19:23:19 PST 2013
Author: zaks
Date: Fri Mar 8 21:23:19 2013
New Revision: 176756
URL: http://llvm.org/viewvc/llvm-project?rev=176756&view=rev
Log:
[analyzer] Make Suppress IDC checker aware that it might not start from the same node it was registered at
The visitor used to assume that the value it’s tracking is null in the first node it examines. This is not true.
If we are registering the Suppress Inlined Defensive checks visitor while traversing in another visitor
(such as FindlastStoreVisitor). When we restart with the IDC visitor, the invariance of the visitor does
not hold since the symbol we are tracking no longer exists at that point.
I had to pass the ErrorNode when creating the IDC visitor, because, in some cases, node N is
neither the error node nor will be visible along the path (we had not finalized the path at that point
and are dealing with ExplodedGraph.)
We should revisit the other visitors which might not be aware that they might get nodes, which are
later in path than the trigger point.
This suppresses a number of inline defensive checks in JavaScriptCore.
Added:
cfe/trunk/test/Analysis/inlining/inline-defensive-checks.cpp
Modified:
cfe/trunk/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h
cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
Modified: cfe/trunk/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h?rev=176756&r1=176755&r2=176756&view=diff
==============================================================================
--- cfe/trunk/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h (original)
+++ cfe/trunk/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h Fri Mar 8 21:23:19 2013
@@ -55,8 +55,8 @@ public:
///
/// The last parameter can be used to register a new visitor with the given
/// BugReport while processing a node.
- virtual PathDiagnosticPiece *VisitNode(const ExplodedNode *N,
- const ExplodedNode *PrevN,
+ virtual PathDiagnosticPiece *VisitNode(const ExplodedNode *Succ,
+ const ExplodedNode *Pred,
BugReporterContext &BRC,
BugReport &BR) = 0;
@@ -283,13 +283,20 @@ public:
class SuppressInlineDefensiveChecksVisitor
: public BugReporterVisitorImpl<SuppressInlineDefensiveChecksVisitor>
{
- // The symbolic value for which we are tracking constraints.
- // This value is constrained to null in the end of path.
+ /// The symbolic value for which we are tracking constraints.
+ /// This value is constrained to null in the end of path.
DefinedSVal V;
- // Track if we found the node where the constraint was first added.
+ /// Track if we found the node where the constraint was first added.
bool IsSatisfied;
+ /// \brief The node from which we should start tracking the value.
+ /// Note: Since the visitors can be registered on nodes previous to the last
+ /// node in the BugReport, but the path traversal always starts with the last
+ /// node, the visitor invariant (that we start with a node in which V is null)
+ /// might not hold when node visitation starts.
+ const ExplodedNode *StartN;
+
public:
SuppressInlineDefensiveChecksVisitor(DefinedSVal Val, const ExplodedNode *N);
@@ -299,8 +306,12 @@ public:
/// to make all PathDiagnosticPieces created by this visitor.
static const char *getTag();
- PathDiagnosticPiece *VisitNode(const ExplodedNode *N,
- const ExplodedNode *PrevN,
+ PathDiagnosticPiece *getEndPath(BugReporterContext &BRC,
+ const ExplodedNode *N,
+ BugReport &BR);
+
+ PathDiagnosticPiece *VisitNode(const ExplodedNode *Succ,
+ const ExplodedNode *Pred,
BugReporterContext &BRC,
BugReport &BR);
};
Modified: cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp?rev=176756&r1=176755&r2=176756&view=diff
==============================================================================
--- cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp Fri Mar 8 21:23:19 2013
@@ -695,7 +695,7 @@ TrackConstraintBRVisitor::VisitNode(cons
SuppressInlineDefensiveChecksVisitor::
SuppressInlineDefensiveChecksVisitor(DefinedSVal Value, const ExplodedNode *N)
- : V(Value), IsSatisfied(false) {
+ : V(Value), IsSatisfied(false), StartN(N) {
assert(N->getState()->isNull(V).isConstrainedTrue() &&
"The visitor only tracks the cases where V is constrained to 0");
@@ -704,6 +704,7 @@ SuppressInlineDefensiveChecksVisitor(Def
void SuppressInlineDefensiveChecksVisitor::Profile(FoldingSetNodeID &ID) const {
static int id = 0;
ID.AddPointer(&id);
+ ID.AddPointer(StartN);
ID.Add(V);
}
@@ -712,13 +713,28 @@ const char *SuppressInlineDefensiveCheck
}
PathDiagnosticPiece *
-SuppressInlineDefensiveChecksVisitor::VisitNode(const ExplodedNode *N,
- const ExplodedNode *PrevN,
+SuppressInlineDefensiveChecksVisitor::getEndPath(BugReporterContext &BRC,
+ const ExplodedNode *N,
+ BugReport &BR) {
+ if (StartN == BR.getErrorNode())
+ StartN = 0;
+ return 0;
+}
+
+PathDiagnosticPiece *
+SuppressInlineDefensiveChecksVisitor::VisitNode(const ExplodedNode *Succ,
+ const ExplodedNode *Pred,
BugReporterContext &BRC,
BugReport &BR) {
if (IsSatisfied)
return 0;
+ // Start tracking after we see node StartN.
+ if (StartN == Succ)
+ StartN = 0;
+ if (StartN)
+ return 0;
+
AnalyzerOptions &Options =
BRC.getBugReporter().getEngine().getAnalysisManager().options;
if (!Options.shouldSuppressInlinedDefensiveChecks())
@@ -726,14 +742,13 @@ SuppressInlineDefensiveChecksVisitor::Vi
// Check if in the previous state it was feasible for this value
// to *not* be null.
- if (PrevN->getState()->assume(V, true)) {
+ if (Pred->getState()->assume(V, true)) {
IsSatisfied = true;
- // TODO: Investigate if missing the transition point, where V
- // is non-null in N could lead to false negatives.
+ assert(!Succ->getState()->assume(V, true));
- // Check if this is inline defensive checks.
- const LocationContext *CurLC = N->getLocationContext();
+ // Check if this is inlined defensive checks.
+ const LocationContext *CurLC =Succ->getLocationContext();
const LocationContext *ReportLC = BR.getErrorNode()->getLocationContext();
if (CurLC != ReportLC && !CurLC->isParentOf(ReportLC))
BR.markInvalid("Suppress IDC", CurLC);
@@ -741,14 +756,17 @@ SuppressInlineDefensiveChecksVisitor::Vi
return 0;
}
-bool bugreporter::trackNullOrUndefValue(const ExplodedNode *N, const Stmt *S,
+bool bugreporter::trackNullOrUndefValue(const ExplodedNode *ErrorNode,
+ const Stmt *S,
BugReport &report, bool IsArg) {
- if (!S || !N)
+ if (!S || !ErrorNode)
return false;
if (const OpaqueValueExpr *OVE = dyn_cast<OpaqueValueExpr>(S))
S = OVE->getSourceExpr();
+ const ExplodedNode *N = ErrorNode;
+
const Expr *LValue = 0;
if (const Expr *Ex = dyn_cast<Expr>(S)) {
Ex = Ex->IgnoreParenCasts();
@@ -850,10 +868,10 @@ bool bugreporter::trackNullOrUndefValue(
report.addVisitor(ConstraintTracker);
// Add visitor, which will suppress inline defensive checks.
- if (N->getState()->isNull(V).isConstrainedTrue()) {
+ if (ErrorNode->getState()->isNull(V).isConstrainedTrue()) {
BugReporterVisitor *IDCSuppressor =
new SuppressInlineDefensiveChecksVisitor(V.castAs<DefinedSVal>(),
- N);
+ ErrorNode);
report.addVisitor(IDCSuppressor);
}
}
Added: cfe/trunk/test/Analysis/inlining/inline-defensive-checks.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/inlining/inline-defensive-checks.cpp?rev=176756&view=auto
==============================================================================
--- cfe/trunk/test/Analysis/inlining/inline-defensive-checks.cpp (added)
+++ cfe/trunk/test/Analysis/inlining/inline-defensive-checks.cpp Fri Mar 8 21:23:19 2013
@@ -0,0 +1,32 @@
+// RUN: %clang_cc1 -analyze -analyzer-checker=core -verify %s
+// expected-no-diagnostics
+
+class ButterFly {
+private:
+ ButterFly() { }
+public:
+ int triggerderef() {
+ return 0;
+ }
+};
+ButterFly *getInP();
+class X{
+ ButterFly *p;
+ void setP(ButterFly *inP) {
+ if(inP)
+ ;
+ p = inP;
+ };
+ void subtest1() {
+ ButterFly *inP = getInP();
+ setP(inP);
+ }
+ int subtest2() {
+ int c = p->triggerderef(); // no-warning
+ return c;
+ }
+ int test() {
+ subtest1();
+ return subtest2();
+ }
+};
\ No newline at end of file
More information about the cfe-commits
mailing list