From: Ted Kremenek Date: Thu, 28 Feb 2008 20:32:03 +0000 (+0000) Subject: Added checking for undefined results of '<<' and '>>' (shifting by too many bits... X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=8cc13ea74fea1c04042a2f4087665bc5182e8408;p=clang Added checking for undefined results of '<<' and '>>' (shifting by too many bits, etc.) This current implementation only works when both operands are concrete values; later we will add support for symbolic values. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@47726 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/Analysis/GRExprEngine.cpp b/Analysis/GRExprEngine.cpp index 6106ef39cb..74f3a9a48d 100644 --- a/Analysis/GRExprEngine.cpp +++ b/Analysis/GRExprEngine.cpp @@ -1008,14 +1008,11 @@ void GRExprEngine::VisitBinaryOperator(BinaryOperator* B, bool isFeasible = false; ValueState* ZeroSt = Assume(St, RightV, false, isFeasible); - if (isFeasible) { - NodeTy* DivZeroNode = Builder->generateNode(B, ZeroSt, N2); - - if (DivZeroNode) { + if (isFeasible) + if (NodeTy* DivZeroNode = Builder->generateNode(B, ZeroSt, N2)) { DivZeroNode->markAsSink(); BadDivides.insert(DivZeroNode); } - } // Second, "assume" that the denominator cannot be 0. @@ -1029,6 +1026,7 @@ void GRExprEngine::VisitBinaryOperator(BinaryOperator* B, // Fall-through. The logic below processes the divide. } + if (Op <= BinaryOperator::Or) { // Process non-assignements except commas or short-circuited @@ -1041,6 +1039,18 @@ void GRExprEngine::VisitBinaryOperator(BinaryOperator* B, continue; } + if (Result.isUndef() && !LeftV.isUndef() && !RightV.isUndef()) { + + // The operands were not undefined, but the result is undefined. + + if (NodeTy* UndefNode = Builder->generateNode(B, St, N2)) { + UndefNode->markAsSink(); + UndefResults.insert(UndefNode); + } + + continue; + } + Nodify(Dst, B, N2, SetRVal(St, B, Result)); continue; } @@ -1195,6 +1205,19 @@ void GRExprEngine::VisitBinaryOperator(BinaryOperator* B, } RVal Result = EvalCast(EvalBinOp(Op, V, RightV), B->getType()); + + if (Result.isUndef()) { + + // The operands were not undefined, but the result is undefined. + + if (NodeTy* UndefNode = Builder->generateNode(B, St, N2)) { + UndefNode->markAsSink(); + UndefResults.insert(UndefNode); + } + + continue; + } + St = SetRVal(SetRVal(St, B, Result), LeftLV, Result); } } @@ -1591,9 +1614,13 @@ struct VISIBILITY_HIDDEN DOTGraphTraits : GraphPrintCheckerState->isUndefDeref(N) || GraphPrintCheckerState->isUndefStore(N) || GraphPrintCheckerState->isUndefControlFlow(N) || - GraphPrintCheckerState->isBadDivide(N)) + GraphPrintCheckerState->isBadDivide(N) || + GraphPrintCheckerState->isUndefResult(N)) return "color=\"red\",style=\"filled\""; + if (GraphPrintCheckerState->isNoReturnCall(N)) + return "color=\"blue\",style=\"filled\""; + return ""; } @@ -1635,6 +1662,11 @@ struct VISIBILITY_HIDDEN DOTGraphTraits : else if (GraphPrintCheckerState->isBadDivide(N)) { Out << "\\|Divide-by zero or undefined value."; } + else if (GraphPrintCheckerState->isUndefResult(N)) { + Out << "\\|Result of operation is undefined."; + } + else if (GraphPrintCheckerState->isNoReturnCall(N)) + Out << "\\|Call to function marked \"noreturn\"."; break; } diff --git a/Analysis/GRSimpleVals.cpp b/Analysis/GRSimpleVals.cpp index 4ed80afe10..8b5e3c85a9 100644 --- a/Analysis/GRSimpleVals.cpp +++ b/Analysis/GRSimpleVals.cpp @@ -82,6 +82,11 @@ unsigned RunGRSimpleVals(CFG& cfg, FunctionDecl& FD, ASTContext& Ctx, CheckerState->bad_divides_begin(), CheckerState->bad_divides_end(), "Division by zero/undefined value."); + + EmitWarning(Diag, SrcMgr, + CheckerState->undef_results_begin(), + CheckerState->undef_results_end(), + "Result of operation is undefined."); #ifndef NDEBUG if (Visualize) CheckerState->ViewGraph(); diff --git a/Analysis/RValues.cpp b/Analysis/RValues.cpp index d7c4dac252..6369da4d7f 100644 --- a/Analysis/RValues.cpp +++ b/Analysis/RValues.cpp @@ -48,11 +48,16 @@ RVal::symbol_iterator RVal::symbol_end() const { // Transfer function dispatch for Non-LVals. //===----------------------------------------------------------------------===// -nonlval::ConcreteInt +RVal nonlval::ConcreteInt::EvalBinOp(ValueManager& ValMgr, BinaryOperator::Opcode Op, const nonlval::ConcreteInt& R) const { - return ValMgr.EvaluateAPSInt(Op, getValue(), R.getValue()); + const llvm::APSInt* X = ValMgr.EvaluateAPSInt(Op, getValue(), R.getValue()); + + if (X) + return nonlval::ConcreteInt(*X); + else + return UndefinedVal(); } @@ -76,14 +81,19 @@ nonlval::ConcreteInt::EvalMinus(ValueManager& ValMgr, UnaryOperator* U) const { // Transfer function dispatch for LVals. //===----------------------------------------------------------------------===// -lval::ConcreteInt +RVal lval::ConcreteInt::EvalBinOp(ValueManager& ValMgr, BinaryOperator::Opcode Op, const lval::ConcreteInt& R) const { assert (Op == BinaryOperator::Add || Op == BinaryOperator::Sub || (Op >= BinaryOperator::LT && Op <= BinaryOperator::NE)); - return ValMgr.EvaluateAPSInt(Op, getValue(), R.getValue()); + const llvm::APSInt* X = ValMgr.EvaluateAPSInt(Op, getValue(), R.getValue()); + + if (X) + return lval::ConcreteInt(*X); + else + return UndefinedVal(); } NonLVal LVal::EQ(ValueManager& ValMgr, const LVal& R) const { diff --git a/Analysis/ValueManager.cpp b/Analysis/ValueManager.cpp index 64f4b27d3f..2a8d23d02c 100644 --- a/Analysis/ValueManager.cpp +++ b/Analysis/ValueManager.cpp @@ -76,7 +76,7 @@ ValueManager::getConstraint(SymbolID sym, BinaryOperator::Opcode Op, return *C; } -const llvm::APSInt& +const llvm::APSInt* ValueManager::EvaluateAPSInt(BinaryOperator::Opcode Op, const llvm::APSInt& V1, const llvm::APSInt& V2) { @@ -85,53 +85,83 @@ ValueManager::EvaluateAPSInt(BinaryOperator::Opcode Op, assert (false && "Invalid Opcode."); case BinaryOperator::Mul: - return getValue( V1 * V2 ); + return &getValue( V1 * V2 ); case BinaryOperator::Div: - return getValue( V1 / V2 ); + return &getValue( V1 / V2 ); case BinaryOperator::Rem: - return getValue( V1 % V2 ); + return &getValue( V1 % V2 ); case BinaryOperator::Add: - return getValue( V1 + V2 ); + return &getValue( V1 + V2 ); case BinaryOperator::Sub: - return getValue( V1 - V2 ); + return &getValue( V1 - V2 ); - case BinaryOperator::Shl: - return getValue( V1.operator<<( (unsigned) V2.getZExtValue() )); + case BinaryOperator::Shl: { + + // FIXME: This logic should probably go higher up, where we can + // test these conditions symbolically. + + // FIXME: Expand these checks to include all undefined behavior. + + if (V2.isSigned() && V2.isNegative()) + return NULL; + + uint64_t Amt = V2.getZExtValue(); + + if (Amt > V1.getBitWidth()) + return NULL; + + return &getValue( V1.operator<<( (unsigned) Amt )); + } + + case BinaryOperator::Shr: { + + // FIXME: This logic should probably go higher up, where we can + // test these conditions symbolically. + + // FIXME: Expand these checks to include all undefined behavior. + + if (V2.isSigned() && V2.isNegative()) + return NULL; + + uint64_t Amt = V2.getZExtValue(); + + if (Amt > V1.getBitWidth()) + return NULL; - case BinaryOperator::Shr: - return getValue( V1.operator>>( (unsigned) V2.getZExtValue() )); + return &getValue( V1.operator>>( (unsigned) Amt )); + } case BinaryOperator::LT: - return getTruthValue( V1 < V2 ); + return &getTruthValue( V1 < V2 ); case BinaryOperator::GT: - return getTruthValue( V1 > V2 ); + return &getTruthValue( V1 > V2 ); case BinaryOperator::LE: - return getTruthValue( V1 <= V2 ); + return &getTruthValue( V1 <= V2 ); case BinaryOperator::GE: - return getTruthValue( V1 >= V2 ); + return &getTruthValue( V1 >= V2 ); case BinaryOperator::EQ: - return getTruthValue( V1 == V2 ); + return &getTruthValue( V1 == V2 ); case BinaryOperator::NE: - return getTruthValue( V1 != V2 ); + return &getTruthValue( V1 != V2 ); // Note: LAnd, LOr, Comma are handled specially by higher-level logic. case BinaryOperator::And: - return getValue( V1 & V2 ); + return &getValue( V1 & V2 ); case BinaryOperator::Or: - return getValue( V1 | V2 ); + return &getValue( V1 | V2 ); case BinaryOperator::Xor: - return getValue( V1 ^ V2 ); + return &getValue( V1 ^ V2 ); } } diff --git a/include/clang/Analysis/PathSensitive/GRExprEngine.h b/include/clang/Analysis/PathSensitive/GRExprEngine.h index f4984e1e5d..efccf02f6e 100644 --- a/include/clang/Analysis/PathSensitive/GRExprEngine.h +++ b/include/clang/Analysis/PathSensitive/GRExprEngine.h @@ -92,6 +92,8 @@ protected: typedef llvm::SmallPtrSet BadDerefTy; typedef llvm::SmallPtrSet BadDividesTy; typedef llvm::SmallPtrSet NoReturnCallsTy; + typedef llvm::SmallPtrSet UndefResultsTy; + /// UndefBranches - Nodes in the ExplodedGraph that result from /// taking a branch based on an undefined value. @@ -121,6 +123,10 @@ protected: /// a divide-by-zero or divide-by-undefined. BadDividesTy BadDivides; + /// UndefResults - Nodes in the ExplodedGraph where the operands are defined + /// by the result is not. Excludes divide-by-zero errors. + UndefResultsTy UndefResults; + bool StateCleaned; public: @@ -183,7 +189,11 @@ public: bool isNoReturnCall(const NodeTy* N) const { return N->isSink() && NoReturnCalls.count(const_cast(N)) != 0; } - + + bool isUndefResult(const NodeTy* N) const { + return N->isSink() && UndefResults.count(const_cast(N)) != 0; + } + typedef BadDerefTy::iterator null_deref_iterator; null_deref_iterator null_derefs_begin() { return ExplicitNullDeref.begin(); } null_deref_iterator null_derefs_end() { return ExplicitNullDeref.end(); } @@ -196,6 +206,10 @@ public: bad_divide_iterator bad_divides_begin() { return BadDivides.begin(); } bad_divide_iterator bad_divides_end() { return BadDivides.end(); } + typedef UndefResultsTy::iterator undef_result_iterator; + undef_result_iterator undef_results_begin() { return UndefResults.begin(); } + undef_result_iterator undef_results_end() { return UndefResults.end(); } + /// ProcessStmt - Called by GRCoreEngine. Used to generate new successor /// nodes by processing the 'effects' of a block-level statement. void ProcessStmt(Stmt* S, StmtNodeBuilder& builder); diff --git a/include/clang/Analysis/PathSensitive/RValues.h b/include/clang/Analysis/PathSensitive/RValues.h index ca94e4ddba..e2d07c0856 100644 --- a/include/clang/Analysis/PathSensitive/RValues.h +++ b/include/clang/Analysis/PathSensitive/RValues.h @@ -211,8 +211,8 @@ public: } // Transfer functions for binary/unary operations on ConcreteInts. - ConcreteInt EvalBinOp(ValueManager& ValMgr, BinaryOperator::Opcode Op, - const ConcreteInt& R) const; + RVal EvalBinOp(ValueManager& ValMgr, BinaryOperator::Opcode Op, + const ConcreteInt& R) const; ConcreteInt EvalComplement(ValueManager& ValMgr) const; @@ -341,9 +341,8 @@ public: } // Transfer functions for binary/unary operations on ConcreteInts. - ConcreteInt EvalBinOp(ValueManager& ValMgr, - BinaryOperator::Opcode Op, - const ConcreteInt& R) const; + RVal EvalBinOp(ValueManager& ValMgr, BinaryOperator::Opcode Op, + const ConcreteInt& R) const; // Implement isa support. static inline bool classof(const RVal* V) { diff --git a/include/clang/Analysis/PathSensitive/ValueManager.h b/include/clang/Analysis/PathSensitive/ValueManager.h index 29d392cda6..0b463fba3b 100644 --- a/include/clang/Analysis/PathSensitive/ValueManager.h +++ b/include/clang/Analysis/PathSensitive/ValueManager.h @@ -65,7 +65,7 @@ public: const SymIntConstraint& getConstraint(SymbolID sym, BinaryOperator::Opcode Op, const llvm::APSInt& V); - const llvm::APSInt& EvaluateAPSInt(BinaryOperator::Opcode Op, + const llvm::APSInt* EvaluateAPSInt(BinaryOperator::Opcode Op, const llvm::APSInt& V1, const llvm::APSInt& V2); };