[cfe-commits] r75884 - in /cfe/trunk: include/clang/Analysis/PathSensitive/SValuator.h lib/Analysis/MemRegion.cpp lib/Analysis/RegionStore.cpp lib/Analysis/SimpleSValuator.cpp test/Analysis/misc-ps.m
Ted Kremenek
kremenek at apple.com
Wed Jul 15 18:33:37 PDT 2009
Author: kremenek
Date: Wed Jul 15 20:33:37 2009
New Revision: 75884
URL: http://llvm.org/viewvc/llvm-project?rev=75884&view=rev
Log:
Move RegionStoreManager over to using new
ValueManager::makeArrayIndex()/convertArrayIndex() methods. This
handles yet another crash case when reasoning about array indices of
different bitwidth and signedness.
Modified:
cfe/trunk/include/clang/Analysis/PathSensitive/SValuator.h
cfe/trunk/lib/Analysis/MemRegion.cpp
cfe/trunk/lib/Analysis/RegionStore.cpp
cfe/trunk/lib/Analysis/SimpleSValuator.cpp
cfe/trunk/test/Analysis/misc-ps.m
Modified: cfe/trunk/include/clang/Analysis/PathSensitive/SValuator.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Analysis/PathSensitive/SValuator.h?rev=75884&r1=75883&r2=75884&view=diff
==============================================================================
--- cfe/trunk/include/clang/Analysis/PathSensitive/SValuator.h (original)
+++ cfe/trunk/include/clang/Analysis/PathSensitive/SValuator.h Wed Jul 15 20:33:37 2009
@@ -31,9 +31,17 @@
SValuator(ValueManager &valMgr) : ValMgr(valMgr) {}
virtual ~SValuator() {}
- virtual SVal EvalCast(NonLoc val, QualType castTy) = 0;
+ virtual SVal EvalCastNL(NonLoc val, QualType castTy) = 0;
- virtual SVal EvalCast(Loc val, QualType castTy) = 0;
+ virtual SVal EvalCastL(Loc val, QualType castTy) = 0;
+
+ SVal EvalCast(SVal val, QualType castTy) {
+ if (val.isUnknownOrUndef())
+ return val;
+
+ return isa<Loc>(val) ? EvalCastL(cast<Loc>(val), castTy)
+ : EvalCastNL(cast<NonLoc>(val), castTy);
+ }
virtual SVal EvalMinus(NonLoc val) = 0;
Modified: cfe/trunk/lib/Analysis/MemRegion.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Analysis/MemRegion.cpp?rev=75884&r1=75883&r2=75884&view=diff
==============================================================================
--- cfe/trunk/lib/Analysis/MemRegion.cpp (original)
+++ cfe/trunk/lib/Analysis/MemRegion.cpp Wed Jul 15 20:33:37 2009
@@ -259,7 +259,8 @@
ElementRegion*
MemRegionManager::getElementRegion(QualType elementType, SVal Idx,
- const MemRegion* superRegion, ASTContext& Ctx){
+ const MemRegion* superRegion,
+ ASTContext& Ctx){
QualType T = Ctx.getCanonicalType(elementType);
Modified: cfe/trunk/lib/Analysis/RegionStore.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Analysis/RegionStore.cpp?rev=75884&r1=75883&r2=75884&view=diff
==============================================================================
--- cfe/trunk/lib/Analysis/RegionStore.cpp (original)
+++ cfe/trunk/lib/Analysis/RegionStore.cpp Wed Jul 15 20:33:37 2009
@@ -500,6 +500,9 @@
// Pointer of any type can be cast and used as array base.
const ElementRegion *ElemR = dyn_cast<ElementRegion>(BaseRegion);
+ // Convert the offset to the appropriate size and signedness.
+ Offset = ValMgr.convertToArrayIndex(Offset);
+
if (!ElemR) {
//
// If the base region is not an ElementRegion, create one.
@@ -510,16 +513,6 @@
//
// Observe that 'p' binds to an AllocaRegion.
//
-
- // Offset might be unsigned. We have to convert it to signed ConcreteInt.
- if (nonloc::ConcreteInt* CI = dyn_cast<nonloc::ConcreteInt>(&Offset)) {
- const llvm::APSInt& OffI = CI->getValue();
- if (OffI.isUnsigned()) {
- llvm::APSInt Tmp = OffI;
- Tmp.setIsSigned(true);
- Offset = ValMgr.makeIntVal(Tmp);
- }
- }
return loc::MemRegionVal(MRMgr.getElementRegion(elementType, Offset,
BaseRegion, getContext()));
}
@@ -533,29 +526,11 @@
const llvm::APSInt& OffI = cast<nonloc::ConcreteInt>(Offset).getValue();
assert(BaseIdxI.isSigned());
- // FIXME: This appears to be the assumption of this code. We should review
- // whether or not BaseIdxI.getBitWidth() < OffI.getBitWidth(). If it
- // can't we need to put a comment here. If it can, we should handle it.
- assert(BaseIdxI.getBitWidth() >= OffI.getBitWidth());
-
- const MemRegion *ArrayR = ElemR->getSuperRegion();
- SVal NewIdx;
+ // Compute the new index.
+ SVal NewIdx = nonloc::ConcreteInt(getBasicVals().getValue(BaseIdxI + OffI));
- if (OffI.isUnsigned() || OffI.getBitWidth() < BaseIdxI.getBitWidth()) {
- // 'Offset' might be unsigned. We have to convert it to signed and
- // possibly extend it.
- llvm::APSInt Tmp = OffI;
-
- if (OffI.getBitWidth() < BaseIdxI.getBitWidth())
- Tmp.extend(BaseIdxI.getBitWidth());
-
- Tmp.setIsSigned(true);
- Tmp += BaseIdxI; // Compute the new offset.
- NewIdx = ValMgr.makeIntVal(Tmp);
- }
- else
- NewIdx = nonloc::ConcreteInt(getBasicVals().getValue(BaseIdxI + OffI));
-
+ // Construct the new ElementRegion.
+ const MemRegion *ArrayR = ElemR->getSuperRegion();
return loc::MemRegionVal(MRMgr.getElementRegion(elementType, NewIdx, ArrayR,
getContext()));
}
@@ -760,20 +735,13 @@
// Only support concrete integer indexes for now.
if (Base && Offset) {
- // FIXME: For now, convert the signedness and bitwidth of offset in case
- // they don't match. This can result from pointer arithmetic. In reality,
- // we should figure out what are the proper semantics and implement them.
- //
- // This addresses the test case test/Analysis/ptr-arith.c
- //
- nonloc::ConcreteInt OffConverted(getBasicVals().Convert(Base->getValue(),
- Offset->getValue()));
- SVal NewIdx = Base->evalBinOp(ValMgr, Op, OffConverted);
+ // FIXME: Should use SValuator here.
+ SVal NewIdx = Base->evalBinOp(ValMgr, Op,
+ cast<nonloc::ConcreteInt>(ValMgr.convertToArrayIndex(*Offset)));
const MemRegion* NewER =
- MRMgr.getElementRegion(ER->getElementType(), NewIdx,ER->getSuperRegion(),
- getContext());
+ MRMgr.getElementRegion(ER->getElementType(), NewIdx, ER->getSuperRegion(),
+ getContext());
return ValMgr.makeLoc(NewER);
-
}
return UnknownVal();
@@ -837,8 +805,8 @@
ASTContext &Ctx = getContext();
if (!T.isNull() && IsReinterpreted(RTy, T, Ctx)) {
- SVal idx = ValMgr.makeIntVal(0, Ctx.IntTy);
- R = MRMgr.getElementRegion(T, idx, R, Ctx);
+ SVal ZeroIdx = ValMgr.makeZeroArrayIndex();
+ R = MRMgr.getElementRegion(T, ZeroIdx, R, Ctx);
RTy = T;
assert(Ctx.getCanonicalType(RTy) ==
Ctx.getCanonicalType(R->getValueType(Ctx)));
@@ -1083,11 +1051,9 @@
ConstantArrayType* CAT = cast<ConstantArrayType>(T.getTypePtr());
llvm::ImmutableList<SVal> ArrayVal = getBasicVals().getEmptySValList();
- llvm::APSInt Size(CAT->getSize(), false);
- llvm::APSInt i = getBasicVals().getZeroWithPtrWidth(false);
-
- for (; i < Size; ++i) {
- SVal Idx = ValMgr.makeIntVal(i);
+ uint64_t size = CAT->getSize().getZExtValue();
+ for (uint64_t i = 0; i < size; ++i) {
+ SVal Idx = ValMgr.makeArrayIndex(i);
ElementRegion* ER = MRMgr.getElementRegion(CAT->getElementType(), Idx, R,
getContext());
QualType ETy = ER->getElementType();
@@ -1160,15 +1126,14 @@
}
const GRState *RegionStoreManager::BindArray(const GRState *state,
- const TypedRegion* R,
+ const TypedRegion* R,
SVal Init) {
QualType T = R->getValueType(getContext());
ConstantArrayType* CAT = cast<ConstantArrayType>(T.getTypePtr());
QualType ElementTy = CAT->getElementType();
- llvm::APSInt Size(CAT->getSize(), false);
- llvm::APSInt i(llvm::APInt::getNullValue(Size.getBitWidth()), false);
+ uint64_t size = CAT->getSize().getZExtValue();
// Check if the init expr is a StringLiteral.
if (isa<loc::MemRegionVal>(Init)) {
@@ -1181,12 +1146,13 @@
// Copy bytes from the string literal into the target array. Trailing bytes
// in the array that are not covered by the string literal are initialized
// to zero.
- for (; i < Size; ++i, ++j) {
+ for (uint64_t i = 0; i < size; ++i, ++j) {
if (j >= len)
break;
- SVal Idx = ValMgr.makeIntVal(i);
- ElementRegion* ER = MRMgr.getElementRegion(ElementTy, Idx,R,getContext());
+ SVal Idx = ValMgr.makeArrayIndex(i);
+ ElementRegion* ER = MRMgr.getElementRegion(ElementTy, Idx, R,
+ getContext());
SVal V = ValMgr.makeIntVal(str[j], sizeof(char)*8, true);
state = Bind(state, loc::MemRegionVal(ER), V);
@@ -1197,13 +1163,14 @@
nonloc::CompoundVal& CV = cast<nonloc::CompoundVal>(Init);
nonloc::CompoundVal::iterator VI = CV.begin(), VE = CV.end();
-
- for (; i < Size; ++i, ++VI) {
+ uint64_t i = 0;
+
+ for (; i < size; ++i, ++VI) {
// The init list might be shorter than the array length.
if (VI == VE)
break;
- SVal Idx = ValMgr.makeIntVal(i);
+ SVal Idx = ValMgr.makeArrayIndex(i);
ElementRegion* ER = MRMgr.getElementRegion(ElementTy, Idx, R, getContext());
if (CAT->getElementType()->isStructureType())
@@ -1214,7 +1181,7 @@
// If the init list is shorter than the array length, set the array default
// value.
- if (i < Size) {
+ if (i < size) {
if (ElementTy->isIntegerType()) {
SVal V = ValMgr.makeZeroVal(ElementTy);
state = setDefaultValue(state, R, V);
Modified: cfe/trunk/lib/Analysis/SimpleSValuator.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Analysis/SimpleSValuator.cpp?rev=75884&r1=75883&r2=75884&view=diff
==============================================================================
--- cfe/trunk/lib/Analysis/SimpleSValuator.cpp (original)
+++ cfe/trunk/lib/Analysis/SimpleSValuator.cpp Wed Jul 15 20:33:37 2009
@@ -23,8 +23,8 @@
SimpleSValuator(ValueManager &valMgr) : SValuator(valMgr) {}
virtual ~SimpleSValuator() {}
- virtual SVal EvalCast(NonLoc val, QualType castTy);
- virtual SVal EvalCast(Loc val, QualType castTy);
+ virtual SVal EvalCastNL(NonLoc val, QualType castTy);
+ virtual SVal EvalCastL(Loc val, QualType castTy);
virtual SVal EvalMinus(NonLoc val);
virtual SVal EvalComplement(NonLoc val);
virtual SVal EvalBinOpNN(BinaryOperator::Opcode op, NonLoc lhs, NonLoc rhs,
@@ -44,7 +44,7 @@
// Transfer function for Casts.
//===----------------------------------------------------------------------===//
-SVal SimpleSValuator::EvalCast(NonLoc val, QualType castTy) {
+SVal SimpleSValuator::EvalCastNL(NonLoc val, QualType castTy) {
if (!isa<nonloc::ConcreteInt>(val))
return UnknownVal();
@@ -64,7 +64,7 @@
return ValMgr.makeIntVal(i);
}
-SVal SimpleSValuator::EvalCast(Loc val, QualType castTy) {
+SVal SimpleSValuator::EvalCastL(Loc val, QualType castTy) {
// Casts from pointers -> pointers, just return the lval.
//
Modified: cfe/trunk/test/Analysis/misc-ps.m
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/misc-ps.m?rev=75884&r1=75883&r2=75884&view=diff
==============================================================================
--- cfe/trunk/test/Analysis/misc-ps.m (original)
+++ cfe/trunk/test/Analysis/misc-ps.m Wed Jul 15 20:33:37 2009
@@ -415,3 +415,18 @@
*p = 0xDEADBEEF; // no-warning
}
+// This test reproduces a case for a crash when analyzing ClamAV using
+// RegionStoreManager (the crash doesn't exhibit in BasicStoreManager because
+// it isn't doing anything smart about arrays). The problem is that on the
+// second line, 'p = &p[i]', p is assigned an ElementRegion whose index
+// is a 16-bit integer. On the third line, a new ElementRegion is created
+// based on the previous region, but there the region uses a 32-bit integer,
+// resulting in a clash of values (an assertion failure at best). We resolve
+// this problem by implicitly converting index values to 'int' when the
+// ElementRegion is created.
+unsigned char test_array_index_bitwidth(const unsigned char *p) {
+ unsigned short i = 0;
+ for (i = 0; i < 2; i++) p = &p[i];
+ return p[i+1];
+}
+
More information about the cfe-commits
mailing list