[PATCH] D67932: [analyzer] Fix accidentally skipping the call during inlined defensive check suppression.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a_sidorin, rnkovacs, Szelethus, baloghadamsoftware, Charusso.
Herald added subscribers: cfe-commits, dkrupp, donat.nagy, mikhail.ramalho, a.sidorin, szepet.
Herald added a project: clang.

Got myself distracted with a tiny fix for the 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). I'm quite confused about the idea behind it and i highly doubt that it actually works correctly, but one reason why we can't simply use the error node may be that the binding to that variable might have already disappeared from the state by the time the bug is found.

This time i noticed that for 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.

Therefore i 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. I still use "`LVState`" to find out which value should we be tracking, so there shouldn't be any problems with accidentally loading an ill-formed value from a dead variable.

I hope it's one tiny piece of understanding that'll bring us to a better architecture of this code.


Repository:
  rC Clang

https://reviews.llvm.org/D67932

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


Index: clang/test/Analysis/NSContainers.m
===================================================================
--- clang/test/Analysis/NSContainers.m
+++ 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 @@
   // 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
+}
Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -1601,9 +1601,6 @@
   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(
@@ -1634,13 +1631,12 @@
 
   // 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);
@@ -2007,11 +2003,16 @@
 
       // 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; the lvalue may have been computed
+          // before the inlined call was evaluated. InputNode may as well be
+          // too late, 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>(


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D67932.221408.patch
Type: text/x-patch
Size: 3253 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20190923/02e587db/attachment.bin>