Re: [Qemu-devel] [PULL 0/9] Misc patches for QEMU 2.5-rc2 (2015-11-25)

[Peter Maydell]

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