All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH for 2.5] QEMU does not care about left shifts of signed negative values
@ 2015-11-17  9:59 Paolo Bonzini
  2015-11-17 10:19 ` Peter Maydell
  2015-11-17 10:26 ` Markus Armbruster
  0 siblings, 2 replies; 21+ messages in thread
From: Paolo Bonzini @ 2015-11-17  9:59 UTC (permalink / raw)
  To: qemu-devel

There's no reason for the compiler to exploit the undefinedness of left
shifts, In fact GCC explicitly documents that they do not use at all
all this possibility.  They also say this is subject to change, but
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 because the value becomes positive
when extended; using -(a << b) does not express as well that the
intention is to compute -a * 2^N.

Clang has just added an obnoxious, pointless, *totally useless*, unsafe
warning about this.  It's obnoxious and pointless because the compiler
is not using the latitude that the standard gives it, so it 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.

Document this explicitly, and shut up the stupid warning.
</rant>

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 HACKING   | 4 ++++
 configure | 2 +-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/HACKING b/HACKING
index 12fbc8a..ece6d5b 100644
--- a/HACKING
+++ b/HACKING
@@ -157,3 +157,7 @@ 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.
diff --git a/configure b/configure
index 6bfa6f5..e54c2ed 100755
--- a/configure
+++ b/configure
@@ -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
-- 
2.5.0

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* Re: [Qemu-devel] [PATCH for 2.5] QEMU does not care about left shifts of signed negative values
  2015-11-17  9:59 [Qemu-devel] [PATCH for 2.5] QEMU does not care about left shifts of signed negative values Paolo Bonzini
@ 2015-11-17 10:19 ` Peter Maydell
  2015-11-17 10:28   ` Paolo Bonzini
  2015-11-17 10:26 ` Markus Armbruster
  1 sibling, 1 reply; 21+ messages in thread
From: Peter Maydell @ 2015-11-17 10:19 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: QEMU Developers

On 17 November 2015 at 09:59, Paolo Bonzini <pbonzini@redhat.com> wrote:
> There's no reason for the compiler to exploit the undefinedness of left
> shifts, In fact GCC explicitly documents that they do not use at all
> all this possibility.  They also say this is subject to change, but
> 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 because the value becomes positive
> when extended; using -(a << b) does not express as well that the
> intention is to compute -a * 2^N.
>
> Clang has just added an obnoxious, pointless, *totally useless*, unsafe
> warning about this.  It's obnoxious and pointless because the compiler
> is not using the latitude that the standard gives it, so it 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.

I think we should only take this patch if you can get a cast-iron
guarantee from both clang and gcc that they will never use this
UB to drive optimizations. As you say gcc already say this more or
less, but clang doesn't, and if they're warning about it that to
me suggests that they will feel freer to rely on the UB in future.

GCC is not our only supported compiler; UB is a real thing that
compilers in general take advantage of; we should be trying to
reduce our reliance on UB, not carving out extra areas where we
feel free to use it.

thanks
-- PMM

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Qemu-devel] [PATCH for 2.5] QEMU does not care about left shifts of signed negative values
  2015-11-17  9:59 [Qemu-devel] [PATCH for 2.5] QEMU does not care about left shifts of signed negative values Paolo Bonzini
  2015-11-17 10:19 ` Peter Maydell
@ 2015-11-17 10:26 ` Markus Armbruster
  2015-11-17 10:36   ` Laszlo Ersek
  1 sibling, 1 reply; 21+ messages in thread
From: Markus Armbruster @ 2015-11-17 10:26 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: qemu-devel

Paolo Bonzini <pbonzini@redhat.com> writes:

> There's no reason for the compiler to exploit the undefinedness of left
> shifts, In fact GCC explicitly documents that they do not use at all
> all this possibility.  They also say this is subject to change, but

Suggest to scratch one of two "all" :)

> 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 because the value becomes positive
> when extended; using -(a << b) does not express as well that the
> intention is to compute -a * 2^N.
>
> Clang has just added an obnoxious, pointless, *totally useless*, unsafe
> warning about this.  It's obnoxious and pointless because the compiler
> is not using the latitude that the standard gives it, so it 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.
>
> Document this explicitly, and shut up the stupid warning.
> </rant>
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Markus Armbruster <armbru@redhat.com>

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Qemu-devel] [PATCH for 2.5] QEMU does not care about left shifts of signed negative values
  2015-11-17 10:19 ` Peter Maydell
@ 2015-11-17 10:28   ` Paolo Bonzini
  2015-11-17 10:36     ` Peter Maydell
  2015-11-17 10:41     ` Laszlo Ersek
  0 siblings, 2 replies; 21+ messages in thread
