On Thu, Aug 15, 2013 at 2:12 AM, Ted Kremenek <[email protected]> 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 <[email protected]> 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 > [email protected] > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits > > > > _______________________________________________ > cfe-commits mailing list > [email protected] > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits > >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
