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

[Paolo Bonzini <pbonzini@redhat.com>]

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