From: Paolo Bonzini @ 2015-11-17 10:28 UTC (permalink / raw)
  To: Peter Maydell; +Cc: QEMU Developers



On 17/11/2015 11:19, Peter Maydell wrote:
> I think we should only take this patch if you can get a cast-iron
> guarantee from both clang and gcc that they will never use this
> UB to drive optimizations. As you say gcc already say this more or
> less, but clang doesn't, and if they're warning about it that to
> me suggests that they will feel freer to rely on the UB in future.

If and when this happens we will add "-fno-strict-overflow" for clang,
just like we are using "-fno-strict-aliasing" already.

> GCC is not our only supported compiler; UB is a real thing that
> compilers in general take advantage of; we should be trying to
> reduce our reliance on UB, not carving out extra areas where we
> feel free to use it.

We are already feeling free to use it.

Paolo

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Qemu-devel] [PATCH for 2.5] QEMU does not care about left shifts of signed negative values
  2015-11-17 10:28   ` Paolo Bonzini
@ 2015-11-17 10:36     ` Peter Maydell
  2015-11-17 10:37       ` Paolo Bonzini
  2015-11-17 10:41     ` Laszlo Ersek
  1 sibling, 1 reply; 21+ messages in thread
From: Peter Maydell @ 2015-11-17 10:36 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: QEMU Developers

On 17 November 2015 at 10:28, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 17/11/2015 11:19, Peter Maydell wrote:
>> I think we should only take this patch if you can get a cast-iron
>> guarantee from both clang and gcc that they will never use this
>> UB to drive optimizations. As you say gcc already say this more or
>> less, but clang doesn't, and if they're warning about it that to
>> me suggests that they will feel freer to rely on the UB in future.
>
> If and when this happens we will add "-fno-strict-overflow" for clang,
> just like we are using "-fno-strict-aliasing" already.

-fno-strict-overflow in clang is AFAICT just an alias for -fwrapv.
These options control handling of signed overflow of addition,
subtraction and multiplication; there is nothing I can find in the
gcc or clang docs that suggests they have any effect on shift operations.

thanks
-- PMM

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Qemu-devel] [PATCH for 2.5] QEMU does not care about left shifts of signed negative values
  2015-11-17 10:26 ` Markus Armbruster
@ 2015-11-17 10:36   ` Laszlo Ersek
  0 siblings, 0 replies; 21+ messages in thread
From: Laszlo Ersek @ 2015-11-17 10:36 UTC (permalink / raw)
  To: Markus Armbruster, Paolo Bonzini; +Cc: qemu-devel

On 11/17/15 11:26, Markus Armbruster wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
>> There's no reason for the compiler to exploit the undefinedness of left
>> shifts, In fact GCC explicitly documents that they do not use at all
>> all this possibility.  They also say this is subject to change, but
> 
> Suggest to scratch one of two "all" :)
> 
>> 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 because the value becomes positive
>> when extended; using -(a << b) does not express as well that the
>> intention is to compute -a * 2^N.
>>
>> Clang has just added an obnoxious, pointless, *totally useless*, unsafe
>> warning about this.  It's obnoxious and pointless because the compiler
>> is not using the latitude that the standard gives it, so it 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

"wildly more", I think.

>> 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.
>>
>> Document this explicitly, and shut up the stupid warning.
>> </rant>
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> 

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Qemu-devel] [PATCH for 2.5] QEMU does not care about left shifts of signed negative values
  2015-11-17 10:36     ` Peter Maydell
@ 2015-11-17 10:37       ` Paolo Bonzini
  2015-11-17 10:42         ` Peter Maydell
  2015-11-17 10:55         ` Peter Maydell
  0 siblings, 2 replies; 21+ messages in thread
From: Paolo Bonzini @ 2015-11-17 10:37 UTC (permalink / raw)
  To: Peter Maydell; +Cc: QEMU Developers



On 17/11/2015 11:36, Peter Maydell wrote:
> > If and when this happens we will add "-fno-strict-overflow" for clang,
> > just like we are using "-fno-strict-aliasing" already.
>
> -fno-strict-overflow in clang is AFAICT just an alias for -fwrapv.
> These options control handling of signed overflow of addition,
> subtraction and multiplication; there is nothing I can find in the
> gcc or clang docs that suggests they have any effect on shift operations.

In the case of GCC, that's a corollary of the compiler not treating that
overflow as undefined.

Probably the same is true for clang.

