Fix analyzer regression reported in PR 4164:

- Update the old StoreManager::CastRegion to strip off 'ElementRegions' when
  casting to void* (Zhongxing: please validate)
- Pass-by-reference argument invalidation logic in CFRefCount.cpp:
  - Strip ElementRegions when the ElementRegion is just a 'raw data' view
    on top of the underlying typed region.

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@71094 91177308-0d34-0410-b5e6-96231b3b80d8

diff --git a/lib/Analysis/CFRefCount.cpp b/lib/Analysis/CFRefCount.cpp
index ca420006d2439e0a31c5b33bb701280da27f5ee5..4c517fd537ceb49ded43ca572875e35eaa07955f 100644 (file)
--- a/lib/Analysis/CFRefCount.cpp
+++ b/lib/Analysis/CFRefCount.cpp
@@ -2606,7 +2606,29 @@ void CFRefCount::EvalSummary(ExplodedNodeSet<GRState>& Dst,
         
         const TypedRegion* R = dyn_cast<TypedRegion>(MR->getRegion());
 
-        if (R) {          
+        if (R) {
+          // Are we dealing with an ElementRegion?  If the element type is
+          // a basic integer type (e.g., char, int) and the underying region
+          // is also typed then strip off the ElementRegion.
+          // FIXME: We really need to think about this for the general case
+          //   as sometimes we are reasoning about arrays and other times
+          //   about (char*), etc., is just a form of passing raw bytes.
+          //   e.g., void *p = alloca(); foo((char*)p);
+          if (const ElementRegion *ER = dyn_cast<ElementRegion>(R)) {
+            // Checking for 'integral type' is probably too promiscuous, but
+            // we'll leave it in for now until we have a systematic way of
+            // handling all of these cases.  Eventually we need to come up
+            // with an interface to StoreManager so that this logic can be
+            // approriately delegated to the respective StoreManagers while
+            // still allowing us to do checker-specific logic (e.g.,
+            // invalidating reference counts), probably via callbacks.            
+            if (ER->getElementType()->isIntegralType())
+              if (const TypedRegion *superReg =
+                  dyn_cast<TypedRegion>(ER->getSuperRegion()))
+                R = superReg;
+            // FIXME: What about layers of ElementRegions?
+          }
+          
           // Is the invalidated variable something that we were tracking?
           SymbolRef Sym = state.GetSValAsScalarOrLoc(R).getAsLocSymbol();
           

diff --git a/lib/Analysis/Store.cpp b/lib/Analysis/Store.cpp
index 32b186b269c7bcef818081f13f66b7246e7523fd..49411b468e59158ae4f1d3ff5ca7b03c02062a76 100644 (file)
--- a/lib/Analysis/Store.cpp
+++ b/lib/Analysis/Store.cpp
@@ -44,18 +44,37 @@ StoreManager::CastRegion(const GRState* state, const MemRegion* R,
     QualType Pointee = PTy->getPointeeType();
     if (Pointee->isVoidType()) {
 
-      // Casts to void* only removes TypedViewRegion. If there is no
-      // TypedViewRegion, leave the region untouched. This happens when:
-      //
-      // void foo(void*);
-      // ...
-      // void bar() {
-      //   int x;
-      //   foo(&x);
-      // }
-
-      if (const TypedViewRegion *TR = dyn_cast<TypedViewRegion>(R))
-        R = TR->removeViews();
+      do {
+        if (const TypedViewRegion *TR = dyn_cast<TypedViewRegion>(R)) {
+          // Casts to void* removes TypedViewRegion. This happens when:
+          //
+          // void foo(void*);
+          // ...
+          // void bar() {
+          //   int x;
+          //   foo(&x);
+          // }
+          //
+          R = TR->removeViews();
+          continue;
+        }
+        else if (const ElementRegion *ER = dyn_cast<ElementRegion>(R)) {
+          // Casts to void* also removes ElementRegions. This happens when:
+          //
+          // void foo(void*);
+          // ...
+          // void bar() {
+          //   int x;
+          //   foo((char*)&x);
+          // }                
+          //
+          R = ER->getSuperRegion();
+          continue;
+        }
+        else
+          break;
+      }
+      while (0);
       
       return CastResult(state, R);
     }

diff --git a/test/Analysis/pr_4164.c b/test/Analysis/pr_4164.c
new file mode 100644 (file)
index 0000000..cc2479c
--- /dev/null
+++ b/test/Analysis/pr_4164.c
@@ -0,0 +1,41 @@
+// RUN: clang-cc -triple x86_64-apple-darwin9 -analyze -checker-cfref -analyzer-store=basic -verify %s &&
+// RUN: clang-cc -triple x86_64-apple-darwin9 -analyze -checker-cfref -analyzer-store=region -verify %s
+
+// PR 4164: http://llvm.org/bugs/show_bug.cgi?id=4164
+//
+// Eventually this should be pulled into misc-ps.m.  This is in a separate test
+// file for now to play around with the specific issues for BasicStoreManager
+// and StoreManager (i.e., we can make a copy of this file for either
+// StoreManager should one start to fail in the near future).
+//
+// The basic issue is that the VarRegion for 'size' is casted to (char*),
+// resulting in an ElementRegion.  'getsockopt' is an unknown function that
+// takes a void*, which means the ElementRegion should get stripped off.
+typedef unsigned int __uint32_t;
+typedef __uint32_t __darwin_socklen_t;
+typedef __darwin_socklen_t socklen_t;
+int getsockopt(int, int, int, void * restrict, socklen_t * restrict);
+
+int test1() {
+  int s = -1;
+  int size;
+  socklen_t size_len = sizeof(size);
+  if (getsockopt(s, 0xffff, 0x1001, (char *)&size, &size_len) < 0)
+          return -1;
+
+  return size; // no-warning
+}
+
+// Similar case: instead of passing a 'void*', we pass 'char*'.  In this
+// case we pass an ElementRegion to the invalidation logic.  Since it is
+// an ElementRegion that just layers on top of another typed region and the
+// ElementRegion itself has elements whose type are integral (essentially raw
+// data) we strip off the ElementRegion when doing the invalidation.
+int takes_charptr(char* p);
+int test2() {
+  int size;
+  if (takes_charptr((char*)&size))
+    return -1;
+  return size; // no-warning
+}
+