]> granicus.if.org Git - clang/commitdiff
[analyzer] Treat all struct values as regions (even rvalues).
authorJordan Rose <jordan_rose@apple.com>
Sat, 1 Sep 2012 17:39:09 +0000 (17:39 +0000)
committerJordan Rose <jordan_rose@apple.com>
Sat, 1 Sep 2012 17:39:09 +0000 (17:39 +0000)
This allows us to correctly symbolicate the fields of structs returned by
value, as well as get the proper 'this' value for when methods are called
on structs returned by value.

This does require a moderately ugly hack in the StoreManager: if we assign
a "struct value" to a struct region, that now appears as a Loc value being
bound to a region of struct type. We handle this by simply "dereferencing"
the struct value region, which should create a LazyCompoundVal.

This should fix recent crashes analyzing LLVM and on our internal buildbot.

<rdar://problem/12137950>

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

include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h
include/clang/StaticAnalyzer/Core/PathSensitive/SVals.h
lib/StaticAnalyzer/Core/RegionStore.cpp
lib/StaticAnalyzer/Core/SymbolManager.cpp
test/Analysis/array-struct-region.cpp [new file with mode: 0644]
test/Analysis/reference.cpp

index b4a9de76f4d140585157362b7f4b3d34cef05b40..56a932b7099242e4ccd85ee8c94832d7fb40fa70 100644 (file)
@@ -90,6 +90,9 @@ public:
   /// Returns the type of the APSInt used to store values of the given QualType.
   APSIntType getAPSIntType(QualType T) const {
     assert(T->isIntegerType() || Loc::isLocType(T));
+    // Make sure all locations have the same representation in the analyzer.
+    if (Loc::isLocType(T))
+      T = Ctx.VoidPtrTy;
     return APSIntType(Ctx.getTypeSize(T),
                       !T->isSignedIntegerOrEnumerationType());
   }
index e0b5f64b900b54db82dd6a1814bcdf2c34db11ac..52e52e0faa0bfd64ad52d35dbb01cee541a73d8e 100644 (file)
@@ -246,8 +246,16 @@ public:
   }
 
   static inline bool isLocType(QualType T) {
+    // Why are record types included here? Because we want to make sure a
+    // record, even a record rvalue, is always represented with a region.
+    // This is especially necessary in C++, where you can call methods on
+    // struct prvalues, which then need to have a valid 'this' pointer.
+    //
+    // This necessitates a bit of extra hackery in the Store to deal with
+    // the case of binding a "struct value" into a struct region; in
+    // practice it just means "dereferencing" the value before binding.
     return T->isAnyPointerType() || T->isBlockPointerType() || 
-           T->isReferenceType();
+           T->isReferenceType() || T->isRecordType();
   }
 };
 
