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
