[analyzer] ConditionBRVisitor: Unknown condition evaluation support

Summary: If the constraint information is not changed between two program states the analyzer has not learnt new information and made no report. But it is possible to happen because we have no information at all. The new approach evaluates the condition to determine if that is the case and let the user know we just 'Assuming...' some value.

Reviewers: NoQ, george.karpenkov

Reviewed By: NoQ

Subscribers: xazax.hun, baloghadamsoftware, szepet, a.sidorin, mikhail.ramalho, Szelethus, donat.nagy, dkrupp, gsd, gerazo

Tags: #clang

Differential Revision: https://reviews.llvm.org/D57410

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@356319 91177308-0d34-0410-b5e6-96231b3b80d8

diff --git a/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp b/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
index bf7d330e4d5383843212c567ae8252a91f90cf80..2c4277c8b702d05072970765620481bb3e2d8d0b 100644 (file)
--- a/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ b/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -1815,12 +1815,6 @@ std::shared_ptr<PathDiagnosticPiece>
 ConditionBRVisitor::VisitNodeImpl(const ExplodedNode *N,
                                   BugReporterContext &BRC, BugReport &BR) {
   ProgramPoint progPoint = N->getLocation();
-  ProgramStateRef CurrentState = N->getState();
-  ProgramStateRef PreviousState = N->getFirstPred()->getState();
-
-  // If the constraint information does not changed there is no assumption.
-  if (BRC.getStateManager().haveEqualConstraints(CurrentState, PreviousState))
-    return nullptr;
 
   // If an assumption was made on a branch, it should be caught
   // here by looking at the state transition.
@@ -1889,6 +1883,8 @@ std::shared_ptr<PathDiagnosticPiece> ConditionBRVisitor::VisitTerminator(
     break;
   }
 
+  Cond = Cond->IgnoreParens();
+
   // However, when we encounter a logical operator as a branch condition,
   // then the condition is actually its RHS, because LHS would be
   // the condition for the logical operator terminator.
@@ -1908,6 +1904,18 @@ std::shared_ptr<PathDiagnosticPiece>
 ConditionBRVisitor::VisitTrueTest(const Expr *Cond, bool tookTrue,
                                   BugReporterContext &BRC, BugReport &R,
                                   const ExplodedNode *N) {
+  ProgramStateRef CurrentState = N->getState();
+  ProgramStateRef PreviousState = N->getFirstPred()->getState();
+  const LocationContext *LCtx = N->getLocationContext();
+
+  // If the constraint information is changed between the current and the
+  // previous program state we assuming the newly seen constraint information.
+  // If we cannot evaluate the condition (and the constraints are the same)
+  // the analyzer has no information about the value and just assuming it.
+  if (BRC.getStateManager().haveEqualConstraints(CurrentState, PreviousState) &&
+      CurrentState->getSVal(Cond, LCtx).isValid())
+    return nullptr;
+
   // These will be modified in code below, but we need to preserve the original
   //  values in case we want to throw the generic message.
   const Expr *CondTmp = Cond;
@@ -1943,7 +1951,6 @@ ConditionBRVisitor::VisitTrueTest(const Expr *Cond, bool tookTrue,
 
   // Condition too complex to explain? Just say something so that the user
   // knew we've made some path decision at this point.
-  const LocationContext *LCtx = N->getLocationContext();
   PathDiagnosticLocation Loc(Cond, BRC.getSourceManager(), LCtx);
   if (!Loc.isValid() || !Loc.asLocation().isValid())
     return nullptr;

diff --git a/test/Analysis/diagnostics/macros.cpp b/test/Analysis/diagnostics/macros.cpp
index 5aa2c03ab0ed1ef6ed17b77d78a3d528477aa13c..67cdef7f3898554e3139c090a3116c44f87a4b73 100644 (file)
--- a/test/Analysis/diagnostics/macros.cpp
+++ b/test/Analysis/diagnostics/macros.cpp
@@ -30,7 +30,8 @@ void testnullptrMacro(int *p) {
 
 // There are no path notes on the comparison to float types.
 void testDoubleMacro(double d) {
-  if (d == DBL_MAX) { // expected-note {{Taking true branch}}
+  if (d == DBL_MAX) { // expected-note {{Assuming 'd' is equal to DBL_MAX}}
+                      // expected-note@-1 {{Taking true branch}}
 
     char *p = NULL; // expected-note {{'p' initialized to a null pointer value}}
     *p = 7;         // expected-warning {{Dereference of null pointer (loaded from variable 'p')}}

diff --git a/test/Analysis/uninit-vals.m b/test/Analysis/uninit-vals.m
index 33352122ca5aa0e3971b34a8b1911172aaac7a0d..5b959c7bfe19e87105c16cbcc37f89f74b649ccd 100644 (file)
--- a/test/Analysis/uninit-vals.m
+++ b/test/Analysis/uninit-vals.m
@@ -164,7 +164,8 @@ void PR14765_test() {
                                            // expected-note@-1{{TRUE}}
 
   testObj->origin = makePoint(0.0, 0.0);
-  if (testObj->size > 0) { ; } // expected-note{{Taking false branch}}
+  if (testObj->size > 0) { ; } // expected-note{{Assuming the condition is false}}
+                               // expected-note@-1{{Taking false branch}}
 
   // FIXME: Assigning to 'testObj->origin' kills the default binding for the
   // whole region, meaning that we've forgotten that testObj->size should also
@@ -218,10 +219,14 @@ void PR14765_test_int() {
                                                // expected-note@-1{{TRUE}}
 
   testObj->origin = makeIntPoint(1, 2);
-  if (testObj->size > 0) { ; } // expected-note{{Taking false branch}}
+  if (testObj->size > 0) { ; } // expected-note{{Assuming the condition is false}}
                                // expected-note@-1{{Taking false branch}}
-                               // expected-note@-2{{Taking false branch}}
+                               // expected-note@-2{{Assuming the condition is false}}
                                // expected-note@-3{{Taking false branch}}
+                               // expected-note@-4{{Assuming the condition is false}}
+                               // expected-note@-5{{Taking false branch}}
+                               // expected-note@-6{{Assuming the condition is false}}
+                               // expected-note@-7{{Taking false branch}}
 
   // FIXME: Assigning to 'testObj->origin' kills the default binding for the
   // whole region, meaning that we've forgotten that testObj->size should also