[cfe-commits] r84163 - in /cfe/trunk: lib/Analysis/RegionStore.cpp test/Analysis/NSString.m

Ted Kremenek kremenek at apple.com
Wed Oct 14 18:40:35 PDT 2009


Author: kremenek
Date: Wed Oct 14 20:40:34 2009
New Revision: 84163

URL: http://llvm.org/viewvc/llvm-project?rev=84163&view=rev
Log:
Per an astute observation from Zhongxing Xu, remove a "special case" logic in
RegionStoreManager::Retrieve() that was intended to handle conflated uses of pointers as integers.
It turns out this isn't needed, and resulted in inconsistent behavior when creating symbolic values on the following test case in 'tests/Analysis/misc-ps.m':

  typedef struct _BStruct { void *grue; } BStruct;
  void testB_aux(void *ptr);
  void testB(BStruct *b) {
    {
      int *__gruep__ = ((int *)&((b)->grue));
      int __gruev__ = *__gruep__;
      testB_aux(__gruep__);
    }
    {
      int *__gruep__ = ((int *)&((b)->grue));
      int __gruev__ = *__gruep__;
      if (~0 != __gruev__) {}
    }
  }

When the code was analyzed with '-arch x86_64', the value assigned to '__gruev__' be would be a
symbolic integer, but for '-arch i386' the value assigned to '__gruev__' would be a symbolic region
(a blob of memory). With this change the value created is always a symbolic integer.

Since the code being removed was added to support analysis of code calling
OSAtomicCompareAndSwapXXX(), I also modified 'test/Analysis/NSString.m' to analyze the code in both
'-arch i386' and '-arch x86_64', and also added some complementary test cases to test the presence
of leaks when using OSAtomicCompareAndSwap32Barrier()/OSAtomicCompareAndSwap64Barrier() instead of
just their absence. This code change reveals that previously both RegionStore and BasicStore were
handling these cases wrong, and would never cause the analyzer to emit a leak in these cases (false
negatives). Now RegionStore gets it right, but BasicStore still gets it wrong (and hence it has been
disabled temporarily for this test case).

Modified:
    cfe/trunk/lib/Analysis/RegionStore.cpp
    cfe/trunk/test/Analysis/NSString.m

Modified: cfe/trunk/lib/Analysis/RegionStore.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Analysis/RegionStore.cpp?rev=84163&r1=84162&r2=84163&view=diff

==============================================================================
--- cfe/trunk/lib/Analysis/RegionStore.cpp (original)
+++ cfe/trunk/lib/Analysis/RegionStore.cpp Wed Oct 14 20:40:34 2009
@@ -1115,28 +1115,6 @@
     }
   }
 
