From f090930ae29a82e8152fc64a3f539792da020cf5 Mon Sep 17 00:00:00 2001 From: Anton Yartsev Date: Fri, 23 Dec 2016 03:31:00 +0000 Subject: [PATCH] Fix for PR15623. The patch eliminates unwanted ProgramState checker data propagation from an operand of the logical operation to operation result. The patch also simplifies an assume of a constraint of the form: "(exp comparison_op expr) != 0" to true into an assume of "exp comparison_op expr" to true. (And similarly, an assume of the form "(exp comparison_op expr) == 0" to true as an assume of exp comparison_op expr to false.) which improves precision overall. https://reviews.llvm.org/D22862 git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@290413 91177308-0d34-0410-b5e6-96231b3b80d8 --- lib/StaticAnalyzer/Core/ExprEngineC.cpp | 24 ++++++------------- .../Core/SimpleConstraintManager.cpp | 16 ++++++++++++- test/Analysis/malloc.c | 11 +++++++++ test/Analysis/misc-ps.c | 10 ++++++++ 4 files changed, 43 insertions(+), 18 deletions(-) diff --git a/lib/StaticAnalyzer/Core/ExprEngineC.cpp b/lib/StaticAnalyzer/Core/ExprEngineC.cpp index f1238f6d35..3e6343813e 100644 --- a/lib/StaticAnalyzer/Core/ExprEngineC.cpp +++ b/lib/StaticAnalyzer/Core/ExprEngineC.cpp @@ -665,23 +665,13 @@ void ExprEngine::VisitLogicalExpr(const BinaryOperator* B, ExplodedNode *Pred, if (RHSVal.isUndef()) { X = RHSVal; } else { - DefinedOrUnknownSVal DefinedRHS = RHSVal.castAs(); - ProgramStateRef StTrue, StFalse; - std::tie(StTrue, StFalse) = N->getState()->assume(DefinedRHS); - if (StTrue) { - if (StFalse) { - // We can't constrain the value to 0 or 1. - // The best we can do is a cast. - X = getSValBuilder().evalCast(RHSVal, B->getType(), RHS->getType()); - } else { - // The value is known to be true. - X = getSValBuilder().makeIntVal(1, B->getType()); - } - } else { - // The value is known to be false. - assert(StFalse && "Infeasible path!"); - X = getSValBuilder().makeIntVal(0, B->getType()); - } + // We evaluate "RHSVal != 0" expression which result in 0 if the value is + // known to be false, 1 if the value is known to be true and a new symbol + // when the assumption is unknown. + nonloc::ConcreteInt Zero(getBasicVals().getValue(0, B->getType())); + X = evalBinOp(N->getState(), BO_NE, + svalBuilder.evalCast(RHSVal, B->getType(), RHS->getType()), + Zero, B->getType()); } } Bldr.generateNode(B, Pred, state->BindExpr(B, Pred->getLocationContext(), X)); diff --git a/lib/StaticAnalyzer/Core/SimpleConstraintManager.cpp b/lib/StaticAnalyzer/Core/SimpleConstraintManager.cpp index c5e0f9b862..ef2fcefb05 100644 --- a/lib/StaticAnalyzer/Core/SimpleConstraintManager.cpp +++ b/lib/StaticAnalyzer/Core/SimpleConstraintManager.cpp @@ -254,6 +254,21 @@ ProgramStateRef SimpleConstraintManager::assumeSymRel(ProgramStateRef State, assert(BinaryOperator::isComparisonOp(Op) && "Non-comparison ops should be rewritten as comparisons to zero."); + SymbolRef Sym = LHS; + + // Simplification: translate an assume of a constraint of the form + // "(exp comparison_op expr) != 0" to true into an assume of + // "exp comparison_op expr" to true. (And similarly, an assume of the form + // "(exp comparison_op expr) == 0" to true into an assume of + // "exp comparison_op expr" to false.) + if (Int == 0 && (Op == BO_EQ || Op == BO_NE)) { + if (const BinarySymExpr *SE = dyn_cast(Sym)) { + BinaryOperator::Opcode Op = SE->getOpcode(); + if (BinaryOperator::isComparisonOp(Op)) + return assume(State, nonloc::SymbolVal(Sym), (Op == BO_NE ? true : false)); + } + } + // Get the type used for calculating wraparound. BasicValueFactory &BVF = getBasicVals(); APSIntType WraparoundType = BVF.getAPSIntType(LHS->getType()); @@ -265,7 +280,6 @@ ProgramStateRef SimpleConstraintManager::assumeSymRel(ProgramStateRef State, // x < 4 has the solution [0, 3]. x+2 < 4 has the solution [0-2, 3-2], which // in modular arithmetic is [0, 1] U [UINT_MAX-1, UINT_MAX]. It's up to // the subclasses of SimpleConstraintManager to handle the adjustment. - SymbolRef Sym = LHS; llvm::APSInt Adjustment = WraparoundType.getZeroValue(); computeAdjustment(Sym, Adjustment); diff --git a/test/Analysis/malloc.c b/test/Analysis/malloc.c index 51e2cd6043..d5bc6571e4 100644 --- a/test/Analysis/malloc.c +++ b/test/Analysis/malloc.c @@ -1763,6 +1763,17 @@ void testConstEscapeThroughAnotherField() { constEscape(&(s.x)); // could free s->p! } // no-warning +// PR15623 +int testNoCheckerDataPropogationFromLogicalOpOperandToOpResult(void) { + char *param = malloc(10); + char *value = malloc(10); + int ok = (param && value); + free(param); + free(value); + // Previously we ended up with 'Use of memory after it is freed' on return. + return ok; // no warning +} + // ---------------------------------------------------------------------------- // False negatives. diff --git a/test/Analysis/misc-ps.c b/test/Analysis/misc-ps.c index 01cad1549c..ad65437e4c 100644 --- a/test/Analysis/misc-ps.c +++ b/test/Analysis/misc-ps.c @@ -191,3 +191,13 @@ static void PR16131(int x) { clang_analyzer_eval(*ip == 42); // expected-warning{{TRUE}} clang_analyzer_eval(*(int *)&v == 42); // expected-warning{{TRUE}} } + +// PR15623: Currently the analyzer doesn't handle symbolic expressions of the +// form "(exp comparison_op expr) != 0" very well. We perform a simplification +// translating an assume of a constraint of the form "(exp comparison_op expr) +// != 0" to true into an assume of "exp comparison_op expr" to true. +void PR15623(int n) { + if ((n == 0) != 0) { + clang_analyzer_eval(n == 0); // expected-warning{{TRUE}} + } +} -- 2.40.0