[PATCH] [analyzer] Refactor conditional expression evaluating code

Manuel Klimek klimek at google.com
Wed Aug 14 21:06:31 PDT 2013


On Thu, Aug 15, 2013 at 2:12 AM, Ted Kremenek <kremenek at apple.com> wrote:

> Hi Pavel,
>
> Sorry for not getting to this sooner.  It looks like Jordan has been
> carefully reviewing this.   Since I’m the author of this code it’s probably
> worth me taking a pass of your patch as well.  I’ll try and take a careful
> look at it tonight and cross-reference my feedback with any Jordan has
> already provided.
>
> Can you provide some context on what problem you are trying to solve?
>  Your original patch includes no test case which indicates the behavioral
> change (if any) you are trying to capture.  Although your later email
> indicates you don’t expect a behavioral change, I also don’t understand the
> motivation behind the patch.  The second patch includes a test case, but I
> doubt it is for the original motivation of this patch.  Is this purely a
> functional refactoring, or something else?  I can better evaluate this
> change if I understand the motivation.  Otherwise I cannot see approving
> this change unless there is some genuine benefit as it touches some fairly
> sensitive parts of the analyzer.  Specifically, what problem are you trying
> to solve?
>

See http://llvm-reviews.chandlerc.com/D1259 (WIP Fix for temporary
destructors in conditionals), which this patch afaik enables.


