From d1c7cf26ae622ed911ee6d746313dfbedcdfd012 Mon Sep 17 00:00:00 2001 From: Jordan Rose Date: Thu, 21 Feb 2013 01:34:51 +0000 Subject: [PATCH] [analyzer] Tighten up safety in the use of lazy bindings. - When deciding if we can reuse a lazy binding, make sure to check if there are additional bindings in the sub-region. - When reading from a lazy binding, don't accidentally strip off casts or base object regions. This slows down lazy binding reading a bit but is necessary for type sanity when treating one class as another. A bit of minor refactoring allowed these two checks to be unified in a nice early-return-using helper function. llvm-svn: 175703 --- clang/lib/StaticAnalyzer/Core/RegionStore.cpp | 150 ++++++++++++++------------ clang/test/Analysis/array-struct-region.c | 7 ++ clang/test/Analysis/derived-to-base.cpp | 29 +++++ 3 files changed, 115 insertions(+), 71 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Core/RegionStore.cpp b/clang/lib/StaticAnalyzer/Core/RegionStore.cpp index 799022e..3bd624b 100644 --- a/clang/lib/StaticAnalyzer/Core/RegionStore.cpp +++ b/clang/lib/StaticAnalyzer/Core/RegionStore.cpp @@ -477,7 +477,7 @@ public: // Part of public interface to class. QualType Ty, const MemRegion *superR); - SVal getLazyBinding(const MemRegion *LazyBindingRegion, + SVal getLazyBinding(const SubRegion *LazyBindingRegion, RegionBindingsRef LazyBinding); /// Get bindings for the values in a struct and return a CompoundVal, used @@ -500,9 +500,9 @@ public: // Part of public interface to class. /// /// If there is no lazy binding for \p R, the returned value will have a null /// \c second. Note that a null pointer can represents a valid Store. - std::pair - getLazyBinding(RegionBindingsConstRef B, const MemRegion *R, - const MemRegion *originalRegion); + std::pair + findLazyBinding(RegionBindingsConstRef B, const SubRegion *R, + const SubRegion *originalRegion); /// Returns the cached set of interesting SVals contained within a lazy /// binding. @@ -1255,72 +1255,88 @@ SVal RegionStoreManager::getBinding(RegionBindingsConstRef B, Loc L, QualType T) return svalBuilder.getRegionValueSymbolVal(R); } -std::pair -RegionStoreManager::getLazyBinding(RegionBindingsConstRef B, - const MemRegion *R, - const MemRegion *originalRegion) { - typedef std::pair StoreRegionPair; +/// Checks to see if store \p B has a lazy binding for region \p R. +/// +/// If \p AllowSubregionBindings is \c false, a lazy binding will be rejected +/// if there are additional bindings within \p R. +/// +/// Note that unlike RegionStoreManager::findLazyBinding, this will not search +/// for lazy bindings for super-regions of \p R. +static Optional +getExistingLazyBinding(SValBuilder &SVB, RegionBindingsConstRef B, + const SubRegion *R, bool AllowSubregionBindings) { + Optional V = B.getDefaultBinding(R); + if (!V) + return Optional(); + + Optional LCV = V->getAs(); + if (!LCV) + return Optional(); + + // If the LCV is for a subregion, the types won't match, and we shouldn't + // reuse the binding. Unfortuately we can only check this if the destination + // region is a TypedValueRegion. + if (const TypedValueRegion *TVR = dyn_cast(R)) { + QualType RegionTy = TVR->getValueType(); + QualType SourceRegionTy = LCV->getRegion()->getValueType(); + if (!SVB.getContext().hasSameUnqualifiedType(RegionTy, SourceRegionTy)) + return Optional(); + } + + if (!AllowSubregionBindings) { + // If there are any other bindings within this region, we shouldn't reuse + // the top-level binding. + SmallVector Keys; + collectSubRegionKeys(Keys, SVB, *B.lookup(R->getBaseRegion()), R, + /*IncludeAllDefaultBindings=*/true); + if (Keys.size() > 1) + return Optional(); + } + + return *LCV; +} + +std::pair +RegionStoreManager::findLazyBinding(RegionBindingsConstRef B, + const SubRegion *R, + const SubRegion *originalRegion) { if (originalRegion != R) { - if (Optional OV = B.getDefaultBinding(R)) { - if (Optional V = - OV->getAs()) - return std::make_pair(V->getStore(), V->getRegion()); - } + if (Optional V = + getExistingLazyBinding(svalBuilder, B, R, true)) + return std::make_pair(V->getStore(), V->getRegion()); } - + + typedef std::pair StoreRegionPair; + StoreRegionPair Result = StoreRegionPair(); + if (const ElementRegion *ER = dyn_cast(R)) { - StoreRegionPair X = getLazyBinding(B, ER->getSuperRegion(), originalRegion); + Result = findLazyBinding(B, cast(ER->getSuperRegion()), + originalRegion); - if (X.second) - return std::make_pair(X.first, - MRMgr.getElementRegionWithSuper(ER, X.second)); - } - else if (const FieldRegion *FR = dyn_cast(R)) { - StoreRegionPair X = getLazyBinding(B, FR->getSuperRegion(), originalRegion); + if (Result.second) + Result.second = MRMgr.getElementRegionWithSuper(ER, Result.second); + + } else if (const FieldRegion *FR = dyn_cast(R)) { + Result = findLazyBinding(B, cast(FR->getSuperRegion()), + originalRegion); + + if (Result.second) + Result.second = MRMgr.getFieldRegionWithSuper(FR, Result.second); - if (X.second) { - return std::make_pair(X.first, - MRMgr.getFieldRegionWithSuper(FR, X.second)); - } - } else if (const CXXBaseObjectRegion *BaseReg = dyn_cast(R)) { // C++ base object region is another kind of region that we should blast // through to look for lazy compound value. It is like a field region. - StoreRegionPair Result = getLazyBinding(B, BaseReg->getSuperRegion(), - originalRegion); + Result = findLazyBinding(B, cast(BaseReg->getSuperRegion()), + originalRegion); - if (Result.second) { - // Make sure the types match up. - const CXXRecordDecl *LazyD = 0; - if (const TypedValueRegion *LazyR = - dyn_cast(Result.second)) { - LazyD = LazyR->getValueType()->getAsCXXRecordDecl(); - if (LazyD) - LazyD = LazyD->getCanonicalDecl(); - } - - typedef SmallVector BaseListTy; - BaseListTy BaseRegions; - do { - BaseRegions.push_back(BaseReg); - BaseReg = dyn_cast(BaseReg->getSuperRegion()); - } while (BaseReg && LazyD != BaseReg->getDecl()->getCanonicalDecl()); - - // Layer the base regions on the result in least-to-most derived order. - for (BaseListTy::const_reverse_iterator I = BaseRegions.rbegin(), - E = BaseRegions.rend(); - I != E; ++I) { - Result.second = MRMgr.getCXXBaseObjectRegionWithSuper(*I, - Result.second); - } - } - - return Result; + if (Result.second) + Result.second = MRMgr.getCXXBaseObjectRegionWithSuper(BaseReg, + Result.second); } - return StoreRegionPair(); + return Result; } SVal RegionStoreManager::getBindingForElement(RegionBindingsConstRef B, @@ -1440,7 +1456,7 @@ RegionStoreManager::getBindingForDerivedDefaultValue(RegionBindingsConstRef B, return Optional(); } -SVal RegionStoreManager::getLazyBinding(const MemRegion *LazyBindingRegion, +SVal RegionStoreManager::getLazyBinding(const SubRegion *LazyBindingRegion, RegionBindingsRef LazyBinding) { SVal Result; if (const ElementRegion *ER = dyn_cast(LazyBindingRegion)) @@ -1480,8 +1496,8 @@ RegionStoreManager::getBindingForFieldOrElementCommon(RegionBindingsConstRef B, // Lazy binding? Store lazyBindingStore = NULL; - const MemRegion *lazyBindingRegion = NULL; - llvm::tie(lazyBindingStore, lazyBindingRegion) = getLazyBinding(B, R, R); + const SubRegion *lazyBindingRegion = NULL; + llvm::tie(lazyBindingStore, lazyBindingRegion) = findLazyBinding(B, R, R); if (lazyBindingRegion) return getLazyBinding(lazyBindingRegion, getRegionBindings(lazyBindingStore)); @@ -1665,17 +1681,9 @@ RegionStoreManager::getInterestingValues(nonloc::LazyCompoundVal LCV) { NonLoc RegionStoreManager::createLazyBinding(RegionBindingsConstRef B, const TypedValueRegion *R) { - // If we already have a lazy binding, and it's for the whole structure, - // don't create a new lazy binding. - if (Optional V = B.getDefaultBinding(R)) { - if (Optional LCV = - V->getAs()) { - QualType RegionTy = R->getValueType(); - QualType SourceRegionTy = LCV->getRegion()->getValueType(); - if (Ctx.hasSameUnqualifiedType(RegionTy, SourceRegionTy)) - return *LCV; - } - } + if (Optional V = + getExistingLazyBinding(svalBuilder, B, R, false)) + return *V; return svalBuilder.makeLazyCompoundVal(StoreRef(B.asStore(), *this), R); } diff --git a/clang/test/Analysis/array-struct-region.c b/clang/test/Analysis/array-struct-region.c index dfb4cf1..6817124 100644 --- a/clang/test/Analysis/array-struct-region.c +++ b/clang/test/Analysis/array-struct-region.c @@ -285,6 +285,13 @@ void testArrayStructCopy() { clang_analyzer_eval(s3.data[0] == 'a'); // expected-warning{{TRUE}} clang_analyzer_eval(s3.data[1] == 'b'); // expected-warning{{TRUE}} clang_analyzer_eval(s3.data[2] == 'c'); // expected-warning{{TRUE}} + + s3.data[0] = 'z'; + ShortString s4 = s3; + + clang_analyzer_eval(s4.data[0] == 'z'); // expected-warning{{TRUE}} + clang_analyzer_eval(s4.data[1] == 'b'); // expected-warning{{TRUE}} + clang_analyzer_eval(s4.data[2] == 'c'); // expected-warning{{TRUE}} } void testArrayStructCopyNested() { diff --git a/clang/test/Analysis/derived-to-base.cpp b/clang/test/Analysis/derived-to-base.cpp index 80dbc17..6e4a3fa 100644 --- a/clang/test/Analysis/derived-to-base.cpp +++ b/clang/test/Analysis/derived-to-base.cpp @@ -303,4 +303,33 @@ namespace LazyBindings { } #endif } + +#if CONSTRUCTORS + namespace Nested { + struct NonTrivialCopy { + int padding; + NonTrivialCopy() {} + NonTrivialCopy(const NonTrivialCopy &) {} + }; + + struct FullyDerived : private NonTrivialCopy, public Derived { + int z; + }; + + struct Wrapper { + FullyDerived d; + int zz; + + Wrapper(const FullyDerived &d) : d(d), zz(0) {} + }; + + void test5() { + Wrapper w((FullyDerived())); + w.d.x = 1; + + Wrapper w2(w); + clang_analyzer_eval(getX(w2.d) == 1); // expected-warning{{TRUE}} + } + } +#endif } -- 2.7.4