[Qemu-devel] [PULL 2/9] QEMU does not care about left shifts of signed negative values

[Paolo Bonzini <pbonzini@redhat.com>]

It seems like there's no good reason for the compiler to exploit the
undefinedness of left shifts.  GCC explicitly documents that they do not
use at all this possibility and, while they also say this is subject
to change, they have been saying this for 10 years (since the wording
appeared in the GCC 4.0 manual).

Any workaround for this particular case of undefined behavior uglifies the
code.  Using unsigned is unsafe (https://github.com/madler/zlib/pull/112
is the proof) because the value becomes positive when extended; -(a << b)
works but does not express that the intention is to compute -a * 2^N,
especially if "a" is a constant.

<rant>
The straw that broke the camel back is Clang's latest obnoxious,
pointless, unsafe---and did I mention *totally* useless---warning about
left shifting a negative integer.  It's obnoxious and pointless because
the compiler is not using the latitude that the standard gives it, so
the warning just adds noise.  It is useless and unsafe because it does
not catch the widely more common case where the LHS is a variable, and
thus gives a false sense of security.  The noisy nature of the warning
means that it should have never been added to -Wall.  The uselessness
means that it probably should not have even been added to -Wextra.

(It would have made sense to enable the warning for -fsanitize=shift,
as the program would always crash if the point is reached.  But this was
probably too sophisticated to come up with, when you're so busy giving
birth to gems such as -Wabsolute-value).
</rant>

Ubsan also has warnings for undefined behavior of left shifts.  Checks for
left shift overflow and left shift of negative numbers, unfortunately,
cannot be silenced without also silencing the useful ones about out-of-range
shift amounts. -fwrapv ought to shut them up, but doesn't yet
(https://llvm.org/bugs/show_bug.cgi?id=25552; I am taking care of fixing
the same issues in GCC).  Luckily ubsan is optional, and the easy
workaround is to use -fsanitize-recover.

Anyhow, this patch documents our assumptions explicitly, and shuts up the
stupid warning.  -fwrapv is a bit of a heavy hammer, but it is the safest
option and it ought to just work long term as the compilers improve.
Note that -fstrict-overflow does not silence ubsan's overflow warnings,
hence it's reasonable to assume that it won't silence the left shift
warnings either.  QEMU doesn't rely on pointer overflow anyway, and
that's the other major difference between -fwrapv (which only cares
about integer overflow) and -fstrict-overflow.

Thanks to everyone involved in the discussion!

Cc: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Grudgingly-reviewed-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 HACKING   | 6 ++++++
 configure | 4 ++--
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/HACKING b/HACKING
index 12fbc8a..71ad23b 100644
--- a/HACKING
+++ b/HACKING
@@ -157,3 +157,9 @@ painful. These are:
  * you may assume that integers are 2s complement representation
  * you may assume that right shift of a signed integer duplicates
    the sign bit (ie it is an arithmetic shift, not a logical shift)
+
+In addition, QEMU assumes that the compiler does not use the latitude
+given in C99 and C11 to treat aspects of signed '<<' as undefined, as
+documented in the GNU Compiler Collection manual starting at version 4.0.
+If a compiler does not respect this when passed the -fwrapv option,
+it is not supported for compilation of QEMU.
diff --git a/configure b/configure
index 71d6cbc..5bb8187 100755
--- a/configure
+++ b/configure
@@ -413,7 +413,7 @@ sdl2_config="${SDL2_CONFIG-${cross_prefix}sdl2-config}"
 ARFLAGS="${ARFLAGS-rv}"
 
 # default flags for all hosts
-QEMU_CFLAGS="-fno-strict-aliasing -fno-common $QEMU_CFLAGS"
+QEMU_CFLAGS="-fno-strict-aliasing -fno-common -fwrapv $QEMU_CFLAGS"
 QEMU_CFLAGS="-Wall -Wundef -Wwrite-strings -Wmissing-prototypes $QEMU_CFLAGS"
 QEMU_CFLAGS="-Wstrict-prototypes -Wredundant-decls $QEMU_CFLAGS"
 QEMU_CFLAGS="-D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE $QEMU_CFLAGS"
@@ -1461,7 +1461,7 @@ fi
 gcc_flags="-Wold-style-declaration -Wold-style-definition -Wtype-limits"
 gcc_flags="-Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers $gcc_flags"
 gcc_flags="-Wmissing-include-dirs -Wempty-body -Wnested-externs $gcc_flags"
-gcc_flags="-Wendif-labels $gcc_flags"
+gcc_flags="-Wendif-labels -Wno-shift-negative-value $gcc_flags"
 gcc_flags="-Wno-initializer-overrides $gcc_flags"
 gcc_flags="-Wno-string-plus-int $gcc_flags"
 # Note that we do not add -Werror to gcc_flags here, because that would
-- 
1.8.3.1