Paolo

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Qemu-devel] [PATCH for 2.5] QEMU does not care about left shifts of signed negative values
  2015-11-17 10:28   ` Paolo Bonzini
  2015-11-17 10:36     ` Peter Maydell
@ 2015-11-17 10:41     ` Laszlo Ersek
  2015-11-17 10:43       ` Paolo Bonzini
  2015-11-17 11:59       ` Markus Armbruster
  1 sibling, 2 replies; 21+ messages in thread
From: Laszlo Ersek @ 2015-11-17 10:41 UTC (permalink / raw)
  To: Paolo Bonzini, Peter Maydell; +Cc: QEMU Developers

On 11/17/15 11:28, Paolo Bonzini wrote:
> 
> 
> On 17/11/2015 11:19, Peter Maydell wrote:
>> I think we should only take this patch if you can get a cast-iron
>> guarantee from both clang and gcc that they will never use this
>> UB to drive optimizations. As you say gcc already say this more or
>> less, but clang doesn't, and if they're warning about it that to
>> me suggests that they will feel freer to rely on the UB in future.
> 
> If and when this happens we will add "-fno-strict-overflow" for clang,
> just like we are using "-fno-strict-aliasing" already.

How about adding "-fwrapv -fno-strict-overflow" right now? (Spelling out
the latter of those explicitly for pointer arithmetic.)

>> GCC is not our only supported compiler; UB is a real thing that
>> compilers in general take advantage of; we should be trying to
>> reduce our reliance on UB, not carving out extra areas where we
>> feel free to use it.
> 
> We are already feeling free to use it.
> 
> Paolo
> 

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Qemu-devel] [PATCH for 2.5] QEMU does not care about left shifts of signed negative values
  2015-11-17 10:37       ` Paolo Bonzini
@ 2015-11-17 10:42         ` Peter Maydell
  2015-11-17 10:55         ` Peter Maydell
  1 sibling, 0 replies; 21+ messages in thread
From: Peter Maydell @ 2015-11-17 10:42 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: QEMU Developers

On 17 November 2015 at 10:37, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 17/11/2015 11:36, Peter Maydell wrote:
>> > If and when this happens we will add "-fno-strict-overflow" for clang,
>> > just like we are using "-fno-strict-aliasing" already.
>>
>> -fno-strict-overflow in clang is AFAICT just an alias for -fwrapv.
>> These options control handling of signed overflow of addition,
>> subtraction and multiplication; there is nothing I can find in the
>> gcc or clang docs that suggests they have any effect on shift operations.
>
> In the case of GCC, that's a corollary of the compiler not treating that
> overflow as undefined.
>
> Probably the same is true for clang.

Again, can you get both sets of compiler devs to agree and
document this? If not, we can't rely on it.

thanks
-- PMM

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Qemu-devel] [PATCH for 2.5] QEMU does not care about left shifts of signed negative values
  2015-11-17 10:41     ` Laszlo Ersek
@ 2015-11-17 10:43       ` Paolo Bonzini
  2015-11-17 10:52         ` Laszlo Ersek
  2015-11-17 11:59       ` Markus Armbruster
  1 sibling, 1 reply; 21+ messages in thread
From: Paolo Bonzini @ 2015-11-17 10:43 UTC (permalink / raw)
  To: Laszlo Ersek, Peter Maydell; +Cc: QEMU Developers



On 17/11/2015 11:41, Laszlo Ersek wrote:
> > If and when this happens we will add "-fno-strict-overflow" for clang,
> > just like we are using "-fno-strict-aliasing" already.
> 
> How about adding "-fwrapv -fno-strict-overflow" right now? (Spelling out
> the latter of those explicitly for pointer arithmetic.)

If it makes the change to HACKING more palatable, I'm all for it.

Paolo

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Qemu-devel] [PATCH for 2.5] QEMU does not care about left shifts of signed negative values
  2015-11-17 10:43       ` Paolo Bonzini
@ 2015-11-17 10:52         ` Laszlo Ersek
  0 siblings, 0 replies; 21+ messages in thread
From: Laszlo Ersek @ 2015-11-17 10:52 UTC (permalink / raw)
  To: Paolo Bonzini, Peter Maydell; +Cc: QEMU Developers

On 11/17/15 11:43, Paolo Bonzini wrote:
> 
> 
> On 17/11/2015 11:41, Laszlo Ersek wrote:
>>> If and when this happens we will add "-fno-strict-overflow" for clang,
>>> just like we are using "-fno-strict-aliasing" already.
>>
>> How about adding "-fwrapv -fno-strict-overflow" right now? (Spelling out
>> the latter of those explicitly for pointer arithmetic.)
> 
> If it makes the change to HACKING more palatable, I'm all for it.

