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

[NoQ]

Summary

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.

[Szelethus]

Hmm, so before this patch, we just used LVNode everywhere and ignored InputNode. It may not have made much sense, but its still not as confusing as using both if the creation of LVNode is unnecessary overall. Could we just remove it?

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

1912–1922

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.

So, its possible that this function, and its uses in trackExpressionValue is completely unnecessary?

2007–2009

Too late in terms of too close to the root of the ExplodedGraph, or the other way around? This is a bitconfusing since visitors are inspecting the graph in reverse.

[NoQ]

Yeah, i think we should avoid such peeking and instead try to do everything in one pass. I.e., if we need to peek at the node above us, just make a visitor that delays the decision until it has precisely the information it needs. I guess i'll be slooowly moving in this direction.

Also every time i see special handling of DeclRefExprs i get pretty worried. It usually means that this code almost never works.

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

1912–1922

Of course it's necessary! Why would you ever write a system of mutually recursive visitors and then never implement an ad-hoc visitor within one of these visitors that allows you to quadratically visit nodes while you visit nodes, before actually visiting them in the real visitor? (/sarcasm)

[Szelethus]

If you could change that, it would be awesome! But since this revision has its own side effect, let's commit as-is. LGTM!

[Charusso]

[...] 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.

Yes, that is pretty interesting. I have found error nodes which after the purge program point, so when we arbitrarily pick the last node to prevent false alarms we could pick the last node which has more information, 2-3 nodes earlier on the path. I am not sure whether this new approach would be useful here, other than that I like the idea.

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

 
 SuppressInlineDefensiveChecksVisitor::
 SuppressInlineDefensiveChecksVisitor(DefinedSVal Value, const ExplodedNode *N)
     : V(Value) {
   // Check if the visitor is disabled.
   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(
     llvm::FoldingSetNodeID &ID) const {
   static int id = 0;
   ID.AddPointer(&id);
   ID.Add(V);
 }
SuppressInlineDefensiveChecksVisitor::VisitNode(const ExplodedNode *Succ,
   if (!IsTrackingTurnedOn)
     if (Succ->getState()->isNull(V).isConstrainedTrue())
       IsTrackingTurnedOn = true;
   if (!IsTrackingTurnedOn)
     return nullptr;
 
   // 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);
       return nullptr;
     }
 
     // Treat defensive checks in function-like macros as if they were an inlined
     // defensive check. If the bug location is not in a macro and the
if (auto *UO = dyn_cast<UnaryOperator>(Ex)) {
     if (UO->getOpcode() == UO_AddrOf && UO->getSubExpr()->isLValue())
       if (const Expr *DerefEx = bugreporter::getDerefExpr(UO->getSubExpr()))
         return peelOffOuterExpr(DerefEx, N);
   }
 
   return Ex;
 }
 
 /// Find the ExplodedNode where the lvalue (the value of 'Ex')
 /// was computed.
 static const ExplodedNode* findNodeForExpression(const ExplodedNode *N,
                                                  const Expr *Inner) {
   while (N) {
     if (N->getStmtForDiagnostics() == Inner)
       return N;
     N = N->getFirstPred();
   }
   return N;
 }
 
 bool bugreporter::trackExpressionValue(const ExplodedNode *InputNode,
                                        const Expr *E,
                                        PathSensitiveBugReport &report,
                                        bugreporter::TrackingKind TKind,
                                        bool EnableNullFPSuppression) {
 
   if (!E || !InputNode)
if (R) {
       // If the contents are symbolic and null, find out when they became null.
       if (V.getAsLocSymbol(/*IncludeBaseRegions=*/true))
         if (LVState->isNull(V).isConstrainedTrue())
           report.addVisitor(std::make_unique<TrackConstraintBRVisitor>(
               V.castAs<DefinedSVal>(), false));
 
       // 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>(
             *KV, R, EnableNullFPSuppression, TKind, SFC));
       return true;
     }
   }
 

clang/test/Analysis/NSContainers.m

 // RUN: %clang_analyze_cc1  -Wno-objc-literal-conversion -analyzer-checker=core,osx.cocoa.NonNilReturnValue,osx.cocoa.NilArg,osx.cocoa.Loops,debug.ExprInspection -verify -Wno-objc-root-class %s
 
 void clang_analyzer_eval(int);
 
+#define nil ((id)0)
+
 typedef unsigned long NSUInteger;
 typedef signed char BOOL;
 typedef struct _NSZone NSZone;
 @class NSInvocation, NSMethodSignature, NSCoder, NSString, NSEnumerator;
 @protocol NSObject
 @end
 @protocol NSCopying
 - (id)copyWithZone:(NSZone *)zone;
void testNoReportWhenReceiverNil(NSMutableArray *array, int b) {
   }
 
   MyView *view = b ? [[MyView alloc] init] : 0;
   NSMutableArray *subviews = [[view subviews] mutableCopy];
   // When view is nil, subviews is also nil so there should be no warning
   // 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
+}