mbox

[PULL,0/9] Misc patches for QEMU 2.5-rc2 (2015-11-25)

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

Pull-request

git://github.com/bonzini/qemu.git tags/for-upstream

Message

Paolo Bonzini Nov. 25, 2015, 5:19 p.m. UTC
The following changes since commit 4b6eda626fdb8bf90472c6868d502a2ac09abeeb:

  Merge remote-tracking branch 'remotes/lalrae/tags/mips-20151124' into staging (2015-11-24 17:05:06 +0000)

are available in the git repository at:


  git://github.com/bonzini/qemu.git tags/for-upstream

for you to fetch changes up to 6ac504d20a0967da06caadf595388f753908328a:

  target-i386: kvm: Print warning when clearing mcg_cap bits (2015-11-25 17:48:46 +0100)

----------------------------------------------------------------
Small patches, most notably the introduction of -fwrapv.

----------------------------------------------------------------
Daniel P. Berrange (2):
      Revert "exec: silence hugetlbfs warning under qtest"
      exec: remove warning about mempath and hugetlbfs

Eduardo Habkost (3):
      target-i386: kvm: Abort if MCE bank count is not supported by host
      target-i386: kvm: Use env->mcg_cap when setting up MCE
      target-i386: kvm: Print warning when clearing mcg_cap bits

Paolo Bonzini (3):
      MAINTAINERS: Update TCG CPU cores section
      QEMU does not care about left shifts of signed negative values
      target-sparc: fix 32-bit truncation in fpackfix

Wen Congyang (1):
      call bdrv_drain_all() even if the vm is stopped

 HACKING                   |  6 ++++++
 MAINTAINERS               | 17 +++++++++++++----
 configure                 |  4 ++--
 cpus.c                    |  2 ++
 exec.c                    |  6 ------
 target-i386/cpu.h         |  2 ++
 target-i386/kvm.c         | 22 ++++++++++++++--------
 target-sparc/vis_helper.c |  2 +-
 vl.c                      | 28 ++++++++++++++--------------
 9 files changed, 54 insertions(+), 35 deletions(-)

Comments

Peter Maydell Nov. 26, 2015, 9:46 a.m. UTC | #1
On 25 November 2015 at 17:19, Paolo Bonzini <pbonzini@redhat.com> wrote:
> The following changes since commit 4b6eda626fdb8bf90472c6868d502a2ac09abeeb:
>
>   Merge remote-tracking branch 'remotes/lalrae/tags/mips-20151124' into staging (2015-11-24 17:05:06 +0000)
>
> are available in the git repository at:
>
>
>   git://github.com/bonzini/qemu.git tags/for-upstream
>
> for you to fetch changes up to 6ac504d20a0967da06caadf595388f753908328a:
>
>   target-i386: kvm: Print warning when clearing mcg_cap bits (2015-11-25 17:48:46 +0100)
>
> ----------------------------------------------------------------
> Small patches, most notably the introduction of -fwrapv.
>
> ----------------------------------------------------------------
> Daniel P. Berrange (2):
>       Revert "exec: silence hugetlbfs warning under qtest"
>       exec: remove warning about mempath and hugetlbfs
>
> Eduardo Habkost (3):
>       target-i386: kvm: Abort if MCE bank count is not supported by host
>       target-i386: kvm: Use env->mcg_cap when setting up MCE
>       target-i386: kvm: Print warning when clearing mcg_cap bits
>
> Paolo Bonzini (3):
>       MAINTAINERS: Update TCG CPU cores section
>       QEMU does not care about left shifts of signed negative values
>       target-sparc: fix 32-bit truncation in fpackfix
>
> Wen Congyang (1):
>       call bdrv_drain_all() even if the vm is stopped

I definitely don't think we should apply the -fwrapv patch yet;
would you mind respinning this pullrequest without it?

