Re: [Qemu-devel] [PATCH 2/3] Add fno-strict-overflow

[Stefan Hajnoczi]

On Tue, Jul 5, 2011 at 4:36 PM, Raghavendra D Prabhu
<address@hidden> wrote:
> * On Mon, Jul 04, 2011 at 11:38:30PM +0100, Peter Maydell
> <address@hidden> wrote:
>>
>> On 4 July 2011 23:00, Raghavendra D Prabhu <address@hidden>
>> wrote:
>>>
>>> This is to avoid gcc optimizating out the comparison in assert,
>>> due to assumption of signed overflow being undefined by default
>>> (-Werror=strict-overflow).
>>
>>> --- a/Makefile.hw
>>> +++ b/Makefile.hw
>>> @@ -9,7 +9,7 @@ include $(SRC_PATH)/rules.mak
>
>>> $(call set-vpath, $(SRC_PATH):$(SRC_PATH)/hw)
>
>>> -QEMU_CFLAGS+=-I.. -I$(SRC_PATH)/fpu
>>> +QEMU_CFLAGS+=-I.. -I$(SRC_PATH)/fpu -fno-strict-overflow
>>
>> Can you give a more detailed description of the problem this is trying
>> to solve? I think it would be nicer if we could remove the assumptions
>> about signed overflows instead, if that's practical.
>
> Following line in pcie.c:pcie_add_capability:505
>
>    assert(offset < offset + size);
>
> is what the compiler was warning about. The compiler optimizes out that
> comparison without fno-strict-overflow flag. More information about it
> is here -  http://www.airs.com/blog/archives/120 -- as already mentioned by
> Stefan.
>>
>> (Also, if we do want to add this compiler flag then it ought to be
>> done in configure I think, as we do for -fno-strict-aliasing.)
>>
> Globally adding that flag can limits the optimizations of gcc since in
> other places (loops) the undefined behavior can be advantageous, hence
> added only to Makefile.hw.

Doing this on a per-subsystem or per-file basis does not make sense to
me.  This is a general C coding issue that needs to be settled for the
entire codebase.  We will not catch instances of overflow slipping in
during patch review, so limiting the scope of -fno-strict-overflow is
not feasible.

I suggest we cover all of QEMU with -fwrapv instead of worrying about
-fno-strict-overflow.  That way we can get some optimizations and it
reflects the model that we are all assuming:
"This option instructs the compiler to assume that signed arithmetic
overflow of addition, subtraction and multiplication wraps around
using twos-complement representation. This flag enables some
optimizations and disables others. This option is enabled by default
for the Java front-end, as required by the Java language
specification."
http://gcc.gnu.org/onlinedocs/gcc-4.4.2/gcc/Code-Gen-Options.html

Stefan