[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

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

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

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

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

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

[NoQ]

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

192–212

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;
+  static llvm::StringLiteral GenericTrueMessage;
+  static llvm::StringLiteral GenericFalseMessage;
 
 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

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();
-    }
-  }
+  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 DeclCI = V->getAs<nonloc::ConcreteInt>())
+      return &DeclCI->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(
   const LocationContext *LCtx = N->getLocationContext();
   PathDiagnosticLocation Loc(Cond, BRC.getSourceManager(), LCtx);
 
   // 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());
-  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);
 
   Out << (IsAssuming ? "Assuming field '" : "Field '")
       << ME->getMemberDecl()->getName() << "' is ";
 
   if (!printValue(ME, Out, N, TookTrue, IsAssuming))
     return nullptr;
 
   const LocationContext *LCtx = N->getLocationContext();
   PathDiagnosticLocation Loc(Cond, BRC.getSourceManager(), LCtx);
   if (!Loc.isValid() || !Loc.asLocation().isValid())
     return nullptr;
 
   // If we know the value create a pop-up note.
   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 =
+llvm::StringLiteral ConditionBRVisitor::GenericTrueMessage =
     "Assuming the condition is true";
-const char *const ConditionBRVisitor::GenericFalseMessage =
+llvm::StringLiteral ConditionBRVisitor::GenericFalseMessage =
     "Assuming the condition is false";
 
 bool ConditionBRVisitor::isPieceMessageGeneric(
     const PathDiagnosticPiece *Piece) {
   return Piece->getString() == GenericTrueMessage ||
          Piece->getString() == GenericFalseMessage;
 }
 

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

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
 
+namespace collapse_point_not_in_condition_as_field {
+
+[[noreturn]] void halt();
+struct IntWrapper {
+  int b;
+  IntWrapper();
+
+  void check() {
+    if (!b) // tracking-note{{Assuming field 'b' is not equal to 0}}
+            // tracking-note@-1{{Taking false branch}}
+      halt();
+    return;
+  }
+};
+
+void f(IntWrapper i) {
+  int *x = 0; // expected-note{{'x' initialized to a null pointer value}}
+
+  i.check(); // tracking-note{{Calling 'IntWrapper::check'}}
+             // tracking-note@-1{{Returning from 'IntWrapper::check'}}
+  if (i.b)   // expected-note{{Field 'b' is not equal to 0}}
+             // expected-note@-1{{Taking true branch}}
+             // debug-note@-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__))