thanks
-- PMM
Paolo Bonzini Nov. 26, 2015, 10:40 a.m. UTC | #2
On 26/11/2015 10:46, Peter Maydell wrote:
> On 25 November 2015 at 17:19, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> The following changes since commit 4b6eda626fdb8bf90472c6868d502a2ac09abeeb:
>>
>>   Merge remote-tracking branch 'remotes/lalrae/tags/mips-20151124' into staging (2015-11-24 17:05:06 +0000)
>>
>> are available in the git repository at:
>>
>>
>>   git://github.com/bonzini/qemu.git tags/for-upstream
>>
>> for you to fetch changes up to 6ac504d20a0967da06caadf595388f753908328a:
>>
>>   target-i386: kvm: Print warning when clearing mcg_cap bits (2015-11-25 17:48:46 +0100)
>>
>> ----------------------------------------------------------------
>> Small patches, most notably the introduction of -fwrapv.
>>
>> ----------------------------------------------------------------
>> Daniel P. Berrange (2):
>>       Revert "exec: silence hugetlbfs warning under qtest"
>>       exec: remove warning about mempath and hugetlbfs
>>
>> Eduardo Habkost (3):
>>       target-i386: kvm: Abort if MCE bank count is not supported by host
>>       target-i386: kvm: Use env->mcg_cap when setting up MCE
>>       target-i386: kvm: Print warning when clearing mcg_cap bits
>>
>> Paolo Bonzini (3):
>>       MAINTAINERS: Update TCG CPU cores section
>>       QEMU does not care about left shifts of signed negative values
>>       target-sparc: fix 32-bit truncation in fpackfix
>>
>> Wen Congyang (1):
>>       call bdrv_drain_all() even if the vm is stopped
> 
> I definitely don't think we should apply the -fwrapv patch yet;
> would you mind respinning this pullrequest without it?

In what way does that patch make that thing worse?

Paolo
Peter Maydell Nov. 26, 2015, 10:56 a.m. UTC | #3
On 26 November 2015 at 10:40, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 26/11/2015 10:46, Peter Maydell wrote:
>> I definitely don't think we should apply the -fwrapv patch yet;
>> would you mind respinning this pullrequest without it?
>
> In what way does that patch make that thing worse?

It makes a claim about the semantics that the compiler
guarantees us which isn't currently valid. (Or
alternatively, it's implicitly claiming that clang isn't
a supported compiler, which isn't true.) I don't think
we should document or rely on signed-shift semantics
until we have the relevant documented promises from the
compiler developers that that is what they are providing.
(I'm happy that the gcc folks have provided those promises, they
just need to actually document them in the -fwrapv option
docs. The clang folks haven't replied yet so we don't know.)

thanks
-- PMM
Paolo Bonzini Nov. 26, 2015, 11:23 a.m. UTC | #4
On 26/11/2015 11:56, Peter Maydell wrote:
> On 26 November 2015 at 10:40, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>>
>> On 26/11/2015 10:46, Peter Maydell wrote:
>>> I definitely don't think we should apply the -fwrapv patch yet;
>>> would you mind respinning this pullrequest without it?
>>
>> In what way does that patch make that thing worse?
> 
> It makes a claim about the semantics that the compiler
> guarantees us which isn't currently valid. (Or
> alternatively, it's implicitly claiming that clang isn't
> a supported compiler, which isn't true.) I don't think
> we should document or rely on signed-shift semantics

But we are relying on them, and thus we should document them.  Witness
the number of patches fixing so called "undefined" behavior.  And those
patches are _dangerous_.

I can certainly remove the "as documented by the GCC manual" part and
the -fwrapv setting, but silencing -Wshift-negative-value and
documenting what we rely on should go in.

Paolo

> until we have the relevant documented promises from the
> compiler developers that that is what they are providing.
> (I'm happy that the gcc folks have provided those promises, they
> just need to actually document them in the -fwrapv option
> docs. The clang folks haven't replied yet so we don't know.)
Peter Maydell Nov. 26, 2015, 11:28 a.m. UTC | #5
On 26 November 2015 at 11:23, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 26/11/2015 11:56, Peter Maydell wrote:
>> It makes a claim about the semantics that the compiler
>> guarantees us which isn't currently valid. (Or
>> alternatively, it's implicitly claiming that clang isn't
>> a supported compiler, which isn't true.) I don't think
>> we should document or rely on signed-shift semantics
>
> But we are relying on them, and thus we should document them.  Witness
> the number of patches fixing so called "undefined" behavior.  And those
> patches are _dangerous_.

