diff mbox

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

Message ID 1448471956-66873-3-git-send-email-pbonzini@redhat.com
State New
Headers show

Commit Message

Paolo Bonzini Nov. 25, 2015, 5:19 p.m. UTC 
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(-)

Comments

Peter Maydell Nov. 25, 2015, 5:44 p.m. UTC | #1 
On 25 November 2015 at 17:19, Paolo Bonzini <pbonzini@redhat.com> 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
Paolo Bonzini Nov. 25, 2015, 5:50 p.m. UTC | #2 
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
Peter Maydell Nov. 25, 2015, 7:18 p.m. UTC | #3 
On 25 November 2015 at 17:50, Paolo Bonzini <pbonzini@redhat.com> 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
Paolo Bonzini Nov. 25, 2015, 7:30 p.m. UTC | #4 
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
Peter Maydell Nov. 25, 2015, 7:54 p.m. UTC | #5 
On 25 November 2015 at 19:30, Paolo Bonzini <pbonzini@redhat.com> 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
Paolo Bonzini Nov. 25, 2015, 9:05 p.m. UTC | #6 
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)-0x80000000)
    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
Peter Maydell Nov. 25, 2015, 9:22 p.m. UTC | #7 
On 25 November 2015 at 21:05, Paolo Bonzini <pbonzini@redhat.com> 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)-0x80000000)
>     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
diff mbox

Patch

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