In general I'm not overly happy about this change :), but, if the
consensus is that we simply don't want to worry about this kind of UB in
QEMU, then let's actually prevent the compiler from exploiting the
standard's lenience, rather than just suppress the warning.

Thanks!
Laszlo

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Qemu-devel] [PATCH for 2.5] QEMU does not care about left shifts of signed negative values
  2015-11-17 10:37       ` Paolo Bonzini
  2015-11-17 10:42         ` Peter Maydell
@ 2015-11-17 10:55         ` Peter Maydell
  2015-11-17 10:57           ` Paolo Bonzini
  1 sibling, 1 reply; 21+ messages in thread
From: Peter Maydell @ 2015-11-17 10:55 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: QEMU Developers

On 17 November 2015 at 10:37, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 17/11/2015 11:36, Peter Maydell wrote:
>> > If and when this happens we will add "-fno-strict-overflow" for clang,
>> > just like we are using "-fno-strict-aliasing" already.
>>
>> -fno-strict-overflow in clang is AFAICT just an alias for -fwrapv.
>> These options control handling of signed overflow of addition,
>> subtraction and multiplication; there is nothing I can find in the
>> gcc or clang docs that suggests they have any effect on shift operations.
>
> In the case of GCC, that's a corollary of the compiler not treating that
> overflow as undefined.
>
> Probably the same is true for clang.

If you pass clang -fwrapv then this causes -fsanitize=undefined to
no longer complain about signed integer overflows from addition.
However the sanitizer will still complain about left shifts of
negative values. The conclusion I draw is that clang (as per
the documentation) applies fwrapv only to addition &c, not to
shifts.

thanks
-- PMM

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Qemu-devel] [PATCH for 2.5] QEMU does not care about left shifts of signed negative values
  2015-11-17 10:55         ` Peter Maydell
@ 2015-11-17 10:57           ` Paolo Bonzini
  2015-11-17 11:22             ` Peter Maydell
  0 siblings, 1 reply; 21+ messages in thread
From: Paolo Bonzini @ 2015-11-17 10:57 UTC (permalink / raw)
  To: Peter Maydell; +Cc: QEMU Developers



On 17/11/2015 11:55, Peter Maydell wrote:
> If you pass clang -fwrapv then this causes -fsanitize=undefined to
> no longer complain about signed integer overflows from addition.
> However the sanitizer will still complain about left shifts of
> negative values. The conclusion I draw is that clang (as per
> the documentation) applies fwrapv only to addition &c, not to
> shifts.

Ok, I'll open a bug for this.  It's probably unintended, they've already
fixed -fwrapv once for pointers.

Paolo

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Qemu-devel] [PATCH for 2.5] QEMU does not care about left shifts of signed negative values
  2015-11-17 10:57           ` Paolo Bonzini
@ 2015-11-17 11:22             ` Peter Maydell
  2015-11-17 12:10               ` Paolo Bonzini
  0 siblings, 1 reply; 21+ messages in thread
From: Peter Maydell @ 2015-11-17 11:22 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: QEMU Developers

On 17 November 2015 at 10:57, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 17/11/2015 11:55, Peter Maydell wrote:
>> If you pass clang -fwrapv then this causes -fsanitize=undefined to
>> no longer complain about signed integer overflows from addition.
>> However the sanitizer will still complain about left shifts of
>> negative values. The conclusion I draw is that clang (as per
>> the documentation) applies fwrapv only to addition &c, not to
>> shifts.
>
> Ok, I'll open a bug for this.  It's probably unintended, they've already
> fixed -fwrapv once for pointers.

Thanks. Other things that would need to be fixed for -fwrapv to
apply to shifts:
 * clang need to document this
 * gcc need to document this (this is a stronger statement than
   what they currently have since it is a guarantee not to change
   the semantics in the future if -fwrapv is set)
 * -fwrapv in clang should suppress -Wshift-negative-value
 * ideally, test cases in both the clang and gcc test suites to
   defend the 2s-complement signed shift semantics

thanks
-- PMM

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Qemu-devel] [PATCH for 2.5] QEMU does not care about left shifts of signed negative values
  2015-11-17 10:41     ` Laszlo Ersek
  2015-11-17 10:43       ` Paolo Bonzini
