[PATCH] D19057: [analyzer] Let TK_PreserveContents span across the whole base region.

[Artem Dergachev via cfe-commits]

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;


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D19057.53551.patch
Type: text/x-patch
Size: 2898 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160413/b665a08e/attachment.bin>