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

[Segher Boessenkool <segher@kernel.crashing.org>]

On Wed, Feb 23, 2022 at 11:23:39AM -0800, Linus Torvalds 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?
> 
> 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).
> 
> 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.

You won't get this confusion if you write -1 instead.  Not that that
helps your problem here.

The GCC documentation says (at
<https://gcc.gnu.org/onlinedocs/gcc/Integers-implementation.html>):

  - The results of some bitwise operations on signed integers (C90 6.3,
    C99 and C11 6.5).

    Bitwise operators act on the representation of the value including
    both the sign and value bits, where the sign bit is considered
    immediately above the highest-value value bit. Signed ‘>>’ acts on
    negative numbers by sign extension.

    As an extension to the C language, GCC does not use the latitude
    given in C99 and C11 only to treat certain aspects of signed ‘<<’ as
    undefined. However, -fsanitize=shift (and -fsanitize=undefined) will
    diagnose such cases. They are also diagnosed where constant
    expressions are required.

But in fact they are diagnosed more often:
===
unsigned int n;
int f0(int x) { return x & (~0 << 5); }
int f1(int x) { return x & (-1 << 5); }
int f2(int x) { return x & (~0 << n); }
int f3(int x) { return x & (-1 << n); }
===
gets a warning on every line:
===
shifty.c: In function 'f0':
shifty.c:2:32: warning: left shift of negative value [-Wshift-negative-value]
    2 | int f0(int x) { return x & (~0 << 5); }
      |                                ^~
shifty.c: In function 'f1':
shifty.c:3:32: warning: left shift of negative value [-Wshift-negative-value]
    3 | int f1(int x) { return x & (-1 << 5); }
      |                                ^~
shifty.c: In function 'f2':
shifty.c:4:32: warning: left shift of negative value [-Wshift-negative-value]
    4 | int f2(int x) { return x & (~0 << n); }
      |                                ^~
shifty.c: In function 'f3':
shifty.c:5:32: warning: left shift of negative value [-Wshift-negative-value]
    5 | int f3(int x) { return x & (-1 << n); }
      |                                ^~
===
(with -O2 -Wall -W, nothing more).  No constant expression is required
in any of those cases, and in f2 and f3 the shift is not a constant
expression even.

> 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?

-Werror=*anything* is detrimental, *always*.  Warnings are warnings
and errors are errors.  Warnings have false positives: if not, they
should be errors instead!

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

Yes.

The only reason the warning exists is because it is undefined behaviour
(not implementation-defined or anything).  The reason it is that in the
standard is that it is hard to implement and even describe for machines
that are not two's complement.  However relevant that is today :-)


Segher