[analyzer] Don't make ConditionBRVisitor events prunable when the condition is an interesting field

[Szelethus]

Summary

Exactly what it says on the tin! Note that we're talking about interestingness in general, hence this isn't a control-dependency-tracking specific patch.

[NoQ]

Nice!!

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

224–228

https://llvm.org/doxygen/classllvm_1_1StringLiteral.html:

In order to avoid the invocation of a global constructor, StringLiteral should only be used in a constexpr context

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

192–203

Ugh, so this code intentionally avoids the complexity of ExprEngine::VisitCommonDeclRefExpr() so that to work differently with references. Okay.

@xazax.hun, do you think this works correctly with lambda captures? (when the DeclRefExpr to a variable might evaluate to a field within the lambda at run-time).

208

Decl? I guess it made sense in the original code but there's no longer an obvious Decl floating around.

[xazax.hun]

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

192–203

Good point! This definitely worth a test case somewhere somehow!

I would expect DRE->getDecl() to return the captured declaration rather than the field and thus State->getSVal(State->getLValue(VD, LCtx)) to compute the wrong answer. I might be a bit rusty on this topic so of course we would need a carefully crafted test case to make sure :)

[Szelethus]

• Make the generic messages in-class, constexpr fields.
• Provide plenty of test cases for captured lambda variables. In our meeting we had both had a sight of relief, and some still lingering concerns about this, so maybe it deserves fixing in the long term.

[Szelethus]

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

192–203

So, we seem to be okay with this code in the context of this patch, but have voiced concerns that there probably is a testcase on which this doesn't work. What kind of FIXME message shall I put in before commiting?

[Szelethus]

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

192–203

Also, could you please detail why we think this doesn't/shouldn't work?

[NoQ]

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

192–203

Also, could you please detail why we think this doesn't/shouldn't work?

FIXME: As seen in VisitCommonDeclRefExpr, sometimes DeclRefExpr may evaluate to a FieldRegion when it refers to a declaration of a lambda capture variable. We most likely need to duplicate that logic here.

[Szelethus]

• Add the FIXME
• Cascade test changes from D65575

[NoQ]

LGTM!

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

