On 11/17/15 15:09, Paolo Bonzini wrote: > 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. > > Thanks to everyone involved in the discussion! > > Cc: Peter Maydell <peter.mayd...@linaro.org> > Cc: Markus Armbruster <arm...@redhat.com> > Cc: Laszlo Ersek <ler...@redhat.com> > Signed-off-by: Paolo Bonzini <pbonz...@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 6bfa6f5..aa0a6c8 100755 > --- a/configure > +++ b/configure > @@ -380,7 +380,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" > @@ -1428,7 +1428,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 >
I accept this is a defensible, maybe even reasonable choice to make in the QEMU project. On the other hand, I personally cannot stop hating shifting negative values (any direction) -- indeed, the *original* code from <https://github.com/madler/zlib/pull/112> makes me barf too. Therefore, Grudgingly-reviewed-by: Laszlo Ersek <ler...@redhat.com> (I realize the flag for the pointer wraparound has been separated; this is strictly about shifts.) Thanks Laszlo
