| Message ID | bfecbc89c76320fe48a0efaa21fef7085b0d04bb.1309816302.git.rprabhu@wnohang.net |
|---|---|
| State | New |
| Headers | show |
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
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
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>
* 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
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
* 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
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
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(-)