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.

Signed-off-by: Ian Rogers <irogers@google.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;
-- 
2.49.0.906.g1f30a19c02-goog

On Wed, 30 Apr 2025 10:15:33 -0700
Ian Rogers <irogers@google.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.

	David


> 
> Signed-off-by: Ian Rogers <irogers@google.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;

On Thu, May 1, 2025 at 1:07 PM David Laight
<david.laight.linux@gmail.com> wrote:
>
> On Wed, 30 Apr 2025 10:15:33 -0700
> Ian Rogers <irogers@google.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
```
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;
```

Thanks,
Ian

>         David
>
>
> >
> > Signed-off-by: Ian Rogers <irogers@google.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;
>

On Thu, 1 May 2025 13:15:30 -0700
Ian Rogers <irogers@google.com> wrote:

> On Thu, May 1, 2025 at 1:07 PM David Laight
> <david.laight.linux@gmail.com> wrote:
> >
> > On Wed, 30 Apr 2025 10:15:33 -0700
> > Ian Rogers <irogers@google.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.
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).

> ```
> 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.

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.

	David

> ```
> 
> Thanks,
> Ian
> 
> >         David
> >
> >  
> > >
> > > Signed-off-by: Ian Rogers <irogers@google.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;  
> >  

On Thu, May 1, 2025 at 1:27 PM David Laight
<david.laight.linux@gmail.com> wrote:
>
> On Thu, 1 May 2025 13:15:30 -0700
> Ian Rogers <irogers@google.com> wrote:
>
> > On Thu, May 1, 2025 at 1:07 PM David Laight
> > <david.laight.linux@gmail.com> wrote:
> > >
> > > On Wed, 30 Apr 2025 10:15:33 -0700
> > > Ian Rogers <irogers@google.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@google.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;
> > >
>

On Thu, 1 May 2025 14:11:59 -0700
Ian Rogers <irogers@google.com> wrote:

....
> 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/

That code is wrong with or without the (s32) cast.
And the explicit cast will hide the compiler warning.

If you want the compiler to find bugs you need to reduce the number
of casts to an absolute minimum and disable/fix the compiler warning
for false positives.

These type based (rather than value domain) warnings are all a PITA.

Another example is the 'signed v unsigned compare' which bleats for:
	int rval = read(... sizeof (foo));
	if (rval < 0)
		return -1;
	if (rval != sizeof (foo))
		// truncated
Whereas a statically_true(rval >= 0) test will pass.

	David