Re: [Qemu-devel] [PULL 2/9] QEMU does not care about left shifts of signed negative values
On 25 November 2015 at 21:05, Paolo Bonzini wrote: > > > On 25/11/2015 20:54, Peter Maydell wrote: >> > > Your latest patch at >> > > https://gcc.gnu.org/ml/gcc-patches/2015-11/msg03055.html >> > > doesn't seem to touch the documentation of -fwrapv at all, so I >> > > don't think it is sufficient to allow users of the compiler >> > > to say "-fwrapv means signed behaviour for shifts". >> > >> > GCC *always* does signed behavior for shifts, even without -fwrapv. >> > I'll commit tomorrow the patch that promises that for the future. >> > >> > GCC does not need -fwrapv at all. >> >> Yes it does, because without -fwrapv it still wants to warn >> about them. We need to tell the compiler that we really do >> want a C dialect where they have specific behaviour and so no >> warnings are ever correct. By default (as documented, even >> with your patch) GCC just promises that it won't actually >> do undefined behaviour for signed negative shifts. (It doesn't >> even actually say what impdef semantics it does provide, > > It says it above the text I changed: > > GCC supports only two's complement integer types, and all bit > patterns are ordinary values. > [...] > Bitwise operators act on the representation of the value including > both the sign and value bits, where the sign bit is considered > immediately above the highest-value value bit. Oops, yes. I misread the doc there. >> and in practice the impdef semantics include "warn about >> this", which we don't want.) > > No, it doesn't warn with the commonly used options. "They are also diagnosed where constant expressions are required." strongly implies "we feel free to warn about this usage". Constant expressions are required all over the place... > Only with -pedantic, which is documented as > > Issue all the warnings demanded by strict ISO C and ISO C++; > reject all programs that use forbidden extensions, and some > other programs that do not follow ISO C and ISO C++. > > So the combination of -fwrapv and -pedantic is not particularly > interesting. Even then the warning is "initializer element is not > a constant expression"; nothing to do with overflow. For example: > > #define INT_MIN ((int)-0x8000) > int y = INT_MIN - 1; > int z = -1 << 2; > int w = INT_MIN << 1; > int u = 1 << 31; > > $ gcc f.c -std=c11 -Wall -pedantic > f.c:2:1: warning: overflow in constant expression [-Woverflow] > f.c:3:9: warning: initializer element is not a constant expression > [-Wpedantic] > f.c:4:9: warning: initializer element is not a constant expression > [-Wpedantic] > f.c:5:9: warning: initializer element is not a constant expression > [-Wpedantic] > > The first warning is activated by -Wall, the others aren't. Unless the C standard specifically requires the warning text as written, I'd say these are pretty terrible warnings, since they don't actually diagnose the problem with the code. But we don't use -pedantic so I don't care about them specifically. I just care that the documentation should say clearly "we guarantee these semantics; we won't warn unless you specifically ask us to warn about their use". thanks -- PMM
Re: [Qemu-devel] [PULL 2/9] QEMU does not care about left shifts of signed negative values
On 25/11/2015 20:54, Peter Maydell wrote: > > > Your latest patch at > > > https://gcc.gnu.org/ml/gcc-patches/2015-11/msg03055.html > > > doesn't seem to touch the documentation of -fwrapv at all, so I > > > don't think it is sufficient to allow users of the compiler > > > to say "-fwrapv means signed behaviour for shifts". > > > > GCC *always* does signed behavior for shifts, even without -fwrapv. > > I'll commit tomorrow the patch that promises that for the future. > > > > GCC does not need -fwrapv at all. > > Yes it does, because without -fwrapv it still wants to warn > about them. We need to tell the compiler that we really do > want a C dialect where they have specific behaviour and so no > warnings are ever correct. By default (as documented, even > with your patch) GCC just promises that it won't actually > do undefined behaviour for signed negative shifts. (It doesn't > even actually say what impdef semantics it does provide, It says it above the text I changed: GCC supports only two's complement integer types, and all bit patterns are ordinary values. [...] Bitwise operators act on the representation of the value including both the sign and value bits, where the sign bit is considered immediately above the highest-value value bit. > and in practice the impdef semantics include "warn about > this", which we don't want.) No, it doesn't warn with the commonly used options. Only with -pedantic, which is documented as Issue all the warnings demanded by strict ISO C and ISO C++; reject all programs that use forbidden extensions, and some other programs that do not follow ISO C and ISO C++. So the combination of -fwrapv and -pedantic is not particularly interesting. Even then the warning is "initializer element is not a constant expression"; nothing to do with overflow. For example: #define INT_MIN ((int)-0x8000) int y = INT_MIN - 1; int z = -1 << 2; int w = INT_MIN << 1; int u = 1 << 31; $ gcc f.c -std=c11 -Wall -pedantic f.c:2:1: warning: overflow in constant expression [-Woverflow] f.c:3:9: warning: initializer element is not a constant expression [-Wpedantic] f.c:4:9: warning: initializer element is not a constant expression [-Wpedantic] f.c:5:9: warning: initializer element is not a constant expression [-Wpedantic] The first warning is activated by -Wall, the others aren't. Paolo
Re: [Qemu-devel] [PULL 2/9] QEMU does not care about left shifts of signed negative values
On 25 November 2015 at 19:30, Paolo Bonzini wrote: > > > On 25/11/2015 20:18, Peter Maydell wrote: >> And LLVM is its own project and its developers don't always exactly >> follow gcc behaviour. > > True. The patch documents that if LLVM will not respect 2s complement > for signed shifts when passed the -fwrapv option, it will not be > supported for compilation of QEMU. But let;s cross that bridge when we > reach it. So far, -Wshift-negative-value seems to be a misguided > attempt to copy GCC's warning without understanding it. If we don't have documented support for a "signed negative shifts are valid and have these semantics" option from both our compilers, then we definitely should not be committing a patch which silences warnings about them. Instead we need to fix all the places in our code which rely on those semantics. >> Your latest patch at https://gcc.gnu.org/ml/gcc-patches/2015-11/msg03055.html >> doesn't seem to touch the documentation of -fwrapv at all, so I >> don't think it is sufficient to allow users of the compiler >> to say "-fwrapv means signed behaviour for shifts". > > GCC *always* does signed behavior for shifts, even without -fwrapv. > I'll commit tomorrow the patch that promises that for the future. > > GCC does not need -fwrapv at all. Yes it does, because without -fwrapv it still wants to warn about them. We need to tell the compiler that we really do want a C dialect where they have specific behaviour and so no warnings are ever correct. By default (as documented, even with your patch) GCC just promises that it won't actually do undefined behaviour for signed negative shifts. (It doesn't even actually say what impdef semantics it does provide, and in practice the impdef semantics include "warn about this", which we don't want.) thanks -- PMM
Re: [Qemu-devel] [PULL 2/9] QEMU does not care about left shifts of signed negative values
On 25/11/2015 20:18, Peter Maydell wrote: > And LLVM is its own project and its developers don't always exactly > follow gcc behaviour. True. The patch docuuments that if LLVM will not respect 2s complement for signed shifts when passed the -fwrapv option, it will not be supported for compilation of QEMU. But let;s cross that bridge when we reach it. So far, -Wshift-negative-value seems to be a misguided attempt to copy GCC's warning without understanding it. > Your latest patch at https://gcc.gnu.org/ml/gcc-patches/2015-11/msg03055.html > doesn't seem to touch the documentation of -fwrapv at all, so I > don't think it is sufficient to allow users of the compiler > to say "-fwrapv means signed behaviour for shifts". GCC *always* does signed behavior for shifts, even without -fwrapv. I'll commit tomorrow the patch that promises that for the future. GCC does not need -fwrapv at all. Paolo
Re: [Qemu-devel] [PULL 2/9] QEMU does not care about left shifts of signed negative values
On 25 November 2015 at 17:50, Paolo Bonzini wrote: > On 25/11/2015 18:44, Peter Maydell wrote: >> We still haven't had any response from the LLVM/clang folks that >> this interpretation of the meaning of -fwrapv is their interpretation >> of it, have we? (I can't see any comments on the referenced bug.) > > Reasonably, they will have to follow what GCC does, independent of > -fwrapv. GCC has now promised to not exploit << undefined behavior, > even without -fwrapv. I don't think that follows. If -fwrapv is still documented by gcc as only affecting arithmetic and not shifts, I don't see any reason why the llvm people will expect it do to anything else. And LLVM is its own project and its developers don't always exactly follow gcc behaviour. Your latest patch at https://gcc.gnu.org/ml/gcc-patches/2015-11/msg03055.html doesn't seem to touch the documentation of -fwrapv at all, so I don't think it is sufficient to allow users of the compiler to say "-fwrapv means signed behaviour for shifts". > So at this point, -fwrapv is only required to placate ubsan---which it > will do for GCC as soon as my other patch is approved (I talked on IRC > with one of the GCC-ubsan authors and he said he was okay). clang with > ubsan remains broken, but that's no worse than before. I would rather see GCC's documentation explicitly state that -fwrapv means a dialect where [among other things] shifts of signed integers have 2s-complement behaviour, and ditto clang, before we accept this patch. (Or at least have those documentation fixes in the works.) I don't mind if there are still unsuppressed warnings with older compilers. What I want is a clear statement in the docs for both compilers that -fwrapv gives us the semantics we're trying to rely on. thanks -- PMM
Re: [Qemu-devel] [PULL 2/9] QEMU does not care about left shifts of signed negative values
On 25/11/2015 18:44, Peter Maydell wrote: > > 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. > > We still haven't had any response from the LLVM/clang folks that > this interpretation of the meaning of -fwrapv is their interpretation > of it, have we? (I can't see any comments on the referenced bug.) Reasonably, they will have to follow what GCC does, independent of -fwrapv. GCC has now promised to not exploit << undefined behavior, even without -fwrapv. So at this point, -fwrapv is only required to placate ubsan---which it will do for GCC as soon as my other patch is approved (I talked on IRC with one of the GCC-ubsan authors and he said he was okay). clang with ubsan remains broken, but that's no worse than before. Paolo
Re: [Qemu-devel] [PULL 2/9] QEMU does not care about left shifts of signed negative values
On 25 November 2015 at 17:19, Paolo Bonzini wrote: > 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. We still haven't had any response from the LLVM/clang folks that this interpretation of the meaning of -fwrapv is their interpretation of it, have we? (I can't see any comments on the referenced bug.) thanks -- PMM
[Qemu-devel] [PULL 2/9] QEMU does not care about left shifts of signed negative values
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.
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).
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
Reviewed-by: Markus Armbruster
Grudgingly-reviewed-by: Laszlo Ersek
Signed-off-by: Paolo Bonzini
---
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