index b3cf2080006eaef7365650f3c1479c8b602628c3..96342260a0487a90266679659a6afc45cc9e03c5 100644 (file)
@@ -1744,6 +1744,26 @@ StoreRef RegionStoreManager::BindStruct(Store store, const TypedValueRegion* R,
   if (!RD->isCompleteDefinition())
     return StoreRef(store, *this);
 
+  // Handle Loc values by automatically dereferencing the location.
+  // This is necessary because we treat all struct values as regions even if
+  // they are rvalues; we may then be asked to bind one of these
+  // "rvalue regions" to an actual struct region.
+  // (This is necessary for many of the test cases in array-struct-region.cpp.)
+  //
+  // This also handles the case of a struct argument passed by value to an
+  // inlined function. In this case, the C++ standard says that the value
+  // is copy-constructed into the parameter variable. However, the copy-
+  // constructor is processed before we actually know if we're going to inline
+  // the function, and thus we don't actually have the parameter's region
+  // available. Instead, we use a temporary-object region, then copy the
+  // bindings over by value.
+  //
+  // FIXME: This will be a problem when we handle the destructors of
+  // temporaries; the inlined function will modify the parameter region,
+  // but the destructor will act on the temporary region.
+  if (const loc::MemRegionVal *MRV = dyn_cast<loc::MemRegionVal>(&V))
+    V = getBinding(store, *MRV);
+
   // Handle lazy compound values and symbolic values.
   if (isa<nonloc::LazyCompoundVal>(V) || isa<nonloc::SymbolVal>(V))
     return BindAggregate(store, R, V);
index c21df4c31811bab1215cb93b8c9b9abb0a5446a9..65356a9458aa79ebaad13d7ba54afd8edb9fff9f 100644 (file)
@@ -365,9 +365,6 @@ bool SymbolManager::canSymbolicate(QualType T) {
   if (T->isIntegerType())
     return T->isScalarType();
 
-  if (T->isRecordType() && !T->isUnionType())
-    return true;
-
   return false;
 }
 
diff --git a/test/Analysis/array-struct-region.cpp b/test/Analysis/array-struct-region.cpp
new file mode 100644 (file)
index 0000000..3581566
--- /dev/null
@@ -0,0 +1,87 @@
+// RUN: %clang_cc1 -analyze -analyzer-checker=core,alpha.core,debug.ExprInspection -verify -x c %s
+// RUN: %clang_cc1 -analyze -analyzer-checker=core,alpha.core,debug.ExprInspection -verify -x c++ -analyzer-config c++-inlining=constructors %s
+
+void clang_analyzer_eval(int);
+
+struct S {
+  int field;
+
+#if __cplusplus
+  const struct S *getThis() const { return this; }
+#endif
+};
+
+struct S getS();
+
+
+void testAssignment() {
+  struct S s = getS();
+
+  if (s.field != 42) return;
+  clang_analyzer_eval(s.field == 42); // expected-warning{{TRUE}}
+
+  s.field = 0;
+  clang_analyzer_eval(s.field == 0); // expected-warning{{TRUE}}
+
+#if __cplusplus
+  clang_analyzer_eval(s.getThis() == &s); // expected-warning{{TRUE}}
+#endif
+}
+
+
+void testImmediateUse() {
+  int x = getS().field;
+
+  if (x != 42) return;
+  clang_analyzer_eval(x == 42); // expected-warning{{TRUE}}
+
+#if __cplusplus
+  clang_analyzer_eval((void *)getS().getThis() == (void *)&x); // expected-warning{{FALSE}}
+#endif
+}
+
+int getConstrainedField(struct S s) {
+  if (s.field != 42) return 42;
+  return s.field;
+}
+
+int getAssignedField(struct S s) {
+  s.field = 42;
+  return s.field;
+}
+
+void testArgument() {
+  clang_analyzer_eval(getConstrainedField(getS()) == 42); // expected-warning{{TRUE}}
+  clang_analyzer_eval(getAssignedField(getS()) == 42); // expected-warning{{TRUE}}
+}
+
+
+//--------------------
+// C++-only tests
+//--------------------
+
+#if __cplusplus
+void testReferenceAssignment() {
+  const S &s = getS();
+
+  if (s.field != 42) return;
+  clang_analyzer_eval(s.field == 42); // expected-warning{{TRUE}}
+
+  clang_analyzer_eval(s.getThis() == &s); // expected-warning{{TRUE}}
+}
+
+
+int getConstrainedFieldRef(const S &s) {
+  if (s.field != 42) return 42;
+  return s.field;
+}
+
+bool checkThis(const S &s) {
+  return s.getThis() == &s;
+}
+
+void testReferenceArgument() {
+  clang_analyzer_eval(getConstrainedFieldRef(getS()) == 42); // expected-warning{{TRUE}}
+  clang_analyzer_eval(checkThis(getS())); // expected-warning{{TRUE}}
+}
+#endif
index 4a2cbb8e25a5e486013e4fa880fca933006cc628..ce0ee8ed57d07139335319c299e0f927e8cec154 100644 (file)
@@ -116,8 +116,11 @@ void testReferenceAddress(int &x) {
 
   struct S { int &x; };
 
-  extern S *getS();
-  clang_analyzer_eval(&getS()->x != 0); // expected-warning{{TRUE}}
+  extern S getS();
+  clang_analyzer_eval(&getS().x != 0); // expected-warning{{TRUE}}
+
+  extern S *getSP();
+  clang_analyzer_eval(&getSP()->x != 0); // expected-warning{{TRUE}}
 }
 
 
@@ -150,10 +153,3 @@ namespace rdar11212286 {
     return *x; // should warn here!
   }
 }
-
-void testReferenceFieldAddress() {
-  struct S { int &x; };
-
-  extern S getS();
-  clang_analyzer_eval(&getS().x != 0); // expected-warning{{UNKNOWN}}
-}