[analyzer] Mention whether an event is about a condition in a bug report part 2

[Szelethus]

Summary

In D65724, I do a pretty thorough explanation about how I'm solving this problem, I think that summary nails whats happening here ;)

The actual message is kinda dumb here, but I figured that we'd have a couple rounds on word smithing anyways, so I uploaded this as is. Its probably good enough to gather some analysis data with.

Also, with some luck, this should the final technical patch regarding the first part of my GSoC ;)

[NoQ]

Accept as long as we agree on the wording as part of D65575.

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

548–549

Do i understand correctly that this test was passing previously, because expected-note matches substrings rather than the whole string? If we stick to the current wording, should we slowly switch to expected-note-re{{{{^}}...{{$}}}} in order to avoid such situations?

[Szelethus]

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

548–549

I was thinking about this as well. I could do that.

[Szelethus]

Rebase.

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

 getConcreteIntegerValue(const Expr *CondVarExpr, const ExplodedNode *N) {
 
   if (Optional<SVal> V = getSValForVar(CondVarExpr, N))
     if (auto CI = V->getAs<nonloc::ConcreteInt>())
       return &CI->getValue();
   return None;
 }
 
+static bool isVarAnInterestingCondition(const Expr *CondVarExpr,
+                                        const ExplodedNode *N,
+                                        const BugReport *B) {
+  // Even if this condition is marked as interesting, it isn't *that*
+  // interesting if it didn't happen in a nested stackframe, the user could just
+  // follow the arrows.
+  if (!B->getErrorNode()->getStackFrame()->isParentOf(N->getStackFrame()))
+    return false;
+
+  if (Optional<SVal> V = getSValForVar(CondVarExpr, N))
+    if (Optional<bugreporter::TrackingKind> K = B->getInterestingnessKind(*V))
+      return *K == bugreporter::TrackingKind::Condition;
+
+  return false;
+}
+
 static bool isInterestingExpr(const Expr *E, const ExplodedNode *N,
                               const BugReport *B) {
   if (Optional<SVal> V = getSValForVar(E, N))
     return B->getInterestingnessKind(*V).hasValue();
   return false;
 }
 
 /// \return name of the macro inside the location \p Loc.
default:
       Out << BinaryOperator::getOpcodeStr(Op) << ' ';
       break;
   }
 
   Out << (shouldInvert ? LhsString : RhsString);
   const LocationContext *LCtx = N->getLocationContext();
   const SourceManager &SM = BRC.getSourceManager();
 
