Thanks for the comments.
I have added addition and ran it on clang and gimp without any false positives.
Added some test cases and refactored the test.
//Anders
Why only multiplication, shift left, and not -- why not addition as
well? INT_MAX + 1 comes to mind as a case where there's dangerous
truncation.
A few more thoughts below...
> Index: include/clang/Basic/DiagnosticSemaKinds.td
> ===================================================================
> --- include/clang/Basic/DiagnosticSemaKinds.td (revision 221872)
> +++ include/clang/Basic/DiagnosticSemaKinds.td (working copy)
> @@ -2472,6 +2472,9 @@
> def warn_impcast_integer_precision : Warning<
> "implicit conversion loses integer precision: %0 to %1">,
> InGroup<Conversion>, DefaultIgnore;
> +def warn_impcast_trunc : Warning<
> + "possible truncation before conversion from %0 to %1">,
> + InGroup<Conversion>, DefaultIgnore;
> def warn_impcast_integer_64_32 : Warning<
> "implicit conversion loses integer precision: %0 to %1">,
> InGroup<Shorten64To32>, DefaultIgnore;
> Index: lib/Sema/SemaChecking.cpp
> ===================================================================
> --- lib/Sema/SemaChecking.cpp (revision 221872)
> +++ lib/Sema/SemaChecking.cpp (working copy)
> @@ -6408,6 +6408,14 @@
> return DiagnoseImpCast(S, E, T, CC, DiagID);
> }
>
> + if (SourceRange.Width < TargetRange.Width) {
> + if (BinaryOperator *Bop = dyn_cast<BinaryOperator>(E))
> + if (Bop->getOpcode() == BO_Mul || Bop->getOpcode() == BO_Shl)
> + return DiagnoseImpCast(S, E, T, CC, diag::warn_impcast_trunc);
> + if (UnaryOperator *Uop = dyn_cast<UnaryOperator>(E))
> + if (Uop->getOpcode() == UO_Not)
> + return DiagnoseImpCast(S, E, T, CC, diag::warn_impcast_trunc);
> + }
> // Diagnose conversions between different enumeration types.
> // In C, we pretend that the type of an EnumConstantDecl is its enumeration
> // type, to give us better diagnostics.
> @@ -6436,6 +6444,39 @@
> return;
> }
>
> +void CheckExplicitConversion(Sema &S, Expr *E, QualType T,
> + SourceLocation CC, bool *ICContext = nullptr) {
The function should be static, and ICContext is never used.
> + if (E->isTypeDependent() || E->isValueDependent())
> + return;
> +
> + if (CC.isInvalid())
> + return;
> +
> + const Type *Source = S.Context.getCanonicalType(E->getType()).getTypePtr();
> + const Type *Target = S.Context.getCanonicalType(T).getTypePtr();
> +
> + if (Source == Target)
> + return;
> +
> + if (Target->isDependentType())
> + return;
> +
> + if (!Source->isIntegerType() || !Target->isIntegerType())
> + return;
> +
> + IntRange SourceRange = GetExprRange(S.Context, E);
> + IntRange TargetRange = IntRange::forTargetOfCanonicalType(S.Context,
> Target);
> +
> + if (SourceRange.Width < TargetRange.Width) {
> + if (BinaryOperator *Bop = dyn_cast<BinaryOperator>(E))
> + if (Bop->getOpcode() == BO_Mul || Bop->getOpcode() == BO_Shl)
> + return DiagnoseImpCast(S, E, T, CC, diag::warn_impcast_trunc);
> + if (UnaryOperator *Uop = dyn_cast<UnaryOperator>(E))
> + if (Uop->getOpcode() == UO_Not)
> + return DiagnoseImpCast(S, E, T, CC, diag::warn_impcast_trunc);
> + }
This code is duplicated from above -- it should be consolidated into a
single path.
> +}
> +
> void CheckConditionalOperator(Sema &S, ConditionalOperator *E,
> SourceLocation CC, QualType T);
>
> @@ -6518,6 +6559,11 @@
> if (const OpaqueValueExpr *OVE = dyn_cast<OpaqueValueExpr>(E))
> return AnalyzeImplicitConversions(S, OVE->getSourceExpr(), CC);
>
> + // Check explicit conversions for CStyle castings.
> + if (CStyleCastExpr *CStyleExpr = dyn_cast<CStyleCastExpr>(E)) {
> + CheckExplicitConversion(S,
> CStyleExpr->getSubExpr()->IgnoreParenImpCasts(), T, CC);
80-col limit, and the braces can be elided.
> + }
> +
> // Skip past explicit casts.
> if (isa<ExplicitCastExpr>(E)) {
> E = cast<ExplicitCastExpr>(E)->getSubExpr()->IgnoreParenImpCasts();
> Index: test/Sema/conversion.c
> ===================================================================
> --- test/Sema/conversion.c (revision 221872)
> +++ test/Sema/conversion.c (working copy)
> @@ -429,3 +429,13 @@
> ushort16 crCbScale = pairedConstants.s4; // expected-warning {{implicit
> conversion loses integer precision: 'uint32_t' (aka 'unsigned int') to
> 'ushort16'}}
> ushort16 brBias = pairedConstants.s6; // expected-warning {{implicit
> conversion loses integer precision: 'uint32_t' (aka 'unsigned int') to
> 'ushort16'}}
> }
> +
> +void test28(int a, int b) {
> + long l = a * b; // expected-warning {{possible truncation before
> conversion from 'int' to 'long'}}
Should be tests covering the other implicit cases, as well as a
negative test for cases you don't intend to cover (such as +,
possibly).
> + l = (long)a * b;
> +}
> +
> +void test29(unsigned int n) {
> + long a = (long) (n << 8); // expected-warning {{possible truncation before
> conversion from 'unsigned int' to 'long'}}
Same comments here as above.
> + a = (long) n << 8;
> +}
Some template tests would also be good, since we're purposefully not
diagnosing in those cases.
~Aaron
On Tue, Dec 2, 2014 at 10:52 AM, Anders Rönnholm
<[email protected]> wrote:
> Ping.
>
>>Hi,
>>
>>I have a new check i'd like to get reviewed. It warns on dangerous long casts.
>>
>>e.g
>>int a = 105000;
>>int b = 25000;
>>long l = a*b; // possible truncation before conversion from 'int' to 'long'
>>
>>//Anders
>
> _______________________________________________
> cfe-commits mailing list
> [email protected]
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
Index: include/clang/Basic/DiagnosticSemaKinds.td
===================================================================
--- include/clang/Basic/DiagnosticSemaKinds.td (revision 223224)
+++ include/clang/Basic/DiagnosticSemaKinds.td (working copy)
@@ -2481,6 +2481,9 @@
def warn_impcast_integer_precision : Warning<
"implicit conversion loses integer precision: %0 to %1">,
InGroup<Conversion>, DefaultIgnore;
+def warn_impcast_trunc : Warning<
+ "possible truncation before conversion from %0 to %1">,
+ InGroup<Conversion>, DefaultIgnore;
def warn_impcast_integer_64_32 : Warning<
"implicit conversion loses integer precision: %0 to %1">,
InGroup<Shorten64To32>, DefaultIgnore;
Index: lib/Sema/SemaChecking.cpp
===================================================================
--- lib/Sema/SemaChecking.cpp (revision 223224)
+++ lib/Sema/SemaChecking.cpp (working copy)
@@ -6248,6 +6248,19 @@
S.getFixItZeroLiteralForType(T, Loc));
}
+static bool CheckPossibleTruncation(const Expr *E, const IntRange Source,
+ const IntRange Target) {
+ if (Source.Width < Target.Width) {
+ if (const BinaryOperator *Bop = dyn_cast<BinaryOperator>(E))
+ if (Bop->getOpcode() == BO_Mul || Bop->getOpcode() == BO_Shl ||
+ Bop->getOpcode() == BO_Add)
+ return true;
+ if (const UnaryOperator *Uop = dyn_cast<UnaryOperator>(E))
+ if (Uop->getOpcode() == UO_Not)
+ return true;
+ }
+ return false;
+}
void CheckImplicitConversion(Sema &S, Expr *E, QualType T,
SourceLocation CC, bool *ICContext = nullptr) {
if (E->isTypeDependent() || E->isValueDependent()) return;
@@ -6454,6 +6467,9 @@
return DiagnoseImpCast(S, E, T, CC, DiagID);
}
+ if (CheckPossibleTruncation(E, SourceRange, TargetRange))
+ return DiagnoseImpCast(S, E, T, CC, diag::warn_impcast_trunc);
+
// Diagnose conversions between different enumeration types.
// In C, we pretend that the type of an EnumConstantDecl is its enumeration
// type, to give us better diagnostics.
@@ -6482,6 +6498,33 @@
return;
}
+static void CheckExplicitConversion(Sema &S, Expr *E, QualType T,
+ SourceLocation CC) {
+ if (E->isTypeDependent() || E->isValueDependent())
+ return;
+
+ if (CC.isInvalid())
+ return;
+
+ const Type *Source = S.Context.getCanonicalType(E->getType()).getTypePtr();
+ const Type *Target = S.Context.getCanonicalType(T).getTypePtr();
+
+ if (Source == Target)
+ return;
+
+ if (Target->isDependentType())
+ return;
+
+ if (!Source->isIntegerType() || !Target->isIntegerType())
+ return;
+
+ IntRange SourceRange = GetExprRange(S.Context, E);
+ IntRange TargetRange = IntRange::forTargetOfCanonicalType(S.Context, Target);
+
+ if (CheckPossibleTruncation(E, SourceRange, TargetRange))
+ return DiagnoseImpCast(S, E, T, CC, diag::warn_impcast_trunc);
+}
+
void CheckConditionalOperator(Sema &S, ConditionalOperator *E,
SourceLocation CC, QualType T);
@@ -6571,7 +6614,13 @@
if (const OpaqueValueExpr *OVE = dyn_cast<OpaqueValueExpr>(E))
return AnalyzeImplicitConversions(S, OVE->getSourceExpr(), CC);
-
+
+ // Check explicit conversions for CStyle castings.
+ if (CStyleCastExpr *CStyleExpr = dyn_cast<CStyleCastExpr>(E)) {
+ CheckExplicitConversion(S, CStyleExpr->getSubExpr()->IgnoreParenImpCasts(),
+ T, CC);
+ }
+
// Skip past explicit casts.
if (isa<ExplicitCastExpr>(E)) {
E = cast<ExplicitCastExpr>(E)->getSubExpr()->IgnoreParenImpCasts();
Index: test/Sema/conversion.c
===================================================================
--- test/Sema/conversion.c (revision 223224)
+++ test/Sema/conversion.c (working copy)
@@ -429,3 +429,27 @@
ushort16 crCbScale = pairedConstants.s4; // expected-warning {{implicit conversion loses integer precision: 'uint32_t' (aka 'unsigned int') to 'ushort16'}}
ushort16 brBias = pairedConstants.s6; // expected-warning {{implicit conversion loses integer precision: 'uint32_t' (aka 'unsigned int') to 'ushort16'}}
}
+
+void f1(long l);
+void test28(int a, int b) {
+ long l = a * b; // expected-warning {{possible truncation before conversion from 'int' to 'long'}}
+ l = (long)a * b;
+
+ // implicit cast in function call
+ f1(a * 1000); // expected-warning {{possible truncation before conversion from 'int' to 'long'}}
+
+ // implicit cast inside calculation
+ int result = (int)((a * 1000) / 2LL); // expected-warning {{possible truncation before conversion from 'int' to 'long long'}}
+
+ long lAdd = a + b; // expected-warning {{possible truncation before conversion from 'int' to 'long'}}
+}
+
+void test29(unsigned int n) {
+ long a = (long)(n << 8); // expected-warning {{possible truncation before conversion from 'unsigned int' to 'long'}}
+ a = (long)n << 8;
+}
+
+long test30(int a) {
+ // implicit cast of return value to long
+ return a * 1000; // expected-warning {{possible truncation before conversion from 'int' to 'long'}}
+}
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
