From b11a3ada9a22e146c6edd33bcc6301e221fedd7a Mon Sep 17 00:00:00 2001 From: Jordan Rose Date: Mon, 13 Aug 2012 22:11:34 +0000 Subject: [PATCH] [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 --- .../Core/PathSensitive/MemRegion.h | 2 +- .../StaticAnalyzer/Core/PathSensitive/SVals.h | 2 +- lib/StaticAnalyzer/Core/CallEvent.cpp | 1 - lib/StaticAnalyzer/Core/MemRegion.cpp | 35 +++++++++- lib/StaticAnalyzer/Core/RegionStore.cpp | 34 +++++----- lib/StaticAnalyzer/Core/SVals.cpp | 4 +- test/Analysis/derived-to-base.cpp | 65 ++++++++++++++++++- test/Analysis/dynamic-cast.cpp | 22 ++++++- 8 files changed, 140 insertions(+), 25 deletions(-) diff --git a/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h b/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h index 1281cfbda9..01218effff 100644 --- a/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h +++ b/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; diff --git a/include/clang/StaticAnalyzer/Core/PathSensitive/SVals.h b/include/clang/StaticAnalyzer/Core/PathSensitive/SVals.h index e462657728..e0b5f64b90 100644 --- a/include/clang/StaticAnalyzer/Core/PathSensitive/SVals.h +++ b/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 const REGION* getRegionAs() const { diff --git a/lib/StaticAnalyzer/Core/CallEvent.cpp b/lib/StaticAnalyzer/Core/CallEvent.cpp index 619f9b200b..6a5faa054c 100644 --- a/lib/StaticAnalyzer/Core/CallEvent.cpp +++ b/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" diff --git a/lib/StaticAnalyzer/Core/MemRegion.cpp b/lib/StaticAnalyzer/Core/MemRegion.cpp index 19548be5e1..62e602a7e1 100644 --- a/lib/StaticAnalyzer/Core/MemRegion.cpp +++ b/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(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(superRegion)) { + superRegion = Base->getSuperRegion(); + } + assert(superRegion && !isa(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(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(R)->getSuperRegion(); break; default: diff --git a/lib/StaticAnalyzer/Core/RegionStore.cpp b/lib/StaticAnalyzer/Core/RegionStore.cpp index f0eac9dc2b..05d1bd0dcf 100644 --- a/lib/StaticAnalyzer/Core/RegionStore.cpp +++ b/lib/StaticAnalyzer/Core/RegionStore.cpp @@ -934,7 +934,7 @@ SVal RegionStoreManager::evalDynamicCast(SVal base, QualType derivedType, loc::MemRegionVal *baseRegVal = dyn_cast(&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(TSR)) diff --git a/lib/StaticAnalyzer/Core/SVals.cpp b/lib/StaticAnalyzer/Core/SVals.cpp index e32ffe2b48..8437f50f91 100644 --- a/lib/StaticAnalyzer/Core/SVals.cpp +++ b/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 { diff --git a/test/Analysis/derived-to-base.cpp b/test/Analysis/derived-to-base.cpp index f6c9beb465..b065bc25a3 100644 --- a/test/Analysis/derived-to-base.cpp +++ b/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}} + } +} diff --git a/test/Analysis/dynamic-cast.cpp b/test/Analysis/dynamic-cast.cpp index 215bc49742..b1133ac2be 100644 --- a/test/Analysis/dynamic-cast.cpp +++ b/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(&ref); + clang_analyzer_eval(ptr != 0); // expected-warning{{TRUE}} + + // This is actually statically resolved to be a DerivedToBase cast. + ptr = dynamic_cast(&obj); + clang_analyzer_eval(ptr != 0); // expected-warning{{TRUE}} +} + + +// ----------------------------- // False positives/negatives. +// ----------------------------- // Due to symbolic regions not being typed. int testDynCastFalsePositive(BB *c) { -- 2.40.0