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

[Paolo Bonzini]

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