[PATCH] D25596: alpha.core.Conversion - Fix false positive for 'U32 += S16; ' expression, that is not unsafe
This revision was automatically updated to reflect the committed changes.
Closed by commit rL299523: [analyzer] alpha.core.Conversion - Fix false
positive for 'U32 += S16;'… (authored by danielmarjamaki).
Changed prior to commit:
https://reviews.llvm.org/D25596?vs=92810=94176#toc
Repository:
rL LLVM
https://reviews.llvm.org/D25596
Files:
cfe/trunk/lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
cfe/trunk/test/Analysis/conversion.c
Index: cfe/trunk/test/Analysis/conversion.c
===
--- cfe/trunk/test/Analysis/conversion.c
+++ cfe/trunk/test/Analysis/conversion.c
@@ -9,9 +9,67 @@
if (U > 300)
S8 = U; // expected-warning {{Loss of precision in implicit conversion}}
if (S > 10)
-U8 = S;
+U8 = S; // no-warning
if (U < 200)
-S8 = U;
+S8 = U; // no-warning
+}
+
+void addAssign() {
+ unsigned long L = 1000;
+ int I = -100;
+ U8 += L; // expected-warning {{Loss of precision in implicit conversion}}
+ L += I; // no-warning
+}
+
+void subAssign() {
+ unsigned long L = 1000;
+ int I = -100;
+ U8 -= L; // expected-warning {{Loss of precision in implicit conversion}}
+ L -= I; // no-warning
+}
+
+void mulAssign() {
+ unsigned long L = 1000;
+ int I = -1;
+ U8 *= L; // expected-warning {{Loss of precision in implicit conversion}}
+ L *= I; // expected-warning {{Loss of sign in implicit conversion}}
+ I = 10;
+ L *= I; // no-warning
+}
+
+void divAssign() {
+ unsigned long L = 1000;
+ int I = -1;
+ U8 /= L; // no-warning
+ L /= I; // expected-warning {{Loss of sign in implicit conversion}}
+}
+
+void remAssign() {
+ unsigned long L = 1000;
+ int I = -1;
+ U8 %= L; // no-warning
+ L %= I; // expected-warning {{Loss of sign in implicit conversion}}
+}
+
+void andAssign() {
+ unsigned long L = 1000;
+ int I = -1;
+ U8 &= L; // no-warning
+ L &= I; // expected-warning {{Loss of sign in implicit conversion}}
+}
+
+void orAssign() {
+ unsigned long L = 1000;
+ int I = -1;
+ U8 |= L; // expected-warning {{Loss of precision in implicit conversion}}
+ L |= I; // expected-warning {{Loss of sign in implicit conversion}}
+}
+
+void xorAssign() {
+ unsigned long L = 1000;
+ int I = -1;
+ U8 ^= L; // expected-warning {{Loss of precision in implicit conversion}}
+ L ^= I; // expected-warning {{Loss of sign in implicit conversion}}
}
void init1() {
@@ -21,7 +79,7 @@
void relational(unsigned U, signed S) {
if (S > 10) {
-if (U < S) {
+if (U < S) { // no-warning
}
}
if (S < -10) {
@@ -32,14 +90,14 @@
void multiplication(unsigned U, signed S) {
if (S > 5)
-S = U * S;
+S = U * S; // no-warning
if (S < -10)
S = U * S; // expected-warning {{Loss of sign}}
}
void division(unsigned U, signed S) {
if (S > 5)
-S = U / S;
+S = U / S; // no-warning
if (S < -10)
S = U / S; // expected-warning {{Loss of sign}}
}
Index: cfe/trunk/lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
@@ -41,7 +41,8 @@
mutable std::unique_ptr BT;
// Is there loss of precision
- bool isLossOfPrecision(const ImplicitCastExpr *Cast, CheckerContext ) const;
+ bool isLossOfPrecision(const ImplicitCastExpr *Cast, QualType DestType,
+ CheckerContext ) const;
// Is there loss of sign
bool isLossOfSign(const ImplicitCastExpr *Cast, CheckerContext ) const;
@@ -73,16 +74,30 @@
// Loss of sign/precision in binary operation.
if (const auto *B = dyn_cast(Parent)) {
BinaryOperator::Opcode Opc = B->getOpcode();
-if (Opc == BO_Assign || Opc == BO_AddAssign || Opc == BO_SubAssign ||
-Opc == BO_MulAssign) {
+if (Opc == BO_Assign) {
LossOfSign = isLossOfSign(Cast, C);
- LossOfPrecision = isLossOfPrecision(Cast, C);
+ LossOfPrecision = isLossOfPrecision(Cast, Cast->getType(), C);
+} else if (Opc == BO_AddAssign || Opc == BO_SubAssign) {
+ // No loss of sign.
+ LossOfPrecision = isLossOfPrecision(Cast, B->getLHS()->getType(), C);
+} else if (Opc == BO_MulAssign) {
+ LossOfSign = isLossOfSign(Cast, C);
+ LossOfPrecision = isLossOfPrecision(Cast, B->getLHS()->getType(), C);
+} else if (Opc == BO_DivAssign || Opc == BO_RemAssign) {
+ LossOfSign = isLossOfSign(Cast, C);
+ // No loss of precision.
+} else if (Opc == BO_AndAssign) {
+ LossOfSign = isLossOfSign(Cast, C);
+ // No loss of precision.
+} else if (Opc == BO_OrAssign || Opc == BO_XorAssign) {
+ LossOfSign = isLossOfSign(Cast, C);
+ LossOfPrecision = isLossOfPrecision(Cast, B->getLHS()->getType(), C);
} else if (B->isRelationalOp() || B->isMultiplicativeOp()) {
LossOfSign = isLossOfSign(Cast, C);
}
} else if (isa(Parent)) {
LossOfSign = isLossOfSign(Cast,
[PATCH] D25596: alpha.core.Conversion - Fix false positive for 'U32 += S16; ' expression, that is not unsafe
NoQ accepted this revision. NoQ added a comment. This revision is now accepted and ready to land. > There are several undetected "TODO: loss of precision" right now in the tests > that I would like to fix. I think it's a good idea to have `// no-warning` comments as well when testing for lack of false positives. I've a feeling that the original false positive that you've been fixing initially has disappeared from the final tests (we've got stuff with unsigned long += int, but large types behave quite differently when it comes to integral promotion). If i'm wrong, please commit :) Repository: rL LLVM https://reviews.llvm.org/D25596 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D25596: alpha.core.Conversion - Fix false positive for 'U32 += S16; ' expression, that is not unsafe
danielmarjamaki added a comment. Ping Repository: rL LLVM https://reviews.llvm.org/D25596 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D25596: alpha.core.Conversion - Fix false positive for 'U32 += S16; ' expression, that is not unsafe
danielmarjamaki updated this revision to Diff 92810.
danielmarjamaki added a comment.
Updated the patch so all the loss of precision are detected also
Repository:
rL LLVM
https://reviews.llvm.org/D25596
Files:
lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
test/Analysis/conversion.c
Index: test/Analysis/conversion.c
===
--- test/Analysis/conversion.c
+++ test/Analysis/conversion.c
@@ -14,6 +14,64 @@
S8 = U;
}
+void addAssign() {
+ unsigned long L = 1000;
+ int I = -100;
+ U8 += L; // expected-warning {{Loss of precision in implicit conversion}}
+ L += I;
+}
+
+void subAssign() {
+ unsigned long L = 1000;
+ int I = -100;
+ U8 -= L; // expected-warning {{Loss of precision in implicit conversion}}
+ L -= I;
+}
+
+void mulAssign() {
+ unsigned long L = 1000;
+ int I = -1;
+ U8 *= L; // expected-warning {{Loss of precision in implicit conversion}}
+ L *= I; // expected-warning {{Loss of sign in implicit conversion}}
+ I = 10;
+ L *= I;
+}
+
+void divAssign() {
+ unsigned long L = 1000;
+ int I = -1;
+ U8 /= L;
+ L /= I; // expected-warning {{Loss of sign in implicit conversion}}
+}
+
+void remAssign() {
+ unsigned long L = 1000;
+ int I = -1;
+ U8 %= L;
+ L %= I; // expected-warning {{Loss of sign in implicit conversion}}
+}
+
+void andAssign() {
+ unsigned long L = 1000;
+ int I = -1;
+ U8 &= L;
+ L &= I; // expected-warning {{Loss of sign in implicit conversion}}
+}
+
+void orAssign() {
+ unsigned long L = 1000;
+ int I = -1;
+ U8 |= L; // expected-warning {{Loss of precision in implicit conversion}}
+ L |= I; // expected-warning {{Loss of sign in implicit conversion}}
+}
+
+void xorAssign() {
+ unsigned long L = 1000;
+ int I = -1;
+ U8 ^= L; // expected-warning {{Loss of precision in implicit conversion}}
+ L ^= I; // expected-warning {{Loss of sign in implicit conversion}}
+}
+
void init1() {
long long A = 1LL << 60;
short X = A; // expected-warning {{Loss of precision in implicit conversion}}
Index: lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
+++ lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
@@ -41,7 +41,8 @@
mutable std::unique_ptr BT;
// Is there loss of precision
- bool isLossOfPrecision(const ImplicitCastExpr *Cast, CheckerContext ) const;
+ bool isLossOfPrecision(const ImplicitCastExpr *Cast, QualType DestType,
+ CheckerContext ) const;
// Is there loss of sign
bool isLossOfSign(const ImplicitCastExpr *Cast, CheckerContext ) const;
@@ -73,16 +74,30 @@
// Loss of sign/precision in binary operation.
if (const auto *B = dyn_cast(Parent)) {
BinaryOperator::Opcode Opc = B->getOpcode();
-if (Opc == BO_Assign || Opc == BO_AddAssign || Opc == BO_SubAssign ||
-Opc == BO_MulAssign) {
+if (Opc == BO_Assign) {
LossOfSign = isLossOfSign(Cast, C);
- LossOfPrecision = isLossOfPrecision(Cast, C);
+ LossOfPrecision = isLossOfPrecision(Cast, Cast->getType(), C);
+} else if (Opc == BO_AddAssign || Opc == BO_SubAssign) {
+ // No loss of sign.
+ LossOfPrecision = isLossOfPrecision(Cast, B->getLHS()->getType(), C);
+} else if (Opc == BO_MulAssign) {
+ LossOfSign = isLossOfSign(Cast, C);
+ LossOfPrecision = isLossOfPrecision(Cast, B->getLHS()->getType(), C);
+} else if (Opc == BO_DivAssign || Opc == BO_RemAssign) {
+ LossOfSign = isLossOfSign(Cast, C);
+ // No loss of precision.
+} else if (Opc == BO_AndAssign) {
+ LossOfSign = isLossOfSign(Cast, C);
+ // No loss of precision.
+} else if (Opc == BO_OrAssign || Opc == BO_XorAssign) {
+ LossOfSign = isLossOfSign(Cast, C);
+ LossOfPrecision = isLossOfPrecision(Cast, B->getLHS()->getType(), C);
} else if (B->isRelationalOp() || B->isMultiplicativeOp()) {
LossOfSign = isLossOfSign(Cast, C);
}
} else if (isa(Parent)) {
LossOfSign = isLossOfSign(Cast, C);
-LossOfPrecision = isLossOfPrecision(Cast, C);
+LossOfPrecision = isLossOfPrecision(Cast, Cast->getType(), C);
}
if (LossOfSign || LossOfPrecision) {
@@ -113,6 +128,13 @@
unsigned long long Val) {
ProgramStateRef State = C.getState();
SVal EVal = C.getSVal(E);
+ if (EVal.isUnknownOrUndef())
+return false;
+ if (!EVal.getAs() && EVal.getAs()) {
+ProgramStateManager = C.getStateManager();
+EVal =
+Mgr.getStoreManager().getBinding(State->getStore(), EVal.castAs());
+ }
if (EVal.isUnknownOrUndef() || !EVal.getAs())
return false;
@@ -153,22 +175,22 @@
}
bool ConversionChecker::isLossOfPrecision(const ImplicitCastExpr *Cast,
-CheckerContext ) const {
+ QualType DestType,
+ CheckerContext )
[PATCH] D25596: alpha.core.Conversion - Fix false positive for 'U32 += S16; ' expression, that is not unsafe
danielmarjamaki updated this revision to Diff 92634.
danielmarjamaki added a comment.
I added more testcases. There are several undetected "TODO: loss of precision"
right now in the tests that I would like to fix. If this patch to fix FP is
accepted I will commit it and continue working on the TODO tests. If it's not
accepted I will continue investigating the TODO tests anyway..
Repository:
rL LLVM
https://reviews.llvm.org/D25596
Files:
lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
test/Analysis/conversion.c
Index: test/Analysis/conversion.c
===
--- test/Analysis/conversion.c
+++ test/Analysis/conversion.c
@@ -14,6 +14,64 @@
S8 = U;
}
+void addAssign() {
+ unsigned long L = 1000;
+ int I = -100;
+ U8 += L; // TODO: Loss of precision
+ L += I;
+}
+
+void subAssign() {
+ unsigned long L = 1000;
+ int I = -100;
+ U8 -= L; // TODO: Loss of precision
+ L -= I;
+}
+
+void mulAssign() {
+ unsigned long L = 1000;
+ int I = -1;
+ U8 *= L; // TODO: Loss of precision
+ L *= I; // expected-warning {{Loss of sign in implicit conversion}}
+ I = 10;
+ L *= I;
+}
+
+void divAssign() {
+ unsigned long L = 1000;
+ int I = -1;
+ U8 /= L;
+ L /= I; // expected-warning {{Loss of sign in implicit conversion}}
+}
+
+void remAssign() {
+ unsigned long L = 1000;
+ int I = -1;
+ U8 %= L;
+ L %= I; // expected-warning {{Loss of sign in implicit conversion}}
+}
+
+void andAssign() {
+ unsigned long L = 1000;
+ int I = -1;
+ U8 &= L;
+ L &= I; // expected-warning {{Loss of sign in implicit conversion}}
+}
+
+void orAssign() {
+ unsigned long L = 1000;
+ int I = -1;
+ U8 |= L; // TODO: Loss of precision
+ L |= I; // expected-warning {{Loss of sign in implicit conversion}}
+}
+
+void xorAssign() {
+ unsigned long L = 1000;
+ int I = -1;
+ U8 ^= L; // TODO: Loss of precision
+ L ^= I; // expected-warning {{Loss of sign in implicit conversion}}
+}
+
void init1() {
long long A = 1LL << 60;
short X = A; // expected-warning {{Loss of precision in implicit conversion}}
Index: lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
+++ lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
@@ -73,8 +73,22 @@
// Loss of sign/precision in binary operation.
if (const auto *B = dyn_cast(Parent)) {
BinaryOperator::Opcode Opc = B->getOpcode();
-if (Opc == BO_Assign || Opc == BO_AddAssign || Opc == BO_SubAssign ||
-Opc == BO_MulAssign) {
+if (Opc == BO_Assign) {
+ LossOfSign = isLossOfSign(Cast, C);
+ LossOfPrecision = isLossOfPrecision(Cast, C);
+} else if (Opc == BO_AddAssign || Opc == BO_SubAssign) {
+ // No loss of sign.
+ LossOfPrecision = isLossOfPrecision(Cast, C);
+} else if (Opc == BO_MulAssign) {
+ LossOfSign = isLossOfSign(Cast, C);
+ LossOfPrecision = isLossOfPrecision(Cast, C);
+} else if (Opc == BO_DivAssign || Opc == BO_RemAssign) {
+ LossOfSign = isLossOfSign(Cast, C);
+ // No loss of precision.
+} else if (Opc == BO_AndAssign) {
+ LossOfSign = isLossOfSign(Cast, C);
+ // No loss of precision.
+} else if (Opc == BO_OrAssign || Opc == BO_XorAssign) {
LossOfSign = isLossOfSign(Cast, C);
LossOfPrecision = isLossOfPrecision(Cast, C);
} else if (B->isRelationalOp() || B->isMultiplicativeOp()) {
@@ -153,7 +167,7 @@
}
bool ConversionChecker::isLossOfPrecision(const ImplicitCastExpr *Cast,
-CheckerContext ) const {
+ CheckerContext ) const {
// Don't warn about explicit loss of precision.
if (Cast->isEvaluatable(C.getASTContext()))
return false;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D25596: alpha.core.Conversion - Fix false positive for 'U32 += S16; ' expression, that is not unsafe
zaks.anna added a comment. Looks like you've also added handling of Xor, Or , Div, and Rem. Should there be tests for those? Repository: rL LLVM https://reviews.llvm.org/D25596 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D25596: alpha.core.Conversion - Fix false positive for 'U32 += S16; ' expression, that is not unsafe
danielmarjamaki created this revision.
danielmarjamaki added a reviewer: NoQ.
danielmarjamaki added subscribers: cfe-commits, xazax.hun, dcoughlin.
danielmarjamaki set the repository for this revision to rL LLVM.
This patch fix false positives for loss of sign in addition and subtraction
assignment operators.
Repository:
rL LLVM
https://reviews.llvm.org/D25596
Files:
lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
test/Analysis/conversion.c
Index: test/Analysis/conversion.c
===
--- test/Analysis/conversion.c
+++ test/Analysis/conversion.c
@@ -77,6 +77,10 @@
void dontwarn5() {
signed S = -32;
U8 = S + 10;
+
+ unsigned x = 100;
+ signed short delta = -1;
+ x += delta;
}
Index: lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
+++ lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
@@ -73,10 +73,13 @@
// Loss of sign/precision in binary operation.
if (const auto *B = dyn_cast(Parent)) {
BinaryOperator::Opcode Opc = B->getOpcode();
-if (Opc == BO_Assign || Opc == BO_AddAssign || Opc == BO_SubAssign ||
-Opc == BO_MulAssign) {
- LossOfSign = isLossOfSign(Cast, C);
- LossOfPrecision = isLossOfPrecision(Cast, C);
+if (B->isAssignmentOp()) {
+ if (Opc == BO_Assign || Opc == BO_AddAssign || Opc == BO_SubAssign ||
+ Opc == BO_MulAssign || Opc == BO_OrAssign || Opc == BO_XorAssign)
+LossOfPrecision = isLossOfPrecision(Cast, C);
+ if (Opc == BO_Assign || Opc == BO_MulAssign || Opc == BO_DivAssign ||
+ Opc == BO_RemAssign)
+LossOfSign = isLossOfSign(Cast, C);
} else if (B->isRelationalOp() || B->isMultiplicativeOp()) {
LossOfSign = isLossOfSign(Cast, C);
}
@@ -153,7 +156,7 @@
}
bool ConversionChecker::isLossOfPrecision(const ImplicitCastExpr *Cast,
-CheckerContext ) const {
+ CheckerContext ) const {
// Don't warn about explicit loss of precision.
if (Cast->isEvaluatable(C.getASTContext()))
return false;
@@ -177,7 +180,7 @@
}
bool ConversionChecker::isLossOfSign(const ImplicitCastExpr *Cast,
- CheckerContext ) const {
+ CheckerContext ) const {
QualType CastType = Cast->getType();
QualType SubType = Cast->IgnoreParenImpCasts()->getType();
Index: test/Analysis/conversion.c
===
--- test/Analysis/conversion.c
+++ test/Analysis/conversion.c
@@ -77,6 +77,10 @@
void dontwarn5() {
signed S = -32;
U8 = S + 10;
+
+ unsigned x = 100;
+ signed short delta = -1;
+ x += delta;
}
Index: lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
+++ lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
@@ -73,10 +73,13 @@
// Loss of sign/precision in binary operation.
if (const auto *B = dyn_cast(Parent)) {
BinaryOperator::Opcode Opc = B->getOpcode();
-if (Opc == BO_Assign || Opc == BO_AddAssign || Opc == BO_SubAssign ||
-Opc == BO_MulAssign) {
- LossOfSign = isLossOfSign(Cast, C);
- LossOfPrecision = isLossOfPrecision(Cast, C);
+if (B->isAssignmentOp()) {
+ if (Opc == BO_Assign || Opc == BO_AddAssign || Opc == BO_SubAssign ||
+ Opc == BO_MulAssign || Opc == BO_OrAssign || Opc == BO_XorAssign)
+LossOfPrecision = isLossOfPrecision(Cast, C);
+ if (Opc == BO_Assign || Opc == BO_MulAssign || Opc == BO_DivAssign ||
+ Opc == BO_RemAssign)
+LossOfSign = isLossOfSign(Cast, C);
} else if (B->isRelationalOp() || B->isMultiplicativeOp()) {
LossOfSign = isLossOfSign(Cast, C);
}
@@ -153,7 +156,7 @@
}
bool ConversionChecker::isLossOfPrecision(const ImplicitCastExpr *Cast,
-CheckerContext ) const {
+ CheckerContext ) const {
// Don't warn about explicit loss of precision.
if (Cast->isEvaluatable(C.getASTContext()))
return false;
@@ -177,7 +180,7 @@
}
bool ConversionChecker::isLossOfSign(const ImplicitCastExpr *Cast,
- CheckerContext ) const {
+ CheckerContext ) const {
QualType CastType = Cast->getType();
QualType SubType = Cast->IgnoreParenImpCasts()->getType();
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