-  // Special case: the current region represents a cast and it and the super
-  // region both have pointer types or intptr_t types.  If so, perform the
-  // retrieve from the super region and appropriately "cast" the value.
-  // This is needed to support OSAtomicCompareAndSwap and friends or other
-  // loads that treat integers as pointers and vis versa.
-  if (R->getIndex().isZeroConstant()) {
-    if (const TypedRegion *superTR = dyn_cast<TypedRegion>(superR)) {
-      ASTContext &Ctx = getContext();
-      if (IsAnyPointerOrIntptr(superTR->getValueType(Ctx), Ctx)) {
-        QualType valTy = R->getValueType(Ctx);
-        if (IsAnyPointerOrIntptr(valTy, Ctx)) {
-          // Retrieve the value from the super region.  This will be casted to
-          // valTy when we return to 'Retrieve'.
-          const SValuator::CastResult &cr = Retrieve(state,
-                                                     loc::MemRegionVal(superR),
-                                                     valTy);
-          return cr.getSVal();
-        }
-      }
-    }
-  }
-
   // Check if the immediate super region has a direct binding.
   if (Optional<SVal> V = getDirectBinding(B, superR)) {
     if (SymbolRef parentSym = V->getAsSymbol())

Modified: cfe/trunk/test/Analysis/NSString.m
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/NSString.m?rev=84163&r1=84162&r2=84163&view=diff

==============================================================================
--- cfe/trunk/test/Analysis/NSString.m (original)
+++ cfe/trunk/test/Analysis/NSString.m Wed Oct 14 20:40:34 2009
@@ -1,7 +1,13 @@
-// RUN: clang-cc -triple i386-pc-linux-gnu -analyze -checker-cfref -analyzer-store=basic -analyzer-constraints=basic -verify %s &&
-// RUN: clang-cc -triple i386-pc-linux-gnu -analyze -checker-cfref -analyzer-store=basic -analyzer-constraints=range -verify %s &&
-// RUN: clang-cc -triple i386-pc-linux-gnu -analyze -checker-cfref -analyzer-store=region -analyzer-constraints=basic -verify %s &&
-// RUN: clang-cc -triple i386-pc-linux-gnu -analyze -checker-cfref -analyzer-store=region -analyzer-constraints=range -verify %s
+// RUN: clang-cc -triple i386-apple-darwin10 -analyze -checker-cfref -analyzer-store=region -analyzer-constraints=basic -verify %s &&
+// RUN: clang-cc -triple i386-apple-darwin10 -analyze -checker-cfref -analyzer-store=region -analyzer-constraints=range -verify %s &&
+// RUN: clang-cc -DTEST_64 -triple x86_64-apple-darwin10 -analyze -checker-cfref -analyzer-store=region -analyzer-constraints=basic -verify %s &&
+// RUN: clang-cc -DTEST_64 -triple x86_64-apple-darwin10 -analyze -checker-cfref -analyzer-store=region -analyzer-constraints=range -verify %s
+
+// ==-- FIXME: -analyzer-store=basic fails on this file (false negatives). --==
+// NOTWORK: clang-cc -triple i386-apple-darwin10 -analyze -checker-cfref -analyzer-store=basic -analyzer-constraints=range -verify %s &&
+// NOTWORK: clang-cc -triple i386-apple-darwin10 -analyze -checker-cfref -analyzer-store=basic -analyzer-constraints=basic -verify %s &&
+// NOTWORK: clang-cc -DTEST_64 -triple x86_64-apple-darwin10 -analyze -checker-cfref -analyzer-store=basic -analyzer-constraints=basic -verify %s &&
+// NOTWORK: clang-cc -DTEST_64 -triple x86_64-apple-darwin10 -analyze -checker-cfref -analyzer-store=basic -analyzer-constraints=range -verify %s
 
 //===----------------------------------------------------------------------===//
 // The following code is reduced using delta-debugging from
@@ -12,7 +18,18 @@
 // both svelte and portable to non-Mac platforms.
 //===----------------------------------------------------------------------===//
 
+#ifdef TEST_64
+typedef long long int64_t;
+_Bool OSAtomicCompareAndSwap64Barrier( int64_t __oldValue, int64_t __newValue, volatile int64_t *__theValue );
+#define COMPARE_SWAP_BARRIER OSAtomicCompareAndSwap64Barrier
+typedef int64_t intptr_t;
+#else
 typedef int int32_t;
+_Bool OSAtomicCompareAndSwap32Barrier( int32_t __oldValue, int32_t __newValue, volatile int32_t *__theValue );
+#define COMPARE_SWAP_BARRIER OSAtomicCompareAndSwap32Barrier
+typedef int32_t intptr_t;
+#endif
+
 typedef const void * CFTypeRef;
 typedef const struct __CFString * CFStringRef;
 typedef const struct __CFAllocator * CFAllocatorRef;
@@ -263,7 +280,6 @@
 
 // Test OSCompareAndSwap
 _Bool OSAtomicCompareAndSwapPtr( void *__oldValue, void *__newValue, void * volatile *__theValue );
-_Bool OSAtomicCompareAndSwap32Barrier( int32_t __oldValue, int32_t __newValue, volatile int32_t *__theValue );
 extern BOOL objc_atomicCompareAndSwapPtr(id predicate, id replacement, volatile id *objectLocation);
 
 void testOSCompareAndSwap() {
@@ -275,20 +291,29 @@
     [old release];
 }
 
-void testOSCompareAndSwap32Barrier() {
+void testOSCompareAndSwapXXBarrier() {
   NSString *old = 0;
   NSString *s = [[NSString alloc] init]; // no-warning
-  if (!OSAtomicCompareAndSwap32Barrier((int32_t) 0, (int32_t) s, (int32_t*) &old))
+  if (!COMPARE_SWAP_BARRIER((intptr_t) 0, (intptr_t) s, (intptr_t*) &old))
     [s release];
   else    
     [old release];
 }
 
-int testOSCompareAndSwap32Barrier_id(Class myclass, id xclass) {
-  if (OSAtomicCompareAndSwap32Barrier(0, (int32_t) myclass, (int32_t*) &xclass))
+void testOSCompareAndSwapXXBarrier_positive() {
+  NSString *old = 0;
+  NSString *s = [[NSString alloc] init]; // expected-warning{{leak}}
+  if (!COMPARE_SWAP_BARRIER((intptr_t) 0, (intptr_t) s, (intptr_t*) &old))
+    return;
+  else    
+    [old release];
+}
+
+int testOSCompareAndSwapXXBarrier_id(Class myclass, id xclass) {
+  if (COMPARE_SWAP_BARRIER(0, (intptr_t) myclass, (intptr_t*) &xclass))
     return 1;
   return 0;
-}  
+}
 
 void test_objc_atomicCompareAndSwap() {
   NSString *old = 0;
@@ -299,6 +324,16 @@
     [old release];
 }
 
+void test_objc_atomicCompareAndSwap_positive() {
+  NSString *old = 0;
+  NSString *s = [[NSString alloc] init]; // expected-warning{{leak}}
+  if (!objc_atomicCompareAndSwapPtr(0, s, &old))
+    return;
+  else    
+    [old release];
+}
+
+
 // Test stringWithFormat (<rdar://problem/6815234>)
 void test_stringWithFormat() {  
   NSString *string = [[NSString stringWithFormat:@"%ld", (long) 100] retain];





More information about the cfe-commits mailing list