[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

225–226 ↗

(On Diff #213261)

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–194 ↗

(On Diff #213261)

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 ↗

(On Diff #213261)

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–194 ↗

(On Diff #213261)

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–194 ↗

(On Diff #213261)

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–194 ↗

(On Diff #213261)

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

[NoQ]

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

192–194 ↗

(On Diff #215195)

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!

cfe/trunk/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

cfe/trunk/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;
 }
 
 //===----------------------------------------------------------------------===//

cfe/trunk/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();
void f6(int x) {
   if (x == 25) {} // expected-note{{Assuming 'x' is equal to 25}}
                   // expected-note@-1{{Taking true branch}}
   if (a[x] == 123) {} // expected-warning{{The left operand of '==' is a garbage value due to array index out of bounds}}
                       // expected-note@-1{{The left operand of '==' is a garbage value due to array index out of bounds}}
 }
 
 } // end of namespace dont_crash_on_nonlogical_binary_operator
 
+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__))