[cfe-commits] r162154 - in /cfe/trunk: include/clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h lib/StaticAnalyzer/Core/ExplodedGraph.cpp

[Anna Zaks]

On Aug 17, 2012, at 5:30 PM, Jordan Rose wrote:

> Author: jrose
> Date: Fri Aug 17 19:30:10 2012
> New Revision: 162154
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=162154&view=rev
> Log:
> [analyzer] Use PointerUnion to implement ExplodedNode::NodeGroup.
> 
> We shouldn't be reinventing our own wheels. This also paves the way for
> marking different kinds of sinks.
> 
> No functionality change.
> 
> Modified:
>    cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h
>    cfe/trunk/lib/StaticAnalyzer/Core/ExplodedGraph.cpp
> 
> Modified: cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h?rev=162154&r1=162153&r2=162154&view=diff
> ==============================================================================
> --- cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h (original)
> +++ cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h Fri Aug 17 19:30:10 2012
> @@ -61,44 +61,30 @@
>   friend class EndOfFunctionNodeBuilder;
> 

Great refactor!

Could you add some documentation of what the NodeGroup is. The comment below is doing that somewhat but it talks in terms of implementation (what P is) not definition. Also would be good to add comments for all the APIs inside the NodeGroup.


>   class NodeGroup {
> -    enum { Size1 = 0x0, SizeOther = 0x1, AuxFlag = 0x2, Mask = 0x3 };
> +    // Conceptually a discriminated union. If the low bit is set, the node is
> +    // a sink. If the low bit is not set, the pointer refers to the storage
> +    // for the nodes in the group.
>     uintptr_t P;
> -
> -    unsigned getKind() const {
> -      return P & 0x1;
> -    }
> -
> -    void *getPtr() const {
> -      assert (!getFlag());
> -      return reinterpret_cast<void*>(P & ~Mask);
> -    }
> -
> -    ExplodedNode *getNode() const {
> -      return reinterpret_cast<ExplodedNode*>(getPtr());
> -    }
> 
>   public:
> -    NodeGroup() : P(0) {}
> +    NodeGroup(bool Flag = false) : P(Flag) {
> +      assert(getFlag() == Flag);
> +    }
> 
> -    ExplodedNode **begin() const;
> +    ExplodedNode * const *begin() const;
> 
> -    ExplodedNode **end() const;
> +    ExplodedNode * const *end() const;
> 
>     unsigned size() const;
> 
> -    bool empty() const { return (P & ~Mask) == 0; }
> +    bool empty() const { return P == 0 || getFlag() != 0; }
> 
>     void addNode(ExplodedNode *N, ExplodedGraph &G);
> 
>     void replaceNode(ExplodedNode *node);
> 
> -    void setFlag() {
> -      assert(P == 0);
> -      P = AuxFlag;
> -    }
> -
>     bool getFlag() const {
> -      return P & AuxFlag ? true : false;
> +      return (P & 1);
>     }

This is isSink, right?

>   };
> 
> @@ -119,9 +105,8 @@
> 
>   explicit ExplodedNode(const ProgramPoint &loc, ProgramStateRef state,
>                         bool IsSink)
> -    : Location(loc), State(state) {
> -    if (IsSink)
> -      Succs.setFlag();
> +    : Location(loc), State(state), Succs(IsSink) {
> +    assert(isSink() == IsSink);
>   }
> 
>   ~ExplodedNode() {}
> @@ -190,9 +175,9 @@
>   }
> 
>   // Iterators over successor and predecessor vertices.
> -  typedef ExplodedNode**       succ_iterator;
> +  typedef ExplodedNode*       const *       succ_iterator;
>   typedef const ExplodedNode* const * const_succ_iterator;
> -  typedef ExplodedNode**       pred_iterator;
> +  typedef ExplodedNode*       const *       pred_iterator;
>   typedef const ExplodedNode* const * const_pred_iterator;
> 
>   pred_iterator pred_begin() { return Preds.begin(); }
> 
> Modified: cfe/trunk/lib/StaticAnalyzer/Core/ExplodedGraph.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/ExplodedGraph.cpp?rev=162154&r1=162153&r2=162154&view=diff
> ==============================================================================
> --- cfe/trunk/lib/StaticAnalyzer/Core/ExplodedGraph.cpp (original)
> +++ cfe/trunk/lib/StaticAnalyzer/Core/ExplodedGraph.cpp Fri Aug 17 19:30:10 2012
> @@ -162,9 +162,8 @@
> // ExplodedNode.
> //===----------------------------------------------------------------------===//
> 
> -static inline BumpVector<ExplodedNode*>& getVector(void *P) {
> -  return *reinterpret_cast<BumpVector<ExplodedNode*>*>(P);
> -}
> +typedef BumpVector<ExplodedNode *> ExplodedNodeVector;
> +typedef llvm::PointerUnion<ExplodedNode *, ExplodedNodeVector *> GroupStorage;
> 
> void ExplodedNode::addPredecessor(ExplodedNode *V, ExplodedGraph &G) {
>   assert (!V->isSink());
> @@ -176,71 +175,75 @@
> }
> 
> void ExplodedNode::NodeGroup::replaceNode(ExplodedNode *node) {
> -  assert(getKind() == Size1);
> -  P = reinterpret_cast<uintptr_t>(node);
> -  assert(getKind() == Size1);
> +  GroupStorage &Storage = reinterpret_cast<GroupStorage&>(P);
> +  assert(Storage.is<ExplodedNode *>());
> +  Storage = node;
> +  assert(Storage.is<ExplodedNode *>());
> }
> 
> void ExplodedNode::NodeGroup::addNode(ExplodedNode *N, ExplodedGraph &G) {
> -  assert((reinterpret_cast<uintptr_t>(N) & Mask) == 0x0);
>   assert(!getFlag());
> 
> -  if (getKind() == Size1) {
> -    if (ExplodedNode *NOld = getNode()) {
> -      BumpVectorContext &Ctx = G.getNodeAllocator();
> -      BumpVector<ExplodedNode*> *V = 
> -        G.getAllocator().Allocate<BumpVector<ExplodedNode*> >();
> -      new (V) BumpVector<ExplodedNode*>(Ctx, 4);
> -      
> -      assert((reinterpret_cast<uintptr_t>(V) & Mask) == 0x0);
> -      V->push_back(NOld, Ctx);
> -      V->push_back(N, Ctx);
> -      P = reinterpret_cast<uintptr_t>(V) | SizeOther;
> -      assert(getPtr() == (void*) V);
> -      assert(getKind() == SizeOther);
> -    }
> -    else {
> -      P = reinterpret_cast<uintptr_t>(N);
> -      assert(getKind() == Size1);
> -    }
> +  GroupStorage &Storage = reinterpret_cast<GroupStorage&>(P);
> +  if (Storage.isNull()) {
> +    Storage = N;
> +    assert(Storage.is<ExplodedNode *>());
> +    return;
>   }
> -  else {
> -    assert(getKind() == SizeOther);
> -    getVector(getPtr()).push_back(N, G.getNodeAllocator());
> +
> +  ExplodedNodeVector *V = Storage.dyn_cast<ExplodedNodeVector *>();
> +
> +  if (!V) {
> +    // Switch from single-node to multi-node representation.
> +    ExplodedNode *Old = Storage.get<ExplodedNode *>();
> +
> +    BumpVectorContext &Ctx = G.getNodeAllocator();
> +    V = G.getAllocator().Allocate<ExplodedNodeVector>();
> +    new (V) ExplodedNodeVector(Ctx, 4);
> +    V->push_back(Old, Ctx);
> +
> +    Storage = V;
> +    assert(!getFlag());
> +    assert(Storage.is<ExplodedNodeVector *>());
>   }
> +
> +  V->push_back(N, G.getNodeAllocator());
> }
> 
> unsigned ExplodedNode::NodeGroup::size() const {
>   if (getFlag())
>     return 0;
> 
> -  if (getKind() == Size1)
> -    return getNode() ? 1 : 0;
> -  else
> -    return getVector(getPtr()).size();
> +  const GroupStorage &Storage = reinterpret_cast<const GroupStorage &>(P);
> +  if (Storage.isNull())
> +    return 0;
> +  if (ExplodedNodeVector *V = Storage.dyn_cast<ExplodedNodeVector *>())
> +    return V->size();
> +  return 1;
> }
> 
> -ExplodedNode **ExplodedNode::NodeGroup::begin() const {
> +ExplodedNode * const *ExplodedNode::NodeGroup::begin() const {
>   if (getFlag())
> -    return NULL;
> +    return 0;
> 
> -  if (getKind() == Size1)
> -    return (ExplodedNode**) (getPtr() ? &P : NULL);
> -  else
> -    return const_cast<ExplodedNode**>(&*(getVector(getPtr()).begin()));
> +  const GroupStorage &Storage = reinterpret_cast<const GroupStorage &>(P);
> +  if (Storage.isNull())
> +    return 0;
> +  if (ExplodedNodeVector *V = Storage.dyn_cast<ExplodedNodeVector *>())
> +    return V->begin();
> +  return Storage.getAddrOfPtr1();
> }
> 
> -ExplodedNode** ExplodedNode::NodeGroup::end() const {
> +ExplodedNode * const *ExplodedNode::NodeGroup::end() const {
>   if (getFlag())
> -    return NULL;
> +    return 0;
> 
> -  if (getKind() == Size1)
> -    return (ExplodedNode**) (getPtr() ? &P+1 : NULL);
> -  else {
> -    // Dereferencing end() is undefined behaviour. The vector is not empty, so
> -    // we can dereference the last elem and then add 1 to the result.
> -    return const_cast<ExplodedNode**>(getVector(getPtr()).end());
> -  }
> +  const GroupStorage &Storage = reinterpret_cast<const GroupStorage &>(P);
> +  if (Storage.isNull())
> +    return 0;
> +  if (ExplodedNodeVector *V = Storage.dyn_cast<ExplodedNodeVector *>())
> +    return V->end();
> +  return Storage.getAddrOfPtr1() + 1;
> }
> 
> ExplodedNode *ExplodedGraph::getNode(const ProgramPoint &L,
> @@ -338,7 +341,8 @@
>     }
> 
>     // Visit our predecessors and enqueue them.
> -    for (ExplodedNode** I=N->Preds.begin(), **E=N->Preds.end(); I!=E; ++I)
> +    for (ExplodedNode::pred_iterator I = N->Preds.begin(), E = N->Preds.end();
> +         I != E; ++I)
>       WL1.push_back(*I);
>   }
> 
> @@ -375,7 +379,8 @@
> 
>     // Walk through the predecessors of 'N' and hook up their corresponding
>     // nodes in the new graph (if any) to the freshly created node.
> -    for (ExplodedNode **I=N->Preds.begin(), **E=N->Preds.end(); I!=E; ++I) {
> +    for (ExplodedNode::pred_iterator I = N->Preds.begin(), E = N->Preds.end();
> +         I != E; ++I) {
>       Pass2Ty::iterator PI = Pass2.find(*I);
>       if (PI == Pass2.end())
>         continue;
> @@ -387,7 +392,8 @@
>     // been created, we should hook them up as successors.  Otherwise, enqueue
>     // the new nodes from the original graph that should have nodes created
>     // in the new graph.
> -    for (ExplodedNode **I=N->Succs.begin(), **E=N->Succs.end(); I!=E; ++I) {
> +    for (ExplodedNode::succ_iterator I = N->Succs.begin(), E = N->Succs.end();
> +         I != E; ++I) {
>       Pass2Ty::iterator PI = Pass2.find(*I);
>       if (PI != Pass2.end()) {
>         PI->second->addPredecessor(NewN, *G);
> 
> 
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits