[PATCH] Dangerous long casts

[Aaron Ballman]

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
<Anders.Ronnholm at evidente.se> 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
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>