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/