[RFC] Fix undefined behavior with left shifts in whole code base - coreboot

12 Feb 2018

      Dear coreboot folks,
TLDR; If coreboot wants to *make use of the undefined baviour sanitizer
(UBSAN)* a solution needs to be found for left shift errors like in `1
<< 31`, where the sign bit is ignored. The proposal is to adapt the
code accordingly, that means `1U << 31`, although it’s not so pretty.
=== Longer version ===
Trying an image built with the undefined behavior sanitizer (UBSAN) for
the ASRock E350M1, it stops with the error below.
```
runtime error: left shift of 255 by 24 places cannot be represented in type 'int'
```
Please find the code below.
```
$ nl -ba ./src/southbridge/amd/cimx/sb800/ramtop.c | tail -11
    41	{
    42		u32 xdata = 0;
    43		int xnvram_pos = 0xf8, xi;
    44		for (xi = 0; xi < 4; xi++) {
    45			outb(xnvram_pos, BIOSRAM_INDEX);
    46			xdata &= ~(0xff << (xi * 8));
    47			xdata |= inb(BIOSRAM_DATA) << (xi *8);
    48			xnvram_pos++;
    49		}
    50		return xdata;
    51	}
```
The reason is, that the integer is not big enough to hold the left
shift in `~(0xff << (xi * 8)) with `xi = 3``.
That caused me to look more into such things, and GCC and Clang are
able to find quite some of these issues during compile time. GCC needs
`-Wshift-overflow=2` [1][2]. Right now, that warning is disabled for
Clang for example.
Please find the documentation from GCC (`man gcc`) below.
...
   -Wshift-overflow
   -Wshift-overflow=n
       Warn about left shift overflows.  This warning is enabled by default in C99 and C++11 modes (and newer).

       -Wshift-overflow=1
           This is the warning level of -Wshift-overflow and is enabled by default in C99 and C++11 modes (and newer).  This warning level does not warn about left-shifting 1 into the sign bit.
           (However, in C, such an overflow is still rejected in contexts where an integer constant expression is required.)

       -Wshift-overflow=2
           This warning level also warns about left-shifting 1 into the sign bit, unless C++14 mode is active.

That brings up a lot of problems mostly for `1 << 31` used quite a lot
in the coreboot code base.
The question is, how to deal with these. Julius commented quite
pointedly [2].
...
Everyone likes to write (1 << x), and having to change them all to
(1U << x) would not be that great for readability. But making this
different based on whether it touches bit 31 or not is also super
inconsistent. I think if this warning is benign, I'd be more inclined
to just disable it.
I’d really like to *enable as much warnings* as possible to let the
build tools *catch as much errors as possible*, and having to adapt the
code to work with this is in my opinion worth the effort. If we wanted
to work around it by for example ignoring constant shift errors, we
would probably need to adapt the UBSAN for GCC and Clang, and add a new
switch also.
So, I propose to only add the type specifier in the cases, where an
error is thrown. That’s stylistically not so pretty, but would be easy
to implement, as the build tools would find the affected places without
problems, and those can be changed then.
In addtion to the already pushed change-sets, I would submit a patch
fixing all `1 << 31` and `3 << 30` occurrences, and separate change-
sets for the rest. After that, the warning can be enabled for the whole
code base.
What do you think, and what other options do you see?
Kind regards,
Paul
[1] https://review.coreboot.org/#/c/coreboot/+/23360/
[2] https://review.coreboot.org/#/c/coreboot/+/23361/