1. Patchew
  2. linux
  3. [PATCH v2 0/5] Silence some clang -Wshorten-64-to-32 warnings
  4. View patch

[PATCH v2 2/5] bitmap: Silence a clang -Wshorten-64-to-32 warning

Ian Rogers posted 5 patches 9 months, 2 weeks ago
[PATCH v2 2/5] bitmap: Silence a clang -Wshorten-64-to-32 warning
Posted by Ian Rogers 9 months, 2 weeks ago 
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/bitmap.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/bitmap.h b/include/linux/bitmap.h
index 595217b7a6e7..4395e0a618f4 100644
--- a/include/linux/bitmap.h
+++ b/include/linux/bitmap.h
@@ -442,7 +442,7 @@ static __always_inline
 unsigned int bitmap_weight(const unsigned long *src, unsigned int nbits)
 {
 	if (small_const_nbits(nbits))
-		return hweight_long(*src & BITMAP_LAST_WORD_MASK(nbits));
+		return (int)hweight_long(*src & BITMAP_LAST_WORD_MASK(nbits));
 	return __bitmap_weight(src, nbits);
 }
 
-- 
2.49.0.906.g1f30a19c02-goog
Re: [PATCH v2 2/5] bitmap: Silence a clang -Wshorten-64-to-32 warning
Posted by Yury Norov 9 months, 1 week ago 
Hi Ian,

