[analyzer] Be more forgiving about calling methods on struct rvalues.

The problem is that the value of 'this' in a C++ member function call
should always be a region (or NULL). However, if the object is an rvalue,
it has no associated region (only a conjured symbol or LazyCompoundVal).
For now, we handle this in two ways:

1) Actually respect MaterializeTemporaryExpr. Before, it was relying on
   CXXConstructExpr to create temporary regions for all struct values.
   Now it just does the right thing: if the value is not in a temporary
   region, create one.

2) Have CallEvent recognize the case where its 'this' pointer is a
   non-region, and just return UnknownVal to keep from confusing clients.

The long-term problem is being tracked internally in <rdar://problem/12137950>,
but this makes many test cases pass.

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

diff --git a/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h b/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
index d0d15855459c859629c43d8bf646bb62d7e4b2f4..d9712208f033a9384dd2f635e4cd8c9bd1eacb02 100644 (file)
--- a/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
+++ b/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
@@ -504,13 +504,7 @@ public:
   virtual const Expr *getCXXThisExpr() const { return 0; }
 
   /// \brief Returns the value of the implicit 'this' object.
-  virtual SVal getCXXThisVal() const {
-    const Expr *Base = getCXXThisExpr();
-    // FIXME: This doesn't handle an overloaded ->* operator.
-    if (!Base)
-      return UnknownVal();
-    return getSVal(Base);
-  }
+  virtual SVal getCXXThisVal() const;
 
   virtual const FunctionDecl *getDecl() const;
 

diff --git a/lib/StaticAnalyzer/Core/CallEvent.cpp b/lib/StaticAnalyzer/Core/CallEvent.cpp
index 9a2ec0f9c6b64ecc3cf28660ec1e3131e93949a8..1cfa394a07ced3b0f30e6d32fbbd39460d8998bc 100644 (file)
--- a/lib/StaticAnalyzer/Core/CallEvent.cpp
+++ b/lib/StaticAnalyzer/Core/CallEvent.cpp
@@ -384,6 +384,25 @@ void CXXInstanceCall::getExtraInvalidatedRegions(RegionList &Regions) const {
     Regions.push_back(R);
 }
 
+SVal CXXInstanceCall::getCXXThisVal() const {
+  const Expr *Base = getCXXThisExpr();
+  // FIXME: This doesn't handle an overloaded ->* operator.
+  if (!Base)
+    return UnknownVal();
+
+  SVal ThisVal = getSVal(Base);
+
+  // FIXME: This is only necessary because we can call member functions on
+  // struct rvalues, which do not have regions we can use for a 'this' pointer.
+  // Ideally this should eventually be changed to an assert, i.e. all
+  // non-Unknown, non-null 'this' values should be loc::MemRegionVals.
+  if (isa<DefinedSVal>(ThisVal))
+    if (!ThisVal.getAsRegion() && !ThisVal.isConstant())
+      return UnknownVal();
+
+  return ThisVal;
+}
+
 
 RuntimeDefinition CXXInstanceCall::getRuntimeDefinition() const {
   // Do we have a decl at all?

diff --git a/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp b/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
index 84001d3a0c479fca2c63254650f0ec7a81fef849..d3d2837fe512397e4b20d2b451bcedaf899d303f 100644 (file)
--- a/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ b/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -34,9 +34,10 @@ void ExprEngine::CreateCXXTemporaryObject(const MaterializeTemporaryExpr *ME,
   // the expression to the location of the object.
   SVal V = state->getSVal(tempExpr, LCtx);
 
-  // If the object is a record, the constructor will have already created
-  // a temporary object region. If it is not, we need to copy the value over.
-  if (!ME->getType()->isRecordType()) {
+  // If the value is already a CXXTempObjectRegion, it is fine as it is.
+  // Otherwise, create a new CXXTempObjectRegion, and copy the value into it.
+  const MemRegion *MR = V.getAsRegion();
+  if (!MR || !isa<CXXTempObjectRegion>(MR)) {
     const MemRegion *R =
       svalBuilder.getRegionManager().getCXXTempObjectRegion(ME, LCtx);
 

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
+++ b/test/Analysis/array-struct-region.cpp
@@ -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

diff --git a/test/Analysis/reference.cpp b/test/Analysis/reference.cpp
index 4a2cbb8e25a5e486013e4fa880fca933006cc628..374f3f7261c76f47fe716e209a91c60872931043 100644 (file)
--- a/test/Analysis/reference.cpp
+++ b/test/Analysis/reference.cpp
@@ -116,8 +116,13 @@ void testReferenceAddress(int &x) {
 
   struct S { int &x; };
 
-  extern S *getS();
-  clang_analyzer_eval(&getS()->x != 0); // expected-warning{{TRUE}}
+  // FIXME: Should be TRUE. Fields of return-by-value structs are not yet
+  // symbolicated. Tracked by <rdar://problem/12137950>.
+  extern S getS();
+  clang_analyzer_eval(&getS().x != 0); // expected-warning{{UNKNOWN}}
+
+  extern S *getSP();
+  clang_analyzer_eval(&getSP()->x != 0); // expected-warning{{TRUE}}
 }
 
 
@@ -150,10 +155,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}}
-}