@ 2015-11-17 11:59       ` Markus Armbruster
  2015-11-17 12:04         ` Peter Maydell
  2015-11-17 12:18         ` Laszlo Ersek
  1 sibling, 2 replies; 21+ messages in thread
From: Markus Armbruster @ 2015-11-17 11:59 UTC (permalink / raw)
  To: Laszlo Ersek; +Cc: Paolo Bonzini, QEMU Developers, Peter Maydell

Laszlo Ersek <lersek@redhat.com> writes:

> On 11/17/15 11:28, Paolo Bonzini wrote:
>> 
>> 
>> On 17/11/2015 11:19, Peter Maydell wrote:
>>> I think we should only take this patch if you can get a cast-iron
>>> guarantee from both clang and gcc that they will never use this
>>> UB to drive optimizations. As you say gcc already say this more or
>>> less, but clang doesn't, and if they're warning about it that to
>>> me suggests that they will feel freer to rely on the UB in future.
>> 
>> If and when this happens we will add "-fno-strict-overflow" for clang,
>> just like we are using "-fno-strict-aliasing" already.
>
> How about adding "-fwrapv -fno-strict-overflow" right now? (Spelling out
> the latter of those explicitly for pointer arithmetic.)

One of them, not both.

Quote gcc manual:

    Using -fwrapv means that integer signed overflow is fully defined:
    it wraps.  When -fwrapv is used, there is no difference between
    -fstrict-overflow and -fno-strict-overflow for integers.  With
    -fwrapv certain types of overflow are permitted.  For example, if
    the compiler gets an overflow when doing arithmetic on constants,
    the overflowed value can still be used with -fwrapv, but not
    otherwise.

https://gcc.gnu.org/onlinedocs/gcc-5.2.0/gcc/Optimize-Options.html#index-fstrict-overflow-1050

For what it's worth, the kernel uses -fno-strict-overflow
-fno-strict-aliasing.  It doesn't use -fwrapv.  If optimization is good
enough for the kernel, it's probably good enough for us.  I recommend to
follow the kernel's lead here.

Relevant kernel commits:

commit a137802ee839ace40079bebde24cfb416f73208a
Author: Linus Torvalds <torvalds@linux-foundation.org>
Date:   Sun Jul 12 11:25:04 2009 -0700

    Don't use '-fwrapv' compiler option: it's buggy in gcc-4.1.x
    
    This causes kernel images that don't run init to completion with certain
    broken gcc versions.
    
    This fixes kernel bugzilla entry:
    	http://bugzilla.kernel.org/show_bug.cgi?id=13012
    
    I suspect the gcc problem is this:
    	http://gcc.gnu.org/bugzilla/show_bug.cgi?id=28230
    
    Fix the problem by using the -fno-strict-overflow flag instead, which
    not only does not exist in the known-to-be-broken versions of gcc (it
    was introduced later than fwrapv), but seems to be much less disturbing
    to gcc too: the difference in the generated code by -fno-strict-overflow
    are smaller (compared to using neither flag) than when using -fwrapv.
    
    Reported-by: Barry K. Nathan <barryn@pobox.com>
    Pushed-by: Frans Pop <elendil@planet.nl>
    Cc: Andrew Morton <akpm@linux-foundation.org>
    Cc: stable@kernel.org
    Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

commit 68df3755e383e6fecf2354a67b08f92f18536594
Author: Linus Torvalds <torvalds@linux-foundation.org>
Date:   Thu Mar 19 11:10:17 2009 -0700

    Add '-fwrapv' to gcc CFLAGS
    
    This makes sure that gcc doesn't try to optimize away wrapping
    arithmetic, which the kernel occasionally uses for overflow testing, ie
    things like
    
    	if (ptr + offset < ptr)
    
    which technically is undefined for non-unsigned types. See
    
    	http://bugzilla.kernel.org/show_bug.cgi?id=12597
    
    for details.
    
    Not all versions of gcc support it, so we need to make it conditional
    (it looks like it was introduced in gcc-3.4).
    
    Reminded-by: Alan Cox <alan@lxorguk.ukuu.org.uk>
    Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

I don't think we care for gcc 4.1.x anymore, but the kernels long use of
-fno-strict-overflow has provided substantial testing, which -fwrapv may
not have.

[...]

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Qemu-devel] [PATCH for 2.5] QEMU does not care about left shifts of signed negative values
  2015-11-17 11:59       ` Markus Armbruster
@ 2015-11-17 12:04         ` Peter Maydell
  2015-11-17 12:17           ` Laszlo Ersek
  2015-11-17 13:48           ` Paolo Bonzini
  2015-11-17 12:18         ` Laszlo Ersek
  1 sibling, 2 replies; 21+ messages in thread
From: Peter Maydell @ 2015-11-17 12:04 UTC (permalink / raw)
  To: Markus Armbruster; +Cc: Paolo Bonzini, Laszlo Ersek, QEMU Developers