public:
   /// If the statement is a message send expression with nil receiver, returns
   /// the receiver expression. Returns NULL otherwise.
   static const Expr *getNilReceiver(const Stmt *S, const ExplodedNode *N);
 };
 
 /// Visitor that tries to report interesting diagnostics from conditions.
 class ConditionBRVisitor final : public BugReporterVisitor {
   // FIXME: constexpr initialization isn't supported by MSVC2013.
-  static const char *const GenericTrueMessage;
-  static const char *const GenericFalseMessage;
+  constexpr static llvm::StringLiteral GenericTrueMessage =
+      "Assuming the condition is true";
+  constexpr static llvm::StringLiteral GenericFalseMessage =
+      "Assuming the condition is false";
 
 public:
   void Profile(llvm::FoldingSetNodeID &ID) const override {
     static int x = 0;
     ID.AddPointer(&x);
   }
 
   /// Return the tag associated with this visitor.  This tag will be used
   /// to make all PathDiagnosticPieces created by this visitor.

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

static bool hasVisibleUpdate(const ExplodedNode *LeftNode, SVal LeftVal,
   if (!RLCV)
     return false;
 
   return LLCV->getRegion() == RLCV->getRegion() &&
     LLCV->getStore() == LeftNode->getState()->getStore() &&
     RLCV->getStore() == RightNode->getState()->getStore();
 }
 
-static Optional<const llvm::APSInt *>
-getConcreteIntegerValue(const Expr *CondVarExpr, const ExplodedNode *N) {
+static Optional<SVal> getSValForVar(const Expr *CondVarExpr,
+                                    const ExplodedNode *N) {
   ProgramStateRef State = N->getState();
   const LocationContext *LCtx = N->getLocationContext();
 
+  assert(CondVarExpr);
+  CondVarExpr = CondVarExpr->IgnoreImpCasts();
+
   // The declaration of the value may rely on a pointer so take its l-value.
-  if (const auto *DRE = dyn_cast_or_null<DeclRefExpr>(CondVarExpr)) {
-    if (const auto *VD = dyn_cast_or_null<VarDecl>(DRE->getDecl())) {
-      SVal DeclSVal = State->getSVal(State->getLValue(VD, LCtx));
-      if (auto DeclCI = DeclSVal.getAs<nonloc::ConcreteInt>())
-        return &DeclCI->getValue();
-    }
-  }
+  // FIXME: As seen in VisitCommonDeclRefExpr, sometimes DeclRefExpr may
+  // evaluate to a FieldRegion when it refers to a declaration of a lambda
+  // capture variable. We most likely need to duplicate that logic here.
+  if (const auto *DRE = dyn_cast<DeclRefExpr>(CondVarExpr))
+    if (const auto *VD = dyn_cast<VarDecl>(DRE->getDecl()))
+      return State->getSVal(State->getLValue(VD, LCtx));
+
+  if (const auto *ME = dyn_cast<MemberExpr>(CondVarExpr))
+    if (const auto *FD = dyn_cast<FieldDecl>(ME->getMemberDecl()))
+      if (auto FieldL = State->getSVal(ME, LCtx).getAs<Loc>())
+        return State->getRawSVal(*FieldL, FD->getType());
+
+  return None;
+
+static Optional<const llvm::APSInt *>
+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;
 
-  return {};
+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.
 static StringRef getMacroName(SourceLocation Loc,
     BugReporterContext &BRC) {
   return Lexer::getImmediateMacroName(
       Loc,
       BRC.getSourceManager(),
PathDiagnosticPieceRef ConditionBRVisitor::VisitConditionVariable(
   llvm::raw_svector_ostream Out(buf);
   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);
+
   auto event = std::make_shared<PathDiagnosticEventPiece>(Loc, Out.str());
 
-  if (const auto *DR = dyn_cast<DeclRefExpr>(CondVarExpr)) {
-    if (const auto *VD = dyn_cast<VarDecl>(DR->getDecl())) {
-      const ProgramState *state = N->getState().get();
-      if (const MemRegion *R = state->getLValue(VD, LCtx).getAsRegion()) {
-        if (report.isInteresting(R))
-          event->setPrunable(false);
-      }
-    }
-  }
+  if (isInterestingExpr(CondVarExpr, N, &report))
 
   return event;
 }
 
 PathDiagnosticPieceRef ConditionBRVisitor::VisitTrueTest(
     const Expr *Cond, const DeclRefExpr *DRE, BugReporterContext &BRC,
     BugReport &report, const ExplodedNode *N, bool TookTrue, bool IsAssuming) {
   const auto *VD = dyn_cast<VarDecl>(DRE->getDecl());
PathDiagnosticPieceRef ConditionBRVisitor::VisitTrueTest(
   // 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());
-  const ProgramState *state = N->getState().get();
-  if (const MemRegion *R = state->getLValue(VD, LCtx).getAsRegion()) {
-    if (report.isInteresting(R))
-      event->setPrunable(false);
-    else {
-      SVal V = state->getSVal(R);
-      if (report.isInteresting(V))
-        event->setPrunable(false);
-    }
-  }
+
+  if (isInterestingExpr(DRE, N, &report))
+
   return std::move(event);
 }
 
 PathDiagnosticPieceRef ConditionBRVisitor::VisitTrueTest(
     const Expr *Cond, const MemberExpr *ME, BugReporterContext &BRC,
     BugReport &report, const ExplodedNode *N, bool TookTrue, bool IsAssuming) {
   SmallString<256> Buf;
   llvm::raw_svector_ostream Out(Buf);
else
     Loc = PathDiagnosticLocation(Cond, BRC.getSourceManager(), LCtx);
 
   if (!Loc.isValid() || !Loc.asLocation().isValid())
     return nullptr;
 
   if (!IsAssuming)
     return std::make_shared<PathDiagnosticPopUpPiece>(Loc, Out.str());
 
-  return std::make_shared<PathDiagnosticEventPiece>(Loc, Out.str());
+  auto event = std::make_shared<PathDiagnosticEventPiece>(Loc, Out.str());
+  if (isInterestingExpr(ME, N, &report))
+    event->setPrunable(false);
+  return event;
 }
 
 bool ConditionBRVisitor::printValue(const Expr *CondVarExpr, raw_ostream &Out,
                                     const ExplodedNode *N, bool TookTrue,
                                     bool IsAssuming) {
   QualType Ty = CondVarExpr->getType();
 
   if (Ty->isPointerType()) {
if (Ty->isBooleanType())
       Out << (IntValue.getValue()->getBoolValue() ? "true" : "false");
     else
       Out << *IntValue.getValue();
   }
 
   return true;
 }
 
-const char *const ConditionBRVisitor::GenericTrueMessage =
-    "Assuming the condition is true";
-const char *const ConditionBRVisitor::GenericFalseMessage =
-    "Assuming the condition is false";
+constexpr llvm::StringLiteral ConditionBRVisitor::GenericTrueMessage;
+constexpr llvm::StringLiteral ConditionBRVisitor::GenericFalseMessage;
 
 bool ConditionBRVisitor::isPieceMessageGeneric(
     const PathDiagnosticPiece *Piece) {
   return Piece->getString() == GenericTrueMessage ||
          Piece->getString() == GenericFalseMessage;
 }
 
 //===----------------------------------------------------------------------===//

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

void f() {
   if (y)    // expected-note-re{{{{^}}Assuming 'y' is not equal to 0{{$}}}}
             // expected-note-re@-1{{{{^}}Taking true branch{{$}}}}
             // debug-note-re@-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
 
+namespace condition_lambda_capture_by_reference_last_write {
+int getInt();
+
+[[noreturn]] void halt();
+
+void f(int flag) {
+  int *x = 0; // expected-note-re{{{{^}}'x' initialized to a null pointer value{{$}}}}
+
+  auto lambda = [&flag]() {
+    flag = getInt(); // tracking-note-re{{{{^}}Value assigned to 'flag', which participates in a condition later{{$}}}}
+  };
+
+  lambda(); // tracking-note-re{{{{^}}Calling 'operator()'{{$}}}}
+            // tracking-note-re@-1{{{{^}}Returning from 'operator()'{{$}}}}
+
+  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 condition_lambda_capture_by_reference_last_write
+
+namespace condition_lambda_capture_by_value_assumption {
+int getInt();
+
+[[noreturn]] void halt();
+
+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{{$}}}}
+               // 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{{{{^}}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 condition_lambda_capture_by_value_assumption
+
+namespace condition_lambda_capture_by_reference_assumption {
+int getInt();
+
+[[noreturn]] void halt();
+
+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{{$}}}}
+               // 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'}}
+    *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 {
 
 [[noreturn]] void halt();
 
 void assert(int b) {
   if (!b) // tracking-note-re{{{{^}}Assuming 'b' is not equal to 0{{$}}}}
           // tracking-note-re@-1{{{{^}}Taking false branch{{$}}}}
     halt();
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_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{{$}}}}
+            // 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 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__))