From: Ted Kremenek Date: Tue, 26 Feb 2008 19:05:15 +0000 (+0000) Subject: Major cleanup of the transfer function logic for '&&', '||', and '?'. We X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=05a2378c708688c8ef498a5cea40ed7f5db15fa5;p=clang Major cleanup of the transfer function logic for '&&', '||', and '?'. We now store in the state essentially which branch we took. This removes a bunch of bogus assumptions (and likely bugs), reduces the complexity of the implementation, and facilitates more optimizations. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@47613 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/Analysis/GRExprEngine.cpp b/Analysis/GRExprEngine.cpp index cc1e89944e..4479ecf30b 100644 --- a/Analysis/GRExprEngine.cpp +++ b/Analysis/GRExprEngine.cpp @@ -66,6 +66,60 @@ GRExprEngine::SetRVal(StateTy St, const LVal& LV, const RVal& RV) { return StateMgr.SetRVal(St, LV, RV); } +GRExprEngine::StateTy +GRExprEngine::MarkBranch(StateTy St, Stmt* Terminator, bool branchTaken) { + + switch (Terminator->getStmtClass()) { + default: + return St; + + case Stmt::BinaryOperatorClass: { // '&&' and '||' + + BinaryOperator* B = cast(Terminator); + BinaryOperator::Opcode Op = B->getOpcode(); + + assert (Op == BinaryOperator::LAnd || Op == BinaryOperator::LOr); + + // For &&, if we take the true branch, then the value of the whole + // expression is that of the RHS expression. + // + // For ||, if we take the false branch, then the value of the whole + // expression is that of the RHS expression. + + Expr* Ex = (Op == BinaryOperator::LAnd && branchTaken) || + (Op == BinaryOperator::LOr && !branchTaken) + ? B->getRHS() : B->getLHS(); + + return SetBlkExprRVal(St, B, UninitializedVal(Ex)); + } + + case Stmt::ConditionalOperatorClass: { // ?: + + ConditionalOperator* C = cast(Terminator); + + // For ?, if branchTaken == true then the value is either the LHS or + // the condition itself. (GNU extension). + + Expr* Ex; + + if (branchTaken) + Ex = C->getLHS() ? C->getLHS() : C->getCond(); + else + Ex = C->getRHS(); + + return SetBlkExprRVal(St, C, UninitializedVal(Ex)); + } + + case Stmt::ChooseExprClass: { // ?: + + ChooseExpr* C = cast(Terminator); + + Expr* Ex = branchTaken ? C->getLHS() : C->getRHS(); + return SetBlkExprRVal(St, C, UninitializedVal(Ex)); + } + } +} + void GRExprEngine::ProcessBranch(Expr* Condition, Stmt* Term, BranchNodeBuilder& builder) { @@ -126,7 +180,7 @@ void GRExprEngine::ProcessBranch(Expr* Condition, Stmt* Term, StateTy St = Assume(PrevState, V, true, isFeasible); if (isFeasible) - builder.generateNode(St, true); + builder.generateNode(MarkBranch(St, Term, true), true); else builder.markInfeasible(true); } @@ -146,7 +200,7 @@ void GRExprEngine::ProcessBranch(Expr* Condition, Stmt* Term, StateTy St = Assume(PrevState, V, false, isFeasible); if (isFeasible) - builder.generateNode(St, false); + builder.generateNode(MarkBranch(St, Term, false), false); else builder.markInfeasible(false); } @@ -295,59 +349,54 @@ void GRExprEngine::ProcessSwitch(SwitchNodeBuilder& builder) { void GRExprEngine::VisitLogicalExpr(BinaryOperator* B, NodeTy* Pred, NodeSet& Dst) { - - bool hasR2; - StateTy PrevState = Pred->getState(); - - RVal R1 = GetRVal(PrevState, B->getLHS()); - RVal R2 = GetRVal(PrevState, B->getRHS(), hasR2); - if (hasR2) { - if (R2.isUnknownOrUninit()) { - Nodify(Dst, B, Pred, SetRVal(PrevState, B, R2)); - return; - } - } - else if (R1.isUnknownOrUninit()) { - Nodify(Dst, B, Pred, SetRVal(PrevState, B, R1)); - return; - } - - // R1 is an expression that can evaluate to either 'true' or 'false'. - if (B->getOpcode() == BinaryOperator::LAnd) { - // hasR2 == 'false' means that LHS evaluated to 'false' and that - // we short-circuited, leading to a value of '0' for the '&&' expression. - if (hasR2 == false) { - Nodify(Dst, B, Pred, SetRVal(PrevState, B, MakeConstantVal(0U, B))); - return; - } + assert (B->getOpcode() == BinaryOperator::LAnd || + B->getOpcode() == BinaryOperator::LOr); + + assert (B == CurrentStmt && getCFG().isBlkExpr(B)); + + StateTy St = Pred->getState(); + RVal X = GetBlkExprRVal(St, B); + + assert (X.isUninit()); + + Expr* Ex = (Expr*) cast(X).getData(); + + assert (Ex); + + if (Ex == B->getRHS()) { + + X = GetBlkExprRVal(St, Ex); + + // We took the RHS. Because the value of the '&&' or '||' expression must + // evaluate to 0 or 1, we must assume the value of the RHS evaluates to 0 + // or 1. Alternatively, we could take a lazy approach, and calculate this + // value later when necessary. We don't have the machinery in place for + // this right now, and since most logical expressions are used for branches, + // the payoff is not likely to be large. Instead, we do eager evaluation. + + bool isFeasible = false; + StateTy NewState = Assume(St, X, true, isFeasible); + + if (isFeasible) + Nodify(Dst, B, Pred, SetBlkExprRVal(NewState, B, MakeConstantVal(1U, B))); + + isFeasible = false; + NewState = Assume(St, X, false, isFeasible); + + if (isFeasible) + Nodify(Dst, B, Pred, SetBlkExprRVal(NewState, B, MakeConstantVal(0U, B))); } else { - assert (B->getOpcode() == BinaryOperator::LOr); - // hasR2 == 'false' means that the LHS evaluate to 'true' and that - // we short-circuited, leading to a value of '1' for the '||' expression. - if (hasR2 == false) { - Nodify(Dst, B, Pred, SetRVal(PrevState, B, MakeConstantVal(1U, B))); - return; - } - } - - // If we reach here we did not short-circuit. Assume R2 == true and - // R2 == false. + // We took the LHS expression. Depending on whether we are '&&' or + // '||' we know what the value of the expression is via properties of + // the short-circuiting. - bool isFeasible; - StateTy St = Assume(PrevState, R2, true, isFeasible); - - if (isFeasible) - Nodify(Dst, B, Pred, SetRVal(PrevState, B, MakeConstantVal(1U, B))); - - St = Assume(PrevState, R2, false, isFeasible); - - if (isFeasible) - Nodify(Dst, B, Pred, SetRVal(PrevState, B, MakeConstantVal(0U, B))); + X = MakeConstantVal( B->getOpcode() == BinaryOperator::LAnd ? 0U : 1U, B); + Nodify(Dst, B, Pred, SetBlkExprRVal(St, B, X)); + } } - - + void GRExprEngine::ProcessStmt(Stmt* S, StmtNodeBuilder& builder) { @@ -538,12 +587,19 @@ void GRExprEngine::VisitDeclStmt(DeclStmt* DS, GRExprEngine::NodeTy* Pred, void GRExprEngine::VisitGuardedExpr(Expr* Ex, Expr* L, Expr* R, NodeTy* Pred, NodeSet& Dst) { + assert (Ex == CurrentStmt && getCFG().isBlkExpr(Ex)); + StateTy St = Pred->getState(); + RVal X = GetBlkExprRVal(St, Ex); - RVal V = GetRVal(St, L); - if (isa(V)) V = GetRVal(St, R); + assert (X.isUninit()); - Nodify(Dst, Ex, Pred, SetRVal(St, Ex, V)); + Expr* SE = (Expr*) cast(X).getData(); + + assert (SE); + + X = GetBlkExprRVal(St, SE); + Nodify(Dst, Ex, Pred, SetBlkExprRVal(St, Ex, X)); } /// VisitSizeOfAlignOfTypeExpr - Transfer function for sizeof(type). diff --git a/Analysis/ValueState.cpp b/Analysis/ValueState.cpp index 03b2996b65..ab208ff0fe 100644 --- a/Analysis/ValueState.cpp +++ b/Analysis/ValueState.cpp @@ -69,10 +69,14 @@ ValueStateManager::RemoveDeadBindings(ValueState St, Stmt* Loc, MarkedSymbols.insert(*SI); } } - else + else { + RVal X = I.getData(); + + if (X.isUninit() && cast(X).getData()) + continue; + NewSt.BlockExprBindings = Remove(NewSt, BlkExpr); - - continue; + } } // Iterate over the variable bindings. @@ -209,7 +213,7 @@ ValueStateManager::AddEQ(ValueState St, SymbolID sym, const llvm::APSInt& V) { return getPersistentState(NewSt); } -RVal ValueStateManager::GetRVal(ValueState St, Expr* E, bool* hasVal) { +RVal ValueStateManager::GetRVal(ValueState St, Expr* E) { for (;;) { @@ -322,21 +326,15 @@ RVal ValueStateManager::GetRVal(ValueState St, Expr* E, bool* hasVal) { ValueState::ExprBindingsTy::TreeTy* T = St->SubExprBindings.SlimFind(E); - if (T) { - if (hasVal) *hasVal = true; - return T->getValue().second; - } - - T = St->BlockExprBindings.SlimFind(E); + return T ? T->getValue().second : GetBlkExprRVal(St, E); +} + +RVal ValueStateManager::GetBlkExprRVal(ValueState St, Expr* E) { - if (T) { - if (hasVal) *hasVal = true; - return T->getValue().second; - } - else { - if (hasVal) *hasVal = false; - return UnknownVal(); - } + assert (!isa(E)); + + ValueState::ExprBindingsTy::TreeTy* T = St->BlockExprBindings.SlimFind(E); + return T ? T->getValue().second : UnknownVal(); } RVal ValueStateManager::GetLVal(ValueState St, Expr* E) { diff --git a/Analysis/ValueState.h b/Analysis/ValueState.h index 424b832258..3e0012287b 100644 --- a/Analysis/ValueState.h +++ b/Analysis/ValueState.h @@ -60,11 +60,11 @@ private: void operator=(const ValueStateImpl& R) const; public: - vstate::ExprBindingsTy SubExprBindings; - vstate::ExprBindingsTy BlockExprBindings; - vstate::VarBindingsTy VarBindings; - vstate::ConstNotEqTy ConstNotEq; - vstate::ConstEqTy ConstEq; + vstate::ExprBindingsTy SubExprBindings; + vstate::ExprBindingsTy BlockExprBindings; + vstate::VarBindingsTy VarBindings; + vstate::ConstNotEqTy ConstNotEq; + vstate::ConstEqTy ConstEq; /// This ctor is used when creating the first ValueStateImpl object. ValueStateImpl(vstate::ExprBindingsTy EB, vstate::VarBindingsTy VB, @@ -258,10 +258,12 @@ public: ValueState SetRVal(ValueState St, Expr* E, bool isBlkExpr, RVal V); ValueState SetRVal(ValueState St, LVal LV, RVal V); - RVal GetRVal(ValueState St, Expr* E, bool* hasVal = NULL); - RVal GetRVal(ValueState St, const LVal& LV, QualType T = QualType()); + RVal GetRVal(ValueState St, Expr* E); + RVal GetRVal(ValueState St, const LVal& LV, QualType T = QualType()); RVal GetLVal(ValueState St, Expr* E); + RVal GetBlkExprRVal(ValueState St, Expr* Ex); + ValueState getPersistentState(const ValueStateImpl& Impl); ValueState AddEQ(ValueState St, SymbolID sym, const llvm::APSInt& V); diff --git a/include/clang/Analysis/PathSensitive/GRExprEngine.h b/include/clang/Analysis/PathSensitive/GRExprEngine.h index a2d92f1ea6..08d2495196 100644 --- a/include/clang/Analysis/PathSensitive/GRExprEngine.h +++ b/include/clang/Analysis/PathSensitive/GRExprEngine.h @@ -216,7 +216,9 @@ public: /// ProcessSwitch - Called by GRCoreEngine. Used to generate successor /// nodes by processing the 'effects' of a switch statement. - void ProcessSwitch(SwitchNodeBuilder& builder); + void ProcessSwitch(SwitchNodeBuilder& builder); + +protected: /// RemoveDeadBindings - Return a new state that is the same as 'St' except /// that all subexpression mappings are removed and that any @@ -231,6 +233,10 @@ public: StateTy SetRVal(StateTy St, const Expr* Ex, const RVal& V) { return SetRVal(St, const_cast(Ex), V); } + + StateTy SetBlkExprRVal(StateTy St, Expr* Ex, const RVal& V) { + return StateMgr.SetRVal(St, Ex, true, V); + } /// SetRVal - This version of SetRVal is used to batch process a set /// of different possible RVals and return a set of different states. @@ -243,18 +249,16 @@ public: RVal GetRVal(const StateTy& St, Expr* Ex) { return StateMgr.GetRVal(St, Ex); } - - RVal GetRVal(const StateTy& St, Expr* Ex, bool& hasVal) { - return StateMgr.GetRVal(St, Ex, &hasVal); - } - + RVal GetRVal(const StateTy& St, const Expr* Ex) { return GetRVal(St, const_cast(Ex)); } - RVal GetRVal(const StateTy& St, const LVal& LV, - QualType T = QualType()) { + RVal GetBlkExprRVal(const StateTy& St, Expr* Ex) { + return StateMgr.GetBlkExprRVal(St, Ex); + } + RVal GetRVal(const StateTy& St, const LVal& LV, QualType T = QualType()) { return StateMgr.GetRVal(St, LV, T); } @@ -400,5 +404,7 @@ public: StateTy EvalCall(CallExpr* CE, LVal L, StateTy St) { return St; } + + StateTy MarkBranch(StateTy St, Stmt* Terminator, bool branchTaken); }; } // end clang namespace \ No newline at end of file diff --git a/include/clang/Analysis/PathSensitive/RValues.h b/include/clang/Analysis/PathSensitive/RValues.h index e43edb9253..493fc1645c 100644 --- a/include/clang/Analysis/PathSensitive/RValues.h +++ b/include/clang/Analysis/PathSensitive/RValues.h @@ -38,8 +38,8 @@ protected: : Data(const_cast(d)), Kind((isLVal ? LValKind : NonLValKind) | (ValKind << BaseBits)) {} - explicit RVal(BaseKind k) - : Data(0), Kind(k) {} + explicit RVal(BaseKind k, void* D = NULL) + : Data(D), Kind(k) {} public: ~RVal() {}; @@ -106,10 +106,13 @@ public: class UninitializedVal : public RVal { public: UninitializedVal() : RVal(UninitializedKind) {} + UninitializedVal(void* D) : RVal(UninitializedKind, D) {} static inline bool classof(const RVal* V) { return V->getBaseKind() == UninitializedKind; - } + } + + void* getData() const { return Data; } }; class NonLVal : public RVal {