+  if (isVarAnInterestingCondition(BExpr->getLHS(), N, &R) ||
+      isVarAnInterestingCondition(BExpr->getRHS(), N, &R))
+    Out << WillBeUsedForACondition;
+
   // Convert 'field ...' to 'Field ...' if it is a MemberExpr.
   std::string Message = Out.str();
   Message[0] = toupper(Message[0]);
 
   // If we know the value create a pop-up note to the value part of 'BExpr'.
   if (!IsAssuming) {
     PathDiagnosticLocation Loc;
     if (!shouldInvert) {
PathDiagnosticPieceRef ConditionBRVisitor::VisitConditionVariable(
   Out << "Assuming " << LhsString << " is ";
 
   if (!printValue(CondVarExpr, Out, N, TookTrue, /*IsAssuming=*/true))
     return nullptr;
 
   const LocationContext *LCtx = N->getLocationContext();
   PathDiagnosticLocation Loc(CondVarExpr, BRC.getSourceManager(), LCtx);
 
+  if (isVarAnInterestingCondition(CondVarExpr, N, &report))
+    Out << WillBeUsedForACondition;
+
   auto event = std::make_shared<PathDiagnosticEventPiece>(Loc, Out.str());
 
   if (isInterestingExpr(CondVarExpr, N, &report))
     event->setPrunable(false);
 
   return event;
 }
 
PathDiagnosticPieceRef ConditionBRVisitor::VisitTrueTest(
 
   Out << (IsAssuming ? "Assuming '" : "'") << VD->getDeclName() << "' is ";
 
   if (!printValue(DRE, Out, N, TookTrue, IsAssuming))
     return nullptr;
 
   const LocationContext *LCtx = N->getLocationContext();
 
+  if (isVarAnInterestingCondition(DRE, N, &report))
+    Out << WillBeUsedForACondition;
+
   // If we know the value create a pop-up note to the 'DRE'.
   if (!IsAssuming) {
     PathDiagnosticLocation Loc(DRE, BRC.getSourceManager(), LCtx);
     return std::make_shared<PathDiagnosticPopUpPiece>(Loc, Out.str());
   }
 
   PathDiagnosticLocation Loc(Cond, BRC.getSourceManager(), LCtx);
   auto event = std::make_shared<PathDiagnosticEventPiece>(Loc, Out.str());
PathDiagnosticPieceRef ConditionBRVisitor::VisitTrueTest(
   if (!IsAssuming && ME->getMemberLoc().isValid())
     Loc = PathDiagnosticLocation(ME->getMemberLoc(), BRC.getSourceManager());
   else
     Loc = PathDiagnosticLocation(Cond, BRC.getSourceManager(), LCtx);
 
   if (!Loc.isValid() || !Loc.asLocation().isValid())
     return nullptr;
 
+  if (isVarAnInterestingCondition(ME, N, &report))
+    Out << WillBeUsedForACondition;
+
+  // If we know the value create a pop-up note.
   if (!IsAssuming)
     return std::make_shared<PathDiagnosticPopUpPiece>(Loc, Out.str());
 
   auto event = std::make_shared<PathDiagnosticEventPiece>(Loc, Out.str());
   if (isInterestingExpr(ME, N, &report))
     event->setPrunable(false);
   return event;
 }

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

 void bar(int &flag) {
   flag = getInt(); // tracking-note-re{{{{^}}Value assigned to 'flag', which participates in a condition later{{$}}}}
 }
 
 void f(int flag) {
   int *x = 0; // expected-note-re{{{{^}}'x' initialized to a null pointer value{{$}}}}
 
   auto lambda = [flag]() {
-    if (!flag) // tracking-note-re{{{{^}}Assuming 'flag' is not equal to 0{{$}}}}
+    if (!flag) // tracking-note-re{{{{^}}Assuming 'flag' is not equal to 0, which participates in a condition later{{$}}}}
                // tracking-note-re@-1{{{{^}}Taking false branch{{$}}}}
       halt();
   };
 
   bar(flag); // tracking-note-re{{{{^}}Calling 'bar'{{$}}}}
              // tracking-note-re@-1{{{{^}}Returning from 'bar'{{$}}}}
   lambda();  // tracking-note-re{{{{^}}Calling 'operator()'{{$}}}}
              // tracking-note-re@-1{{{{^}}Returning from 'operator()'{{$}}}}

 void bar(int &flag) {
   flag = getInt(); // tracking-note-re{{{{^}}Value assigned to 'flag', which participates in a condition later{{$}}}}
 }
 
 void f(int flag) {
   int *x = 0; // expected-note-re{{{{^}}'x' initialized to a null pointer value{{$}}}}
 
   auto lambda = [&flag]() {
-    if (!flag) // tracking-note-re{{{{^}}Assuming 'flag' is not equal to 0{{$}}}}
+    if (!flag) // tracking-note-re{{{{^}}Assuming 'flag' is not equal to 0, which participates in a condition later{{$}}}}
                // tracking-note-re@-1{{{{^}}Taking false branch{{$}}}}
       halt();
   };
 
   bar(flag); // tracking-note-re{{{{^}}Calling 'bar'{{$}}}}
              // tracking-note-re@-1{{{{^}}Returning from 'bar'{{$}}}}
   lambda();  // tracking-note-re{{{{^}}Calling 'operator()'{{$}}}}
              // tracking-note-re@-1{{{{^}}Returning from 'operator()'{{$}}}}
 
   if (flag) // expected-note-re{{{{^}}'flag' is not equal to 0{{$}}}}
             // expected-note-re@-1{{{{^}}Taking true branch{{$}}}}
-            // debug-note-re@-2{{{{^}}Tracking condition 'flag'}}
+            // debug-note-re@-2{{{{^}}Tracking condition 'flag'{{$}}}}
     *x = 5; // expected-warning{{Dereference of null pointer}}
             // expected-note@-1{{Dereference of null pointer}}
 }
 } // end of namespace condition_lambda_capture_by_reference_assumption
 
+namespace collapse_point_not_in_condition_bool {
+
+[[noreturn]] void halt();
+
+void check(bool b) {
+  if (!b) // tracking-note-re{{{{^}}Assuming 'b' is true, which participates in a condition later{{$}}}}
+          // tracking-note-re@-1{{{{^}}Taking false branch{{$}}}}
+    halt();
+}
+
+void f(bool flag) {
+  int *x = 0; // expected-note-re{{{{^}}'x' initialized to a null pointer value{{$}}}}
+
+  check(flag); // tracking-note-re{{{{^}}Calling 'check'{{$}}}}
+                // tracking-note-re@-1{{{{^}}Returning from 'check'{{$}}}}
+
+  if (flag) // expected-note-re{{{{^}}'flag' is true{{$}}}}
+            // expected-note-re@-1{{{{^}}Taking true branch{{$}}}}
+            // debug-note-re@-2{{{{^}}Tracking condition 'flag'{{$}}}}
+    *x = 5; // expected-warning{{Dereference of null pointer}}
+            // expected-note@-1{{Dereference of null pointer}}
+}
+} // end of namespace collapse_point_not_in_condition_bool
+
 namespace collapse_point_not_in_condition {
 
 [[noreturn]] void halt();
 
 void assert(int b) {
-  if (!b) // tracking-note-re{{{{^}}Assuming 'b' is not equal to 0{{$}}}}
+  if (!b) // tracking-note-re{{{{^}}Assuming 'b' is not equal to 0, which participates in a condition later{{$}}}}
           // tracking-note-re@-1{{{{^}}Taking false branch{{$}}}}
     halt();
 }
 
 void f(int flag) {
   int *x = 0; // expected-note-re{{{{^}}'x' initialized to a null pointer value{{$}}}}
 
   assert(flag); // tracking-note-re{{{{^}}Calling 'assert'{{$}}}}
                 // tracking-note-re@-1{{{{^}}Returning from 'assert'{{$}}}}
 
   if (flag) // expected-note-re{{{{^}}'flag' is not equal to 0{{$}}}}
             // expected-note-re@-1{{{{^}}Taking true branch{{$}}}}
             // debug-note-re@-2{{{{^}}Tracking condition 'flag'{{$}}}}
     *x = 5; // expected-warning{{Dereference of null pointer}}
             // expected-note@-1{{Dereference of null pointer}}
 }
 
 } // end of namespace collapse_point_not_in_condition
 
 namespace unimportant_write_before_collapse_point {
 
 [[noreturn]] void halt();
 
 void assert(int b) {
-  if (!b) // tracking-note-re{{{{^}}Assuming 'b' is not equal to 0{{$}}}}
+  if (!b) // tracking-note-re{{{{^}}Assuming 'b' is not equal to 0, which participates in a condition later{{$}}}}
           // tracking-note-re@-1{{{{^}}Taking false branch{{$}}}}
     halt();
 }
 int getInt();
 
 void f(int flag) {
   int *x = 0; // expected-note-re{{{{^}}'x' initialized to a null pointer value{{$}}}}
 
   flag = getInt();
   assert(flag); // tracking-note-re{{{{^}}Calling 'assert'{{$}}}}
                 // tracking-note-re@-1{{{{^}}Returning from 'assert'{{$}}}}
 
   if (flag) // expected-note-re{{{{^}}'flag' is not equal to 0{{$}}}}
             // expected-note-re@-1{{{{^}}Taking true branch{{$}}}}
             // debug-note-re@-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
 
+namespace collapse_point_not_in_condition_binary_op {
+
+[[noreturn]] void halt();
+
+void check(int b) {
+  if (b == 1) // tracking-note-re{{{{^}}Assuming 'b' is not equal to 1, which participates in a condition later{{$}}}}
+              // tracking-note-re@-1{{{{^}}Taking false branch{{$}}}}
+    halt();
+}
+
+void f(int flag) {
+  int *x = 0; // expected-note-re{{{{^}}'x' initialized to a null pointer value{{$}}}}
+
+  check(flag); // tracking-note-re{{{{^}}Calling 'check'{{$}}}}
+               // tracking-note-re@-1{{{{^}}Returning from 'check'{{$}}}}
+
+  if (flag) // expected-note-re{{{{^}}Assuming 'flag' is not equal to 0{{$}}}}
+            // expected-note-re@-1{{{{^}}Taking true branch{{$}}}}
+            // debug-note-re@-2{{{{^}}Tracking condition 'flag'{{$}}}}
+    *x = 5; // expected-warning{{Dereference of null pointer}}
+            // expected-note@-1{{Dereference of null pointer}}
+}
+
+} // end of namespace collapse_point_not_in_condition_binary_op
+
 namespace collapse_point_not_in_condition_as_field {
 
 [[noreturn]] void halt();
 struct IntWrapper {
   int b;
   IntWrapper();
 
   void check() {
-    if (!b) // tracking-note-re{{{{^}}Assuming field 'b' is not equal to 0{{$}}}}
+    if (!b) // tracking-note-re{{{{^}}Assuming field 'b' is not equal to 0, which participates in a condition later{{$}}}}
             // tracking-note-re@-1{{{{^}}Taking false branch{{$}}}}
       halt();
     return;
   }
 };
 
 void f(IntWrapper i) {
   int *x = 0; // expected-note-re{{{{^}}'x' initialized to a null pointer value{{$}}}}
 
   i.check(); // tracking-note-re{{{{^}}Calling 'IntWrapper::check'{{$}}}}
              // tracking-note-re@-1{{{{^}}Returning from 'IntWrapper::check'{{$}}}}
   if (i.b)   // expected-note-re{{{{^}}Field 'b' is not equal to 0{{$}}}}
              // expected-note-re@-1{{{{^}}Taking true branch{{$}}}}
              // debug-note-re@-2{{{{^}}Tracking condition 'i.b'{{$}}}}
     *x = 5;  // expected-warning{{Dereference of null pointer}}
              // expected-note@-1{{Dereference of null pointer}}
 }
 
 } // end of namespace collapse_point_not_in_condition_as_field
 
+namespace assignemnt_in_condition_in_nested_stackframe {
+int flag;
+
+bool coin();
+
+[[noreturn]] void halt();
+
+void foo() {
+  if ((flag = coin()))
+    // tracking-note-re@-1{{{{^}}Value assigned to 'flag', which participates in a condition later{{$}}}}
+    // tracking-note-re@-2{{{{^}}Assuming 'flag' is not equal to 0, which participates in a condition later{{$}}}}
+    // tracking-note-re@-3{{{{^}}Taking true branch{{$}}}}
+    return;
+  halt();
+  return;
+}
+
+void f() {
+  int *x = 0; // expected-note-re{{{{^}}'x' initialized to a null pointer value{{$}}}}
+
+  foo();    // tracking-note-re{{{{^}}Calling 'foo'{{$}}}}
+            // tracking-note-re@-1{{{{^}}Returning from 'foo'{{$}}}}
+  if (flag) // expected-note-re{{{{^}}'flag' is not equal to 0{{$}}}}
+            // expected-note-re@-1{{{{^}}Taking true branch{{$}}}}
+            // debug-note-re@-2{{{{^}}Tracking condition 'flag'{{$}}}}
+    *x = 5; // expected-warning{{Dereference of null pointer}}
+            // expected-note@-1{{Dereference of null pointer}}
+}
+} // end of namespace assignemnt_in_condition_in_nested_stackframe
+
+namespace condition_variable_less {
+int flag;
+
+bool coin();
+
+[[noreturn]] void halt();
+
+void foo() {
+  if (flag > 0)
+    // tracking-note-re@-1{{{{^}}Assuming 'flag' is > 0, which participates in a condition later{{$}}}}
+    // tracking-note-re@-2{{{{^}}Taking true branch{{$}}}}
+    return;
+  halt();
+  return;
+}
+
+void f() {
+  int *x = 0; // expected-note-re{{{{^}}'x' initialized to a null pointer value{{$}}}}
+
+  foo();    // tracking-note-re{{{{^}}Calling 'foo'{{$}}}}
+            // tracking-note-re@-1{{{{^}}Returning from 'foo'{{$}}}}
+  if (flag) // expected-note-re{{{{^}}'flag' is not equal to 0{{$}}}}
+            // expected-note-re@-1{{{{^}}Taking true branch{{$}}}}
+            // debug-note-re@-2{{{{^}}Tracking condition 'flag'{{$}}}}
+    *x = 5; // expected-warning{{Dereference of null pointer}}
+            // expected-note@-1{{Dereference of null pointer}}
+}
+} // end of namespace condition_variable_less
+
 namespace dont_track_assertlike_conditions {
 
 extern void __assert_fail(__const char *__assertion, __const char *__file,
                           unsigned int __line, __const char *__function)
     __attribute__((__noreturn__));
 #define assert(expr) \
   ((expr) ? (void)(0) : __assert_fail(#expr, __FILE__, __LINE__, __func__))