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

[Szelethus]

Summary

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

[Szelethus]

Rebase after D64271 being abandoned.

[Szelethus]

Gentle ping.

[NoQ]

Yup, looks good!

clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h

146–149 ↗

(On Diff #208275)

I definitely don't don't don't don't dislike quadruple negations.

[xazax.hun]

I have one small question otherwise looks good.

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

1457 ↗

(On Diff #210340)

I wonder if "nested" is a good term in the title of this revision. We could also show notes in the caller right? So it might also be an "enclosing" frame. And if so, is it desired? Since the "enclosing" frames should already be visible for the user without the additional notes. But correct me if I'm wrong :)

[NoQ]

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

1447–1454 ↗

(On Diff #210340)

Yeah, interesting, i guess we should also not track the value further. Otherwise it'd be weird that one piece in the middle is missing but everything else is still there.

[Szelethus]

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

1447–1454 ↗

(On Diff #210340)

That is what D64271 was for, and I think it would be a bad idea. What we should rather do is have a better note message:
"The value of 'flag' will be used in a condition expression later" or something like that.

clang/test/Analysis/track-control-dependency-conditions.cpp

263 ↗

(On Diff #210340)

Like here. We'd lose this note if we didn't track further. I'm already gathering data tho!

[Szelethus]

• Only in a nested stackframe. Not different stackframe.

cfe/trunk/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h

class FindLastStoreBRVisitor final : public BugReporterVisitor {
   bool Satisfied = false;
 
   /// If the visitor is tracking the value directly responsible for the
   /// bug, we are going to employ false positive suppression.
   bool EnableNullFPSuppression;
 
   using TrackingKind = bugreporter::TrackingKind;
   TrackingKind TKind;
+  const StackFrameContext *OriginSFC;
 
 public:
   /// Creates a visitor for every VarDecl inside a Stmt and registers it with
   /// the BugReport.
   static void registerStatementVarDecls(BugReport &BR, const Stmt *S,
                                         bool EnableNullFPSuppression,
                                         TrackingKind TKind);
 
   /// \param V We're searching for the store where \c R received this value.
   /// \param R The region we're tracking.
   /// \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);
   }
 
   void Profile(llvm::FoldingSetNodeID &ID) const override;
 
   PathDiagnosticPieceRef VisitNode(const ExplodedNode *N,
                                    BugReporterContext &BRC,
                                    BugReport &BR) override;

cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

FindLastStoreBRVisitor::VisitNode(const ExplodedNode *Succ,
   if (InitE) {
     if (!IsParam)
       InitE = InitE->IgnoreParenCasts();
 
     bugreporter::trackExpressionValue(
         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);
 
   if (Optional<PostStmt> PS = StoreSite->getLocationAs<PostStmt>()) {
     const Stmt *S = PS->getStmt();
     const char *action = nullptr;
     const auto *DS = dyn_cast<DeclStmt>(S);
if (DS) {
         // See if we can get the BlockVarRegion.
         ProgramStateRef State = StoreSite->getState();
         SVal V = StoreSite->getSVal(S);
         if (const auto *BDR =
               dyn_cast_or_null<BlockDataRegion>(V.getAsRegion())) {
           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));
           }
         }
       }
     }
     if (action)
       showBRDiagnostics(action, os, R, V, DS);
 
   } else if (StoreSite->getLocation().getAs<CallEnter>()) {
if (!E || !InputNode)
     return false;
 
   const Expr *Inner = peelOffOuterExpr(E, InputNode);
   const ExplodedNode *LVNode = findNodeForExpression(InputNode, Inner);
   if (!LVNode)
     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
   // because of this, let's explain why we believe control reached this point.
   // TODO: Shouldn't we track control dependencies of every bug location, rather
   // than only tracked expressions?
   if (LVState->getAnalysisManager().getAnalyzerOptions().ShouldTrackConditions)
     report.addVisitor(llvm::make_unique<TrackControlDependencyCondBRVisitor>(
if (ExplodedGraph::isInterestingLValueExpr(Inner)) {
     bool LVIsNull = LVState->isNull(LVal).isConstrainedTrue();
 
     // If this is a C++ reference to a null pointer, we are tracking the
     // pointer. In addition, we should find the store at which the reference
     // got initialized.
     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.
     // If the LVal is null, check if we are dealing with null reference.
     // For those, we want to track the location of the reference.
     const MemRegion *R = (RR && LVIsNull) ? RR :
         LVNode->getSVal(Inner).getAsRegion();
 
if (R) {
         if (!DV->isZeroConstant() && LVState->isNull(*DV).isConstrainedTrue() &&
             EnableNullFPSuppression)
           report.addVisitor(
               llvm::make_unique<SuppressInlineDefensiveChecksVisitor>(*DV,
                                                                       LVNode));
 
       if (auto KV = V.getAs<KnownSVal>())
         report.addVisitor(llvm::make_unique<FindLastStoreBRVisitor>(
-              *KV, R, EnableNullFPSuppression, TKind));
+            *KV, R, EnableNullFPSuppression, TKind, SFC));
       return true;
     }
   }
 
   // If the expression is not an "lvalue expression", we can still
   // track the constraints on its contents.
   SVal V = LVState->getSValAsScalarOrLoc(Inner, LVNode->getLocationContext());
 
if (auto L = V.getAs<loc::MemRegionVal>()) {
     if (ExplodedGraph::isInterestingLValueExpr(Inner))
       RVal = LVState->getRawSVal(L.getValue(), Inner->getType());
     else if (CanDereference)
       RVal = LVState->getSVal(L->getRegion());
 
     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)) {
       report.markInteresting(RegionRVal);
       report.addVisitor(llvm::make_unique<TrackConstraintBRVisitor>(
             loc::MemRegionVal(RegionRVal), /*assumption=*/false));
     }
   }

cfe/trunk/test/Analysis/track-control-dependency-conditions.cpp

bool foo() {
   return coin();
 }
 
 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}}
 }
 } // end of namespace variable_declaration_in_condition
 
 namespace conversion_to_bool {
 bool coin();
if (ConvertsToBool())
     // expected-note@-2{{Assuming the condition is true}}
     // expected-note@-3{{Taking true branch}}
     *x = 5; // expected-warning{{Dereference of null pointer}}
             // expected-note@-1{{Dereference of null pointer}}
 }
 
 } // 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();
 
 int *getIntPtr();
 
 void storeValue(int **i) {
   *i = getIntPtr(); // tracking-note{{Value assigned to 'i'}}
 }

 } // end of namespace important_returning_pointer_loaded_from
 
 namespace unimportant_returning_pointer_loaded_from {
 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) {
   if (ptr) // expected-note{{Assuming 'ptr' is null}}
            // 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}}
 }
 } // end of namespace unimportant_returning_pointer_loaded_from
 
 namespace unimportant_returning_pointer_loaded_from_through_cast {
 
 void *conjure();
 
 int *cast(void *P) {
   return static_cast<int *>(P);
 }
 
 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}}
 }
 
 } // end of namespace unimportant_returning_pointer_loaded_from_through_cast
 
 namespace unimportant_returning_value_note {
*ptr = 5; // expected-warning{{Dereference of null pointer}}
               // expected-note@-1{{Dereference of null pointer}}
 }
 } // end of namespace important_returning_value_note_in_linear_function
 
 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}}
             // debug-note@-2{{Tracking condition 'flag'}}
     *x = 5; // expected-warning{{Dereference of null pointer}}
             // expected-note@-1{{Dereference of null pointer}}
 }
 } // end of namespace tracked_condition_is_only_initialized
 
 namespace tracked_condition_written_in_same_stackframe {
 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}}
             // expected-note@-1{{Taking true branch}}
             // debug-note@-2{{Tracking condition 'flag'}}
     *x = 5; // expected-warning{{Dereference of null pointer}}
             // expected-note@-1{{Dereference of null pointer}}
 }
 } // end of namespace tracked_condition_written_in_same_stackframe
 
 namespace tracked_condition_written_in_nested_stackframe {
 int flag;
 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) {
   int *x = 0; // expected-note{{'x' initialized to a null pointer value}}
 
   foo(); // tracking-note{{Calling 'foo'}}
          // tracking-note@-1{{Returning from 'foo'}}
 

 void foo() {
   flag = getInt(); // tracking-note{{Value assigned to 'flag'}}
 }
 
 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'}}
+  y = flag;
 
   if (y)    // expected-note{{Assuming 'y' is not equal to 0}}
             // expected-note@-1{{Taking true branch}}
             // debug-note@-2{{Tracking condition 'y'}}
     *x = 5; // expected-warning{{Dereference of null pointer}}
             // expected-note@-1{{Dereference of null pointer}}
 }
 } // end of namespace condition_written_in_nested_stackframe_before_assignment
if (!b) // tracking-note{{Assuming 'b' is not equal to 0}}
           // tracking-note@-1{{Taking false branch}}
     halt();
 }
 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'}}
 
   if (flag) // expected-note{{'flag' is not equal to 0}}
             // expected-note@-1{{Taking true branch}}
             // debug-note@-2{{Tracking condition 'flag'}}
     *x = 5; // expected-warning{{Dereference of null pointer}}
             // expected-note@-1{{Dereference of null pointer}}
 }
 
 } // end of namespace unimportant_write_before_collapse_point