[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

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

1450

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

1431–1447

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

1431–1447

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

260–261

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.

clang/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. For
   ///        conditions, ignores the initialization point.
+  /// \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)
+                         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;
 
   std::shared_ptr<PathDiagnosticPiece> VisitNode(const ExplodedNode *N,
                                                  BugReporterContext &BRC,
                                                  BugReport &BR) override;

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

if (!StoreSite) {
     // If this is a CXXTempObjectRegion, the Expr responsible for its creation
     // is wrapped inside of it.
     if (const auto *TmpR = dyn_cast<CXXTempObjectRegion>(R))
       InitE = TmpR->getExpr();
   }
 
   if (!StoreSite)
     return nullptr;
+
+  if (TKind != TrackingKind::ThoroughTracking &&
+      StoreSite->getStackFrame() == OriginSFC)
+    return nullptr;
+
   Satisfied = true;
 
   // If we have an expression that provided the value, try to track where it
   // came from.
   // For tracked conditions, this isn't really important, it would pollute the
   // bug report far too much.
   if (TKind != TrackingKind::ConditionTracking) {
     if (InitE) {
       if (V.isUndef() ||
           V.getAs<loc::ConcreteInt>() || V.getAs<nonloc::ConcreteInt>()) {
         if (!IsParam)
           InitE = InitE->IgnoreParenCasts();
         bugreporter::trackExpressionValue(StoreSite, InitE, BR,
                                      EnableNullFPSuppression);
       }
       ReturnVisitor::addVisitorIfNecessary(StoreSite, InitE->IgnoreParenCasts(),
                                            BR, EnableNullFPSuppression);
     }
   }
 
   // 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);
     const auto *VR = dyn_cast<VarRegion>(R);
 
     if (DS) {
       action = R->canPrintPretty() ? "initialized to " :
                                      "Initializing to ";
     } else if (isa<BlockExpr>(S)) {
       action = R->canPrintPretty() ? "captured by block as " :
                                      "Captured by block as ";
       if (VR) {
         // 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 (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));
     }
   }

clang/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();
*ptr = 5; // expected-warning{{Dereference of null pointer}}
               // expected-note@-1{{Dereference of null pointer}}
 }
 } // end of namespace important_returning_value_note
 
 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;
-  flag = y; // tracking-note{{The value 1 is assigned to 'flag'}}
+  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
void f(int y) {
 
   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_nested_stackframe
 
 namespace collapse_point_not_in_condition {
 
 [[noreturn]] void halt();
 
 void assert(int b) {
   if (!b) // tracking-note{{Assuming 'b' is not equal to 0}}
           // tracking-note@-1{{Taking false branch}}
     halt();
 }
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