[cfe-commits] [patch] Tracking simple arithmetic constraints (PR2695) (resubmitted)
Jordy Rose
jediknil at belkadan.com
Sat Jun 5 18:01:24 PDT 2010
*ping*
Very basic support for handling conditions involving addition and
subtraction, such as this:
char* name = malloc(1);
if (length+1 == 10) {
free(name);
}
if (length+1 == 10) {
name = malloc(1); // no-warning
}
free(name);
Fixes PR2695; next on the list would be to expand this for the case in
PR4550, which uses shifts. These will be harder, of course, since shifts
and the rest of the binary operations (except XOR) destroy information.
Hoping this is a reasonable way to implement this? In particular, I get
the feeling that there's an easier way to perform APSInt operations and
catch overflow.
-------------- next part --------------
Index: test/Analysis/linearity.c
===================================================================
--- test/Analysis/linearity.c (revision 0)
+++ test/Analysis/linearity.c (revision 0)
@@ -0,0 +1,136 @@
+// RUN: %clang_cc1 -analyze -analyzer-check-objc-mem -analyzer-experimental-checks -verify %s
+#include <stdlib.h>
+#include <limits.h>
+
+void subtractRegular(unsigned int length) {
+ char* name = malloc(1);
+ if (length-1 == 1) {
+ free(name);
+ name = 0;
+ }
+ if (length-1 == 1) {
+ name = malloc(1); // no-warning
+ }
+
+ free(name);
+}
+
+void subtractEqZero(unsigned int length) {
+ char* name = malloc(1);
+ if (length-1 == 0) {
+ free(name);
+ name = 0;
+ }
+ if (length-1 == 0) {
+ name = malloc(1); // no-warning
+ }
+
+ free(name);
+}
+
+void subtractMultiple(unsigned int length) {
+ char* name = malloc(1);
+ if (length-2 == 1) {
+ free(name);
+ name = 0;
+ } else {
+ length -= 2;
+ }
+ if (length-1 == 0) {
+ name = malloc(1); // no-warning
+ }
+
+ free(name);
+}
+
+void addRegular(unsigned int length) {
+ char* name = malloc(1);
+ if (length+1 == 10) {
+ free(name);
+ name = 0;
+ }
+ if (length+1 == 10) {
+ name = malloc(1); // no-warning
+ }
+
+ free(name);
+}
+
+void addUnderflow(unsigned int length) {
+ char* name = malloc(1);
+ if (length+1 == 0) {
+ name = malloc(1); // no-warning
+ }
+ if (length+1 <= 0) {
+ name = malloc(1); // no-warning
+ }
+
+ free(name);
+}
+
+void subtractOverflow(unsigned int length) {
+ char* name = malloc(1);
+ if (length-1 == UINT_MAX) {
+ name = malloc(1); // no-warning
+ }
+ if (length-1 >= UINT_MAX) {
+ name = malloc(1); // no-warning
+ }
+
+ free(name);
+}
+
+void compoundSafe(unsigned int length) {
+ char* name = malloc(1);
+ unsigned int lots = length - 10;
+ if (lots == 10) {
+ free(name);
+ name = 0;
+ }
+ if (lots+10 == 20) {
+ name = malloc(1); // no-warning
+ }
+
+ free(name);
+}
+
+void compoundDangerous(unsigned int length) {
+ char* name = malloc(1);
+ unsigned int lots = length - 10;
+ if (lots == 10) {
+ free(name);
+ name = 0;
+ lots += 10;
+ }
+ if (lots == 20) {
+ name = malloc(1); // expected-warning{{Allocated memory never released. Potential memory leak.}}
+ }
+
+ free(name);
+}
+
+void wraparoundSafeGT(unsigned int length) {
+ char* name = malloc(1);
+ unsigned int temp = length-1;
+ if (length > 0) {
+ free(name);
+ }
+ if (temp+1 > 0) {
+ name = malloc(1); // no-warning
+ }
+
+ free(name);
+}
+
+void wraparoundSafeEQ(unsigned int length) {
+ char* name = malloc(1);
+ unsigned int temp = length-1;
+ if (length == 0) {
+ free(name);
+ }
+ if (temp+1 == 0) {
+ name = malloc(1); // no-warning
+ }
+
+ free(name);
+}
Index: include/clang/Checker/PathSensitive/BasicValueFactory.h
===================================================================
--- include/clang/Checker/PathSensitive/BasicValueFactory.h (revision 104412)
+++ include/clang/Checker/PathSensitive/BasicValueFactory.h (working copy)
@@ -76,6 +76,12 @@
llvm::FoldingSet<LazyCompoundValData> LazyCompoundValDataSet;
public:
+ enum OverflowKind {
+ NoOverflow = 0,
+ AdditionOverflow = 1,
+ AdditionUnderflow = -AdditionOverflow
+ };
+
BasicValueFactory(ASTContext& ctx, llvm::BumpPtrAllocator& Alloc)
: Ctx(ctx), BPAlloc(Alloc), PersistentSVals(0), PersistentSValPairs(0),
SValListFactory(Alloc) {}
@@ -181,7 +187,8 @@
const llvm::APSInt* EvaluateAPSInt(BinaryOperator::Opcode Op,
const llvm::APSInt& V1,
- const llvm::APSInt& V2);
+ const llvm::APSInt& V2,
+ OverflowKind* OverflowDirection = NULL);
const std::pair<SVal, uintptr_t>&
getPersistentSValWithData(const SVal& V, uintptr_t Data);
Index: lib/Checker/SimpleSValuator.cpp
===================================================================
--- lib/Checker/SimpleSValuator.cpp (revision 104412)
+++ lib/Checker/SimpleSValuator.cpp (working copy)
@@ -261,17 +261,17 @@
}
}
case nonloc::SymExprValKind: {
- // Logical not?
- if (!(op == BinaryOperator::EQ && rhs.isZeroConstant()))
- return UnknownVal();
-
const SymExpr *symExpr =
cast<nonloc::SymExprVal>(lhs).getSymbolicExpression();
-
- // Only handle ($sym op constant) for now.
- if (const SymIntExpr *symIntExpr = dyn_cast<SymIntExpr>(symExpr)) {
- BinaryOperator::Opcode opc = symIntExpr->getOpcode();
- switch (opc) {
+
+ if (op == BinaryOperator::EQ && rhs.isZeroConstant()) {
+ // Logical not. We can special case some of this
+
+ // Only handle ($sym op constant) for now.
+ if (const SymIntExpr *symIntExpr = dyn_cast<SymIntExpr>(symExpr)) {
+ BinaryOperator::Opcode opc = symIntExpr->getOpcode();
+ switch (opc) {
+ default: break; // and try to handle it the normal way
case BinaryOperator::LAnd:
case BinaryOperator::LOr:
assert(false && "Logical operators handled by branching logic.");
@@ -294,18 +294,6 @@
case BinaryOperator::PtrMemI:
assert(false && "Pointer arithmetic not handled here.");
return UnknownVal();
- case BinaryOperator::Mul:
- case BinaryOperator::Div:
- case BinaryOperator::Rem:
- case BinaryOperator::Add:
- case BinaryOperator::Sub:
- case BinaryOperator::Shl:
- case BinaryOperator::Shr:
- case BinaryOperator::And:
- case BinaryOperator::Xor:
- case BinaryOperator::Or:
- // Not handled yet.
- return UnknownVal();
case BinaryOperator::LT:
case BinaryOperator::GT:
case BinaryOperator::LE:
@@ -316,8 +304,18 @@
assert(symIntExpr->getType(ValMgr.getContext()) == resultTy);
return ValMgr.makeNonLoc(symIntExpr->getLHS(), opc,
symIntExpr->getRHS(), resultTy);
+ }
}
}
+
+ // for now, only handle ($sym op constant) here as well
+ if (isa<nonloc::ConcreteInt>(rhs)) {
+ return ValMgr.makeNonLoc(symExpr, op,
+ cast<nonloc::ConcreteInt>(rhs).getValue(),
+ resultTy);
+ }
+
+ return UnknownVal();
}
case nonloc::ConcreteIntKind: {
if (isa<nonloc::ConcreteInt>(rhs)) {
Index: lib/Checker/BasicValueFactory.cpp
===================================================================
--- lib/Checker/BasicValueFactory.cpp (revision 104412)
+++ lib/Checker/BasicValueFactory.cpp (working copy)
@@ -143,7 +143,9 @@
const llvm::APSInt*
BasicValueFactory::EvaluateAPSInt(BinaryOperator::Opcode Op,
- const llvm::APSInt& V1, const llvm::APSInt& V2) {
+ const llvm::APSInt& V1,
+ const llvm::APSInt& V2,
+ BasicValueFactory::OverflowKind* OverflowDirection) {
switch (Op) {
default:
@@ -159,9 +161,33 @@
return &getValue( V1 % V2 );
case BinaryOperator::Add:
+ // FIXME: expand this for all types
+ if (OverflowDirection) {
+ if (V2.isSigned() && V2.isNegative()) {
+ if (getMinValue(V2) - V2 > V1) {
+ *OverflowDirection = AdditionUnderflow;
+ }
+ } else {
+ if (getMaxValue(V2) - V2 < V1) {
+ *OverflowDirection = AdditionOverflow;
+ }
+ }
+ }
return &getValue( V1 + V2 );
case BinaryOperator::Sub:
+ // FIXME: expand this for all types
+ if (OverflowDirection) {
+ if (V2.isSigned() && V2.isNegative()) {
+ if (getMaxValue(V2) + V2 < V1) {
+ *OverflowDirection = AdditionOverflow;
+ }
+ } else {
+ if (getMinValue(V2) + V2 > V1) {
+ *OverflowDirection = AdditionUnderflow;
+ }
+ }
+ }
return &getValue( V1 - V2 );
case BinaryOperator::Shl: {
Index: lib/Checker/SimpleConstraintManager.cpp
===================================================================
--- lib/Checker/SimpleConstraintManager.cpp (revision 104412)
+++ lib/Checker/SimpleConstraintManager.cpp (working copy)
@@ -39,11 +39,12 @@
case BinaryOperator::Mul:
case BinaryOperator::Div:
case BinaryOperator::Rem:
- case BinaryOperator::Add:
- case BinaryOperator::Sub:
case BinaryOperator::Shl:
case BinaryOperator::Shr:
return false;
+ case BinaryOperator::Add:
+ case BinaryOperator::Sub:
+ return true;
// All other cases.
default:
return true;
@@ -157,11 +158,61 @@
// we support truncation/extension of symbolic values.
GRStateManager &StateMgr = state->getStateManager();
ASTContext &Ctx = StateMgr.getContext();
- QualType LHSType = SE->getLHS()->getType(Ctx);
+ const SymExpr *LHS = SE->getLHS();
+ QualType LHSType = LHS->getType(Ctx);
BasicValueFactory &BasicVals = StateMgr.getBasicVals();
- const llvm::APSInt &RHS = BasicVals.Convert(LHSType, SE->getRHS());
- SymIntExpr SENew(SE->getLHS(), SE->getOpcode(), RHS, SE->getType(Ctx));
+
+ const llvm::APSInt *RHS = &SE->getRHS();
+ int TotalOverflow = 0;
+
+ while (const SymIntExpr *LSE = dyn_cast<SymIntExpr>(LHS)) {
+ LHS = LSE->getLHS();
+ BinaryOperator::Opcode op = LSE->getOpcode();
+
+ // TODO: wrap me in a procedure?
+ // FIXME: only handles plus and minus!
+ switch (op) {
+ default:
+ // can't reverse the expression, so give up and assume feasible
+ return state;
+ case BinaryOperator::Add:
+ op = BinaryOperator::Sub;
+ break;
+ case BinaryOperator::Sub:
+ op = BinaryOperator::Add;
+ break;
+ }
+ BasicValueFactory::OverflowKind Overflow =
+ BasicValueFactory::NoOverflow;
+ const llvm::APSInt *newRHS = BasicVals.EvaluateAPSInt(op,
+ *RHS,
+ LSE->getRHS(),
+ &Overflow);
+ TotalOverflow += Overflow;
+ RHS = newRHS;
+ }
+
+ // Overflow may have canceled out if we wrap back and forth
+ // Ex: (n + 1) - 1
+ if (TotalOverflow) {
+ switch (SE->getOpcode()) {
+ default:
+ return state;
+ case BinaryOperator::EQ:
+ return Assumption ? NULL : state;
+ case BinaryOperator::GE:
+ case BinaryOperator::GT:
+ return (Assumption ^ (TotalOverflow > 0)) ? state : NULL;
+ case BinaryOperator::LE:
+ case BinaryOperator::LT:
+ return (Assumption ^ (TotalOverflow < 0)) ? state : NULL;
+ }
+ }
+
+ const llvm::APSInt &finalRHS = BasicVals.Convert(LHSType, *RHS);
+ SymIntExpr SENew(LHS, SE->getOpcode(), finalRHS, SE->getType(Ctx));
+
return AssumeSymInt(state, Assumption, &SENew);
}
@@ -186,9 +237,12 @@
bool Assumption,
const SymIntExpr *SE) {
+ // If we failed to symplify the expression, just assume feasible.
+ if (!isa<SymbolData>(SE->getLHS()))
+ return state;
- // Here we assume that LHS is a symbol. This is consistent with the
- // rest of the constraint manager logic.
+ // At this point we can assume that LHS is a symbol.
+ // This is consistent with the rest of the constraint manager logic.
SymbolRef Sym = cast<SymbolData>(SE->getLHS());
const llvm::APSInt &Int = SE->getRHS();
More information about the cfe-commits
mailing list