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

[Raghavendra D Prabhu <raghu.prabhu13@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3754 bytes --]

* On Tue, Jul 05, 2011 at 09:30:44PM +0100, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>On Tue, Jul 5, 2011 at 4:36 PM, Raghavendra D Prabhu
><raghu.prabhu13@gmail.com> wrote:
>> * On Mon, Jul 04, 2011 at 11:38:30PM +0100, Peter Maydell
>> <peter.maydell@linaro.org> wrote:

>>> On 4 July 2011 23:00, Raghavendra D Prabhu <raghu.prabhu13@gmail.com>
>>> 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
>
I have removed that option from Makefile; instead replaced it with another
assert which shouldn't be affected by overflow.
=======================================
  diff --git a/Makefile.hw b/Makefile.hw
  index 23dac45..b9181ab 100644
  --- 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 -fno-strict-overflow
  +QEMU_CFLAGS+=-I.. -I$(SRC_PATH)/fpu

   include $(SRC_PATH)/Makefile.objs

  diff --git a/hw/pcie.c b/hw/pcie.c
  index 39607bf..cfb11fe 100644
  --- a/hw/pcie.c
  +++ b/hw/pcie.c
  @@ -502,7 +502,7 @@ void pcie_add_capability(PCIDevice *dev,
       uint16_t next;

       assert(offset >= PCI_CONFIG_SPACE_SIZE);
  -    assert(offset < offset + size);
  +    assert(UINT_MAX - size > offset);
       assert(offset + size < PCIE_CONFIG_SPACE_SIZE);
       assert(size >= 8);
       assert(pci_is_express(dev));
--------------------------
Raghavendra Prabhu
GPG Id : 0xD72BE977
Fingerprint: B93F EBCB 8E05 7039 CD3C A4B8 A616 DCA1 D72B E977
www: wnohang.net

[-- Attachment #2: Type: application/pgp-signature, Size: 490 bytes --]