Until and unless the compiler guarantees us the semantics that
we want, then silently hoping we get something we're not getting
is even more dangerous, and avoiding the UB is better than
just crossing our fingers and hoping.

> I can certainly remove the "as documented by the GCC manual" part and
> the -fwrapv setting, but silencing -Wshift-negative-value and
> documenting what we rely on should go in.

I don't see much point in documenting what we rely on
if we can't rely on it and need to stop relying on it.

thanks
-- PMM
Markus Armbruster Nov. 26, 2015, 12:15 p.m. UTC | #6
Peter Maydell <peter.maydell@linaro.org> writes:

> On 26 November 2015 at 11:23, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> On 26/11/2015 11:56, Peter Maydell wrote:
>>> It makes a claim about the semantics that the compiler
>>> guarantees us which isn't currently valid. (Or
>>> alternatively, it's implicitly claiming that clang isn't
>>> a supported compiler, which isn't true.) I don't think
>>> we should document or rely on signed-shift semantics
>>
>> But we are relying on them, and thus we should document them.  Witness
>> the number of patches fixing so called "undefined" behavior.  And those
>> patches are _dangerous_.

I'm pretty sure us screwing some of them up is a much larger risk than
gcc suddenly starting to screw up our signed shifts without anybody
noticing.

If gcc ever started to mess up signed shifts, I'd fully expect the
kernel and many applications to go down in flames.  Hopefully quickly
enough to avoid real damage.  I don't think gcc has become *that*
reckless.  Clang neither.

> Until and unless the compiler guarantees us the semantics that
> we want, then silently hoping we get something we're not getting
> is even more dangerous, and avoiding the UB is better than
> just crossing our fingers and hoping.

The cost and risk of proactively fixing a hypothetical^Wlatent problem
needs to be weighed against the probability of it ever becoming real.

>> I can certainly remove the "as documented by the GCC manual" part and
>> the -fwrapv setting, but silencing -Wshift-negative-value and
>> documenting what we rely on should go in.
>
> I don't see much point in documenting what we rely on
> if we can't rely on it and need to stop relying on it.

"Can't" and "need" are too strong.  The kernel can, and I fail to see
what makes us so special that we absolutely cannot.

For what it's worth, I'm sick and tired of patches "fixing" signed
shifts, and the unnecessary risk that comes with them.
Peter Maydell Nov. 26, 2015, 12:19 p.m. UTC | #7
On 26 November 2015 at 12:15, Markus Armbruster <armbru@redhat.com> wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
>> I don't see much point in documenting what we rely on
>> if we can't rely on it and need to stop relying on it.
>
> "Can't" and "need" are too strong.  The kernel can, and I fail to see
> what makes us so special that we absolutely cannot.

The kernel has the luxury of being able to say "we only compile
with gcc".

> For what it's worth, I'm sick and tired of patches "fixing" signed
> shifts, and the unnecessary risk that comes with them.

Me too. I just want us to fix this by getting the compiler authors
to document that we can rely on this stuff, not just by silencing
warnings in QEMU's makefiles.

thanks
-- PMM
Paolo Bonzini Nov. 26, 2015, 1:04 p.m. UTC | #8
On 26/11/2015 12:28, Peter Maydell wrote:
>> But we are relying on them, and thus we should document them.  Witness
>> the number of patches fixing so called "undefined" behavior.  And those
>> patches are _dangerous_.
> 
> Until and unless the compiler guarantees us the semantics that
> we want, then silently hoping we get something we're not getting
> is even more dangerous, and avoiding the UB is better than
> just crossing our fingers and hoping.
> 
>> I can certainly remove the "as documented by the GCC manual" part and
>> the -fwrapv setting, but silencing -Wshift-negative-value and
>> documenting what we rely on should go in.
> 
> I don't see much point in documenting what we rely on
> if we can't rely on it and need to stop relying on it.

I'm having a hard, hard time believing that we can't rely on it.  And if
we can rely on it, we don't need to stop relying on it.

To sum up:

- GCC promises that signed shift of << is implementation-defined except
for overflow in constant expressions, and defines it as operating on bit
values.  This was approved.  For GCC, -fwrapv does not even apply except
again for constant expressions.

