[PATCH] Dangerous long casts

[Aaron Ballman]

On Thu, Dec 4, 2014 at 8:45 AM, Anders Rönnholm
<Anders.Ronnholm at evidente.se> wrote:
> 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.

Some minor nits below, but generally LGTM. I think Richard should take
a look as well -- my only concern is adding a warning that's
off-by-default (we usually try to avoid that unless there's
significant benefit). I think this patch has benefit, I've seen it
implemented by other vendors as well. However, Richard may have other
opinions.

> 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);
> +  }

Can elide the braces.

> +
>    // 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'}}
> +}

Can still use some template tests, since the warning behavior changes
based on value/type dependence.

Thanks!

~Aaron