[PATCH] bitops: avoid clang shift-count-overflow warnings

[PATCH] bitops: avoid clang shift-count-overflow warnings

[Arnd Bergmann]

Clang normally does not warn about certain issues in inline functions when
it only happens in an eliminated code path. However if something else
goes wrong, it does tend to complain about the definition of hweight_long()
on 32-bit targets:

include/linux/bitops.h:75:41: error: shift count >= width of type [-Werror,-Wshift-count-overflow]
        return sizeof(w) == 4 ? hweight32(w) : hweight64(w);
                                               ^~~~~~~~~~~~
include/asm-generic/bitops/const_hweight.h:29:49: note: expanded from macro 'hweight64'
 define hweight64(w) (__builtin_constant_p(w) ? __const_hweight64(w) : __arch_hweight64(w))
                                                ^~~~~~~~~~~~~~~~~~~~
include/asm-generic/bitops/const_hweight.h:21:76: note: expanded from macro '__const_hweight64'
 define __const_hweight64(w) (__const_hweight32(w) + __const_hweight32((w) >> 32))
                                                                           ^  ~~
include/asm-generic/bitops/const_hweight.h:20:49: note: expanded from macro '__const_hweight32'
 define __const_hweight32(w) (__const_hweight16(w) + __const_hweight16((w) >> 16))
                                                ^
include/asm-generic/bitops/const_hweight.h:19:72: note: expanded from macro '__const_hweight16'
 define __const_hweight16(w) (__const_hweight8(w)  + __const_hweight8((w)  >> 8 ))
                                                                       ^
include/asm-generic/bitops/const_hweight.h:12:9: note: expanded from macro '__const_hweight8'
          (!!((w) & (1ULL << 2))) +     \

