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

[Artem Dergachev via cfe-commits]

Author: Artem Dergachev
Date: 2019-11-08T18:27:14-08:00
New Revision: e4da37e8a0a3197baca674d683cb05341c6a4097

URL: https://github.com/llvm/llvm-project/commit/e4da37e8a0a3197baca674d683cb05341c6a4097
DIFF: https://github.com/llvm/llvm-project/commit/e4da37e8a0a3197baca674d683cb05341c6a4097.diff

LOG: [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

Added: 
    

Modified: 
    clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
    clang/test/Analysis/NSContainers.m

Removed: 
    


################################################################################
diff  --git a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
index 7ba93b858baf..16047055862e 100644
--- a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ b/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>(

diff  --git a/clang/test/Analysis/NSContainers.m b/clang/test/Analysis/NSContainers.m
index ac33efc1174a..74db771a52d0 100644
--- a/clang/test/Analysis/NSContainers.m
+++ b/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
+}