[cfe-commits] r169571 - /cfe/trunk/lib/StaticAnalyzer/Core/RegionStore.cpp

[Jordan Rose]

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.