[analyzer] Don't track the right hand side of the last store for conditions

[Szelethus]

Summary

Exactly what it says on the tin!

[NoQ]

Consider:

int flag;
bool coin();

void foo() {
  flag = coin();
}

void test() {
  int *x = 0;
  int local_flag;
  flag = 1;

  foo();
  local_flag = flag;
  if (local_flag)
    x = new int;

  foo();
  local_flag = flag;
  if (local_flag)
    *x = 5;
}

I'd rather track flag when i reach local_flag. I believe that we must track the RHS, but only as long as it's an overwrite on its own (or it's the value that participated in the collapse).

This is why the heuristic that i suggested was "track normally until the *last* out-of-frame overwrite point or until the collapse point".

[Szelethus]

You're right. If condition tracking only adds necessary information, this shouldn't hurt that much anyways.

[NoQ]

I'd rather not abandon this patch, because it looks like a strict improvement over the lack of condition tracking, and it might as well still be an improvement over "zealous" condition tracking, as my counterexample is fairly artificial. It indicates that a slightly more sophisticated algorithm is necessary (i'm not sure if it's single-pass or even linear). But i'll be perfectly happy with simply adding it as a FIXME test.

[Szelethus]

In D64271#1576872, @NoQ wrote:

I'd rather not abandon this patch, because it looks like a strict improvement over the lack of condition tracking, and it might as well still be an improvement over "zealous" condition tracking, as my counterexample is fairly artificial. It indicates that a slightly more sophisticated algorithm is necessary (i'm not sure if it's single-pass or even linear). But i'll be perfectly happy with simply adding it as a FIXME test.

My other patches actually deal with this far more nicely. D64272 + D64232 combined almost achieve the same thing, but I'm running analyses to find gather some real world experience.
.

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

public:
   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 TKind May limit the amount of notes added to the bug report. For
+  ///        conditions, ignores the initialization point.
   FindLastStoreBRVisitor(KnownSVal V, const MemRegion *R,
                          bool InEnableNullFPSuppression,
                          TrackingKind TKind)
       : R(R), V(V), EnableNullFPSuppression(InEnableNullFPSuppression),
         TKind(TKind) {
     assert(R);
   }
 

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

FindLastStoreBRVisitor::VisitNode(const ExplodedNode *Succ,
   }
 
   if (!StoreSite)
     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;

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

if (flag) // expected-note   {{Assuming 'flag' is not equal to 0}}
               // expected-note@-1{{Dereference of null pointer}}
 }
 } // end of namespace global_variable_invalidation
 
 namespace variable_declaration_in_condition {
 bool coin();
 
 bool foo() {
-  return coin(); // tracking-note{{Returning value}}
+  return coin();
 }
 
 int bar;
 
 void test() {
   int *x = 0; // expected-note{{'x' initialized to a null pointer value}}
 
-  if (int flag = foo()) // tracking-note{{Calling 'foo'}}
-                        // tracking-note@-1{{Returning from 'foo'}}
-                        // tracking-note@-2{{'flag' initialized here}}
-                        // debug-note@-3{{Tracking condition 'flag'}}
-                        // expected-note@-4{{Assuming 'flag' is not equal to 0}}
-                        // expected-note@-5{{Taking true branch}}
+  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}}
 
     *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();

 }
 } // 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'}}
+  y = 1;
   flag = y; // tracking-note{{The value 1 is assigned to 'flag'}}
-
   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'}}