On Wed, Apr 30, 2025 at 10:15:31AM -0700, Ian Rogers 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.
> 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  include/linux/bitmap.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/linux/bitmap.h b/include/linux/bitmap.h
> index 595217b7a6e7..4395e0a618f4 100644
> --- a/include/linux/bitmap.h
> +++ b/include/linux/bitmap.h
> @@ -442,7 +442,7 @@ static __always_inline
>  unsigned int bitmap_weight(const unsigned long *src, unsigned int nbits)
>  {
>  	if (small_const_nbits(nbits))
> -		return hweight_long(*src & BITMAP_LAST_WORD_MASK(nbits));
> +		return (int)hweight_long(*src & BITMAP_LAST_WORD_MASK(nbits));

This should return unsigned int, I guess?

Also, most of the functions you touch here have their copies in tools.
Can you please keep them synchronized?

Thanks,
Yury

>  	return __bitmap_weight(src, nbits);
>  }
>  
> -- 
> 2.49.0.906.g1f30a19c02-goog
Re: [PATCH v2 2/5] bitmap: Silence a clang -Wshorten-64-to-32 warning
Posted by Ian Rogers 9 months, 1 week ago 
On Fri, May 2, 2025 at 9:03 AM Yury Norov <yury.norov@gmail.com> wrote:
>
> Hi Ian,
>
> On Wed, Apr 30, 2025 at 10:15:31AM -0700, Ian Rogers 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.
> >
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >  include/linux/bitmap.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/include/linux/bitmap.h b/include/linux/bitmap.h
> > index 595217b7a6e7..4395e0a618f4 100644
> > --- a/include/linux/bitmap.h
> > +++ b/include/linux/bitmap.h
> > @@ -442,7 +442,7 @@ static __always_inline
> >  unsigned int bitmap_weight(const unsigned long *src, unsigned int nbits)
> >  {
> >       if (small_const_nbits(nbits))
> > -             return hweight_long(*src & BITMAP_LAST_WORD_MASK(nbits));
> > +             return (int)hweight_long(*src & BITMAP_LAST_WORD_MASK(nbits));
>
> This should return unsigned int, I guess?

Hi Yury, I don't disagree. The issue there is that this could break
printf flags, etc. reliant on the return type. I've tried to keep the
patch minimal in this regard.

> Also, most of the functions you touch here have their copies in tools.
> Can you please keep them synchronized?

Yes, I do most of my work on the perf tool in the tools directory and
these patches come from adding -Wshorten-64-to-32 there due to a bug
found in ARM code that -Wshorten-64-to-32 would have caught:
https://lore.kernel.org/lkml/20250331172759.115604-1-leo.yan@arm.com/
The most recent patch series for tools is:
https://lore.kernel.org/linux-perf-users/20250430175036.184610-1-irogers@google.com/
However, I wanted to get the kernel versions of these headers agreed
before syncing them into the tools directory.

Thanks,
Ian



> Thanks,
> Yury
>
> >       return __bitmap_weight(src, nbits);
> >  }
> >
> > --
> > 2.49.0.906.g1f30a19c02-goog
Re: [PATCH v2 2/5] bitmap: Silence a clang -Wshorten-64-to-32 warning
Posted by Yury Norov 9 months, 1 week ago 
On Fri, May 02, 2025 at 09:43:12AM -0700, Ian Rogers wrote:
> On Fri, May 2, 2025 at 9:03 AM Yury Norov <yury.norov@gmail.com> wrote:
> >
> > Hi Ian,
> >
> > On Wed, Apr 30, 2025 at 10:15:31AM -0700, Ian Rogers 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.
> > >
> > > Signed-off-by: Ian Rogers <irogers@google.com>
> > > ---
> > >  include/linux/bitmap.h | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/include/linux/bitmap.h b/include/linux/bitmap.h
> > > index 595217b7a6e7..4395e0a618f4 100644
> > > --- a/include/linux/bitmap.h
> > > +++ b/include/linux/bitmap.h
> > > @@ -442,7 +442,7 @@ static __always_inline
> > >  unsigned int bitmap_weight(const unsigned long *src, unsigned int nbits)
> > >  {
> > >       if (small_const_nbits(nbits))
> > > -             return hweight_long(*src & BITMAP_LAST_WORD_MASK(nbits));
> > > +             return (int)hweight_long(*src & BITMAP_LAST_WORD_MASK(nbits));
> >
> > This should return unsigned int, I guess?
> 
> Hi Yury, I don't disagree. The issue there is that this could break
> printf flags, etc. reliant on the return type. I've tried to keep the
> patch minimal in this regard.

Not sure I understand... 

I mean just 
        return (unsigned int)hweight_long(...);

because the bitmap_weight return type is unsigned int. Do I miss
something?
 
> > Also, most of the functions you touch here have their copies in tools.
> > Can you please keep them synchronized?
> 
> Yes, I do most of my work on the perf tool in the tools directory and
> these patches come from adding -Wshorten-64-to-32 there due to a bug
> found in ARM code that -Wshorten-64-to-32 would have caught:
> https://lore.kernel.org/lkml/20250331172759.115604-1-leo.yan@arm.com/
> The most recent patch series for tools is:
> https://lore.kernel.org/linux-perf-users/20250430175036.184610-1-irogers@google.com/
> However, I wanted to get the kernel versions of these headers agreed
> before syncing them into the tools directory.

Yes, I'm in CC for that series, but I didn't find the changes for
bitmap_weight(), fls64() and other functions you touch in this series.
Anyways, it would be logical to sync tools with the mother kernel in
the same series.
Re: [PATCH v2 2/5] bitmap: Silence a clang -Wshorten-64-to-32 warning
Posted by Ian Rogers 9 months, 1 week ago 
On Fri, May 2, 2025 at 9:55 AM Yury Norov <yury.norov@gmail.com> wrote:
>
> On Fri, May 02, 2025 at 09:43:12AM -0700, Ian Rogers wrote:
> > On Fri, May 2, 2025 at 9:03 AM Yury Norov <yury.norov@gmail.com> wrote:
> > >
> > > Hi Ian,
> > >
> > > On Wed, Apr 30, 2025 at 10:15:31AM -0700, Ian Rogers 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.
> > > >
> > > > Signed-off-by: Ian Rogers <irogers@google.com>
> > > > ---
> > > >  include/linux/bitmap.h | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/include/linux/bitmap.h b/include/linux/bitmap.h
> > > > index 595217b7a6e7..4395e0a618f4 100644
> > > > --- a/include/linux/bitmap.h
> > > > +++ b/include/linux/bitmap.h
> > > > @@ -442,7 +442,7 @@ static __always_inline
> > > >  unsigned int bitmap_weight(const unsigned long *src, unsigned int nbits)
> > > >  {
> > > >       if (small_const_nbits(nbits))
> > > > -             return hweight_long(*src & BITMAP_LAST_WORD_MASK(nbits));
> > > > +             return (int)hweight_long(*src & BITMAP_LAST_WORD_MASK(nbits));
> > >
> > > This should return unsigned int, I guess?
> >
> > Hi Yury, I don't disagree. The issue there is that this could break
> > printf flags, etc. reliant on the return type. I've tried to keep the
> > patch minimal in this regard.
>
> Not sure I understand...
>
> I mean just
>         return (unsigned int)hweight_long(...);
>
> because the bitmap_weight return type is unsigned int. Do I miss
> something?

Oh sorry, my misunderstanding.

> > > Also, most of the functions you touch here have their copies in tools.
> > > Can you please keep them synchronized?
> >
> > Yes, I do most of my work on the perf tool in the tools directory and
> > these patches come from adding -Wshorten-64-to-32 there due to a bug
> > found in ARM code that -Wshorten-64-to-32 would have caught:
> > https://lore.kernel.org/lkml/20250331172759.115604-1-leo.yan@arm.com/
> > The most recent patch series for tools is:
> > https://lore.kernel.org/linux-perf-users/20250430175036.184610-1-irogers@google.com/
> > However, I wanted to get the kernel versions of these headers agreed
> > before syncing them into the tools directory.
>
> Yes, I'm in CC for that series, but I didn't find the changes for
> bitmap_weight(), fls64() and other functions you touch in this series.
> Anyways, it would be logical to sync tools with the mother kernel in
> the same series.

Ok, I'll add it. Fwiw, I'm not particularly fond of syncing the files
as it's not clear how to do it and keep the attribution/changes clear.
I've patches to do things like make the tools/include more hermetic,
but they've died a death on LKML:
https://lore.kernel.org/lkml/20201015223119.1712121-1-irogers@google.com/
Tbh, I want to completely change how tools/include works as perf and
other things casually use -I into the tools/include and
tools/include/uapi directories meaning strange things like the types.h
in these things is usually the linux/types.h rather than
uapi/linux/types.h. Amongst other things, the licenses on these files
are not the same. I think we should be building the
tools/include/linux and associated tools/lib files as if they were a
library and installing their headers as is done for libbpf, libsubcmd,
etc. The -I would then reflect the install_headers output path. I'm
less clear on the value of doing this for uapi.

Thanks,
Ian

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.