Adding an explicit cast to __u64 avoids that warning and makes it easier
to read other output.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 include/linux/bitops.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/bitops.h b/include/linux/bitops.h
index 9acf654f0b19..99f2ac30b1d9 100644
--- a/include/linux/bitops.h
+++ b/include/linux/bitops.h
@@ -72,7 +72,7 @@ static inline int get_bitmask_order(unsigned int count)
 
 static __always_inline unsigned long hweight_long(unsigned long w)
 {
-	return sizeof(w) == 4 ? hweight32(w) : hweight64(w);
+	return sizeof(w) == 4 ? hweight32(w) : hweight64((__u64)w);
 }
 
 /**
-- 
2.26.0

Re: [PATCH] bitops: avoid clang shift-count-overflow warnings

[Christian Brauner]

On Tue, May 05, 2020 at 03:54:57PM +0200, Arnd Bergmann wrote:
> Clang normally does not warn about certain issues in inline functions when
> it only happens in an eliminated code path. However if something else
> goes wrong, it does tend to complain about the definition of hweight_long()
> on 32-bit targets:
> 
> include/linux/bitops.h:75:41: error: shift count >= width of type [-Werror,-Wshift-count-overflow]
>         return sizeof(w) == 4 ? hweight32(w) : hweight64(w);
>                                                ^~~~~~~~~~~~
> include/asm-generic/bitops/const_hweight.h:29:49: note: expanded from macro 'hweight64'
>  define hweight64(w) (__builtin_constant_p(w) ? __const_hweight64(w) : __arch_hweight64(w))
>                                                 ^~~~~~~~~~~~~~~~~~~~
> include/asm-generic/bitops/const_hweight.h:21:76: note: expanded from macro '__const_hweight64'
>  define __const_hweight64(w) (__const_hweight32(w) + __const_hweight32((w) >> 32))
>                                                                            ^  ~~
> include/asm-generic/bitops/const_hweight.h:20:49: note: expanded from macro '__const_hweight32'
>  define __const_hweight32(w) (__const_hweight16(w) + __const_hweight16((w) >> 16))
>                                                 ^
> include/asm-generic/bitops/const_hweight.h:19:72: note: expanded from macro '__const_hweight16'
>  define __const_hweight16(w) (__const_hweight8(w)  + __const_hweight8((w)  >> 8 ))
>                                                                        ^
> include/asm-generic/bitops/const_hweight.h:12:9: note: expanded from macro '__const_hweight8'
>           (!!((w) & (1ULL << 2))) +     \
> 
> Adding an explicit cast to __u64 avoids that warning and makes it easier
> to read other output.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---

Acked-by: Christian Brauner <christian.brauner@ubuntu.com>

Re: [PATCH] bitops: avoid clang shift-count-overflow warnings

[Andy Shevchenko]

On Tue, May 5, 2020 at 4:58 PM Arnd Bergmann <arnd@arndb.de> wrote:
>
> Clang normally does not warn about certain issues in inline functions when
> it only happens in an eliminated code path. However if something else
> goes wrong, it does tend to complain about the definition of hweight_long()
> on 32-bit targets:

Shouldn't it be fixed in CLang?

> include/linux/bitops.h:75:41: error: shift count >= width of type [-Werror,-Wshift-count-overflow]
>         return sizeof(w) == 4 ? hweight32(w) : hweight64(w);
>                                                ^~~~~~~~~~~~

sizeof(w) is compile-time constant. It can easily drop the second part
without even looking at it.

> Adding an explicit cast to __u64 avoids that warning and makes it easier
> to read other output.

Looks like papering over the real issue.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] bitops: avoid clang shift-count-overflow warnings

[Arnd Bergmann]

On Tue, May 5, 2020 at 4:08 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Tue, May 5, 2020 at 4:58 PM Arnd Bergmann <arnd@arndb.de> wrote:
> > Clang normally does not warn about certain issues in inline functions when
> > it only happens in an eliminated code path. However if something else
> > goes wrong, it does tend to complain about the definition of hweight_long()
> > on 32-bit targets:
>
> Shouldn't it be fixed in CLang?
>
> > include/linux/bitops.h:75:41: error: shift count >= width of type [-Werror,-Wshift-count-overflow]
> >         return sizeof(w) == 4 ? hweight32(w) : hweight64(w);
> >                                                ^~~~~~~~~~~~
>
> sizeof(w) is compile-time constant. It can easily drop the second part
> without even looking at it.
>
> > Adding an explicit cast to __u64 avoids that warning and makes it easier
> > to read other output.
>
> Looks like papering over the real issue.

I'm not sure if there is anything to be done about it in clang, since it
always does syntactic analysis before dead-code elimination by design.

It is a bit odd though that it only prints the warning sometimes, but
I suspect this is also something that works as designed. Maybe someone
on the clang-built-linux list knows more about the background.

         Arnd

Re: [PATCH] bitops: avoid clang shift-count-overflow warnings

[Nick Desaulniers]

On Tue, May 5, 2020 at 8:33 AM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Tue, May 5, 2020 at 4:08 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Tue, May 5, 2020 at 4:58 PM Arnd Bergmann <arnd@arndb.de> wrote:
> > > Clang normally does not warn about certain issues in inline functions when
> > > it only happens in an eliminated code path. However if something else
> > > goes wrong, it does tend to complain about the definition of hweight_long()
> > > on 32-bit targets:
> >
> > Shouldn't it be fixed in CLang?
> >
> > > include/linux/bitops.h:75:41: error: shift count >= width of type [-Werror,-Wshift-count-overflow]
> > >         return sizeof(w) == 4 ? hweight32(w) : hweight64(w);
> > >                                                ^~~~~~~~~~~~
> >
> > sizeof(w) is compile-time constant. It can easily drop the second part
> > without even looking at it.
> >
> > > Adding an explicit cast to __u64 avoids that warning and makes it easier
> > > to read other output.
> >
> > Looks like papering over the real issue.
>
> I'm not sure if there is anything to be done about it in clang, since it
> always does syntactic analysis before dead-code elimination by design.

That's pretty much it.  We had a patch to Clang to use delayed
diagnostics to delay emitting the warning in case the AST node was
dropped, but it wasn't accepted in code review.

>
> It is a bit odd though that it only prints the warning sometimes, but

Sometimes?

> I suspect this is also something that works as designed. Maybe someone
> on the clang-built-linux list knows more about the background.


-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] bitops: avoid clang shift-count-overflow warnings

[Arnd Bergmann]

On Tue, May 5, 2020 at 5:36 PM 'Nick Desaulniers' via Clang Built
Linux <clang-built-linux@googlegroups.com> wrote:
> On Tue, May 5, 2020 at 8:33 AM Arnd Bergmann <arnd@arndb.de> wrote:
> >
> > I'm not sure if there is anything to be done about it in clang, since it
> > always does syntactic analysis before dead-code elimination by design.
>
> That's pretty much it.  We had a patch to Clang to use delayed
> diagnostics to delay emitting the warning in case the AST node was
> dropped, but it wasn't accepted in code review.
>
> >
> > It is a bit odd though that it only prints the warning sometimes, but
>
> Sometimes?

Well, the file is included everywhere in the kernel, but we normally
don't get the
warning at all. However, I sometimes make incorrect changes to one file that
cause some other warning, and the result is an stream of warnings about
things like this one that are normally hidden. The shift count warning is the
one that shows up the most. I should try to come up with a better way to
reproduce it.

        Arnd