Re: [PATCH v1 09/48] perf tests: Silence -Wshorten-64-to-32 warnings

[Ian Rogers]

On Wed, Apr 2, 2025 at 3:01 PM David Laight
<david.laight.linux@xxxxxxxxx> wrote:
>
> On Wed, 2 Apr 2025 17:38:07 +0100
> Leo Yan <leo.yan@xxxxxxx> wrote:
>
> > Hi Ian,
> ...
> > If we use casts just to silence warnings, we also lose the opportunity
> > to improve code quality.  The changes in this series that fix type
> > mismatches are good, but I think the use of casts is not particularly
> > helpful - we're simply switching from implicit compiler casts to
> > explicit ones.
>
> It is certainly my experience (a lot of years) that casts between
> integers of different sizes just make code harder to read and possibly
> even more buggy.
>
> If the compiler really knew the valid range of the value (rather than
> just the type) then perhaps a warning that significant bits were being
> discarded might be more reasonable.
> But even then you don't want anything for code like:
>         hi_32 = val_64 >> 32;
>         lo_32 = val_64;

There is an instance of this in:
https://lore.kernel.org/lkml/20250401182347.3422199-3-irogers@xxxxxxxxxx/

The particular problem that Leo Yan [1] found in the code base is if a
compare function like:

  int cmp(long *a, long *b)
  {
      return *a - *b;
  }

which are typically passed to routines like list_sort, qsort or
bsearch. The subtract is potentially 64-bit and the implicit cast to
32-bit loses the sign information. Generally the problem is more
opaque than this as here you can quite easily see the types of "a" and
"b". If we say we don't warn for implicit truncating assignments then:

  int cmp(long *a, long *b)
  {
      int ret = *a - *b;
      return ret;
  }

becomes valid, but it is identical and as broken as the code before.
I'm happy for a better alternative than clang's `-Wshorten-64-to-32`
but haven't found it and I agree it's unfortunate for the churn it
kicks up.

As an aside, I wrote a Java check for this as I found a similar bug in
Android. That check found instances where the subtract and truncate
were happening in a cache that would sort elements deleting the
oldest. Unfortunately the truncation meant it could also be deleting
the newest elements. In Java lossy conversions require a cast so I
could target the warning on casts within compare routines. C of course
has given us a good way to shoot ourselves in the foot with implicit
conversion and -Wshorten-64-to-32 seems to be a good way of avoiding
that particular foot cannon. Making ordering routines use a less-than
comparison avoids the problem in C++.

Thanks,
Ian

[1] https://lore.kernel.org/lkml/20250331172759.115604-1-leo.yan@xxxxxxx/
>     David