On 17 November 2015 at 11:59, Markus Armbruster <armbru@redhat.com> wrote:
> Laszlo Ersek <lersek@redhat.com> writes:
>
>> On 11/17/15 11:28, Paolo Bonzini wrote:
>>>
>>>
>>> On 17/11/2015 11:19, Peter Maydell wrote:
>>>> I think we should only take this patch if you can get a cast-iron
>>>> guarantee from both clang and gcc that they will never use this
>>>> UB to drive optimizations. As you say gcc already say this more or
>>>> less, but clang doesn't, and if they're warning about it that to
>>>> me suggests that they will feel freer to rely on the UB in future.
>>>
>>> If and when this happens we will add "-fno-strict-overflow" for clang,
>>> just like we are using "-fno-strict-aliasing" already.
>>
>> How about adding "-fwrapv -fno-strict-overflow" right now? (Spelling out
>> the latter of those explicitly for pointer arithmetic.)
>
> One of them, not both.
>
> Quote gcc manual:
>
>     Using -fwrapv means that integer signed overflow is fully defined:
>     it wraps.  When -fwrapv is used, there is no difference between
>     -fstrict-overflow and -fno-strict-overflow for integers.

I thought this too, but note that it says "for integers". As Laszlo
says, the reason to provide both is to get the -fno-strict-overflow
behaviour for pointer arithmetic, which is not affected by -fwrapv.

thanks
-- PMM

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Qemu-devel] [PATCH for 2.5] QEMU does not care about left shifts of signed negative values
  2015-11-17 11:22             ` Peter Maydell
@ 2015-11-17 12:10               ` Paolo Bonzini
  2015-11-17 12:22                 ` Peter Maydell
  0 siblings, 1 reply; 21+ messages in thread
From: Paolo Bonzini @ 2015-11-17 12:10 UTC (permalink / raw)
  To: Peter Maydell; +Cc: QEMU Developers



On 17/11/2015 12:22, Peter Maydell wrote:
> On 17 November 2015 at 10:57, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>>
>> On 17/11/2015 11:55, Peter Maydell wrote:
>>> If you pass clang -fwrapv then this causes -fsanitize=undefined to
>>> no longer complain about signed integer overflows from addition.
>>> However the sanitizer will still complain about left shifts of
>>> negative values. The conclusion I draw is that clang (as per
>>> the documentation) applies fwrapv only to addition &c, not to
>>> shifts.
>>
>> Ok, I'll open a bug for this.  It's probably unintended, they've already
>> fixed -fwrapv once for pointers.
> 
> Thanks. Other things that would need to be fixed for -fwrapv to
> apply to shifts:
>  * gcc need to document this (this is a stronger statement than
>    what they currently have since it is a guarantee not to change
>    the semantics in the future if -fwrapv is set)

Actually they document it under -fstrict-overflow ("Using '-fwrapv'
means that integer signed overflow is fully defined: it wraps") but it
would be nice to add it under -fwrapv as well.  I'll send a patch.

>  * clang need to document this
>  * -fwrapv in clang should suppress -Wshift-negative-value
>  * ideally, test cases in both the clang and gcc test suites to
>    defend the 2s-complement signed shift semantics

This + the ubsan issue is now https://llvm.org/bugs/show_bug.cgi?id=25552

Paolo

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Qemu-devel] [PATCH for 2.5] QEMU does not care about left shifts of signed negative values
  2015-11-17 12:04         ` Peter Maydell
@ 2015-11-17 12:17           ` Laszlo Ersek
  2015-11-17 13:48           ` Paolo Bonzini
  1 sibling, 0 replies; 21+ messages in thread
From: Laszlo Ersek @ 2015-11-17 12:17 UTC (permalink / raw)
  To: Peter Maydell, Markus Armbruster; +Cc: Paolo Bonzini, QEMU Developers

