NoQ created this revision. NoQ added reviewers: zaks.anna, dcoughlin. NoQ added a subscriber: cfe-commits.
Essentially, if `s` is a structure, and `foo(const void *)` is evaluated conservatively, then `foo(&s)` does not invalidate `s`, but `foo(&(s.x))` invalidates the whole `s`, because the store only looks at traits of base regions (inside binding keys), and `s.x` is a field region. This patch represents the idea that only whole base regions should carry the `TK_PreserveContents` trait. This also makes a bit of sense, because no matter what pointer arithmetic we do with a const pointer, it's still a const pointer. There's an extra complication with mutable fields in C++ classes, which i neither added nor fixed here. In `CallEvent.cpp` below the changed code there's a FIXME comment, but i'm not sure what it means; if anybody thinks it means exactly what this patch is about, then i'd have to update it :) What i don't like about the approach this patch implements, is that it makes the core rely on an implementation detail of RegionStoreManager ("only base regions are relevant" is such implementation detail). Instead, i also tried to add a few extra virtual methods into the StoreManager to avoid this problem, but it made the patch much heavier. I can post that, unless anybody else thinks that it's a natural thing (rather than implementation detail) to propagate this trait to base regions. Instead, it should be possible to auto-replace the region with a base region inside `setTrait()` and `hasTrait()` methods. http://reviews.llvm.org/D19057 Files: lib/StaticAnalyzer/Checkers/CStringChecker.cpp lib/StaticAnalyzer/Core/CallEvent.cpp test/Analysis/call-invalidation.cpp Index: test/Analysis/call-invalidation.cpp =================================================================== --- test/Analysis/call-invalidation.cpp +++ test/Analysis/call-invalidation.cpp @@ -118,3 +118,47 @@ } +struct PlainStruct { + int x, y; + mutable int z; +}; + +PlainStruct glob; + +void useAnything(void *); +void useAnythingConst(const void *); + +void testInvalidationThroughBaseRegionPointer() { + PlainStruct s1; + s1.x = 1; + s1.z = 1; + clang_analyzer_eval(s1.x == 1); // expected-warning{{TRUE}} + clang_analyzer_eval(s1.z == 1); // expected-warning{{TRUE}} + useAnythingConst(&(s1.y)); + clang_analyzer_eval(s1.x == 1); // expected-warning{{TRUE}} + // FIXME: Should say "UNKNOWN", because it is not uncommon to + // modify a mutable member variable through const pointer. + clang_analyzer_eval(s1.z == 1); // expected-warning{{TRUE}} + useAnything(&(s1.y)); + clang_analyzer_eval(s1.x == 1); // expected-warning{{UNKNOWN}} +} + + +void useFirstConstSecondNonConst(const void *x, void *y); +void useFirstNonConstSecondConst(void *x, const void *y); + +void testMixedConstNonConstCalls() { + PlainStruct s2; + s2.x = 1; + useFirstConstSecondNonConst(&(s2.x), &(s2.y)); + clang_analyzer_eval(s2.x == 1); // expected-warning{{UNKNOWN}} + s2.x = 1; + useFirstNonConstSecondConst(&(s2.x), &(s2.y)); + clang_analyzer_eval(s2.x == 1); // expected-warning{{UNKNOWN}} + s2.y = 1; + useFirstConstSecondNonConst(&(s2.x), &(s2.y)); + clang_analyzer_eval(s2.y == 1); // expected-warning{{UNKNOWN}} + s2.y = 1; + useFirstNonConstSecondConst(&(s2.x), &(s2.y)); + clang_analyzer_eval(s2.y == 1); // expected-warning{{UNKNOWN}} +} Index: lib/StaticAnalyzer/Core/CallEvent.cpp =================================================================== --- lib/StaticAnalyzer/Core/CallEvent.cpp +++ lib/StaticAnalyzer/Core/CallEvent.cpp @@ -177,7 +177,7 @@ // below for efficiency. if (PreserveArgs.count(Idx)) if (const MemRegion *MR = getArgSVal(Idx).getAsRegion()) - ETraits.setTrait(MR->StripCasts(), + ETraits.setTrait(MR->getBaseRegion(), RegionAndSymbolInvalidationTraits::TK_PreserveContents); // TODO: Factor this out + handle the lower level const pointers. Index: lib/StaticAnalyzer/Checkers/CStringChecker.cpp =================================================================== --- lib/StaticAnalyzer/Checkers/CStringChecker.cpp +++ lib/StaticAnalyzer/Checkers/CStringChecker.cpp @@ -920,7 +920,7 @@ // Invalidate and escape only indirect regions accessible through the source // buffer. if (IsSourceBuffer) { - ITraits.setTrait(R, + ITraits.setTrait(R->getBaseRegion(), RegionAndSymbolInvalidationTraits::TK_PreserveContents); ITraits.setTrait(R, RegionAndSymbolInvalidationTraits::TK_SuppressEscape); CausesPointerEscape = true;
Index: test/Analysis/call-invalidation.cpp =================================================================== --- test/Analysis/call-invalidation.cpp +++ test/Analysis/call-invalidation.cpp @@ -118,3 +118,47 @@ } +struct PlainStruct { + int x, y; + mutable int z; +}; + +PlainStruct glob; + +void useAnything(void *); +void useAnythingConst(const void *); + +void testInvalidationThroughBaseRegionPointer() { + PlainStruct s1; + s1.x = 1; + s1.z = 1; + clang_analyzer_eval(s1.x == 1); // expected-warning{{TRUE}} + clang_analyzer_eval(s1.z == 1); // expected-warning{{TRUE}} + useAnythingConst(&(s1.y)); + clang_analyzer_eval(s1.x == 1); // expected-warning{{TRUE}} + // FIXME: Should say "UNKNOWN", because it is not uncommon to + // modify a mutable member variable through const pointer. + clang_analyzer_eval(s1.z == 1); // expected-warning{{TRUE}} + useAnything(&(s1.y)); + clang_analyzer_eval(s1.x == 1); // expected-warning{{UNKNOWN}} +} + + +void useFirstConstSecondNonConst(const void *x, void *y); +void useFirstNonConstSecondConst(void *x, const void *y); + +void testMixedConstNonConstCalls() { + PlainStruct s2; + s2.x = 1; + useFirstConstSecondNonConst(&(s2.x), &(s2.y)); + clang_analyzer_eval(s2.x == 1); // expected-warning{{UNKNOWN}} + s2.x = 1; + useFirstNonConstSecondConst(&(s2.x), &(s2.y)); + clang_analyzer_eval(s2.x == 1); // expected-warning{{UNKNOWN}} + s2.y = 1; + useFirstConstSecondNonConst(&(s2.x), &(s2.y)); + clang_analyzer_eval(s2.y == 1); // expected-warning{{UNKNOWN}} + s2.y = 1; + useFirstNonConstSecondConst(&(s2.x), &(s2.y)); + clang_analyzer_eval(s2.y == 1); // expected-warning{{UNKNOWN}} +} Index: lib/StaticAnalyzer/Core/CallEvent.cpp =================================================================== --- lib/StaticAnalyzer/Core/CallEvent.cpp +++ lib/StaticAnalyzer/Core/CallEvent.cpp @@ -177,7 +177,7 @@ // below for efficiency. if (PreserveArgs.count(Idx)) if (const MemRegion *MR = getArgSVal(Idx).getAsRegion()) - ETraits.setTrait(MR->StripCasts(), + ETraits.setTrait(MR->getBaseRegion(), RegionAndSymbolInvalidationTraits::TK_PreserveContents); // TODO: Factor this out + handle the lower level const pointers. Index: lib/StaticAnalyzer/Checkers/CStringChecker.cpp =================================================================== --- lib/StaticAnalyzer/Checkers/CStringChecker.cpp +++ lib/StaticAnalyzer/Checkers/CStringChecker.cpp @@ -920,7 +920,7 @@ // Invalidate and escape only indirect regions accessible through the source // buffer. if (IsSourceBuffer) { - ITraits.setTrait(R, + ITraits.setTrait(R->getBaseRegion(), RegionAndSymbolInvalidationTraits::TK_PreserveContents); ITraits.setTrait(R, RegionAndSymbolInvalidationTraits::TK_SuppressEscape); CausesPointerEscape = true;
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits