Re: [Qemu-devel] [PATCH v2 for 2.5] QEMU does not care about left shifts of signed negative values

[Laszlo Ersek <lersek@redhat.com>]

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.maydell@linaro.org>
> Cc: Markus Armbruster <armbru@redhat.com>
> Cc: 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 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 <lersek@redhat.com>

(I realize the flag for the pointer wraparound has been separated; this
is strictly about shifts.)

Thanks
Laszlo