On 11/17/15 13:04, Peter Maydell wrote:
> On 17 November 2015 at 11:59, Markus Armbruster <armbru@redhat.com> wrote:
>> Laszlo Ersek <lersek@redhat.com> writes:
>>
>>> On 11/17/15 11:28, Paolo Bonzini wrote:
>>>>
>>>>
>>>> On 17/11/2015 11:19, Peter Maydell wrote:
>>>>> I think we should only take this patch if you can get a cast-iron
>>>>> guarantee from both clang and gcc that they will never use this
>>>>> UB to drive optimizations. As you say gcc already say this more or
>>>>> less, but clang doesn't, and if they're warning about it that to
>>>>> me suggests that they will feel freer to rely on the UB in future.
>>>>
>>>> If and when this happens we will add "-fno-strict-overflow" for clang,
>>>> just like we are using "-fno-strict-aliasing" already.
>>>
>>> How about adding "-fwrapv -fno-strict-overflow" right now? (Spelling out
>>> the latter of those explicitly for pointer arithmetic.)
>>
>> One of them, not both.
>>
>> Quote gcc manual:
>>
>>     Using -fwrapv means that integer signed overflow is fully defined:
>>     it wraps.  When -fwrapv is used, there is no difference between
>>     -fstrict-overflow and -fno-strict-overflow for integers.
> 
> I thought this too, but note that it says "for integers". As Laszlo
> says, the reason to provide both is to get the -fno-strict-overflow
> behaviour for pointer arithmetic, which is not affected by -fwrapv.

Correct, that's what I meant.

> 
> thanks
> -- PMM
> 

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Qemu-devel] [PATCH for 2.5] QEMU does not care about left shifts of signed negative values
  2015-11-17 11:59       ` Markus Armbruster
  2015-11-17 12:04         ` Peter Maydell
@ 2015-11-17 12:18         ` Laszlo Ersek
  1 sibling, 0 replies; 21+ messages in thread
From: Laszlo Ersek @ 2015-11-17 12:18 UTC (permalink / raw)
  To: Markus Armbruster; +Cc: Paolo Bonzini, QEMU Developers, Peter Maydell

On 11/17/15 12:59, Markus Armbruster wrote:
> Laszlo Ersek <lersek@redhat.com> writes:
> 
>> On 11/17/15 11:28, Paolo Bonzini wrote:
>>>
>>>
>>> On 17/11/2015 11:19, Peter Maydell wrote:
>>>> I think we should only take this patch if you can get a cast-iron
>>>> guarantee from both clang and gcc that they will never use this
>>>> UB to drive optimizations. As you say gcc already say this more or
>>>> less, but clang doesn't, and if they're warning about it that to
>>>> me suggests that they will feel freer to rely on the UB in future.
>>>
>>> If and when this happens we will add "-fno-strict-overflow" for clang,
>>> just like we are using "-fno-strict-aliasing" already.
>>
>> How about adding "-fwrapv -fno-strict-overflow" right now? (Spelling out
>> the latter of those explicitly for pointer arithmetic.)
> 
> One of them, not both.
> 
> Quote gcc manual:
> 
>     Using -fwrapv means that integer signed overflow is fully defined:
>     it wraps.  When -fwrapv is used, there is no difference between
>     -fstrict-overflow and -fno-strict-overflow for integers.  With
>     -fwrapv certain types of overflow are permitted.  For example, if
>     the compiler gets an overflow when doing arithmetic on constants,
>     the overflowed value can still be used with -fwrapv, but not
>     otherwise.
> 
> https://gcc.gnu.org/onlinedocs/gcc-5.2.0/gcc/Optimize-Options.html#index-fstrict-overflow-1050
> 
> For what it's worth, the kernel uses -fno-strict-overflow
> -fno-strict-aliasing.  It doesn't use -fwrapv.  If optimization is good
> enough for the kernel, it's probably good enough for us.  I recommend to
> follow the kernel's lead here.
> 
> Relevant kernel commits:
> 
> commit a137802ee839ace40079bebde24cfb416f73208a
> Author: Linus Torvalds <torvalds@linux-foundation.org>
> Date:   Sun Jul 12 11:25:04 2009 -0700
> 
>     Don't use '-fwrapv' compiler option: it's buggy in gcc-4.1.x

OMG.

I guess "whatever works" then. :/

Laszlo

>     
>     This causes kernel images that don't run init to completion with certain
>     broken gcc versions.
>     
>     This fixes kernel bugzilla entry:
>     	http://bugzilla.kernel.org/show_bug.cgi?id=13012
>     
>     I suspect the gcc problem is this:
>     	http://gcc.gnu.org/bugzilla/show_bug.cgi?id=28230
>     
>     Fix the problem by using the -fno-strict-overflow flag instead, which
>     not only does not exist in the known-to-be-broken versions of gcc (it
>     was introduced later than fwrapv), but seems to be much less disturbing
>     to gcc too: the difference in the generated code by -fno-strict-overflow
>     are smaller (compared to using neither flag) than when using -fwrapv.
>     
>     Reported-by: Barry K. Nathan <barryn@pobox.com>
>     Pushed-by: Frans Pop <elendil@planet.nl>
>     Cc: Andrew Morton <akpm@linux-foundation.org>
>     Cc: stable@kernel.org
>     Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> 
> commit 68df3755e383e6fecf2354a67b08f92f18536594
> Author: Linus Torvalds <torvalds@linux-foundation.org>
> Date:   Thu Mar 19 11:10:17 2009 -0700
> 
>     Add '-fwrapv' to gcc CFLAGS
>     
>     This makes sure that gcc doesn't try to optimize away wrapping
>     arithmetic, which the kernel occasionally uses for overflow testing, ie
>     things like
>     
>     	if (ptr + offset < ptr)
>     
>     which technically is undefined for non-unsigned types. See
>     
>     	http://bugzilla.kernel.org/show_bug.cgi?id=12597
>     
>     for details.
>     
>     Not all versions of gcc support it, so we need to make it conditional
>     (it looks like it was introduced in gcc-3.4).
>     
>     Reminded-by: Alan Cox <alan@lxorguk.ukuu.org.uk>
>     Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> 
> I don't think we care for gcc 4.1.x anymore, but the kernels long use of
> -fno-strict-overflow has provided substantial testing, which -fwrapv may
> not have.
> 
> [...]
> 

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Qemu-devel] [PATCH for 2.5] QEMU does not care about left shifts of signed negative values
  2015-11-17 12:10               ` Paolo Bonzini
