[analyzer] Don't strip CXXBaseObjectRegions when checking dynamic_casts.

...and /do/ strip CXXBaseObjectRegions when casting to a virtual base class.

This allows us to enforce the invariant that a CXXBaseObjectRegion can always
provide an offset for its base region if its base region has a known class
type, by only allowing virtual bases and direct non-virtual bases to form
CXXBaseObjectRegions.

This does mean some slight problems for our modeling of dynamic_cast, which
needs to be resolved by finding a path from the current region to the class
we're trying to cast to.

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

include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h

@@ -138,7 +138,7 @@ public:
 
   const MemRegion *getBaseRegion() const;
 
-  const MemRegion *StripCasts() const;
+  const MemRegion *StripCasts(bool StripBaseCasts = true) const;
 
   bool hasGlobalsOrParametersStorage() const;
 

include/clang/StaticAnalyzer/Core/PathSensitive/SVals.h

@@ -433,7 +433,7 @@ public:
   }
 
   /// \brief Get the underlining region and strip casts.
-  const MemRegion* stripCasts() const;
+  const MemRegion* stripCasts(bool StripBaseCasts = true) const;
 
   template <typename REGION>
   const REGION* getRegionAs() const {

lib/StaticAnalyzer/Core/CallEvent.cpp

@@ -15,7 +15,6 @@
 
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
 #include "clang/Analysis/ProgramPoint.h"
-#include "clang/AST/CXXInheritance.h"
 #include "clang/AST/ParentMap.h"
 #include "llvm/ADT/SmallSet.h"
 #include "llvm/ADT/StringExtras.h"

lib/StaticAnalyzer/Core/MemRegion.cpp

@@ -888,6 +888,37 @@ MemRegionManager::getCXXTempObjectRegion(Expr const *E,
 const CXXBaseObjectRegion *
 MemRegionManager::getCXXBaseObjectRegion(const CXXRecordDecl *decl,
                                          const MemRegion *superRegion) {
+  // Check that the base class is actually a direct base of this region.
+  if (const TypedValueRegion *TVR = dyn_cast<TypedValueRegion>(superRegion)) {
+    if (const CXXRecordDecl *Class = TVR->getValueType()->getAsCXXRecordDecl()){
+      if (Class->isVirtuallyDerivedFrom(decl)) {
+        // Virtual base regions should not be layered, since the layout rules
+        // are different.
+        while (const CXXBaseObjectRegion *Base =
+                 dyn_cast<CXXBaseObjectRegion>(superRegion)) {
+          superRegion = Base->getSuperRegion();
+        }
+        assert(superRegion && !isa<MemSpaceRegion>(superRegion));
+
+      } else {
+        // Non-virtual bases should always be direct bases.
+#ifndef NDEBUG
+        bool FoundBase = false;
+        for (CXXRecordDecl::base_class_const_iterator I = Class->bases_begin(),
+                                                      E = Class->bases_end();
+             I != E; ++I) {
+          if (I->getType()->getAsCXXRecordDecl() == decl) {
+            FoundBase = true;
+            break;
+          }
+        }
+
+        assert(FoundBase && "Not a direct base class of this region");
+#endif
+      }
+    }
+  }
+
   return getSubRegion<CXXBaseObjectRegion>(decl, superRegion);
 }
 
@@ -963,7 +994,7 @@ const MemRegion *MemRegion::getBaseRegion() const {
 // View handling.
 //===----------------------------------------------------------------------===//
 
-const MemRegion *MemRegion::StripCasts() const {
+const MemRegion *MemRegion::StripCasts(bool StripBaseCasts) const {
   const MemRegion *R = this;
   while (true) {
     switch (R->getKind()) {
@@ -975,6 +1006,8 @@ const MemRegion *MemRegion::StripCasts() const {
       break;
     }
     case CXXBaseObjectRegionKind:
+      if (!StripBaseCasts)
+        return R;
       R = cast<CXXBaseObjectRegion>(R)->getSuperRegion();
       break;
     default:

lib/StaticAnalyzer/Core/RegionStore.cpp

@@ -934,7 +934,7 @@ SVal RegionStoreManager::evalDynamicCast(SVal base, QualType derivedType,
   loc::MemRegionVal *baseRegVal = dyn_cast<loc::MemRegionVal>(&base);
   if (!baseRegVal)
     return UnknownVal();
-  const MemRegion *BaseRegion = baseRegVal->stripCasts();
+  const MemRegion *BaseRegion = baseRegVal->stripCasts(/*StripBases=*/false);
 
   // Assume the derived class is a pointer or a reference to a CXX record.
   derivedType = derivedType->getPointeeType();
@@ -957,23 +957,23 @@ SVal RegionStoreManager::evalDynamicCast(SVal base, QualType derivedType,
     if (SRDecl == DerivedDecl)
       return loc::MemRegionVal(TSR);
 
-    // If the region type is a subclass of the derived type.
-    if (!derivedType->isVoidType() && SRDecl->isDerivedFrom(DerivedDecl)) {
-      // This occurs in two cases.
-      // 1) We are processing an upcast.
-      // 2) We are processing a downcast but we jumped directly from the
-      // ancestor to a child of the cast value, so conjure the
-      // appropriate region to represent value (the intermediate node).
-      return loc::MemRegionVal(MRMgr.getCXXBaseObjectRegion(DerivedDecl,
-                                                            BaseRegion));
-    }
+    if (!derivedType->isVoidType()) {
+      // Static upcasts are marked as DerivedToBase casts by Sema, so this will
+      // only happen when multiple or virtual inheritance is involved.
+      // FIXME: We should build the correct stack of CXXBaseObjectRegions here,
+      // instead of just punting.
+      if (SRDecl->isDerivedFrom(DerivedDecl))
+        return UnknownVal();
 
-    // If super region is not a parent of derived class, the cast definitely
-    // fails.
-    if (!derivedType->isVoidType() &&
-        DerivedDecl->isProvablyNotDerivedFrom(SRDecl)) {
-      Failed = true;
-      return UnknownVal();
+      // If super region is not a parent of derived class, the cast definitely
+      // fails.
+      // FIXME: This and the above test each require walking the entire
+      // inheritance hierarchy, and this will happen for each
+      // CXXBaseObjectRegion wrapper. We should probably be combining the two.
+      if (DerivedDecl->isProvablyNotDerivedFrom(SRDecl)) {
+        Failed = true;
+        return UnknownVal();
+      }
     }
 
     if (const CXXBaseObjectRegion *R = dyn_cast<CXXBaseObjectRegion>(TSR))

lib/StaticAnalyzer/Core/SVals.cpp

@@ -133,9 +133,9 @@ const MemRegion *SVal::getAsRegion() const {
   return 0;
 }
 
-const MemRegion *loc::MemRegionVal::stripCasts() const {
+const MemRegion *loc::MemRegionVal::stripCasts(bool StripBaseCasts) const {
   const MemRegion *R = getRegion();
-  return R ?  R->StripCasts() : NULL;
+  return R ?  R->StripCasts(StripBaseCasts) : NULL;
 }
 
 const void *nonloc::LazyCompoundVal::getStore() const {

test/Analysis/derived-to-base.cpp

@@ -1,4 +1,6 @@
-// RUN: %clang_cc1 -analyze -analyzer-checker=core -analyzer-store region %s
+// RUN: %clang_cc1 -analyze -analyzer-checker=core,debug.ExprInspection -analyzer-store=region -verify %s
+
+void clang_analyzer_eval(bool);
 
 class A {
 protected:
@@ -22,3 +24,64 @@ public:
     x = 5;
   }
 };
+
+
+namespace VirtualBaseClasses {
+  class A {
+  protected:
+    int x;
+  };
+
+  class B : public virtual A {
+  public:
+    int getX() { return x; }
+  };
+
+  class C : public virtual A {
+  public:
+    void setX() { x = 42; }
+  };
+
+  class D : public B, public C {};
+  class DV : virtual public B, public C {};
+  class DV2 : public B, virtual public C {};
+
+  void test() {
+    D d;
+    d.setX();
+    clang_analyzer_eval(d.getX() == 42); // expected-warning{{TRUE}}
+
+    DV dv;
+    dv.setX();
+    clang_analyzer_eval(dv.getX() == 42); // expected-warning{{TRUE}}
+
+    DV2 dv2;
+    dv2.setX();
+    clang_analyzer_eval(dv2.getX() == 42); // expected-warning{{TRUE}}
+  }
+
+
+  // Make sure we're consistent about the offset of the A subobject within an
+  // Intermediate virtual base class.
+  class Padding1 { int unused; };
+  class Padding2 { int unused; };
+  class Intermediate : public Padding1, public A, public Padding2 {};
+
+  class BI : public virtual Intermediate {
+  public:
+    int getX() { return x; }
+  };
+
+  class CI : public virtual Intermediate {
+  public:
+    void setX() { x = 42; }
+  };
+
+  class DI : public BI, public CI {};
+
+  void testIntermediate() {
+    DI d;
+    d.setX();
+    clang_analyzer_eval(d.getX() == 42); // expected-warning{{TRUE}}
+  }
+}

test/Analysis/dynamic-cast.cpp

@@ -1,4 +1,6 @@
-// RUN: %clang_cc1 -triple i386-apple-darwin10 -analyze -analyzer-checker=core -analyzer-ipa=none -verify %s
+// RUN: %clang_cc1 -triple i386-apple-darwin10 -analyze -analyzer-checker=core,debug.ExprInspection -analyzer-ipa=none -verify %s
+
+void clang_analyzer_eval(bool);
 
 class A {
 public:
@@ -208,7 +210,25 @@ void callTestDynCastMostLikelyWillFail() {
   testDynCastMostLikelyWillFail(&m);
 }
 
+
+void testDynCastToMiddleClass () {
+  class BBB : public BB {};
+  BBB obj;
+  A &ref = obj;
+
+  // These didn't always correctly layer base regions.
+  B *ptr = dynamic_cast<B*>(&ref);
+  clang_analyzer_eval(ptr != 0); // expected-warning{{TRUE}}
+
+  // This is actually statically resolved to be a DerivedToBase cast.
+  ptr = dynamic_cast<B*>(&obj);
+  clang_analyzer_eval(ptr != 0); // expected-warning{{TRUE}}
+}
+
+
+// -----------------------------
 // False positives/negatives.
+// -----------------------------
 
 // Due to symbolic regions not being typed.
 int testDynCastFalsePositive(BB *c) {