>
> Thanks,
> Ted
>
> On Aug 9, 2013, at 5:39 AM, Pavel Labath <labath at google.com> wrote:
>
> Hi jordan_rose,
>
> Instead of digging through the ExplodedGraph, to figure out which edge
> brought
> us here, I compute the value of conditional expression by looking at the
> sub-expression values.
>
> To do this, I needed to change the liveness algorithm a bit -- now, the
> full
> conditional expression depends on all atomic sub-expressions, not only the
> outermost ones.
>
> http://llvm-reviews.chandlerc.com/D1340
>
> Files:
>  lib/Analysis/LiveVariables.cpp
>  lib/StaticAnalyzer/Core/ExprEngineC.cpp
>
> Index: lib/Analysis/LiveVariables.cpp
> ===================================================================
> --- lib/Analysis/LiveVariables.cpp
> +++ lib/Analysis/LiveVariables.cpp
> @@ -212,6 +212,8 @@
>   LiveVariables::LivenessValues &val;
>   LiveVariables::Observer *observer;
>   const CFGBlock *currentBlock;
> +
> +  void MarkLogicalOpsLive(const Expr *E);
> public:
>   TransferFunctions(LiveVariablesImpl &im,
>                     LiveVariables::LivenessValues &Val,
> @@ -368,9 +370,28 @@
>         if (observer)
>           observer->observerKill(DR);
>       }
> +  } else {
> +    // All logical sub-operations are live until we reach the outermost
> +    // operator. Static analyzer relies on this behaviour.
> +    MarkLogicalOpsLive(B);
>   }
> }
>
> +void TransferFunctions::MarkLogicalOpsLive(const Expr *E) {
> +  const BinaryOperator *BO = dyn_cast<BinaryOperator>(E);
> +  if (!BO || !BO->isLogicalOp())
> +    return;
> +
> +  const Expr *LHS = BO->getLHS()->IgnoreParens();
> +  const Expr *RHS = BO->getRHS()->IgnoreParens();
> +
> +  val.liveStmts = LV.SSetFact.add(val.liveStmts, LHS);
> +  val.liveStmts = LV.SSetFact.add(val.liveStmts, RHS);
> +
> +  MarkLogicalOpsLive(LHS);
> +  MarkLogicalOpsLive(RHS);
> +}
> +
> void TransferFunctions::VisitBlockExpr(BlockExpr *BE) {
>   AnalysisDeclContext::referenced_decls_iterator I, E;
>   llvm::tie(I, E) =
> Index: lib/StaticAnalyzer/Core/ExprEngineC.cpp
> ===================================================================
> --- lib/StaticAnalyzer/Core/ExprEngineC.cpp
> +++ lib/StaticAnalyzer/Core/ExprEngineC.cpp
> @@ -501,72 +501,64 @@
>   }
> }
>
> +static ProgramStateRef EvaluateLogicalExpression(const Expr *E,
> +                                                 const LocationContext
> *LC,
> +                                                 ProgramStateRef State) {
> +  SVal X = State->getSVal(E, LC);
> +  if (! X.isUnknown())
> +    return State;
> +
> +  const BinaryOperator *B =
> cast_or_null<BinaryOperator>(E->IgnoreParens());
> +  if (!B || (B->getOpcode() != BO_LAnd && B->getOpcode() != BO_LOr))
> +    return State;
> +
> +  State = EvaluateLogicalExpression(B->getLHS(), LC, State);
> +  X = State->getSVal(B->getLHS(), LC);
> +  QualType XType = B->getLHS()->getType();
> +  assert(!X.isUnknownOrUndef() && "Value should have already been
> computed.");
> +
> +  ProgramStateRef StTrue, StFalse;
> +  llvm::tie(StTrue, StFalse) =
> State->assume(X.castAs<DefinedOrUnknownSVal>());
> +
> +  assert(!StTrue != !StFalse && "Value should be evaluate to true or
> false.");
> +  if(!StFalse == (B->getOpcode() == BO_LAnd)) {
> +    // LHS not sufficient, we need to check RHS as well
> +    State = EvaluateLogicalExpression(B->getRHS(), LC, State);
> +    X = State->getSVal(B->getRHS(), LC);
> +    XType = B->getRHS()->getType();
> +  }
> +
> +  SValBuilder &SVB = State->getStateManager().getSValBuilder();
> +  return State->BindExpr(E, LC, SVB.evalCast(X, B->getType(), XType));
> +}
> +
> void ExprEngine::VisitLogicalExpr(const BinaryOperator* B, ExplodedNode
> *Pred,
>                                   ExplodedNodeSet &Dst) {
>   assert(B->getOpcode() == BO_LAnd ||
>          B->getOpcode() == BO_LOr);
>
>   StmtNodeBuilder Bldr(Pred, Dst, *currBldrCtx);
>   ProgramStateRef state = Pred->getState();
>
> -  ExplodedNode *N = Pred;
> -  while (!N->getLocation().getAs<BlockEntrance>()) {
> -    ProgramPoint P = N->getLocation();
> -    assert(P.getAs<PreStmt>()|| P.getAs<PreStmtPurgeDeadSymbols>());
> -    (void) P;
> -    assert(N->pred_size() == 1);
> -    N = *N->pred_begin();
> -  }
> -  assert(N->pred_size() == 1);
> -  N = *N->pred_begin();
> -  BlockEdge BE = N->getLocation().castAs<BlockEdge>();
> -  SVal X;
> -
> -  // Determine the value of the expression by introspecting how we
> -  // got this location in the CFG.  This requires looking at the previous
> -  // block we were in and what kind of control-flow transfer was involved.
> -  const CFGBlock *SrcBlock = BE.getSrc();
> -  // The only terminator (if there is one) that makes sense is a logical
> op.
> -  CFGTerminator T = SrcBlock->getTerminator();
> -  if (const BinaryOperator *Term =
> cast_or_null<BinaryOperator>(T.getStmt())) {
> -    (void) Term;
> -    assert(Term->isLogicalOp());
> -    assert(SrcBlock->succ_size() == 2);
> -    // Did we take the true or false branch?
> -    unsigned constant = (*SrcBlock->succ_begin() == BE.getDst()) ? 1 : 0;
> -    X = svalBuilder.makeIntVal(constant, B->getType());
> -  }
> -  else {
> -    // If there is no terminator, by construction the last statement
> -    // in SrcBlock is the value of the enclosing expression.
> -    // However, we still need to constrain that value to be 0 or 1.
> -    assert(!SrcBlock->empty());
> -    CFGStmt Elem = SrcBlock->rbegin()->castAs<CFGStmt>();
> -    const Expr *RHS = cast<Expr>(Elem.getStmt());
> -    SVal RHSVal = N->getState()->getSVal(RHS, Pred->getLocationContext());
> -
> -    if (RHSVal.isUndef()) {
> -      X = RHSVal;
> -    } else {
> -      DefinedOrUnknownSVal DefinedRHS =
> RHSVal.castAs<DefinedOrUnknownSVal>();
> -      ProgramStateRef StTrue, StFalse;
> -      llvm::tie(StTrue, StFalse) = N->getState()->assume(DefinedRHS);
> -      if (StTrue) {
> -        if (StFalse) {
> -          // We can't constrain the value to 0 or 1.
> -          // The best we can do is a cast.
> -          X = getSValBuilder().evalCast(RHSVal, B->getType(),
> RHS->getType());
> -        } else {
> -          // The value is known to be true.
> -          X = getSValBuilder().makeIntVal(1, B->getType());
> -        }
> -      } else {
> -        // The value is known to be false.
> -        assert(StFalse && "Infeasible path!");
> -        X = getSValBuilder().makeIntVal(0, B->getType());
> +  state = EvaluateLogicalExpression(B, Pred->getLocationContext(), state);
> +  SVal X = state->getSVal(B, Pred->getLocationContext());
> +
> +  if (!X.isUndef()) {
> +    DefinedOrUnknownSVal DefinedRHS = X.castAs<DefinedOrUnknownSVal>();
> +    ProgramStateRef StTrue, StFalse;
> +    llvm::tie(StTrue, StFalse) = state->assume(DefinedRHS);
> +    if (StTrue) {
> +      if (!StFalse) {
> +        // The value is known to be true.
> +        X = getSValBuilder().makeIntVal(1, B->getType());
>       }
> +    } else {
> +      // The value is known to be false.
> +      assert(StFalse && "Infeasible path!");
> +      X = getSValBuilder().makeIntVal(0, B->getType());
>     }
>   }
> +
>   Bldr.generateNode(B, Pred, state->BindExpr(B,
> Pred->getLocationContext(), X));
> }
> <D1340.1.patch>_______________________________________________
>
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130815/b47a4ea3/attachment.html>


More information about the cfe-commits mailing list