[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PULL 0/9] Misc patches for QEMU 2.5-rc2 (2015-11-25)
From: |
Peter Maydell |
Subject: |
Re: [Qemu-devel] [PULL 0/9] Misc patches for QEMU 2.5-rc2 (2015-11-25) |
Date: |
Thu, 26 Nov 2015 15:01:49 +0000 |
On 26 November 2015 at 13:04, Paolo Bonzini <address@hidden> 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
- [Qemu-devel] [PULL 9/9] target-i386: kvm: Print warning when clearing mcg_cap bits, (continued)
- [Qemu-devel] [PULL 9/9] target-i386: kvm: Print warning when clearing mcg_cap bits, Paolo Bonzini, 2015/11/25
- Re: [Qemu-devel] [PULL 0/9] Misc patches for QEMU 2.5-rc2 (2015-11-25), Peter Maydell, 2015/11/26
- Re: [Qemu-devel] [PULL 0/9] Misc patches for QEMU 2.5-rc2 (2015-11-25), Paolo Bonzini, 2015/11/26
- Re: [Qemu-devel] [PULL 0/9] Misc patches for QEMU 2.5-rc2 (2015-11-25), Peter Maydell, 2015/11/26
- Re: [Qemu-devel] [PULL 0/9] Misc patches for QEMU 2.5-rc2 (2015-11-25), Paolo Bonzini, 2015/11/26
- Re: [Qemu-devel] [PULL 0/9] Misc patches for QEMU 2.5-rc2 (2015-11-25), Peter Maydell, 2015/11/26
- Re: [Qemu-devel] [PULL 0/9] Misc patches for QEMU 2.5-rc2 (2015-11-25), Markus Armbruster, 2015/11/26
- Re: [Qemu-devel] [PULL 0/9] Misc patches for QEMU 2.5-rc2 (2015-11-25), Peter Maydell, 2015/11/26
- Re: [Qemu-devel] [PULL 0/9] Misc patches for QEMU 2.5-rc2 (2015-11-25), Paolo Bonzini, 2015/11/26
- Re: [Qemu-devel] [PULL 0/9] Misc patches for QEMU 2.5-rc2 (2015-11-25), Paolo Bonzini, 2015/11/26
- Re: [Qemu-devel] [PULL 0/9] Misc patches for QEMU 2.5-rc2 (2015-11-25),
Peter Maydell <=
- Re: [Qemu-devel] [PULL 0/9] Misc patches for QEMU 2.5-rc2 (2015-11-25), Paolo Bonzini, 2015/11/26
- Re: [Qemu-devel] [PULL 0/9] Misc patches for QEMU 2.5-rc2 (2015-11-25), Peter Maydell, 2015/11/26
- Re: [Qemu-devel] [PULL 0/9] Misc patches for QEMU 2.5-rc2 (2015-11-25), Paolo Bonzini, 2015/11/26