From: Anna Zaks Date: Tue, 28 May 2013 17:31:43 +0000 (+0000) Subject: [analyzer] Use a more generic MemRegion.getAsOffset to evaluate bin operators on... X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=4e9179a3d0ec612a4d540281020b200254348a6b;p=clang [analyzer] Use a more generic MemRegion.getAsOffset to evaluate bin operators on MemRegions In addition to enabling more code reuse, this suppresses some false positives by allowing us to compare an element region to its base. See the ptr-arith.cpp test cases for an example. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@182780 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp b/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp index ee627f2baa..cb9e451b24 100644 --- a/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp +++ b/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp @@ -507,6 +507,53 @@ SVal SimpleSValBuilder::evalBinOpNN(ProgramStateRef state, } } +static SVal evalBinOpFieldRegionFieldRegion(const FieldRegion *LeftFR, + const FieldRegion *RightFR, + BinaryOperator::Opcode op, + QualType resultTy, + SimpleSValBuilder &SVB) { + // Only comparisons are meaningful here! + if (!BinaryOperator::isComparisonOp(op)) + return UnknownVal(); + + // Next, see if the two FRs have the same super-region. + // FIXME: This doesn't handle casts yet, and simply stripping the casts + // doesn't help. + if (LeftFR->getSuperRegion() != RightFR->getSuperRegion()) + return UnknownVal(); + + const FieldDecl *LeftFD = LeftFR->getDecl(); + const FieldDecl *RightFD = RightFR->getDecl(); + const RecordDecl *RD = LeftFD->getParent(); + + // Make sure the two FRs are from the same kind of record. Just in case! + // FIXME: This is probably where inheritance would be a problem. + if (RD != RightFD->getParent()) + return UnknownVal(); + + // We know for sure that the two fields are not the same, since that + // would have given us the same SVal. + if (op == BO_EQ) + return SVB.makeTruthVal(false, resultTy); + if (op == BO_NE) + return SVB.makeTruthVal(true, resultTy); + + // Iterate through the fields and see which one comes first. + // [C99 6.7.2.1.13] "Within a structure object, the non-bit-field + // members and the units in which bit-fields reside have addresses that + // increase in the order in which they are declared." + bool leftFirst = (op == BO_LT || op == BO_LE); + for (RecordDecl::field_iterator I = RD->field_begin(), + E = RD->field_end(); I!=E; ++I) { + if (*I == LeftFD) + return SVB.makeTruthVal(leftFirst, resultTy); + if (*I == RightFD) + return SVB.makeTruthVal(!leftFirst, resultTy); + } + + llvm_unreachable("Fields not found in parent record's definition"); +} + // FIXME: all this logic will change if/when we have MemRegion::getLocation(). SVal SimpleSValBuilder::evalBinOpLL(ProgramStateRef state, BinaryOperator::Opcode op, @@ -699,14 +746,10 @@ SVal SimpleSValBuilder::evalBinOpLL(ProgramStateRef state, } } - // FIXME: If/when there is a getAsRawOffset() for FieldRegions, this - // ElementRegion path and the FieldRegion path below should be unified. - if (const ElementRegion *LeftER = dyn_cast(LeftMR)) { - // First see if the right region is also an ElementRegion. - const ElementRegion *RightER = dyn_cast(RightMR); - if (!RightER) - return UnknownVal(); - + // Handle special cases for when both regions are element regions. + const ElementRegion *RightER = dyn_cast(RightMR); + const ElementRegion *LeftER = dyn_cast(LeftMR); + if (RightER && LeftER) { // Next, see if the two ERs have the same super-region and matching types. // FIXME: This should do something useful even if the types don't match, // though if both indexes are constant the RegionRawOffset path will @@ -738,17 +781,29 @@ SVal SimpleSValBuilder::evalBinOpLL(ProgramStateRef state, // evalBinOpNN expects the two indexes to already be the right type. return evalBinOpNN(state, op, *LeftIndex, *RightIndex, resultTy); } + } - // If the element indexes aren't comparable, see if the raw offsets are. - RegionRawOffset LeftOffset = LeftER->getAsArrayOffset(); - RegionRawOffset RightOffset = RightER->getAsArrayOffset(); + // Special handling of the FieldRegions, even with symbolic offsets. + const FieldRegion *RightFR = dyn_cast(RightMR); + const FieldRegion *LeftFR = dyn_cast(LeftMR); + if (RightFR && LeftFR) { + SVal R = evalBinOpFieldRegionFieldRegion(LeftFR, RightFR, op, resultTy, + *this); + if (!R.isUnknown()) + return R; + } - if (LeftOffset.getRegion() != NULL && - LeftOffset.getRegion() == RightOffset.getRegion()) { - CharUnits left = LeftOffset.getOffset(); - CharUnits right = RightOffset.getOffset(); + // Compare the regions using the raw offsets. + RegionOffset LeftOffset = LeftMR->getAsOffset(); + RegionOffset RightOffset = RightMR->getAsOffset(); - switch (op) { + if (LeftOffset.getRegion() != NULL && + LeftOffset.getRegion() == RightOffset.getRegion() && + !LeftOffset.hasSymbolicOffset() && !RightOffset.hasSymbolicOffset()) { + int64_t left = LeftOffset.getOffset(); + int64_t right = RightOffset.getOffset(); + + switch (op) { default: return UnknownVal(); case BO_LT: @@ -763,60 +818,7 @@ SVal SimpleSValBuilder::evalBinOpLL(ProgramStateRef state, return makeTruthVal(left == right, resultTy); case BO_NE: return makeTruthVal(left != right, resultTy); - } } - - // If we get here, we have no way of comparing the ElementRegions. - } - - // See if both regions are fields of the same structure. - // FIXME: This doesn't handle nesting, inheritance, or Objective-C ivars. - if (const FieldRegion *LeftFR = dyn_cast(LeftMR)) { - // Only comparisons are meaningful here! - if (!BinaryOperator::isComparisonOp(op)) - return UnknownVal(); - - // First see if the right region is also a FieldRegion. - const FieldRegion *RightFR = dyn_cast(RightMR); - if (!RightFR) - return UnknownVal(); - - // Next, see if the two FRs have the same super-region. - // FIXME: This doesn't handle casts yet, and simply stripping the casts - // doesn't help. - if (LeftFR->getSuperRegion() != RightFR->getSuperRegion()) - return UnknownVal(); - - const FieldDecl *LeftFD = LeftFR->getDecl(); - const FieldDecl *RightFD = RightFR->getDecl(); - const RecordDecl *RD = LeftFD->getParent(); - - // Make sure the two FRs are from the same kind of record. Just in case! - // FIXME: This is probably where inheritance would be a problem. - if (RD != RightFD->getParent()) - return UnknownVal(); - - // We know for sure that the two fields are not the same, since that - // would have given us the same SVal. - if (op == BO_EQ) - return makeTruthVal(false, resultTy); - if (op == BO_NE) - return makeTruthVal(true, resultTy); - - // Iterate through the fields and see which one comes first. - // [C99 6.7.2.1.13] "Within a structure object, the non-bit-field - // members and the units in which bit-fields reside have addresses that - // increase in the order in which they are declared." - bool leftFirst = (op == BO_LT || op == BO_LE); - for (RecordDecl::field_iterator I = RD->field_begin(), - E = RD->field_end(); I!=E; ++I) { - if (*I == LeftFD) - return makeTruthVal(leftFirst, resultTy); - if (*I == RightFD) - return makeTruthVal(!leftFirst, resultTy); - } - - llvm_unreachable("Fields not found in parent record's definition"); } // At this point we're not going to get a good answer, but we can try diff --git a/test/Analysis/ptr-arith.c b/test/Analysis/ptr-arith.c index 35faff4a17..4a15bc24b9 100644 --- a/test/Analysis/ptr-arith.c +++ b/test/Analysis/ptr-arith.c @@ -280,3 +280,19 @@ void canonical_equal(int *lhs, int *rhs) { // FIXME: Should be FALSE. clang_analyzer_eval(rhs == lhs); // expected-warning{{UNKNOWN}} } + +void compare_element_region_and_base(int *p) { + int *q = p - 1; + clang_analyzer_eval(p == q); // expected-warning{{FALSE}} +} + +struct Point { + int x; + int y; +}; +void symbolicFieldRegion(struct Point *points, int i, int j) { + clang_analyzer_eval(&points[i].x == &points[j].x);// expected-warning{{UNKNOWN}} + clang_analyzer_eval(&points[i].x == &points[i].y);// expected-warning{{FALSE}} + clang_analyzer_eval(&points[i].x < &points[i].y);// expected-warning{{TRUE}} +} + diff --git a/test/Analysis/ptr-arith.cpp b/test/Analysis/ptr-arith.cpp new file mode 100644 index 0000000000..5f0951857b --- /dev/null +++ b/test/Analysis/ptr-arith.cpp @@ -0,0 +1,22 @@ +// RUN: %clang_cc1 -analyze -analyzer-checker=core,debug.ExprInspection -verify %s +// expected-no-diagnostics +struct X { + int *p; + int zero; + void foo () { + reset(p - 1); + } + void reset(int *in) { + while (in != p) // Loop must be entered. + zero = 1; + } +}; + +int test (int *in) { + X littleX; + littleX.zero = 0; + littleX.p = in; + littleX.foo(); + return 5/littleX.zero; // no-warning +} +