[analyzer] Change the way in which IDC Visitor decides to kick in and make sure it attaches in the given edge case

In the test case below, the value V is not constrained to 0 in ErrorNode but it is in node N.
So we used to fail to register the Suppression visitor.

We also need to change the way we determine that the Visitor should kick in because the node N belongs to
the ExplodedGraph and might not be on the BugReporter path that the visitor sees. Instead of trying to match the node,
turn on the visitor when we see the last node in which the symbol is ‘0’.

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@177121 91177308-0d34-0410-b5e6-96231b3b80d8

include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h

@@ -290,12 +290,12 @@ class SuppressInlineDefensiveChecksVisitor
   /// 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
+  /// 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;
+  /// might not hold when node visitation starts. We are going to start tracking
+  /// from the last node in which the value is null.
+  bool IsTrackingTurnedOn;
 
 public:
   SuppressInlineDefensiveChecksVisitor(DefinedSVal Val, const ExplodedNode *N);
@@ -306,10 +306,6 @@ public:
   /// to make all PathDiagnosticPieces created by this visitor.
   static const char *getTag();
 
-  PathDiagnosticPiece *getEndPath(BugReporterContext &BRC,
-                                  const ExplodedNode *N,
-                                  BugReport &BR);
-
   PathDiagnosticPiece *VisitNode(const ExplodedNode *Succ,
                                  const ExplodedNode *Pred,
                                  BugReporterContext &BRC,

lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

@@ -700,16 +700,14 @@ TrackConstraintBRVisitor::VisitNode(const ExplodedNode *N,
 
 SuppressInlineDefensiveChecksVisitor::
 SuppressInlineDefensiveChecksVisitor(DefinedSVal Value, const ExplodedNode *N)
-  : V(Value), IsSatisfied(false), StartN(N) {
-
-  assert(N->getState()->isNull(V).isConstrainedTrue() &&
-         "The visitor only tracks the cases where V is constrained to 0");
+  : V(Value), IsSatisfied(false), IsTrackingTurnedOn(false) {
+    assert(N->getState()->isNull(V).isConstrainedTrue() &&
+           "The visitor only tracks the cases where V is constrained to 0");
 }
 
 void SuppressInlineDefensiveChecksVisitor::Profile(FoldingSetNodeID &ID) const {
   static int id = 0;
   ID.AddPointer(&id);
-  ID.AddPointer(StartN);
   ID.Add(V);
 }
 
@@ -717,15 +715,6 @@ const char *SuppressInlineDefensiveChecksVisitor::getTag() {
   return "IDCVisitor";
 }
 
-PathDiagnosticPiece *
-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,
@@ -733,18 +722,19 @@ SuppressInlineDefensiveChecksVisitor::VisitNode(const ExplodedNode *Succ,
                                                 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;
+  BRC.getBugReporter().getEngine().getAnalysisManager().options;
   if (!Options.shouldSuppressInlinedDefensiveChecks())
     return 0;
 
+  // Start tracking after we see the first state in which the value is null.
+  if (!IsTrackingTurnedOn)
+    if (Succ->getState()->isNull(V).isConstrainedTrue())
+      IsTrackingTurnedOn = true;
+  if (!IsTrackingTurnedOn)
+    return 0;
+
+
   // Check if in the previous state it was feasible for this value
   // to *not* be null.
   if (Pred->getState()->assume(V, true)) {
@@ -899,10 +889,10 @@ bool bugreporter::trackNullOrUndefValue(const ExplodedNode *ErrorNode,
         report.addVisitor(ConstraintTracker);
 
         // Add visitor, which will suppress inline defensive checks.
-        if (ErrorNode->getState()->isNull(V).isConstrainedTrue()) {
+        if (N->getState()->isNull(V).isConstrainedTrue()) {
           BugReporterVisitor *IDCSuppressor =
             new SuppressInlineDefensiveChecksVisitor(V.castAs<DefinedSVal>(),
-                                                     ErrorNode);
+                                                     N);
           report.addVisitor(IDCSuppressor);
         }
       }

test/Analysis/inlining/inline-defensive-checks.cpp

@@ -1,6 +1,12 @@
 // RUN: %clang_cc1 -analyze -analyzer-checker=core -verify %s
 // expected-no-diagnostics
 
+extern void __assert_fail (__const char *__assertion, __const char *__file,
+                           unsigned int __line, __const char *__function)
+__attribute__ ((__noreturn__));
+#define assert(expr) \
+((expr)  ? (void)(0)  : __assert_fail (#expr, __FILE__, __LINE__, __func__))
+
 class ButterFly {
 private:
   ButterFly() { }
@@ -29,4 +35,21 @@ class X{
 		subtest1();
 		return subtest2();
 	}
-};
+};
+
+typedef const int *Ty;
+extern
+Ty notNullArg(Ty cf) __attribute__((nonnull));
+typedef const void *CFTypeRef;
+extern Ty getTyVal();
+inline void radar13224271_callee(Ty def, Ty& result ) {
+	result = def;
+  // Clearly indicates that result cannot be 0 if def is not NULL.
+	assert( (result != 0) || (def == 0) );
+}
+void radar13224271_caller()
+{
+	Ty value;
+	radar13224271_callee(getTyVal(), value );
+	notNullArg(value); // no-warning
+}