Message-ID: <CAP-5=fVjAR0g=wN8sWetHoNWdoDVGNoKb8d8UdwxF_te=wmMLA@mail.gmail.com>
Date: Thu, 1 May 2025 14:11:59 -0700
From: Ian Rogers <irogers@...gle.com>
To: David Laight <david.laight.linux@...il.com>
Cc: Yury Norov <yury.norov@...il.com>, Rasmus Villemoes <linux@...musvillemoes.dk>, 
	Arnd Bergmann <arnd@...db.de>, Nathan Chancellor <nathan@...nel.org>, 
	Nick Desaulniers <nick.desaulniers+lkml@...il.com>, Bill Wendling <morbo@...gle.com>, 
	Justin Stitt <justinstitt@...gle.com>, Adrian Hunter <adrian.hunter@...el.com>, 
	Thomas Gleixner <tglx@...utronix.de>, Jakub Kicinski <kuba@...nel.org>, 
	Jacob Keller <jacob.e.keller@...el.com>, linux-arch@...r.kernel.org, 
	linux-kernel@...r.kernel.org, llvm@...ts.linux.dev, Leo Yan <leo.yan@....com>
Subject: Re: [PATCH v2 4/5] math64: Silence a clang -Wshorten-64-to-32 warning

On Thu, May 1, 2025 at 1:27 PM David Laight
<david.laight.linux@...il.com> wrote:
>
> On Thu, 1 May 2025 13:15:30 -0700
> Ian Rogers <irogers@...gle.com> wrote:
>
> > On Thu, May 1, 2025 at 1:07 PM David Laight
> > <david.laight.linux@...il.com> wrote:
> > >
> > > On Wed, 30 Apr 2025 10:15:33 -0700
> > > Ian Rogers <irogers@...gle.com> wrote:
> > >
> > > > The clang warning -Wshorten-64-to-32 can be useful to catch
> > > > inadvertent truncation. In some instances this truncation can lead to
> > > > changing the sign of a result, for example, truncation to return an
> > > > int to fit a sort routine. Silence the warning by making the implicit
> > > > truncation explicit. This isn't to say the code is currently incorrect
> > > > but without silencing the warning it is hard to spot the erroneous
> > > > cases.
> > >
> > > Except that the extra casts make the reader think something 'extra'
> > > is going on.
> > > For readability you want as few casts as possible.
> >
> > Agreed except when not having the cast can introduce bugs which is why
> > the cast is always required in other languages. Consider in Java:
> > ```
> > class a {
> >   public static void main(String args[]) {
> >      long x = args.length;
> >      int y = x;
> >  }
> > }
> > $ javac a.java
> > a.java:4: error: incompatible types: possible lossy conversion from long to int
> >      int y = x;
> >              ^
> > 1 error
>
> I'm not a java expert, but I suspect it has 'softer' type conversions
> for integers than C casts.

Sorry I don't understand what you're saying. Java certainly has bugs
in this area which is why I've written checkers like:
https://errorprone.info/bugpattern/BadComparable
For code similar to:
```
s32 compare(s64 a, s64 b) { return (s32)(a - b); }
```
where the truncation is going to throw away the sign of the subtract
and is almost certainly a bug. This matches the bugs that are fixed in
this patch series for the perf code, in particular an issue on ARM
that Leo Yan originally provided the fix for:
https://lore.kernel.org/lkml/20250331172759.115604-1-leo.yan@arm.com/

> I've been badly bitten by C casts that make code compile when it really
> shouldn't, or incorrectly mask off high bits.
> There are actually loads of them in the Linux kernel.
> As well as all the dubious min_t(u16,...) there are the (__force ...)
> where the compiler shouldn't see a cast at all (it is for sparse).

Are you arguing for or against checks here? It seems to be about
casts. I'm not getting you.

> > ```
> > Having -Wshorten-64-to-32 enabled for building with clang would allow
> > possible mistakes to be spotted, but that's not currently possible
> > without wading through warnings that this series cleans up.
> >
> > I also don't really think anyone will be confused about the purpose of
> > the cast in something like:
> > ```
> > al = (u32)a;
>
> And no one is confused by what the code is doing without the cast.

Someone who saw that `a` was 64-bit may assume from the assignment
that `al` were also 64-bit. The cast is making explicit that you want
to throw away bits after the bottom 32, so I'd disagree.

> We live with the 'integer to pointer of differ size' warning,
> but even that was only really useful 30 years ago and is well
> past its 'best before' date.

You want C to be weakly typed more than it is? Not sure and it seems
we've drifted far from the topic of the patch series.

Thanks,
Ian

>         David
>
> > ```
> >
> > Thanks,
> > Ian
> >
> > >         David
> > >
> > >
> > > >
> > > > Signed-off-by: Ian Rogers <irogers@...gle.com>
> > > > ---
> > > >  include/linux/math64.h | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/include/linux/math64.h b/include/linux/math64.h
> > > > index 6aaccc1626ab..f32fcb2a2331 100644
> > > > --- a/include/linux/math64.h
> > > > +++ b/include/linux/math64.h
> > > > @@ -179,7 +179,7 @@ static __always_inline u64 mul_u64_u64_shr(u64 a, u64 mul, unsigned int shift)
> > > >  #ifndef mul_u64_u32_shr
> > > >  static __always_inline u64 mul_u64_u32_shr(u64 a, u32 mul, unsigned int shift)
> > > >  {
> > > > -     u32 ah = a >> 32, al = a;
> > > > +     u32 ah = a >> 32, al = (u32)a;
> > > >       u64 ret;
> > > >
> > > >       ret = mul_u32_u32(al, mul) >> shift;
> > >
>

Powered by blists - more mailing lists