- signed shift of negative values in constant expressions _are_ covered
by GCC's promise.  The implementation-defined behavior of signed <<
gives a meaning to all signed shifts, and all that the C standard
requires is "Each constant expression shall evaluate to a constant that
is in the range of representable values for its type" (6.6p4).
Therefore we should at least disable -Wshift-negative-value, because it
doesn't buy us anything.

- regarding overflow, in addition to the weird -Wpedantic warning, GCC 6
adds a new -Wshift-overflow flag which is enabled by default in C99 and
C11 modes, and which only applies to constant expressions.  So the
remaining case where the compiler may change its behavior on overflow,
i.e. constant expressions, will thus be caught by our usage of -Werror
(because -Wshift-overflow is enabled by default).  So, independent of
the limited scope of GCC's promise, with GCC 6 we will never have
constant expressions that overflow because of left shifts.

- if a compiler actually treated signed << overflow undefined behavior,
a possible fix would be to use C89 mode (-std=gnu89), since signed << is
unspecified there rather than undefined.  With C89, GCC's promise is
complete.  We do use C89 with GCC <= 4.9 anyway, it makes sense to be
homogeneous across all supported compilers.

Now, -fwrapv was really included in my patch only to silence ubsan in
the future.  I think it should, and I will work on patches to fix that.
 However, an advantage of -std=gnu89 is that it silences ubsan _now_ at
least for GCC.  So let's just drop -fwrapv and use -std=gnu89 instead.
This buys us time, and in the meanwhile I'll gett -fwrapv complete in GCC.

If this is okay, I'll remove the patch, respin the pull request, and
post a new configure change to use -std=gnu89.

Yes, we haven't heard anything from clang developers.  But clang does
not even document implementation-defined behavior
(https://llvm.org/bugs/show_bug.cgi?id=11272).  The bug says:

> The clang documentation should specify how clang behaves in cases
> specified to be implementation-defined in the relevant standards.
> Perhaps simply saying that our behavior is the same as GCC's would suffice?

This is about implementation-defined rather than undefined behavior, but
I think it's enough to not care about clang developer's silence.

Paolo
Paolo Bonzini Nov. 26, 2015, 1:07 p.m. UTC | #9
On 26/11/2015 13:19, Peter Maydell wrote:
> On 26 November 2015 at 12:15, Markus Armbruster <armbru@redhat.com> wrote:
>> Peter Maydell <peter.maydell@linaro.org> writes:
>>> I don't see much point in documenting what we rely on
>>> if we can't rely on it and need to stop relying on it.
>>
>> "Can't" and "need" are too strong.  The kernel can, and I fail to see
>> what makes us so special that we absolutely cannot.
> 
> The kernel has the luxury of being able to say "we only compile
> with gcc".

Actually no, there are people interested in compiling it with clang
(mostly because of GPL FUD and LLVM koolaid, but that's secondary in
this context).

>> For what it's worth, I'm sick and tired of patches "fixing" signed
>> shifts, and the unnecessary risk that comes with them.
> 
> Me too.

Great, that's a start.  And I'm totally not sarcastic about this.

Paolo

> I just want us to fix this by getting the compiler authors
> to document that we can rely on this stuff, not just by silencing
> warnings in QEMU's makefiles.
Peter Maydell Nov. 26, 2015, 3:01 p.m. UTC | #10
On 26 November 2015 at 13:04, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 26/11/2015 12:28, Peter Maydell wrote:
>>> But we are relying on them, and thus we should document them.  Witness
>>> the number of patches fixing so called "undefined" behavior.  And those
>>> patches are _dangerous_.
>>
>> Until and unless the compiler guarantees us the semantics that
>> we want, then silently hoping we get something we're not getting
>> is even more dangerous, and avoiding the UB is better than
>> just crossing our fingers and hoping.
>>
>>> I can certainly remove the "as documented by the GCC manual" part and
>>> the -fwrapv setting, but silencing -Wshift-negative-value and
>>> documenting what we rely on should go in.
>>
>> I don't see much point in documenting what we rely on
>> if we can't rely on it and need to stop relying on it.
>
> I'm having a hard, hard time believing that we can't rely on it.  And if
> we can rely on it, we don't need to stop relying on it.

Really my concern here is simply an ordering one: we should
(1) confirm with the clang and gcc developers that what we think
-fwrapv means is what they agree (and will document) as what it means
(2) update our makefiles/docs/etc appropriately

and also I think that (1) is the significant part of the exercise,
whereas (2) is just a cleanup/tidyup that we can do afterwards.
This patch is doing (2) before we've done (1).

> To sum up:
>
> - GCC promises that signed shift of << is implementation-defined except
> for overflow in constant expressions, and defines it as operating on bit
> values.  This was approved.  For GCC, -fwrapv does not even apply except
> again for constant expressions.
>
> - signed shift of negative values in constant expressions _are_ covered
> by GCC's promise.  The implementation-defined behavior of signed <<
> gives a meaning to all signed shifts, and all that the C standard
> requires is "Each constant expression shall evaluate to a constant that
> is in the range of representable values for its type" (6.6p4).
> Therefore we should at least disable -Wshift-negative-value, because it
> doesn't buy us anything.
>
> - regarding overflow, in addition to the weird -Wpedantic warning, GCC 6
> adds a new -Wshift-overflow flag which is enabled by default in C99 and
> C11 modes, and which only applies to constant expressions.  So the
> remaining case where the compiler may change its behavior on overflow,
> i.e. constant expressions, will thus be caught by our usage of -Werror
> (because -Wshift-overflow is enabled by default).  So, independent of
> the limited scope of GCC's promise, with GCC 6 we will never have
> constant expressions that overflow because of left shifts.

I'm confused by all this text about constant expressions. Does
-fwrapv guarantee that signed shift of << behaves as we want
in all situations (including constant expressions) or doesn't it?
If it doesn't do that for constant expressions I don't think we should
rely on it, because it's way too confusing to have "this is OK except
when it isn't OK". (And a lot of our uses of "-1 << 8" are indeed
in constant expressions.)

> - if a compiler actually treated signed << overflow undefined behavior,
> a possible fix would be to use C89 mode (-std=gnu89), since signed << is
> unspecified there rather than undefined.  With C89, GCC's promise is
> complete.  We do use C89 with GCC <= 4.9 anyway, it makes sense to be
> homogeneous across all supported compilers.

"unspecified" isn't a great deal better than "undefined" really.

> Now, -fwrapv was really included in my patch only to silence ubsan in
> the future.  I think it should, and I will work on patches to fix that.
>  However, an advantage of -std=gnu89 is that it silences ubsan _now_ at
> least for GCC.  So let's just drop -fwrapv and use -std=gnu89 instead.
> This buys us time, and in the meanwhile I'll gett -fwrapv complete in GCC.
>
> If this is okay, I'll remove the patch, respin the pull request, and
> post a new configure change to use -std=gnu89.

I don't think this gains us much as a different approach, and it's
still trying to do cleanup (2) before we have dealt with the main
issue (1).

> Yes, we haven't heard anything from clang developers.  But clang does
> not even document implementation-defined behavior
> (https://llvm.org/bugs/show_bug.cgi?id=11272).  The bug says:
>
>> The clang documentation should specify how clang behaves in cases
>> specified to be implementation-defined in the relevant standards.
>> Perhaps simply saying that our behavior is the same as GCC's would suffice?
>
> This is about implementation-defined rather than undefined behavior, but
> I think it's enough to not care about clang developer's silence.

I disagree here. I think it's important to get the clang developers
to tell us what they mean by -fwrapv and what they want to guarantee
about signed shifts. That's because if they decide to say "no, we
don't guarantee behaviour here because the C spec says it's UB" then
we need to change how we deal with these in QEMU.

thanks
-- PMM
Paolo Bonzini Nov. 26, 2015, 3:40 p.m. UTC | #11
On 26/11/2015 16:01, Peter Maydell wrote:
> > - regarding overflow, in addition to the weird -Wpedantic warning, GCC 6
> > adds a new -Wshift-overflow flag which is enabled by default in C99 and
> > C11 modes, and which only applies to constant expressions.  So the
> > remaining case where the compiler may change its behavior on overflow,
> > i.e. constant expressions, will thus be caught by our usage of -Werror
> > (because -Wshift-overflow is enabled by default).  So, independent of
> > the limited scope of GCC's promise, with GCC 6 we will never have
> > constant expressions that overflow because of left shifts.
> 
> I'm confused by all this text about constant expressions. Does
> -fwrapv guarantee that signed shift of << behaves as we want
> in all situations (including constant expressions) or doesn't it?

Until GCC 5, it does even without -fwrapv.

Until GCC 6, the generated code is OK but you get warnings.  For (-1 <<
8) you have to enable them manually, for (0xFF << 28) you always get
them.  I am working on a patch to make GCC 6 shut up if you specify
-fwrapv, but my point is that -fwrapv is _not_ necessary because:

- GCC very sensibly does not make -Wall enable -Wshift-negative-value

- warnings such as 0xFF << 28 are much less contentious, and even shifts
into the sign bit (e.g. 1 << 31 or 0xFF << 24) are not enabled by
default either.

> (And a lot of our uses of "-1 << 8" are indeed in constant expressions.)

Those are okay as long as you do not use -Wextra.

Again, the _value_ is perfectly specified by the GCC documentation (and
as of this morning, it's promised to remain that way).  GCC leaves the
door open for warning in constant expressions, and indeed GCC 6 warns
more than GCC 5 in this regard.

> "unspecified" isn't a great deal better than "undefined" really.

It is better inasmuch as it shuts up ubsan.

>> If this is okay, I'll remove the patch, respin the pull request, and
>> post a new configure change to use -std=gnu89.
> 
> I don't think this gains us much as a different approach, and it's
> still trying to do cleanup (2) before we have dealt with the main
> issue (1).

What I'm saying is that:

- you were (rightly) worried about the compiler's behavior for constant
expressions

- but since we use -Werror, we do not have to worry about them.  With
-Werror, anything that GCC 6 can compile is perfectly specified by the
GCC documentation, including left shifts of negative values.

So GCC does not even need -fwrapv, and -std=gnu89 is a better way to
shut up ubsan because it already works.

Regarding clang, we cannot be hostage of their silence.  And recalling
that they were the ones who f***ed up their warning levels in the first
place, and are making us waste so much time, I couldn't care less about
their opinions.

> > This is about implementation-defined rather than undefined behavior, but
> > I think it's enough to not care about clang developer's silence.
>
> I disagree here. I think it's important to get the clang developers
> to tell us what they mean by -fwrapv and what they want to guarantee
> about signed shifts. That's because if they decide to say "no, we
> don't guarantee behaviour here because the C spec says it's UB" then
> we need to change how we deal with these in QEMU.

No, we need to change our list of supported compilers (if that happens).

Paolo
Peter Maydell Nov. 26, 2015, 3:55 p.m. UTC | #12
On 26 November 2015 at 15:40, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 26/11/2015 16:01, Peter Maydell wrote:
>> I'm confused by all this text about constant expressions. Does
>> -fwrapv guarantee that signed shift of << behaves as we want
>> in all situations (including constant expressions) or doesn't it?
>
> Until GCC 5, it does even without -fwrapv.
>
> Until GCC 6, the generated code is OK but you get warnings.  For (-1 <<
> 8) you have to enable them manually, for (0xFF << 28) you always get
> them.  I am working on a patch to make GCC 6 shut up if you specify
> -fwrapv, but my point is that -fwrapv is _not_ necessary because:
>
> - GCC very sensibly does not make -Wall enable -Wshift-negative-value
>
> - warnings such as 0xFF << 28 are much less contentious, and even shifts
> into the sign bit (e.g. 1 << 31 or 0xFF << 24) are not enabled by
> default either.
>
>> (And a lot of our uses of "-1 << 8" are indeed in constant expressions.)
>
> Those are okay as long as you do not use -Wextra.

This is all talking about in-practice behaviour of the compiler.
What I'm interested in is what the documented guarantees are.

> Again, the _value_ is perfectly specified by the GCC documentation (and
> as of this morning, it's promised to remain that way).  GCC leaves the
> door open for warning in constant expressions, and indeed GCC 6 warns
> more than GCC 5 in this regard.

I still don't understand why the GCC documentation can't straightforwardly
say "-fwrapv means you get 2s complement behaviour for all operations
including shifts and arithmetic, in all contexts, and we will not warn
or otherwise complain about it". If that's what they're in practice
agreeing to then why not just say so in a comprehensible way, rather
than splitting the information across two different sections of the
documentation and including a confusing bit of text about constant
expressions?

>> I don't think this gains us much as a different approach, and it's
>> still trying to do cleanup (2) before we have dealt with the main
>> issue (1).
>
> What I'm saying is that:
>
> - you were (rightly) worried about the compiler's behavior for constant
> expressions
>
> - but since we use -Werror, we do not have to worry about them.  With
> -Werror, anything that GCC 6 can compile is perfectly specified by the
> GCC documentation, including left shifts of negative values.
>
> So GCC does not even need -fwrapv, and -std=gnu89 is a better way to
> shut up ubsan because it already works.
>
> Regarding clang, we cannot be hostage of their silence.  And recalling
> that they were the ones who f***ed up their warning levels in the first
> place, and are making us waste so much time, I couldn't care less about
> their opinions.

It's been less than 10 days since you filed a bug report with them.
Why are we in such a hurry? I would much rather we took the time to
hear from the clang developers as well as the gcc developers, and then
made our decision about what the right compiler flags are. There doesn't
seem to be any sudden change here that means we need to adjust our
compiler flags for the 2.5 release.

>> > This is about implementation-defined rather than undefined behavior, but
>> > I think it's enough to not care about clang developer's silence.
>>
>> I disagree here. I think it's important to get the clang developers
>> to tell us what they mean by -fwrapv and what they want to guarantee
>> about signed shifts. That's because if they decide to say "no, we
>> don't guarantee behaviour here because the C spec says it's UB" then
>> we need to change how we deal with these in QEMU.
>
> No, we need to change our list of supported compilers (if that happens).

I would strongly favour avoiding this UB rather than dropping clang
as a supported compiler,and implicitly dropping OSX as a supported
platform. (But it doesn't seem to me very likely we'd end up having
to make that awkward choice.)

thanks
-- PMM
Paolo Bonzini Nov. 26, 2015, 4:06 p.m. UTC | #13
On 26/11/2015 16:55, Peter Maydell wrote:
> This is all talking about in-practice behaviour of the compiler.
> What I'm interested in is what the documented guarantees are.

They are these:

>> Again, the _value_ is perfectly specified by the GCC documentation (and
>> as of this morning, it's promised to remain that way).  GCC leaves the
>> door open for warning in constant expressions, and indeed GCC 6 warns
>> more than GCC 5 in this regard.

and they don't require -fwrapv at all.  I only added -fwrapv for ubsan,
but I now believe that C89 is a better way to shut it up.

> I still don't understand why the GCC documentation can't straightforwardly
> say "-fwrapv means you get 2s complement behaviour for all operations
> including shifts and arithmetic, in all contexts, and we will not warn
> or otherwise complain about it". If that's what they're in practice
> agreeing to then why not just say so in a comprehensible way, rather
> than splitting the information across two different sections of the
> documentation and including a confusing bit of text about constant
> expressions?

Because for example -fwrapv still gives you a SIGFPE for INT_MIN / -1.

>>>> This is about implementation-defined rather than undefined behavior, but
>>>> I think it's enough to not care about clang developer's silence.
>>>
>>> I disagree here. I think it's important to get the clang developers
>>> to tell us what they mean by -fwrapv and what they want to guarantee
>>> about signed shifts. That's because if they decide to say "no, we
>>> don't guarantee behaviour here because the C spec says it's UB" then
>>> we need to change how we deal with these in QEMU.
>>
>> No, we need to change our list of supported compilers (if that happens).
> 
> I would strongly favour avoiding this UB rather than dropping clang
> as a supported compiler,and implicitly dropping OSX as a supported
> platform.

gcc supports OS X, but...

> (But it doesn't seem to me very likely we'd end up having
> to make that awkward choice.)

... to me neither.   Also, if we had to make the choice, it'd probably
be a good idea anyway. :)

Paolo