[PATCH] [analyzer] Refactor conditional expression evaluating code
Ted Kremenek
kremenek at apple.com
Wed Aug 14 17:12:23 PDT 2013
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?
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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130814/a47e3c8e/attachment.html>
More information about the cfe-commits
mailing list