[PATCH] D30295: [analyzer] clarify undef shift result when shift count is negative or exceeds the bit width
This revision was automatically updated to reflect the committed changes.
Closed by commit rL315462: [Analyzer] Clarify error messages for undefined
result (authored by danielmarjamaki).
Changed prior to commit:
https://reviews.llvm.org/D30295?vs=116865&id=118620#toc
Repository:
rL LLVM
https://reviews.llvm.org/D30295
Files:
cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h
cfe/trunk/lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Core/CheckerContext.cpp
cfe/trunk/test/Analysis/bitwise-ops.c
Index: cfe/trunk/lib/StaticAnalyzer/Core/CheckerContext.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Core/CheckerContext.cpp
+++ cfe/trunk/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()) {
+LHSVal = Mgr.getStoreManager().getBinding(State->getStore(),
+ LHSVal.castAs());
+if (LHSVal.isUnknownOrUndef() || !LHSVal.getAs())
+ 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());
+ 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: cfe/trunk/lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
+++ cfe/trunk/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() && EVal.getAs()) {
-ProgramStateManager &Mgr = C.getStateManager();
-EVal =
-Mgr.getStoreManager().getBinding(State->getStore(), EVal.castAs());
- }
- if (EVal.isUnknownOrUndef() || !EVal.getAs())
-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());
- 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())
-return false;
- DefinedSVal DefinedEVal = EVal.castAs();
-
- 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());
-
- 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();
Q
[PATCH] D30295: [analyzer] clarify undef shift result when shift count is negative or exceeds the bit width
zaks.anna accepted this revision. zaks.anna added a comment. Once the comments by @paquette are addressed, LGTM. Thanks! Comment at: lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp:138 + +OS << " larger or equal to the width of type '" + << B->getLHS()->getType().getAsString() << "'."; paquette wrote: > Maybe "greater than or equal to" instead of "larger or equal to" just for > convention? I hear/read that more often, so seeing "larger" is a little weird. > > Minor point though, so if it makes the message too long it doesn't matter. I agree that "greater than or equal to" is better, so let's change to that. Repository: rL LLVM https://reviews.llvm.org/D30295 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30295: [analyzer] clarify undef shift result when shift count is negative or exceeds the bit width
paquette added inline comments. Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h:201 + /// comparison)? + bool isGreaterOrEqual(const Expr *E, unsigned long long Val); + Maybe something like ``` /// Returns true if the value of \p E is greater than or equal to \p Val under unsigned comparison. ``` Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h:203 + + /// \brief Is value of expression negative? + bool isNegative(const Expr *E); This shouldn't need a \brief, since it's a single line comment. It could also be something like ``` /// Returns true if the value of \p E is negative. ``` Comment at: lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp:138 + +OS << " larger or equal to the width of type '" + << B->getLHS()->getType().getAsString() << "'."; Maybe "greater than or equal to" instead of "larger or equal to" just for convention? I hear/read that more often, so seeing "larger" is a little weird. Minor point though, so if it makes the message too long it doesn't matter. Repository: rL LLVM https://reviews.llvm.org/D30295 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30295: [analyzer] clarify undef shift result when shift count is negative or exceeds the bit width
danielmarjamaki added a comment. ping Repository: rL LLVM https://reviews.llvm.org/D30295 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30295: [analyzer] clarify undef shift result when shift count is negative or exceeds the bit width
danielmarjamaki updated this revision to Diff 116865.
danielmarjamaki added a comment.
fixed 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 or equal to 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 or equal to 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 or equal to 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()) {
+LHSVal = Mgr.getStoreManager().getBinding(State->getStore(),
+ LHSVal.castAs());
+if (LHSVal.isUnknownOrUndef() || !LHSVal.getAs())
+ 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());
+ 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,11 @@
return StOutBound && !StInBound;
}
+static bool isShiftOverflow(const BinaryOperator *B, CheckerContext &C) {
+ return C.isGreaterOrEqual(
+ B->getRHS(), C.getASTContext().getIntWidth(B->getLHS()->getType()));
+}
+
void UndefResultChecker::checkPostStmt(const BinaryOperator *B,
CheckerContext &C) const {
ProgramStateRef state = C.getState();
@@ -97,18 +102,46 @@
}
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")
+ << " shi
[PATCH] D30295: [analyzer] clarify undef shift result when shift count is negative or exceeds the bit width
zaks.anna added inline comments. Comment at: lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp:134 +else if (I->isUnsigned()) + OS << I->getZExtValue() << ", which is"; +else Please print single quotes around the value. Comment at: lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp:138 + +OS << " larger or equal with the width of type '" + << B->getLHS()->getType().getAsString() << "'."; "equal with the width" -> "equal to the width" Repository: rL LLVM https://reviews.llvm.org/D30295 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30295: [analyzer] clarify undef shift result when shift count is negative or exceeds the bit width
zaks.anna added a comment. Sorry for the wait! Repository: rL LLVM https://reviews.llvm.org/D30295 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30295: [analyzer] clarify undef shift result when shift count is negative or exceeds the bit width
danielmarjamaki added a comment. ping Repository: rL LLVM https://reviews.llvm.org/D30295 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30295: [analyzer] clarify undef shift result when shift count is negative or exceeds the bit width
danielmarjamaki added a comment. ping Repository: rL LLVM https://reviews.llvm.org/D30295 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30295: [analyzer] clarify undef shift result when shift count is negative or exceeds the bit width
danielmarjamaki added a comment. ping Repository: rL LLVM https://reviews.llvm.org/D30295 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30295: [analyzer] clarify undef shift result when shift count is negative or exceeds the bit width
danielmarjamaki added a comment. ping Repository: rL LLVM https://reviews.llvm.org/D30295 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30295: [analyzer] clarify undef shift result when shift count is negative or exceeds the bit width
danielmarjamaki added a comment. ping Repository: rL LLVM https://reviews.llvm.org/D30295 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30295: [analyzer] clarify undef shift result when shift count is negative or exceeds the bit width
xazax.hun accepted this revision. xazax.hun added a comment. This revision is now accepted and ready to land. It looks good to me but let's wait for Anna, NoQ, or Devin for the final word. Repository: rL LLVM https://reviews.llvm.org/D30295 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30295: [analyzer] clarify undef shift result when shift count is negative or exceeds the bit width
danielmarjamaki updated this revision to Diff 109590.
danielmarjamaki added a comment.
Cleaned up the patch a little. Thanks Gabor for telling me about
SValBuilder::getKnownValue()
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 or equal with 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 or equal with 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 or equal with 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()) {
+LHSVal = Mgr.getStoreManager().getBinding(State->getStore(),
+ LHSVal.castAs());
+if (LHSVal.isUnknownOrUndef() || !LHSVal.getAs())
+ 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());
+ 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,11 @@
return StOutBound && !StInBound;
}
+static bool isShiftOverflow(const BinaryOperator *B, CheckerContext &C) {
+ return C.isGreaterOrEqual(
+ B->getRHS(), C.getASTContext().getIntWidth(B->getLHS()->getType()));
+}
+
void UndefResultChecker::checkPostStmt(const BinaryOperator *B,
CheckerContext &C) const {
ProgramStateRef state = C.getState();
@@ -97,18 +102,46 @@
}
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"
+
[PATCH] D30295: [analyzer] clarify undef shift result when shift count is negative or exceeds the bit width
xazax.hun added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp:126
+ << BinaryOperator::getOpcodeStr(B->getOpcode())
+ << "' expression is undefined due to shift count >= width of type";
+ } else {
danielmarjamaki wrote:
> zaks.anna wrote:
> > It's best not to use ">=" in diagnostic messages.
> > Suggestions: "due to shift count >= width of type" ->
> > - "due to shifting by a value larger than the width of type"
> > - "due to shifting by 5, which is larger than the width of type 'int'" //
> > Providing the exact value and the type would be very useful and this
> > information is readily available to us. Note that the users might not see
> > the type or the value because of macros and such.
> I used "due to shifting by 5, which is larger than the width of type 'int'"
>
> However I did not see an easy way to show the exact value. So I added
> getConcreteValue(). Maybe you have a better suggestion. If it's a ConcreteInt
> I show the exact value, but if it's some range etc then I write "due to
> shifting by a value that is larger..." instead.
>
> The message "due to shifting by 64, which is larger than the width of type
> 'unsigned long long'" is a bit weird imho. Because 64 is not larger than the
> width. Not sure how this can be rephrazed better though.
SValBuilder has a getKnownValue, does that help?
Repository:
rL LLVM
https://reviews.llvm.org/D30295
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30295: [analyzer] clarify undef shift result when shift count is negative or exceeds the bit width
danielmarjamaki marked 4 inline comments as done.
danielmarjamaki added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp:126
+ << BinaryOperator::getOpcodeStr(B->getOpcode())
+ << "' expression is undefined due to shift count >= width of type";
+ } else {
zaks.anna wrote:
> It's best not to use ">=" in diagnostic messages.
> Suggestions: "due to shift count >= width of type" ->
> - "due to shifting by a value larger than the width of type"
> - "due to shifting by 5, which is larger than the width of type 'int'" //
> Providing the exact value and the type would be very useful and this
> information is readily available to us. Note that the users might not see the
> type or the value because of macros and such.
I used "due to shifting by 5, which is larger than the width of type 'int'"
However I did not see an easy way to show the exact value. So I added
getConcreteValue(). Maybe you have a better suggestion. If it's a ConcreteInt I
show the exact value, but if it's some range etc then I write "due to shifting
by a value that is larger..." instead.
The message "due to shifting by 64, which is larger than the width of type
'unsigned long long'" is a bit weird imho. Because 64 is not larger than the
width. Not sure how this can be rephrazed better though.
Repository:
rL LLVM
https://reviews.llvm.org/D30295
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30295: [analyzer] clarify undef shift result when shift count is negative or exceeds the bit width
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()) {
+LHSVal = Mgr.getStoreManager().getBinding(State->getStore(),
+ LHSVal.castAs());
+if (LHSVal.isUnknownOrUndef() || !LHSVal.getAs())
+ 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());
+ 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()) {
+ProgramStateRef State = C.getState();
+ProgramStateManager &Mgr = State->getStateManager();
+V = Mgr.getStoreManager().getBinding(State->getStore(), V.castAs());
+ }
+ if (V.getSubKind() == nonloc::ConcreteIntKind) {
+const auto &CI = V.castAs().castAs();
+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 '"
[PATCH] D30295: [analyzer] clarify undef shift result when shift count is negative or exceeds the bit width
zaks.anna added a comment.
These are great additions!
Going back to my comment about adding these to CheckerContext, I think we
should be adding helper functions as methods on CheckerContext as it is **the
primary place where checker writers look for helpers**. Two of the three
methods added take CheckerContext as an argument, so what is the reason for
adding them elsewhere?
As for the svalComparesTo method, if we want to make it available to the two
callbacks that do not have checker context, we can include a method on checker
context that calls into a helper in CheckerHelpers.h. However, given that even
this patch is not using the function, I would argue for leaving it as a helper
function internal to CheckerContext.cpp.
Comment at: lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp:119
+ << BinaryOperator::getOpcodeStr(B->getOpcode())
+ << "' expression is undefined due to negative value on the right "
+ "side of operand";
"right side of operand" does not sound right..
How about:
"The result of the '<<' expression is undefined due to negative value on the
right side of operand"
->
"The result of the left shift is undefined because the right operand is
negative"
or
"The result of the '<<' expression is undefined because the right operand is
negative"
Comment at: lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp:126
+ << BinaryOperator::getOpcodeStr(B->getOpcode())
+ << "' expression is undefined due to shift count >= width of type";
+ } else {
It's best not to use ">=" in diagnostic messages.
Suggestions: "due to shift count >= width of type" ->
- "due to shifting by a value larger than the width of type"
- "due to shifting by 5, which is larger than the width of type 'int'" //
Providing the exact value and the type would be very useful and this
information is readily available to us. Note that the users might not see the
type or the value because of macros and such.
Comment at: lib/StaticAnalyzer/Core/CheckerHelpers.cpp:98
+
+bool clang::ento::svalComparesTo(SVal LHSVal, BinaryOperatorKind ComparisonOp,
+ SVal RHSVal, ProgramStateRef State) {
How about evalComparison as a name for this?
Comment at: lib/StaticAnalyzer/Core/CheckerHelpers.cpp:121
+
+// Is E value greater or equal than Val?
+bool clang::ento::isGreaterOrEqual(const Expr *E, unsigned long long Val,
Please, use doxygen comment style here and below.
Repository:
rL LLVM
https://reviews.llvm.org/D30295
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30295: [analyzer] clarify undef shift result when shift count is negative or exceeds the bit width
NoQ added a comment.
I have just one comment and i think it'd be good to land.
Comment at: lib/StaticAnalyzer/Core/CheckerHelpers.cpp:104
+ ProgramStateManager &Mgr = State->getStateManager();
+ if (!LHSVal.getAs() && LHSVal.getAs()) {
+LHSVal = Mgr.getStoreManager().getBinding(State->getStore(),
Every `SVal` is either `Unknown` or `Undefined` or `NonLoc` or `Loc`, so the
check after `&&` is unnecessary.
Also, i believe that it'd be more correct to look at the AST's
`Expr::isLValue()` (in the caller function, where the expression is still
available) instead of looking at the `SVal` type here. These approaches are
significantly different: you need to discriminate between pointer-type rvalues
and integer-type lvalues, both of which are represented as `Loc` values; in the
former case, we shouldn't blindly get the binding. I've seen these incorrectly
discriminated-between in multiple places in the analyzer, and i believe we
should fix this eventually.
Repository:
rL LLVM
https://reviews.llvm.org/D30295
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30295: [analyzer] clarify undef shift result when shift count is negative or exceeds the bit width
danielmarjamaki added a comment. ping Repository: rL LLVM https://reviews.llvm.org/D30295 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30295: [analyzer] clarify undef shift result when shift count is negative or exceeds the bit width
danielmarjamaki updated this revision to Diff 99022.
danielmarjamaki added a comment.
renamed exprComparesTo to svalComparesTo
Repository:
rL LLVM
https://reviews.llvm.org/D30295
Files:
include/clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h
lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
lib/StaticAnalyzer/Core/CheckerHelpers.cpp
test/Analysis/bitwise-ops.c
Index: test/Analysis/bitwise-ops.c
===
--- test/Analysis/bitwise-ops.c
+++ test/Analysis/bitwise-ops.c
@@ -29,4 +29,18 @@
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 '<<' expression is undefined due to shift count >= width of type}}
+ }
+ return 0;
+}
+
+int testNegativeShift(int a) {
+ if (a == -5) {
+return 1 << a; // expected-warning{{The result of the '<<' expression is undefined due to negative value on the right side of operand}}
+ }
+ return 0;
+}
Index: lib/StaticAnalyzer/Core/CheckerHelpers.cpp
===
--- lib/StaticAnalyzer/Core/CheckerHelpers.cpp
+++ lib/StaticAnalyzer/Core/CheckerHelpers.cpp
@@ -94,3 +94,40 @@
return std::make_pair(VD, RHS);
}
+
+bool clang::ento::svalComparesTo(SVal LHSVal, BinaryOperatorKind ComparisonOp,
+ SVal RHSVal, ProgramStateRef State) {
+
+ if (LHSVal.isUnknownOrUndef())
+return false;
+ ProgramStateManager &Mgr = State->getStateManager();
+ if (!LHSVal.getAs() && LHSVal.getAs()) {
+LHSVal = Mgr.getStoreManager().getBinding(State->getStore(),
+ LHSVal.castAs());
+ }
+ if (LHSVal.isUnknownOrUndef() || !LHSVal.getAs())
+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());
+ return StTrue && !StFalse;
+}
+
+// Is E value greater or equal than Val?
+bool clang::ento::isGreaterOrEqual(const Expr *E, unsigned long long Val,
+ CheckerContext &C) {
+ DefinedSVal V =
+ C.getSValBuilder().makeIntVal(Val, C.getASTContext().LongLongTy);
+ return svalComparesTo(C.getSVal(E), BO_GE, V, C.getState());
+}
+
+// Is E value negative?
+bool clang::ento::isNegative(const Expr *E, CheckerContext &C) {
+ DefinedSVal V = C.getSValBuilder().makeIntVal(0, false);
+ return svalComparesTo(C.getSVal(E), BO_LT, V, C.getState());
+}
Index: lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
+++ lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
@@ -17,6 +17,7 @@
#include "clang/StaticAnalyzer/Core/Checker.h"
#include "clang/StaticAnalyzer/Core/CheckerManager.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h"
#include "llvm/ADT/SmallString.h"
#include "llvm/Support/raw_ostream.h"
@@ -59,6 +60,11 @@
return StOutBound && !StInBound;
}
+static bool isShiftOverflow(const BinaryOperator *B, CheckerContext &C) {
+ return isGreaterOrEqual(
+ B->getRHS(), C.getASTContext().getIntWidth(B->getLHS()->getType()), C);
+}
+
void UndefResultChecker::checkPostStmt(const BinaryOperator *B,
CheckerContext &C) const {
ProgramStateRef state = C.getState();
@@ -103,12 +109,26 @@
<< "' 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) &&
+ isNegative(B->getRHS(), C)) {
+OS << "The result of the '"
+ << BinaryOperator::getOpcodeStr(B->getOpcode())
+ << "' expression is undefined due to negative value on the right "
+ "side of operand";
+ } else if ((B->getOpcode() == BinaryOperatorKind::BO_Shl ||
+ B->getOpcode() == BinaryOperatorKind::BO_Shr) &&
+ isShiftOverflow(B, C)) {
+OS << "The result of the '"
+ << BinaryOperator::getOpcodeStr(B->getOpcode())
+ << "' expression is undefined due to shift count >= width of type";
+ } else {
+
[PATCH] D30295: [analyzer] clarify undef shift result when shift count is negative or exceeds the bit width
danielmarjamaki updated this revision to Diff 98970.
danielmarjamaki added a comment.
minor tweak
Repository:
rL LLVM
https://reviews.llvm.org/D30295
Files:
include/clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h
lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
lib/StaticAnalyzer/Core/CheckerHelpers.cpp
test/Analysis/bitwise-ops.c
Index: test/Analysis/bitwise-ops.c
===
--- test/Analysis/bitwise-ops.c
+++ test/Analysis/bitwise-ops.c
@@ -29,4 +29,18 @@
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 '<<' expression is undefined due to shift count >= width of type}}
+ }
+ return 0;
+}
+
+int testNegativeShift(int a) {
+ if (a == -5) {
+return 1 << a; // expected-warning{{The result of the '<<' expression is undefined due to negative value on the right side of operand}}
+ }
+ return 0;
+}
Index: lib/StaticAnalyzer/Core/CheckerHelpers.cpp
===
--- lib/StaticAnalyzer/Core/CheckerHelpers.cpp
+++ lib/StaticAnalyzer/Core/CheckerHelpers.cpp
@@ -94,3 +94,40 @@
return std::make_pair(VD, RHS);
}
+
+bool clang::ento::exprComparesTo(SVal LHSVal, BinaryOperatorKind ComparisonOp,
+ SVal RHSVal, ProgramStateRef State) {
+
+ if (LHSVal.isUnknownOrUndef())
+return false;
+ ProgramStateManager &Mgr = State->getStateManager();
+ if (!LHSVal.getAs() && LHSVal.getAs()) {
+LHSVal = Mgr.getStoreManager().getBinding(State->getStore(),
+ LHSVal.castAs());
+ }
+ if (LHSVal.isUnknownOrUndef() || !LHSVal.getAs())
+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());
+ return StTrue && !StFalse;
+}
+
+// Is E value greater or equal than Val?
+bool clang::ento::isGreaterOrEqual(const Expr *E, unsigned long long Val,
+ CheckerContext &C) {
+ DefinedSVal V =
+ C.getSValBuilder().makeIntVal(Val, C.getASTContext().LongLongTy);
+ return exprComparesTo(C.getSVal(E), BO_GE, V, C.getState());
+}
+
+// Is E value negative?
+bool clang::ento::isNegative(const Expr *E, CheckerContext &C) {
+ DefinedSVal V = C.getSValBuilder().makeIntVal(0, false);
+ return exprComparesTo(C.getSVal(E), BO_LT, V, C.getState());
+}
Index: lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
+++ lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
@@ -17,6 +17,7 @@
#include "clang/StaticAnalyzer/Core/Checker.h"
#include "clang/StaticAnalyzer/Core/CheckerManager.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h"
#include "llvm/ADT/SmallString.h"
#include "llvm/Support/raw_ostream.h"
@@ -59,6 +60,11 @@
return StOutBound && !StInBound;
}
+static bool isShiftOverflow(const BinaryOperator *B, CheckerContext &C) {
+ return isGreaterOrEqual(
+ B->getRHS(), C.getASTContext().getIntWidth(B->getLHS()->getType()), C);
+}
+
void UndefResultChecker::checkPostStmt(const BinaryOperator *B,
CheckerContext &C) const {
ProgramStateRef state = C.getState();
@@ -106,9 +112,24 @@
}
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) &&
+ isNegative(B->getRHS(), C)) {
+OS << "The result of the '"
+ << BinaryOperator::getOpcodeStr(B->getOpcode())
+ << "' expression is undefined due to negative value on the right "
+ "side of operand";
+ } else if ((B->getOpcode() == BinaryOperatorKind::BO_Shl ||
+ B->getOpcode() == BinaryOperatorKind::BO_Shr) &&
+ isShiftOverflow(B, C)) {
+OS << "The result of the '"
+ << BinaryOperator::getOpcodeStr(B->getOpcode())
+ << "' expression is undefined due to shift count >= width of type";
+ } else {
+OS << "The result of the '"
+ << BinaryOperator::getOpcodeStr(B->getOpcode())
+ << "' expression is undefined";
+ }
}
auto report = llvm::mak
[PATCH] D30295: [analyzer] clarify undef shift result when shift count is negative or exceeds the bit width
xazax.hun added inline comments. Comment at: lib/StaticAnalyzer/Core/CheckerHelpers.cpp:116 +return false; + ConstraintManager &CM = State->getConstraintManager(); + ProgramStateRef StTrue, StFalse; danielmarjamaki wrote: > xazax.hun wrote: > > Any reason why do you get the constraint manager and not using > > ProgramState::assume? > Mostly that it's just 1 call instead of 2. assumeDual() has some extra logic > (early return , assertion). are there some special reason to use assume()? Basically it would be a bit shorter, it also calls AssumeDual in the background. See https://clang.llvm.org/doxygen/ProgramState_8h_source.html#l00652 Repository: rL LLVM https://reviews.llvm.org/D30295 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30295: [analyzer] clarify undef shift result when shift count is negative or exceeds the bit width
danielmarjamaki added inline comments. Comment at: lib/StaticAnalyzer/Core/CheckerHelpers.cpp:116 +return false; + ConstraintManager &CM = State->getConstraintManager(); + ProgramStateRef StTrue, StFalse; xazax.hun wrote: > Any reason why do you get the constraint manager and not using > ProgramState::assume? Mostly that it's just 1 call instead of 2. assumeDual() has some extra logic (early return , assertion). are there some special reason to use assume()? Repository: rL LLVM https://reviews.llvm.org/D30295 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30295: [analyzer] clarify undef shift result when shift count is negative or exceeds the bit width
xazax.hun added a comment. I only found two nits otherwise looks good to me. Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h:46 +bool exprComparesTo(SVal LHSVal, BinaryOperatorKind ComparisonOp, SVal RHSVal, +ProgramStateRef State); Right now the name of this function is exprComparesTo, but none of its arguments are expressions. You should either rename it to svalComparesTo, or use expr as its arguments. Comment at: lib/StaticAnalyzer/Core/CheckerHelpers.cpp:116 +return false; + ConstraintManager &CM = State->getConstraintManager(); + ProgramStateRef StTrue, StFalse; Any reason why do you get the constraint manager and not using ProgramState::assume? Repository: rL LLVM https://reviews.llvm.org/D30295 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30295: [analyzer] clarify undef shift result when shift count is negative or exceeds the bit width
danielmarjamaki marked an inline comment as done. danielmarjamaki added a comment. Ping. Repository: rL LLVM https://reviews.llvm.org/D30295 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30295: [analyzer] clarify undef shift result when shift count is negative or exceeds the bit width
danielmarjamaki marked 4 inline comments as done. danielmarjamaki added inline comments. Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h:46 -} // end GR namespace +bool isExprResultConformsComparisonRule(CheckerContext &C, +BinaryOperatorKind CompRule, zaks.anna wrote: > NoQ wrote: > > Because you are making these functions public, i think it's worth it to > > make it obvious what they do without looking at the particular checker > > code. Comments are definitely worth it. I think function names could be > > worded better; i suggest `exprComparesTo(const Expr *LHSExpr, > > BinaryOperatorKind ComparisonOp, SVal RHSVal, CheckerContext &C);` and > > `isGreaterOrEqual(...)`; i'm personally fond of having CheckerContext at > > the back because that's where we have it in checker callbacks, but that's > > not important. > + 1 on Artem's comment of making the names more clear and providing > documentation. Also, should these be part of CheckerContext? > Also, should these be part of CheckerContext? I chose not to. Then as NoQ suggested it's not possible to use this when CheckerContext is not available: "The good thing about requiring only State is that we'd be able to use these functions in checker callbacks that don't provide the CheckerContext object, eg. checkEndAnalysis or checkRegionChanges." Repository: rL LLVM https://reviews.llvm.org/D30295 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30295: [analyzer] clarify undef shift result when shift count is negative or exceeds the bit width
danielmarjamaki updated this revision to Diff 95740.
danielmarjamaki added a comment.
Fix review comments
- renamed
- reorder function arguments (CheckerContext last)
Repository:
rL LLVM
https://reviews.llvm.org/D30295
Files:
include/clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h
lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
lib/StaticAnalyzer/Core/CheckerHelpers.cpp
test/Analysis/bitwise-ops.c
Index: test/Analysis/bitwise-ops.c
===
--- test/Analysis/bitwise-ops.c
+++ test/Analysis/bitwise-ops.c
@@ -29,4 +29,18 @@
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 '<<' expression is undefined due to shift count >= width of type}}
+ }
+ return 0;
+}
+
+int testNegativeShift(int a) {
+ if (a == -5) {
+return 1 << a; // expected-warning{{The result of the '<<' expression is undefined due to negative value on the right side of operand}}
+ }
+ return 0;
+}
Index: lib/StaticAnalyzer/Core/CheckerHelpers.cpp
===
--- lib/StaticAnalyzer/Core/CheckerHelpers.cpp
+++ lib/StaticAnalyzer/Core/CheckerHelpers.cpp
@@ -94,3 +94,41 @@
return std::make_pair(VD, RHS);
}
+
+bool clang::ento::exprComparesTo(SVal LHSVal, BinaryOperatorKind ComparisonOp,
+ SVal RHSVal, ProgramStateRef State) {
+
+ if (LHSVal.isUnknownOrUndef())
+return false;
+ ProgramStateManager &Mgr = State->getStateManager();
+ if (!LHSVal.getAs() && LHSVal.getAs()) {
+LHSVal = Mgr.getStoreManager().getBinding(State->getStore(),
+ LHSVal.castAs());
+ }
+ if (LHSVal.isUnknownOrUndef() || !LHSVal.getAs())
+return false;
+
+ SValBuilder &Bldr = Mgr.getSValBuilder();
+ SVal Eval = Bldr.evalBinOp(State, ComparisonOp, LHSVal, RHSVal,
+ Bldr.getConditionType());
+ if (Eval.isUnknownOrUndef())
+return false;
+ ConstraintManager &CM = State->getConstraintManager();
+ ProgramStateRef StTrue, StFalse;
+ std::tie(StTrue, StFalse) = CM.assumeDual(State, Eval.castAs());
+ return StTrue && !StFalse;
+}
+
+// Is E value greater or equal than Val?
+bool clang::ento::isGreaterOrEqual(const Expr *E, unsigned long long Val,
+ CheckerContext &C) {
+ DefinedSVal V =
+ C.getSValBuilder().makeIntVal(Val, C.getASTContext().LongLongTy);
+ return exprComparesTo(C.getSVal(E), BO_GE, V, C.getState());
+}
+
+// Is E value negative?
+bool clang::ento::isNegative(const Expr *E, CheckerContext &C) {
+ DefinedSVal V = C.getSValBuilder().makeIntVal(0, false);
+ return exprComparesTo(C.getSVal(E), BO_LT, V, C.getState());
+}
Index: lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
+++ lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
@@ -17,6 +17,7 @@
#include "clang/StaticAnalyzer/Core/Checker.h"
#include "clang/StaticAnalyzer/Core/CheckerManager.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h"
#include "llvm/ADT/SmallString.h"
#include "llvm/Support/raw_ostream.h"
@@ -59,6 +60,11 @@
return StOutBound && !StInBound;
}
+static bool isShiftOverflow(const BinaryOperator *B, CheckerContext &C) {
+ return isGreaterOrEqual(
+ B->getRHS(), C.getASTContext().getIntWidth(B->getLHS()->getType()), C);
+}
+
void UndefResultChecker::checkPostStmt(const BinaryOperator *B,
CheckerContext &C) const {
ProgramStateRef state = C.getState();
@@ -106,9 +112,24 @@
}
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) &&
+ isNegative(B->getRHS(), C)) {
+OS << "The result of the '"
+ << BinaryOperator::getOpcodeStr(B->getOpcode())
+ << "' expression is undefined due to negative value on the right "
+ "side of operand";
+ } else if ((B->getOpcode() == BinaryOperatorKind::BO_Shl ||
+ B->getOpcode() == BinaryOperatorKind::BO_Shr) &&
+ isShiftOverflow(B, C)) {
+OS << "The result of the '"
+ << BinaryOperator::getOpcodeStr(B->getOpcode())
+ << "' expression is undefined due to shift count >= width of type";
+ } else {
+OS << "The result of the '"
+ <
[PATCH] D30295: [analyzer] clarify undef shift result when shift count is negative or exceeds the bit width
zaks.anna added inline comments. Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h:46 -} // end GR namespace +bool isExprResultConformsComparisonRule(CheckerContext &C, +BinaryOperatorKind CompRule, NoQ wrote: > Because you are making these functions public, i think it's worth it to make > it obvious what they do without looking at the particular checker code. > Comments are definitely worth it. I think function names could be worded > better; i suggest `exprComparesTo(const Expr *LHSExpr, BinaryOperatorKind > ComparisonOp, SVal RHSVal, CheckerContext &C);` and `isGreaterOrEqual(...)`; > i'm personally fond of having CheckerContext at the back because that's where > we have it in checker callbacks, but that's not important. + 1 on Artem's comment of making the names more clear and providing documentation. Also, should these be part of CheckerContext? Repository: rL LLVM https://reviews.llvm.org/D30295 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30295: [analyzer] clarify undef shift result when shift count is negative or exceeds the bit width
NoQ added a comment.
Hello, sorry for your having to wait on me. The implementation looks good, i'm
only having a couple of public API concerns.
Comment at:
include/clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h:46-49
+bool isExprResultConformsComparisonRule(CheckerContext &C,
+BinaryOperatorKind CompRule,
+const Expr *LHSExpr, const SVal
RHSVal);
+bool isGreaterEqual(CheckerContext &C, const Expr *E, unsigned long long Val);
Because you are making these functions public, i think it's worth it to make it
obvious what they do without looking at the particular checker code. Comments
are definitely worth it. I think function names could be worded better; i
suggest `exprComparesTo(const Expr *LHSExpr, BinaryOperatorKind ComparisonOp,
SVal RHSVal, CheckerContext &C);` and `isGreaterOrEqual(...)`; i'm personally
fond of having CheckerContext at the back because that's where we have it in
checker callbacks, but that's not important.
Comment at: lib/StaticAnalyzer/Core/CheckerHelpers.cpp:101
+const SVal RHSVal) {
+ ProgramStateRef State = C.getState();
+
I believe it is enough to pass only `State` to this function. `SValBuilder` and
`ConstraintManager` objects can be obtained from the state's
`ProgramStateManager`. The good thing about requiring only `State` is that we'd
be able to use these functions in checker callbacks that don't provide the
`CheckerContext` object, eg. `checkEndAnalysis` or `checkRegionChanges`.
Repository:
rL LLVM
https://reviews.llvm.org/D30295
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30295: [analyzer] clarify undef shift result when shift count is negative or exceeds the bit width
danielmarjamaki added a comment. Ping Repository: rL LLVM https://reviews.llvm.org/D30295 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30295: [analyzer] clarify undef shift result when shift count is negative or exceeds the bit width
danielmarjamaki updated this revision to Diff 92315.
danielmarjamaki marked an inline comment as done.
danielmarjamaki added a comment.
Fix review comment. Made isShiftOverflow() static.
Repository:
rL LLVM
https://reviews.llvm.org/D30295
Files:
include/clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h
lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
lib/StaticAnalyzer/Core/CheckerHelpers.cpp
test/Analysis/bitwise-ops.c
Index: test/Analysis/bitwise-ops.c
===
--- test/Analysis/bitwise-ops.c
+++ test/Analysis/bitwise-ops.c
@@ -29,4 +29,18 @@
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 '<<' expression is undefined due to shift count >= width of type}}
+ }
+ return 0;
+}
+
+int testNegativeShift(int a) {
+ if (a == -5) {
+return 1 << a; // expected-warning{{The result of the '<<' expression is undefined due to negative value on the right side of operand}}
+ }
+ return 0;
+}
Index: lib/StaticAnalyzer/Core/CheckerHelpers.cpp
===
--- lib/StaticAnalyzer/Core/CheckerHelpers.cpp
+++ lib/StaticAnalyzer/Core/CheckerHelpers.cpp
@@ -94,3 +94,40 @@
return std::make_pair(VD, RHS);
}
+
+bool clang::ento::isExprResultConformsComparisonRule(
+CheckerContext &C, BinaryOperatorKind CompRule, const Expr *LHSExpr,
+const SVal RHSVal) {
+ ProgramStateRef State = C.getState();
+
+ SVal LHSVal = C.getSVal(LHSExpr);
+ if (LHSVal.isUnknownOrUndef() || !LHSVal.getAs())
+return false;
+
+ SValBuilder &Bldr = C.getSValBuilder();
+ SVal Eval =
+ Bldr.evalBinOp(State, CompRule, LHSVal, RHSVal, Bldr.getConditionType());
+ if (Eval.isUnknownOrUndef())
+return false;
+ ConstraintManager &CM = C.getConstraintManager();
+ ProgramStateRef StTrue, StFalse;
+ std::tie(StTrue, StFalse) = CM.assumeDual(State, Eval.castAs());
+ return StTrue && !StFalse;
+}
+
+// Is E value greater or equal than Val?
+bool clang::ento::isGreaterEqual(CheckerContext &C, const Expr *E,
+ unsigned long long Val) {
+ llvm::APSInt In;
+ E->EvaluateAsInt(In, C.getASTContext());
+
+ DefinedSVal V =
+ C.getSValBuilder().makeIntVal(Val, C.getASTContext().LongLongTy);
+ return isExprResultConformsComparisonRule(C, BO_GE, E, V);
+}
+
+// Is E value negative?
+bool clang::ento::isNegative(CheckerContext &C, const Expr *E) {
+ DefinedSVal V = C.getSValBuilder().makeIntVal(0, false);
+ return isExprResultConformsComparisonRule(C, BO_LT, E, V);
+}
Index: lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
+++ lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
@@ -17,6 +17,7 @@
#include "clang/StaticAnalyzer/Core/Checker.h"
#include "clang/StaticAnalyzer/Core/CheckerManager.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h"
#include "llvm/ADT/SmallString.h"
#include "llvm/Support/raw_ostream.h"
@@ -59,6 +60,11 @@
return StOutBound && !StInBound;
}
+static bool isShiftOverflow(CheckerContext &C, const BinaryOperator *B) {
+ return isGreaterEqual(C, B->getRHS(),
+C.getASTContext().getIntWidth(B->getLHS()->getType()));
+}
+
void UndefResultChecker::checkPostStmt(const BinaryOperator *B,
CheckerContext &C) const {
ProgramStateRef state = C.getState();
@@ -97,18 +103,31 @@
}
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) &&
+ isNegative(C, B->getRHS())) {
+OS << "The result of the '"
+ << BinaryOperator::getOpcodeStr(B->getOpcode())
+ << "' expression is undefined due to negative value on the right "
+ "side of operand";
+ } else if ((B->getOpcode() == BinaryOperatorKind::BO_Shl ||
+ B->getOpcode() == BinaryOperatorKind::BO_Shr) &&
+ isShiftOverflow(C, B)) {
+OS <<
[PATCH] D30295: [analyzer] clarify undef shift result when shift count is negative or exceeds the bit width
a.sidorin added inline comments. Comment at: lib/StaticAnalyzer/Core/CheckerHelpers.cpp:99 +bool clang::ento::isExprResultConformsComparisonRule(CheckerContext &C, + BinaryOperatorKind BOK, + const Expr *LExpr, a.sidorin wrote: > CompRule? Oops. I meant renaming of the BOK argument, not the method :) Sorry for misleading. Repository: rL LLVM https://reviews.llvm.org/D30295 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30295: [analyzer] clarify undef shift result when shift count is negative or exceeds the bit width
danielmarjamaki marked 2 inline comments as done.
danielmarjamaki added inline comments.
Comment at: lib/StaticAnalyzer/Core/CheckerHelpers.cpp:100
+ BinaryOperatorKind BOK,
+ const Expr *LExpr,
+ const SVal RVal) {
a.sidorin wrote:
> I think we should rename these variables to LHSExpr, RHSVal, LHSVal. I don't
> like LVal/RVal because they may be associated with rvalue/lvalue types which
> is not what we want.
I agree. Good point.
Repository:
rL LLVM
https://reviews.llvm.org/D30295
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30295: [analyzer] clarify undef shift result when shift count is negative or exceeds the bit width
danielmarjamaki updated this revision to Diff 92150.
danielmarjamaki added a comment.
Fix review comments
Repository:
rL LLVM
https://reviews.llvm.org/D30295
Files:
include/clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h
lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
lib/StaticAnalyzer/Core/CheckerHelpers.cpp
test/Analysis/bitwise-ops.c
Index: test/Analysis/bitwise-ops.c
===
--- test/Analysis/bitwise-ops.c
+++ test/Analysis/bitwise-ops.c
@@ -29,4 +29,18 @@
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 '<<' expression is undefined due to shift count >= width of type}}
+ }
+ return 0;
+}
+
+int testNegativeShift(int a) {
+ if (a == -5) {
+return 1 << a; // expected-warning{{The result of the '<<' expression is undefined due to negative value on the right side of operand}}
+ }
+ return 0;
+}
Index: lib/StaticAnalyzer/Core/CheckerHelpers.cpp
===
--- lib/StaticAnalyzer/Core/CheckerHelpers.cpp
+++ lib/StaticAnalyzer/Core/CheckerHelpers.cpp
@@ -94,3 +94,39 @@
return std::make_pair(VD, RHS);
}
+
+bool clang::ento::isExprResultConformsCompRule(CheckerContext &C,
+ BinaryOperatorKind BOK,
+ const Expr *LHSExpr,
+ const SVal RHSVal) {
+ ProgramStateRef State = C.getState();
+
+ SVal LHSVal = C.getSVal(LHSExpr);
+ if (LHSVal.isUnknownOrUndef() || !LHSVal.getAs())
+return false;
+
+ SValBuilder &Bldr = C.getSValBuilder();
+ SVal Eval =
+ Bldr.evalBinOp(State, BOK, LHSVal, RHSVal, Bldr.getConditionType());
+ if (Eval.isUnknownOrUndef())
+return false;
+
+ ConstraintManager &CM = C.getConstraintManager();
+ ProgramStateRef StTrue, StFalse;
+ std::tie(StTrue, StFalse) = CM.assumeDual(State, Eval.castAs());
+ return StTrue && !StFalse;
+}
+
+// Is E value greater or equal than Val?
+bool clang::ento::isGreaterEqual(CheckerContext &C, const Expr *E,
+ unsigned long long Val) {
+ DefinedSVal V =
+ C.getSValBuilder().makeIntVal(Val, C.getASTContext().LongLongTy);
+ return isExprResultConformsCompRule(C, BO_GE, E, V);
+}
+
+// Is E value negative?
+bool clang::ento::isNegative(CheckerContext &C, const Expr *E) {
+ DefinedSVal V = C.getSValBuilder().makeIntVal(0, false);
+ return isExprResultConformsCompRule(C, BO_LT, E, V);
+}
Index: lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
+++ lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
@@ -17,6 +17,7 @@
#include "clang/StaticAnalyzer/Core/Checker.h"
#include "clang/StaticAnalyzer/Core/CheckerManager.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h"
#include "llvm/ADT/SmallString.h"
#include "llvm/Support/raw_ostream.h"
@@ -59,6 +60,11 @@
return StOutBound && !StInBound;
}
+bool isShiftOverflow(CheckerContext &C, const BinaryOperator *B) {
+ return isGreaterEqual(C, B->getRHS(),
+C.getASTContext().getIntWidth(B->getLHS()->getType()));
+}
+
void UndefResultChecker::checkPostStmt(const BinaryOperator *B,
CheckerContext &C) const {
ProgramStateRef state = C.getState();
@@ -106,9 +112,24 @@
}
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) &&
+ isNegative(C, B->getRHS())) {
+OS << "The result of the '"
+ << BinaryOperator::getOpcodeStr(B->getOpcode())
+ << "' expression is undefined due to negative value on the right "
+ "side of operand";
+ } else if ((B->getOpcode() == BinaryOperatorKind::BO_Shl ||
+ B->getOpcode() == BinaryOperatorKind::BO_Shr) &&
+ isShiftOverflow(C, B)) {
+OS << "The result of the '"
+ << BinaryOperator::getOpcodeStr(B->getOpcode())
+ << "' expression is undefined due to shift count >= width of type";
+ } else {
+OS << "The result of the '"
+ << BinaryOperator::getOpcodeStr(B->getOpcode())
+ << "' expression is undefined";
+ }
}
auto report = llvm::make_unique(*BT, OS.str(), N);
if (Ex) {
Index: lib/StaticAn
[PATCH] D30295: [analyzer] clarify undef shift result when shift count is negative or exceeds the bit width
a.sidorin added a comment.
Hello Daniel,
Your patch looks mostly good to me. I have only minor naming comments.
Comment at: lib/StaticAnalyzer/Core/CheckerHelpers.cpp:99
+bool clang::ento::isExprResultConformsComparisonRule(CheckerContext &C,
+ BinaryOperatorKind BOK,
+ const Expr *LExpr,
CompRule?
Comment at: lib/StaticAnalyzer/Core/CheckerHelpers.cpp:100
+ BinaryOperatorKind BOK,
+ const Expr *LExpr,
+ const SVal RVal) {
I think we should rename these variables to LHSExpr, RHSVal, LHSVal. I don't
like LVal/RVal because they may be associated with rvalue/lvalue types which is
not what we want.
Repository:
rL LLVM
https://reviews.llvm.org/D30295
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30295: [analyzer] clarify undef shift result when shift count is negative or exceeds the bit width
danielmarjamaki added a comment. Ping Repository: rL LLVM https://reviews.llvm.org/D30295 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30295: [analyzer] clarify undef shift result when shift count is negative or exceeds the bit width
danielmarjamaki created this revision.
The error messages are confusing when shift result is undefined because the
shift count is negative or exceeds the bit width. I have seen that users often
draw the conclusion that Clang thinks some operand is uninitialized and
determine that Clang shows a false positive.
I also know that some users use negative shift count by intention and don't
think that would cause problems.
This patch clarify the error message and refactors the code a little.
Repository:
rL LLVM
https://reviews.llvm.org/D30295
Files:
include/clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h
lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
lib/StaticAnalyzer/Core/CheckerHelpers.cpp
test/Analysis/bitwise-ops.c
Index: test/Analysis/bitwise-ops.c
===
--- test/Analysis/bitwise-ops.c
+++ test/Analysis/bitwise-ops.c
@@ -29,4 +29,18 @@
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 '<<' expression is undefined due to shift count >= width of type}}
+ }
+ return 0;
+}
+
+int testNegativeShift(int a) {
+ if (a == -5) {
+return 1 << a; // expected-warning{{The result of the '<<' expression is undefined due to negative value on the right side of operand}}
+ }
+ return 0;
+}
Index: lib/StaticAnalyzer/Core/CheckerHelpers.cpp
===
--- lib/StaticAnalyzer/Core/CheckerHelpers.cpp
+++ lib/StaticAnalyzer/Core/CheckerHelpers.cpp
@@ -94,3 +94,37 @@
return std::make_pair(VD, RHS);
}
+
+bool clang::ento::isExprResultConformsComparisonRule(CheckerContext &C,
+ BinaryOperatorKind BOK,
+ const Expr *LExpr,
+ const SVal RVal) {
+ ProgramStateRef State = C.getState();
+
+ SVal LVal = C.getSVal(LExpr);
+ if (LVal.isUnknownOrUndef() || !LVal.getAs())
+return false;
+
+ SValBuilder &Bldr = C.getSValBuilder();
+ SVal Eval = Bldr.evalBinOp(State, BOK, LVal, RVal, Bldr.getConditionType());
+ if (Eval.isUnknownOrUndef())
+return false;
+
+ ConstraintManager &CM = C.getConstraintManager();
+ ProgramStateRef StTrue, StFalse;
+ std::tie(StTrue, StFalse) = CM.assumeDual(State, Eval.castAs());
+ return StTrue && !StFalse;
+}
+
+// Is E value greater or equal than Val?
+bool clang::ento::isGreaterEqual(CheckerContext &C, const Expr *E,
+ unsigned long long Val) {
+ DefinedSVal V = C.getSValBuilder().makeIntVal(Val, C.getASTContext().LongLongTy);
+ return isExprResultConformsComparisonRule(C, BO_GE, E, V);
+}
+
+// Is E value negative?
+bool clang::ento::isNegative(CheckerContext &C, const Expr *E) {
+ DefinedSVal V = C.getSValBuilder().makeIntVal(0, false);
+ return isExprResultConformsComparisonRule(C, BO_LT, E, V);
+}
Index: lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
+++ lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
@@ -17,6 +17,7 @@
#include "clang/StaticAnalyzer/Core/Checker.h"
#include "clang/StaticAnalyzer/Core/CheckerManager.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h"
#include "llvm/ADT/SmallString.h"
#include "llvm/Support/raw_ostream.h"
@@ -35,6 +36,11 @@
};
} // end anonymous namespace
+bool isShiftOverflow(CheckerContext &C, const BinaryOperator *B) {
+ return isGreaterEqual(C, B->getRHS(),
+C.getASTContext().getIntWidth(B->getLHS()->getType()));
+}
+
void UndefResultChecker::checkPostStmt(const BinaryOperator *B,
CheckerContext &C) const {
ProgramStateRef state = C.getState();
@@ -80,9 +86,24 @@
}
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) &&
+ isNegative(C, B->getRHS())) {
+OS << "The result of the '"
+ << BinaryOperator::getOpcodeStr(B->getOpcode())
+ << "' expression is undefined due to negative value on the right "
+ "side of operand";
+ } else if ((B->getOpcode() == BinaryOperatorKind::BO_Shl ||
+ B->getOpcode() == BinaryOperatorKind::BO_Shr) &&
+ isShiftOverflow(C, B)) {
+OS << "The result of the '"
+ <<
