[analyzer] Note last writes to a condition only in a nested stackframe

Exactly what it says on the tin! The comments in the code detail this a
little more too.

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

llvm-svn: 368817

3 files changed, 61 insertions, 31 deletions

diff --git a/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h b/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
index 4ca38079d02..9069270a38e 100644
--- a/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
+++ b/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
@@ -132,6 +132,7 @@ class FindLastStoreBRVisitor final : public BugReporterVisitor {
 
   using TrackingKind = bugreporter::TrackingKind;
   TrackingKind TKind;
+  const StackFrameContext *OriginSFC;
 
 public:
   /// Creates a visitor for every VarDecl inside a Stmt and registers it with
@@ -145,11 +146,18 @@ public:
   /// \param EnableNullFPSuppression Whether we should employ false positive
   ///         suppression (inlined defensive checks, returned null).
   /// \param TKind May limit the amount of notes added to the bug report.
+  /// \param OriginSFC Only adds notes when the last store happened in a
+  ///        different stackframe to this one. Disregarded if the tracking kind
+  ///        is thorough.
+  ///        This is useful, because for non-tracked regions, notes about
+  ///        changes to its value in a nested stackframe could be pruned, and
+  ///        this visitor can prevent that without polluting the bugpath too
+  ///        much.
   FindLastStoreBRVisitor(KnownSVal V, const MemRegion *R,
-                         bool InEnableNullFPSuppression,
-                         TrackingKind TKind)
+                         bool InEnableNullFPSuppression, TrackingKind TKind,
+                         const StackFrameContext *OriginSFC = nullptr)
       : R(R), V(V), EnableNullFPSuppression(InEnableNullFPSuppression),
-        TKind(TKind) {
+        TKind(TKind), OriginSFC(OriginSFC) {
     assert(R);
   }
 
diff --git a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
index f0c8de26bc1..8c17a367b8a 100644
--- a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -1440,6 +1440,10 @@ FindLastStoreBRVisitor::VisitNode(const ExplodedNode *Succ,
         StoreSite, InitE, BR, TKind, EnableNullFPSuppression);
   }
 
+  if (TKind == TrackingKind::Condition &&
+      !OriginSFC->isParentOf(StoreSite->getStackFrame()))
+    return nullptr;
+
   // Okay, we've found the binding. Emit an appropriate message.
   SmallString<256> sbuf;
   llvm::raw_svector_ostream os(sbuf);
@@ -1465,7 +1469,7 @@ FindLastStoreBRVisitor::VisitNode(const ExplodedNode *Succ,
           if (const VarRegion *OriginalR = BDR->getOriginalRegion(VR)) {
             if (auto KV = State->getSVal(OriginalR).getAs<KnownSVal>())
               BR.addVisitor(llvm::make_unique<FindLastStoreBRVisitor>(
-                  *KV, OriginalR, EnableNullFPSuppression, TKind));
+                  *KV, OriginalR, EnableNullFPSuppression, TKind, OriginSFC));
           }
         }
       }
@@ -1890,6 +1894,7 @@ bool bugreporter::trackExpressionValue(const ExplodedNode *InputNode,
     return false;
 
   ProgramStateRef LVState = LVNode->getState();
+  const StackFrameContext *SFC = LVNode->getStackFrame();
 
   // We only track expressions if we believe that they are important. Chances
   // are good that control dependencies to the tracking point are also improtant
@@ -1926,7 +1931,7 @@ bool bugreporter::trackExpressionValue(const ExplodedNode *InputNode,
     if (RR && !LVIsNull)
       if (auto KV = LVal.getAs<KnownSVal>())
         report.addVisitor(llvm::make_unique<FindLastStoreBRVisitor>(
-              *KV, RR, EnableNullFPSuppression, TKind));
+            *KV, RR, EnableNullFPSuppression, TKind, SFC));
 
     // In case of C++ references, we want to differentiate between a null
     // reference and reference to null pointer.
@@ -1963,7 +1968,7 @@ bool bugreporter::trackExpressionValue(const ExplodedNode *InputNode,
 
       if (auto KV = V.getAs<KnownSVal>())
         report.addVisitor(llvm::make_unique<FindLastStoreBRVisitor>(
-              *KV, R, EnableNullFPSuppression, TKind));
+            *KV, R, EnableNullFPSuppression, TKind, SFC));
       return true;
     }
   }
@@ -2002,7 +2007,7 @@ bool bugreporter::trackExpressionValue(const ExplodedNode *InputNode,
     if (CanDereference)
       if (auto KV = RVal.getAs<KnownSVal>())
         report.addVisitor(llvm::make_unique<FindLastStoreBRVisitor>(
-            *KV, L->getRegion(), EnableNullFPSuppression, TKind));
+            *KV, L->getRegion(), EnableNullFPSuppression, TKind, SFC));
 
     const MemRegion *RegionRVal = RVal.getAsRegion();
     if (RegionRVal && isa<SymbolicRegion>(RegionRVal)) {
diff --git a/clang/test/Analysis/track-control-dependency-conditions.cpp b/clang/test/Analysis/track-control-dependency-conditions.cpp
index 644eb6296fe..35769f24739 100644
--- a/clang/test/Analysis/track-control-dependency-conditions.cpp
+++ b/clang/test/Analysis/track-control-dependency-conditions.cpp
@@ -127,10 +127,9 @@ int bar;
 void test() {
   int *x = 0; // expected-note{{'x' initialized to a null pointer value}}
 
-  if (int flag = foo()) // tracking-note{{'flag' initialized here}}
-                        // debug-note@-1{{Tracking condition 'flag'}}
-                        // expected-note@-2{{Assuming 'flag' is not equal to 0}}
-                        // expected-note@-3{{Taking true branch}}
+  if (int flag = foo()) // debug-note{{Tracking condition 'flag'}}
+                        // expected-note@-1{{Assuming 'flag' is not equal to 0}}
+                        // expected-note@-2{{Taking true branch}}
 
     *x = 5; // expected-warning{{Dereference of null pointer}}
             // expected-note@-1{{Dereference of null pointer}}
@@ -157,6 +156,28 @@ void test() {
 
 } // end of namespace variable_declaration_in_condition
 
+namespace note_from_different_but_not_nested_stackframe {
+
+void nullptrDeref(int *ptr, bool True) {
+  if (True) // expected-note{{'True' is true}}
+            // expected-note@-1{{Taking true branch}}
+            // debug-note@-2{{Tracking condition 'True}}
+    *ptr = 5;
+  // expected-note@-1{{Dereference of null pointer (loaded from variable 'ptr')}}
+  // expected-warning@-2{{Dereference of null pointer (loaded from variable 'ptr')}}
+}
+
+void f() {
+  int *ptr = nullptr;
+  // expected-note@-1{{'ptr' initialized to a null pointer value}}
+  bool True = true;
+  nullptrDeref(ptr, True);
+  // expected-note@-1{{Passing null pointer value via 1st parameter 'ptr'}}
+  // expected-note@-2{{Calling 'nullptrDeref'}}
+}
+
+} // end of namespace note_from_different_but_not_nested_stackframe
+
 namespace important_returning_pointer_loaded_from {
 bool coin();
 
@@ -194,8 +215,8 @@ bool coin();
 int *getIntPtr();
 
 int *conjurePointer() {
-  int *i = getIntPtr(); // tracking-note{{'i' initialized here}}
-  return i;             // tracking-note{{Returning pointer (loaded from 'i')}}
+  int *i = getIntPtr();
+  return i;
 }
 
 void f(int *ptr) {
@@ -203,11 +224,9 @@ void f(int *ptr) {
            // expected-note@-1{{Taking false branch}}
     ;
   if (!conjurePointer())
-    // tracking-note@-1{{Calling 'conjurePointer'}}
-    // tracking-note@-2{{Returning from 'conjurePointer'}}
-    // debug-note@-3{{Tracking condition '!conjurePointer()'}}
-    // expected-note@-4{{Assuming the condition is true}}
-    // expected-note@-5{{Taking true branch}}
+    // debug-note@-1{{Tracking condition '!conjurePointer()'}}
+    // expected-note@-2{{Assuming the condition is true}}
+    // expected-note@-3{{Taking true branch}}
     *ptr = 5; // expected-warning{{Dereference of null pointer}}
               // expected-note@-1{{Dereference of null pointer}}
 }
@@ -225,10 +244,9 @@ void f() {
   int *x = 0; // expected-note{{'x' initialized to a null pointer value}}
 
   if (cast(conjure()))
-    // tracking-note@-1{{Passing value via 1st parameter 'P'}}
-    // debug-note@-2{{Tracking condition 'cast(conjure())'}}
-    // expected-note@-3{{Assuming the condition is false}}
-    // expected-note@-4{{Taking false branch}}
+    // debug-note@-1{{Tracking condition 'cast(conjure())'}}
+    // expected-note@-2{{Assuming the condition is false}}
+    // expected-note@-3{{Taking false branch}}
     return;
   *x = 5; // expected-warning{{Dereference of null pointer}}
           // expected-note@-1{{Dereference of null pointer}}
@@ -314,7 +332,7 @@ namespace tracked_condition_is_only_initialized {
 int getInt();
 
 void f() {
-  int flag = getInt(); // tracking-note{{'flag' initialized here}}
+  int flag = getInt();
   int *x = 0; // expected-note{{'x' initialized to a null pointer value}}
   if (flag) // expected-note{{Assuming 'flag' is not equal to 0}}
             // expected-note@-1{{Taking true branch}}
@@ -329,8 +347,8 @@ int flag;
 int getInt();
 
 void f(int y) {
-  y = 1; // tracking-note{{The value 1 is assigned to 'y'}}
-  flag = y; // tracking-note{{The value 1 is assigned to 'flag'}}
+  y = 1;
+  flag = y;
 
   int *x = 0; // expected-note{{'x' initialized to a null pointer value}}
   if (flag) // expected-note{{'flag' is 1}}
@@ -347,9 +365,8 @@ int getInt();
 
 void foo() {
   int y;
-  y = 1; // tracking-note{{The value 1 is assigned to 'y'}}
+  y = 1;
   flag = y; // tracking-note{{The value 1 is assigned to 'flag'}}
-
 }
 
 void f(int y) {
@@ -378,9 +395,9 @@ void f() {
   int *x = 0; // expected-note{{'x' initialized to a null pointer value}}
   int y = 0;
 
-  foo();    // tracking-note{{Calling 'foo'}}
-            // tracking-note@-1{{Returning from 'foo'}}
-  y = flag; // tracking-note{{Value assigned to 'y'}}
+  foo(); // tracking-note{{Calling 'foo'}}
+         // tracking-note@-1{{Returning from 'foo'}}
+  y = flag;
 
   if (y)    // expected-note{{Assuming 'y' is not equal to 0}}
             // expected-note@-1{{Taking true branch}}
@@ -429,7 +446,7 @@ int getInt();
 void f(int flag) {
   int *x = 0; // expected-note{{'x' initialized to a null pointer value}}
 
-  flag = getInt(); // tracking-note{{Value assigned to 'flag'}}
+  flag = getInt();
   assert(flag); // tracking-note{{Calling 'assert'}}
                 // tracking-note@-1{{Returning from 'assert'}}