@ 2015-11-17 12:22                 ` Peter Maydell
  0 siblings, 0 replies; 21+ messages in thread
From: Peter Maydell @ 2015-11-17 12:22 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: QEMU Developers

On 17 November 2015 at 12:10, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 17/11/2015 12:22, Peter Maydell wrote:
>>  * gcc need to document this (this is a stronger statement than
>>    what they currently have since it is a guarantee not to change
>>    the semantics in the future if -fwrapv is set)
>
> Actually they document it under -fstrict-overflow ("Using '-fwrapv'
> means that integer signed overflow is fully defined: it wraps") but it
> would be nice to add it under -fwrapv as well.  I'll send a patch.

My interpretation of that quote was that the gcc devs don't include
shifts in their definition of "integer signed overflows" (since
-fwrapv is very clear about what it affects.) Either way clarity
would be good, so thanks for taking on the patch.

thanks
-- PMM

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Qemu-devel] [PATCH for 2.5] QEMU does not care about left shifts of signed negative values
  2015-11-17 12:04         ` Peter Maydell
  2015-11-17 12:17           ` Laszlo Ersek
@ 2015-11-17 13:48           ` Paolo Bonzini
  1 sibling, 0 replies; 21+ messages in thread
From: Paolo Bonzini @ 2015-11-17 13:48 UTC (permalink / raw)
  To: Peter Maydell, Markus Armbruster; +Cc: Laszlo Ersek, QEMU Developers



On 17/11/2015 13:04, Peter Maydell wrote:
> >     Using -fwrapv means that integer signed overflow is fully defined:
> >     it wraps.  When -fwrapv is used, there is no difference between
> >     -fstrict-overflow and -fno-strict-overflow for integers.
>
> I thought this too, but note that it says "for integers". As Laszlo
> says, the reason to provide both is to get the -fno-strict-overflow
> behaviour for pointer arithmetic, which is not affected by -fwrapv.

And ubsan is not affected by -fno-strict-overflow...

Paolo

^ permalink raw reply	[flat|nested] 21+ messages in thread

end of thread, other threads:[~2015-11-17 13:48 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-11-17  9:59 [Qemu-devel] [PATCH for 2.5] QEMU does not care about left shifts of signed negative values Paolo Bonzini
2015-11-17 10:19 ` Peter Maydell
2015-11-17 10:28   ` Paolo Bonzini
2015-11-17 10:36     ` Peter Maydell
2015-11-17 10:37       ` Paolo Bonzini
2015-11-17 10:42         ` Peter Maydell
2015-11-17 10:55         ` Peter Maydell
2015-11-17 10:57           ` Paolo Bonzini
2015-11-17 11:22             ` Peter Maydell
2015-11-17 12:10               ` Paolo Bonzini
2015-11-17 12:22                 ` Peter Maydell
2015-11-17 10:41     ` Laszlo Ersek
2015-11-17 10:43       ` Paolo Bonzini
2015-11-17 10:52         ` Laszlo Ersek
2015-11-17 11:59       ` Markus Armbruster
2015-11-17 12:04         ` Peter Maydell
2015-11-17 12:17           ` Laszlo Ersek
2015-11-17 13:48           ` Paolo Bonzini
2015-11-17 12:18         ` Laszlo Ersek
2015-11-17 10:26 ` Markus Armbruster
2015-11-17 10:36   ` Laszlo Ersek

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.