Re: [RFC PATCH 03/13] usb: remove the usage of the list iterator after the loop

[Arnd Bergmann <arnd@arndb.de>]

On Wed, Feb 23, 2022 at 8:23 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Wed, Feb 23, 2022 at 10:47 AM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > Arnd - remind me, please.. Was there some other problem than just gcc-4.9?


I'm pretty sure this was the only thing. And not  even because gcc-4.9 didn't
support later standards, but because it caused some false-postivee
warnings like

In file included from ../drivers/usb/core/notify.c:16:0:
../include/linux/notifier.h:117:9: error: initializer element is not constant
  struct blocking_notifier_head name =   \
         ^
../drivers/usb/core/notify.c:21:8: note: in expansion of macro
'BLOCKING_NOTIFIER_HEAD'
 static BLOCKING_NOTIFIER_HEAD(usb_notifier_list);
        ^
../include/linux/notifier.h:117:9: error: (near initialization for
'usb_notifier_list.rwsem.wait_lock')
  struct blocking_notifier_head name =   \
         ^
../drivers/usb/core/notify.c:21:8: note: in expansion of macro
'BLOCKING_NOTIFIER_HEAD'
 static BLOCKING_NOTIFIER_HEAD(usb_notifier_list);
        ^

> Hmm. Interesting. I decided to just try it and see for the compiler I
> have, and changing the gnu89 to gnu99 I get new warnings
> (-Werror=shift-negative-value).

FWIW, I think we can go straight to gnu11 in this case even with gcc-5, no
need to take gnu99 as an intermediate step. I don't know if there is a
practical difference between the two though.

gcc-8 and higher also support --std=gnu18 and --std=gnu2x, which also
work for building the kernel but would break gcc-5/6/7 support

> Very annoying.  Especially since negative values are in many contexts
> actually *safer* than positive ones when used as a mask, because as
> long as the top bit is set in 'int', if the end result is then
> expanded to some wider type, the top bit stays set.
>
> Example:
>
>   unsigned long mask(unsigned long x)
>   { return x & (~0 << 5); }
>
>   unsigned long badmask(unsigned long x)
>   { return x & (~0u << 5); }
>
> One does it properly, the other is buggy.
>
> Now, with an explicit "unsigned long" like this, some clueless
> compiler person  might say "just use "~0ul", but that's completely
> wrong - because quite often the type is *not* this visible, and the
> signed version works *regardless* of type.
>
> So this Werror=shift-negative-value warning seems to be actively
> detrimental, and I'm not seeing the reason for it. Can somebody
> explain the thinking for that stupid warning?
>
> That said, we seem to only have two cases of it in the kernel, at
> least by a x86-64 allmodconfig build. So we could examine the types
> there, or we could just add '-Wno-shift-negative-value".

I looked at the gcc documentation for this flag, and it tells me that
it's default-enabled for std=c99 or higher. Turning it on for --std=gnu89
shows the same warning, so at least it doesn't sound like the actual
behavior changed, only the warning output. clang does not warn
for this code at all, regardless of the --std= flag.

        Arnd