Patchwork [2/3] Add fno-strict-overflow

login
register
mail settings
Submitter Raghavendra D Prabhu
Date July 4, 2011, 10 p.m.
Message ID <bfecbc89c76320fe48a0efaa21fef7085b0d04bb.1309816302.git.rprabhu@wnohang.net>
Download mbox | patch
Permalink /patch/103176/
State New
Headers show

Comments

Raghavendra D Prabhu - July 4, 2011, 10 p.m.
This is to avoid gcc optimizating out the comparison in assert,
due to assumption of signed overflow being undefined by default (-Werror=strict-overflow).

Signed-off-by: Raghavendra D Prabhu <rprabhu@wnohang.net>
---
 Makefile.hw |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Peter Maydell - July 4, 2011, 10:38 p.m.
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.

(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.)

-- PMM
Stefan Hajnoczi - July 5, 2011, 5:41 a.m.
On Mon, Jul 4, 2011 at 11:38 PM, 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.
>
> (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.)

"a correct C/C++ program must never generate signed overflow when
computing an expression. It also means that a compiler may assume that
a program will never generated signed overflow."

http://www.airs.com/blog/archives/120

Stefan
Stefan Hajnoczi - July 5, 2011, 9:34 a.m.
On Tue, Jul 5, 2011 at 6:41 AM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Mon, Jul 4, 2011 at 11:38 PM, 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.
>>
>> (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.)
>
> "a correct C/C++ program must never generate signed overflow when
> computing an expression. It also means that a compiler may assume that
> a program will never generated signed overflow."
>
> http://www.airs.com/blog/archives/120

You can check out the warnings that gcc raises with ./configure
--extra-cflags="-Wstrict-overflow -fstrict-overflow" --disable-werror.

Either we'd have to fix the warnings or we could use
-fno-strict-overflow/-fwrapv.

This patch seems reasonable to me.  We're telling gcc not to take
advantage of the undefined behavior of signed overflow.  It also means
QEMU code is assuming two's complement representation and wrapping on
overflow, but that is a common assumption (what QEMU-capable hardware
doesn't?).

Reviewed-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
Raghavendra D Prabhu - July 5, 2011, 3:36 p.m.
* 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.
>-- PMM
>

--------------------------
Raghavendra Prabhu
GPG Id : 0xD72BE977
Fingerprint: B93F EBCB 8E05 7039 CD3C A4B8 A616 DCA1 D72B E977
www: wnohang.net
Stefan Hajnoczi - July 5, 2011, 8:30 p.m.
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
Raghavendra D Prabhu - July 7, 2011, 9:51 p.m.
* 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

Patch

diff --git a/Makefile.hw b/Makefile.hw
index b9181ab..23dac45 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
+QEMU_CFLAGS+=-I.. -I$(SRC_PATH)/fpu -fno-strict-overflow
 
 include $(SRC_PATH)/Makefile.objs