[cfe-commits] r169571 - /cfe/trunk/lib/StaticAnalyzer/Core/RegionStore.cpp
Jordan Rose
jordan_rose at apple.com
Fri Dec 7 09:36:28 PST 2012
A lot touched, but seems reasonable. A few comments inline, though not many of them are actionable.
On Dec 6, 2012, at 17:55 , Ted Kremenek <kremenek at apple.com> wrote:
> Author: kremenek
> Date: Thu Dec 6 19:55:21 2012
> New Revision: 169571
>
> URL: http://llvm.org/viewvc/llvm-project?rev=169571&view=rev
> Log:
> Change RegionStore to always use ImmutableMapRef for processing cluster bindings.
>
> This reduces analysis time by 1.2% on one test case (Objective-C), but
> also cleans up some of the code conceptually as well. We can possible
> just make RegionBindingsRef -> RegionBindings, but I wanted to stage
> things.
>
> After this, we should revisit Jordan's optimization of not canonicalizing
> the immutable AVL trees for the cluster bindings as well.
>
> Modified:
> cfe/trunk/lib/StaticAnalyzer/Core/RegionStore.cpp
>
> Modified: cfe/trunk/lib/StaticAnalyzer/Core/RegionStore.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/RegionStore.cpp?rev=169571&r1=169570&r2=169571&view=diff
> ==============================================================================
> --- cfe/trunk/lib/StaticAnalyzer/Core/RegionStore.cpp (original)
> +++ cfe/trunk/lib/StaticAnalyzer/Core/RegionStore.cpp Thu Dec 6 19:55:21 2012
> @@ -132,7 +132,132 @@
> //===----------------------------------------------------------------------===//
>
> typedef llvm::ImmutableMap<BindingKey, SVal> ClusterBindings;
> -typedef llvm::ImmutableMap<const MemRegion *, ClusterBindings> RegionBindings;
> +
> +typedef llvm::ImmutableMap<const MemRegion *, ClusterBindings>
> + RegionBindings;
> +
> +namespace {
> +class RegionBindingsRef : public llvm::ImmutableMapRef<const MemRegion *,
> + ClusterBindings> {
> + ClusterBindings::Factory &CBFactory;
> +public:
> + typedef llvm::ImmutableMapRef<const MemRegion *, ClusterBindings>
> + ParentTy;
> +
> + RegionBindingsRef(ClusterBindings::Factory &CBFactory,
> + const RegionBindings::TreeTy *T,
> + RegionBindings::TreeTy::Factory *F)
> + : ImmutableMapRef(T, F), CBFactory(CBFactory) {}
> +
> + RegionBindingsRef(const ParentTy &P, ClusterBindings::Factory &CBFactory)
> + : ImmutableMapRef(P), CBFactory(CBFactory) {}
> +
> + RegionBindingsRef add(key_type_ref K, data_type_ref D) {
> + return RegionBindingsRef(static_cast<ParentTy*>(this)->add(K, D),
> + CBFactory);
> + }
> +
> + RegionBindingsRef remove(key_type_ref K) {
> + return RegionBindingsRef(static_cast<ParentTy*>(this)->remove(K),
> + CBFactory);
> + }
> +
> + RegionBindingsRef addBinding(BindingKey K, SVal V);
> +
> + RegionBindingsRef addBinding(const MemRegion *R,
> + BindingKey::Kind k, SVal V);
> +
> + RegionBindingsRef &operator=(const RegionBindingsRef &X) {
> + *static_cast<ParentTy*>(this) = X;
> + return *this;
> + }
This is the default copy-constructor; better to leave it out.
> //===----------------------------------------------------------------------===//
> // Fine-grained control of RegionStoreManager.
> @@ -164,19 +289,15 @@
> namespace {
>
> class RegionStoreManager : public StoreManager {
> +public:
> const RegionStoreFeatures Features;
> RegionBindings::Factory RBFactory;
> - ClusterBindings::Factory CBFactory;
> + mutable ClusterBindings::Factory CBFactory;
Is there a reason this is mutable? When is the RSM ever const? (Left over from an intermediate attempt?)
>
> // BindDefault is only used to initialize a region with a default value.
> StoreRef BindDefault(Store store, const MemRegion *R, SVal V) {
> - RegionBindings B = GetRegionBindings(store);
> - assert(!lookup(B, R, BindingKey::Default));
> - assert(!lookup(B, R, BindingKey::Direct));
> - return StoreRef(addBinding(B, R, BindingKey::Default, V)
> - .getRootWithoutRetain(), *this);
> + RegionBindingsRef B = getRegionBindings(store);
> + assert(!B.lookup(R, BindingKey::Default));
> + assert(!B.lookup(R, BindingKey::Direct));
> + return StoreRef(B.addBinding(R, BindingKey::Default, V)
> + .asImmutableMap()
> + .getRootWithoutRetain(), *this);
I was thinking about this; I think we should have a makeStoreRef(RegionBindingsRef) helper, since we see this pattern so much.
> // invalidate that region. This is because a block may capture
> // a pointer value, but the thing pointed by that pointer may
> // get invalidated.
> - Store store = B.getRootWithoutRetain();
> + Store store = B.asImmutableMap().getRootWithoutRetain();
> SVal V = RM.getBinding(store, loc::MemRegionVal(VR));
This is an unfortunate impedance mismatch.
> StoreRef RegionStoreManager::BindArray(Store store, const TypedValueRegion* R,
> @@ -1724,9 +1813,9 @@
>
> // There may be fewer values in the initialize list than the fields of struct.
> if (FI != FE) {
> - RegionBindings B = GetRegionBindings(newStore.getStore());
> - B = addBinding(B, R, BindingKey::Default, svalBuilder.makeIntVal(0, false));
> - newStore = StoreRef(B.getRootWithoutRetain(), *this);
> + RegionBindingsRef B = getRegionBindings(newStore.getStore());
> + B = B.addBinding(R, BindingKey::Default, svalBuilder.makeIntVal(0, false));
> + newStore = StoreRef(B.asImmutableMap().getRootWithoutRetain(), *this);
> }
Same impedance mismatch in BindStruct, BindVector, and BindArray. For that matter, it's worse: we know that all the bindings will happen in a single cluster.
More information about the cfe-commits
mailing list