[cfe-commits] r90967 - /cfe/trunk/lib/Analysis/GRExprEngine.cpp

[Ted Kremenek]

Hi Zhongxing,

What is the invariant that we are going for?  It seems incorrect that if 'GR_EvalCallExpr()' returns false but DstTmp contains nodes then we'll just throw those nodes away.  Right now we also don't pass processed nodes from one Checker to the next; if that is the invariant, then it seems we can make the interface to GR_EvalCallExpr() simpler (and less bug prone) by doing the following:

(1) Change the return type of GR_EvalCallExpr() to 'void'

(2) If the populated ExplodedNodeSet contains a node, we stop processing the expression.  If it doesn't, we call GR_EvalCallExpr() on the next Checker.

What do you think?  The implication is that Checkers just have to worry about populating the ExplodedNodeSet, and not care about getting the return value right.

Ted

On Dec 9, 2009, at 4:16 AM, Zhongxing Xu wrote:

> Author: zhongxingxu
> Date: Wed Dec  9 06:16:07 2009
> New Revision: 90967
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=90967&view=rev
> Log:
> Use a temporary destination set such that we can clear fake auto transitions.
> Otherwise, even when real evaluation occurs, the previous fake auto 
> transitions would still be in the destination set, causing fake state 
> bifurcation.
> 
> Modified:
>    cfe/trunk/lib/Analysis/GRExprEngine.cpp
> 
> Modified: cfe/trunk/lib/Analysis/GRExprEngine.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Analysis/GRExprEngine.cpp?rev=90967&r1=90966&r2=90967&view=diff
> 
> ==============================================================================
> --- cfe/trunk/lib/Analysis/GRExprEngine.cpp (original)
> +++ cfe/trunk/lib/Analysis/GRExprEngine.cpp Wed Dec  9 06:16:07 2009
> @@ -152,13 +152,26 @@
>                                           ExplodedNodeSet &Dst,
>                                           const GRState *state,
>                                           ExplodedNode *Pred) {
> +  bool Evaluated = false;
> +  ExplodedNodeSet DstTmp;
> +
>   for (CheckersOrdered::iterator I=Checkers.begin(),E=Checkers.end();I!=E;++I) {
>     void *tag = I->first;
>     Checker *checker = I->second;
> 
> -    if (checker->GR_EvalNilReceiver(Dst, *Builder, *this, ME, Pred, state, tag))
> +    if (checker->GR_EvalNilReceiver(DstTmp, *Builder, *this, ME, Pred, state,
> +                                    tag)) {
> +      Evaluated = true;
>       break;
> +    } else
> +      // The checker didn't evaluate the expr. Restore the Dst.
> +      DstTmp.clear();
>   }
> +
> +  if (Evaluated)
> +    Dst.insert(DstTmp);
> +  else
> +    Dst.insert(Pred);
> }
> 
> // CheckerEvalCall returns true if one of the checkers processed the node.
> @@ -168,17 +181,25 @@
>                                    ExplodedNodeSet &Dst, 
>                                    ExplodedNode *Pred) {
>   bool Evaluated = false;
> +  ExplodedNodeSet DstTmp;
> 
>   for (CheckersOrdered::iterator I=Checkers.begin(),E=Checkers.end();I!=E;++I) {
>     void *tag = I->first;
>     Checker *checker = I->second;
> 
> -    if (checker->GR_EvalCallExpr(Dst, *Builder, *this, CE, Pred, tag)) {
> +    if (checker->GR_EvalCallExpr(DstTmp, *Builder, *this, CE, Pred, tag)) {
>       Evaluated = true;
>       break;
> -    }
> +    } else
> +      // The checker didn't evaluate the expr. Restore the DstTmp set.
> +      DstTmp.clear();
>   }
> 
> +  if (Evaluated)
> +    Dst.insert(DstTmp);
> +  else
> +    Dst.insert(Pred);
> +
>   return Evaluated;
> }
> 
> 
> 
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits