From 1d92e20334c33d090f930865d60db248247e2c94 Mon Sep 17 00:00:00 2001 From: Csaba Dabis Date: Sat, 16 Mar 2019 09:24:30 +0000 Subject: [PATCH] [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 --- .../Core/BugReporterVisitors.cpp | 21 ++++++++++++------- test/Analysis/diagnostics/macros.cpp | 3 ++- test/Analysis/uninit-vals.m | 11 +++++++--- 3 files changed, 24 insertions(+), 11 deletions(-) diff --git a/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp b/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp index bf7d330e4d..2c4277c8b7 100644 --- a/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp +++ b/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp @@ -1815,12 +1815,6 @@ std::shared_ptr 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 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 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 5aa2c03ab0..67cdef7f38 100644 --- 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 33352122ca..5b959c7bfe 100644 --- 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 -- 2.40.0