danielmarjamaki updated this revision to Diff 103585. danielmarjamaki added a comment.
Fix review comments Repository: rL LLVM https://reviews.llvm.org/D30295 Files: include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h lib/StaticAnalyzer/Checkers/ConversionChecker.cpp lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp lib/StaticAnalyzer/Core/CheckerContext.cpp test/Analysis/bitwise-ops.c
Index: test/Analysis/bitwise-ops.c =================================================================== --- test/Analysis/bitwise-ops.c +++ test/Analysis/bitwise-ops.c @@ -22,11 +22,25 @@ case 1: return 0ULL << 63; // no-warning case 2: - return 0ULL << 64; // expected-warning{{The result of the '<<' expression is undefined}} + return 0ULL << 64; // expected-warning{{The result of the left shift is undefined due to shifting by 64, which is larger than the width of type 'unsigned long long'}} case 3: - return 0ULL << 65; // expected-warning{{The result of the '<<' expression is undefined}} + return 0ULL << 65; // expected-warning{{The result of the left shift is undefined due to shifting by 65, which is larger than the width of type 'unsigned long long'}} default: return 0; } -} \ No newline at end of file +} + +int testOverflowShift(int a) { + if (a == 323) { + return 1 << a; // expected-warning{{The result of the left shift is undefined due to shifting by 323, which is larger than the width of type 'int'}} + } + return 0; +} + +int testNegativeShift(int a) { + if (a == -5) { + return 1 << a; // expected-warning{{The result of the left shift is undefined because the right operand is negative}} + } + return 0; +} Index: lib/StaticAnalyzer/Core/CheckerContext.cpp =================================================================== --- lib/StaticAnalyzer/Core/CheckerContext.cpp +++ lib/StaticAnalyzer/Core/CheckerContext.cpp @@ -99,3 +99,35 @@ return Lexer::getSpelling(Loc, buf, getSourceManager(), getLangOpts()); } +/// Evaluate comparison and return true if it's known that condition is true +static bool evalComparison(SVal LHSVal, BinaryOperatorKind ComparisonOp, + SVal RHSVal, ProgramStateRef State) { + if (LHSVal.isUnknownOrUndef()) + return false; + ProgramStateManager &Mgr = State->getStateManager(); + if (!LHSVal.getAs<NonLoc>()) { + LHSVal = Mgr.getStoreManager().getBinding(State->getStore(), + LHSVal.castAs<Loc>()); + if (LHSVal.isUnknownOrUndef() || !LHSVal.getAs<NonLoc>()) + return false; + } + + SValBuilder &Bldr = Mgr.getSValBuilder(); + SVal Eval = Bldr.evalBinOp(State, ComparisonOp, LHSVal, RHSVal, + Bldr.getConditionType()); + if (Eval.isUnknownOrUndef()) + return false; + ProgramStateRef StTrue, StFalse; + std::tie(StTrue, StFalse) = State->assume(Eval.castAs<DefinedSVal>()); + return StTrue && !StFalse; +} + +bool CheckerContext::isGreaterOrEqual(const Expr *E, unsigned long long Val) { + DefinedSVal V = getSValBuilder().makeIntVal(Val, getASTContext().LongLongTy); + return evalComparison(getSVal(E), BO_GE, V, getState()); +} + +bool CheckerContext::isNegative(const Expr *E) { + DefinedSVal V = getSValBuilder().makeIntVal(0, false); + return evalComparison(getSVal(E), BO_LT, V, getState()); +} Index: lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp =================================================================== --- lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp +++ lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp @@ -59,6 +59,26 @@ return StOutBound && !StInBound; } +static bool isShiftOverflow(const BinaryOperator *B, CheckerContext &C) { + return C.isGreaterOrEqual( + B->getRHS(), C.getASTContext().getIntWidth(B->getLHS()->getType())); +} + +static const llvm::APSInt *getConcreteValue(const Expr *E, CheckerContext &C) { + SVal V = C.getSVal(E); + if (!V.getAs<NonLoc>()) { + ProgramStateRef State = C.getState(); + ProgramStateManager &Mgr = State->getStateManager(); + V = Mgr.getStoreManager().getBinding(State->getStore(), V.castAs<Loc>()); + } + if (V.getSubKind() == nonloc::ConcreteIntKind) { + const auto &CI = V.castAs<NonLoc>().castAs<nonloc::ConcreteInt>(); + const llvm::APSInt &I = CI.getValue(); + return &I; + } + return nullptr; +} + void UndefResultChecker::checkPostStmt(const BinaryOperator *B, CheckerContext &C) const { ProgramStateRef state = C.getState(); @@ -97,18 +117,44 @@ } if (Ex) { - OS << "The " << (isLeft ? "left" : "right") - << " operand of '" + OS << "The " << (isLeft ? "left" : "right") << " operand of '" << BinaryOperator::getOpcodeStr(B->getOpcode()) << "' is a garbage value"; if (isArrayIndexOutOfBounds(C, Ex)) OS << " due to array index out of bounds"; - } - else { + } else { // Neither operand was undefined, but the result is undefined. - OS << "The result of the '" - << BinaryOperator::getOpcodeStr(B->getOpcode()) - << "' expression is undefined"; + if ((B->getOpcode() == BinaryOperatorKind::BO_Shl || + B->getOpcode() == BinaryOperatorKind::BO_Shr) && + C.isNegative(B->getRHS())) { + OS << "The result of the " + << ((B->getOpcode() == BinaryOperatorKind::BO_Shl) ? "left" + : "right") + << " shift is undefined because the right operand is negative"; + } else if ((B->getOpcode() == BinaryOperatorKind::BO_Shl || + B->getOpcode() == BinaryOperatorKind::BO_Shr) && + isShiftOverflow(B, C)) { + + OS << "The result of the " + << ((B->getOpcode() == BinaryOperatorKind::BO_Shl) ? "left" + : "right") + << " shift is undefined due to shifting by "; + + const llvm::APSInt *I = getConcreteValue(B->getRHS(), C); + if (!I) + OS << "a value that is"; + else if (I->isUnsigned()) + OS << I->getZExtValue() << ", which is"; + else + OS << I->getSExtValue() << ", which is"; + + OS << " larger than the width of type '" + << B->getLHS()->getType().getAsString() << "'."; + } else { + OS << "The result of the '" + << BinaryOperator::getOpcodeStr(B->getOpcode()) + << "' expression is undefined"; + } } auto report = llvm::make_unique<BugReport>(*BT, OS.str(), N); if (Ex) { Index: lib/StaticAnalyzer/Checkers/ConversionChecker.cpp =================================================================== --- lib/StaticAnalyzer/Checkers/ConversionChecker.cpp +++ lib/StaticAnalyzer/Checkers/ConversionChecker.cpp @@ -123,57 +123,6 @@ C.emitReport(std::move(R)); } -// Is E value greater or equal than Val? -static bool isGreaterEqual(CheckerContext &C, const Expr *E, - unsigned long long Val) { - ProgramStateRef State = C.getState(); - SVal EVal = C.getSVal(E); - if (EVal.isUnknownOrUndef()) - return false; - if (!EVal.getAs<NonLoc>() && EVal.getAs<Loc>()) { - ProgramStateManager &Mgr = C.getStateManager(); - EVal = - Mgr.getStoreManager().getBinding(State->getStore(), EVal.castAs<Loc>()); - } - if (EVal.isUnknownOrUndef() || !EVal.getAs<NonLoc>()) - return false; - - SValBuilder &Bldr = C.getSValBuilder(); - DefinedSVal V = Bldr.makeIntVal(Val, C.getASTContext().LongLongTy); - - // Is DefinedEVal greater or equal with V? - SVal GE = Bldr.evalBinOp(State, BO_GE, EVal, V, Bldr.getConditionType()); - if (GE.isUnknownOrUndef()) - return false; - ConstraintManager &CM = C.getConstraintManager(); - ProgramStateRef StGE, StLT; - std::tie(StGE, StLT) = CM.assumeDual(State, GE.castAs<DefinedSVal>()); - return StGE && !StLT; -} - -// Is E value negative? -static bool isNegative(CheckerContext &C, const Expr *E) { - ProgramStateRef State = C.getState(); - SVal EVal = State->getSVal(E, C.getLocationContext()); - if (EVal.isUnknownOrUndef() || !EVal.getAs<NonLoc>()) - return false; - DefinedSVal DefinedEVal = EVal.castAs<DefinedSVal>(); - - SValBuilder &Bldr = C.getSValBuilder(); - DefinedSVal V = Bldr.makeIntVal(0, false); - - SVal LT = - Bldr.evalBinOp(State, BO_LT, DefinedEVal, V, Bldr.getConditionType()); - - // Is E value greater than MaxVal? - ConstraintManager &CM = C.getConstraintManager(); - ProgramStateRef StNegative, StPositive; - std::tie(StNegative, StPositive) = - CM.assumeDual(State, LT.castAs<DefinedSVal>()); - - return StNegative && !StPositive; -} - bool ConversionChecker::isLossOfPrecision(const ImplicitCastExpr *Cast, QualType DestType, CheckerContext &C) const { @@ -195,18 +144,18 @@ return false; unsigned long long MaxVal = 1ULL << W; - return isGreaterEqual(C, Cast->getSubExpr(), MaxVal); + return C.isGreaterOrEqual(Cast->getSubExpr(), MaxVal); } bool ConversionChecker::isLossOfSign(const ImplicitCastExpr *Cast, - CheckerContext &C) const { + CheckerContext &C) const { QualType CastType = Cast->getType(); QualType SubType = Cast->IgnoreParenImpCasts()->getType(); if (!CastType->isUnsignedIntegerType() || !SubType->isSignedIntegerType()) return false; - return isNegative(C, Cast->getSubExpr()); + return C.isNegative(Cast->getSubExpr()); } void ento::registerConversionChecker(CheckerManager &mgr) { Index: include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h =================================================================== --- include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h +++ include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h @@ -196,6 +196,13 @@ return getState()->getSVal(S, getLocationContext()); } + /// \brief Is value of expression greater or equal to value (unsigned + /// comparison)? + bool isGreaterOrEqual(const Expr *E, unsigned long long Val); + + /// \brief Is value of expression negative? + bool isNegative(const Expr *E); + /// \brief Generates a new transition in the program state graph /// (ExplodedGraph). Uses the default CheckerContext predecessor node. ///
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits