[analyzer] Fix skipping the call during inlined defensive check suppression.

When bugreporter::trackExpressionValue() is invoked on a DeclRefExpr,
it tries to do most of its computations over the node in which
this DeclRefExpr is computed, rather than on the error node (or whatever node
is stuffed into it). One reason why we can't simply use the error node is
that the binding to that variable might have already disappeared from the state
by the time the bug is found.

In case of the inlined defensive checks visitor, the DeclRefExpr node
is in fact sometimes too *early*: the call in which the inlined defensive check
has happened might have not been entered yet.

Change the visitor to be fine with tracking dead symbols (which it is totally
capable of - the collapse point for the symbol is still well-defined), and fire
it up directly on the error node. Keep using "LVState" to find out which value
should we be tracking, so that there weren't any problems with accidentally
loading an ill-formed value from a dead variable.

Differential Revision: https://reviews.llvm.org/D67932

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

@@ -1606,9 +1606,6 @@ SuppressInlineDefensiveChecksVisitor(DefinedSVal Value, const ExplodedNode *N)
   AnalyzerOptions &Options = N->getState()->getAnalysisManager().options;
   if (!Options.ShouldSuppressInlinedDefensiveChecks)
     IsSatisfied = true;
-
-  assert(N->getState()->isNull(V).isConstrainedTrue() &&
-         "The visitor only tracks the cases where V is constrained to 0");
 }
 
 void SuppressInlineDefensiveChecksVisitor::Profile(
@@ -1639,13 +1636,12 @@ SuppressInlineDefensiveChecksVisitor::VisitNode(const ExplodedNode *Succ,
 
   // Check if in the previous state it was feasible for this value
   // to *not* be null.
-  if (!Pred->getState()->isNull(V).isConstrainedTrue()) {
+  if (!Pred->getState()->isNull(V).isConstrainedTrue() &&
+      Succ->getState()->isNull(V).isConstrainedTrue()) {
     IsSatisfied = true;
 
-    assert(Succ->getState()->isNull(V).isConstrainedTrue());
-
     // Check if this is inlined defensive checks.
-    const LocationContext *CurLC =Succ->getLocationContext();
+    const LocationContext *CurLC = Succ->getLocationContext();
     const LocationContext *ReportLC = BR.getErrorNode()->getLocationContext();
     if (CurLC != ReportLC && !CurLC->isParentOf(ReportLC)) {
       BR.markInvalid("Suppress IDC", CurLC);
@@ -2012,11 +2008,16 @@ bool bugreporter::trackExpressionValue(const ExplodedNode *InputNode,
 
       // Add visitor, which will suppress inline defensive checks.
       if (auto DV = V.getAs<DefinedSVal>())
-        if (!DV->isZeroConstant() && LVState->isNull(*DV).isConstrainedTrue() &&
-            EnableNullFPSuppression)
+        if (!DV->isZeroConstant() && EnableNullFPSuppression) {
+          // Note that LVNode may be too late (i.e., too far from the InputNode)
+          // because the lvalue may have been computed before the inlined call
+          // was evaluated. InputNode may as well be too early here, because
+          // the symbol is already dead; this, however, is fine because we can
+          // still find the node in which it collapsed to null previously.
           report.addVisitor(
-              std::make_unique<SuppressInlineDefensiveChecksVisitor>(*DV,
-                                                                      LVNode));
+              std::make_unique<SuppressInlineDefensiveChecksVisitor>(
+                  *DV, InputNode));
+        }
 
       if (auto KV = V.getAs<KnownSVal>())
         report.addVisitor(std::make_unique<FindLastStoreBRVisitor>(

clang/test/Analysis/NSContainers.m

@@ -2,6 +2,8 @@
 
 void clang_analyzer_eval(int);
 
+#define nil ((id)0)
+
 typedef unsigned long NSUInteger;
 typedef signed char BOOL;
 typedef struct _NSZone NSZone;
@@ -310,3 +312,14 @@ void testNoReportWhenReceiverNil(NSMutableArray *array, int b) {
   // here either.
   [subviews addObject:view]; // no-warning
 }
+
+NSString *getStringFromString(NSString *string) {
+  if (!string)
+    return nil;
+  return @"New String";
+}
+void testInlinedDefensiveCheck(NSMutableDictionary *dict, id obj) {
+  // The check in getStringFromString() is not a good indication
+  // that 'obj' can be nil in this context.
+  dict[obj] = getStringFromString(obj); // no-warning
+}