[Qemu-devel] [PATCH 0/3] Build fixes

[Qemu-devel] [PATCH 0/3] Build fixes

[Raghavendra D Prabhu]

Hi,
    With default configure, the qemu-kvm client build was failing for me
    since Werror is enabled by default in configure.
    Deprecations (gnutls), gcc signed-overflow optimization
    (Werror=strict-overflows) and few unused-but-set variables were
    causing it. I have attached the patches after applying which, I got
    no further errors.


Raghavendra D Prabhu (3):
  Avoid the use of deprecated gnutls gnutls_*_set_priority functions.
  Add fno-strict-overflow
  Avoid Wunsed-but-set warnings (or errors in case of Werror)

 Makefile.hw            |    2 +-
 hw/device-assignment.c |    6 +++---
 simpletrace.c          |    2 +-
 ui/vnc-tls.c           |   20 +-------------------
 xen-mapcache.c         |    7 ++-----
 5 files changed, 8 insertions(+), 29 deletions(-)

-- 
1.7.6

--------------------------
Raghavendra Prabhu
GPG Id : 0xD72BE977
Fingerprint: B93F EBCB 8E05 7039 CD3C A4B8 A616 DCA1 D72B E977
www: wnohang.net

[Qemu-devel] [PATCH 1/3] Avoid the use of deprecated gnutls gnutls_*_set_priority functions.

[Raghavendra D Prabhu]

The gnutls_*_set_priority family of functions has been marked deprecated
in 2.12.x. These functions have been superceded by
gnutls_priority_set_direct().

Signed-off-by: Raghavendra D Prabhu <rprabhu@wnohang.net>
---
 ui/vnc-tls.c |   20 +-------------------
 1 files changed, 1 insertions(+), 19 deletions(-)

diff --git a/ui/vnc-tls.c b/ui/vnc-tls.c
index dec626c..33a5d8c 100644
--- a/ui/vnc-tls.c
+++ b/ui/vnc-tls.c
@@ -286,10 +286,6 @@ int vnc_tls_validate_certificate(struct VncState *vs)
 
 int vnc_tls_client_setup(struct VncState *vs,
                          int needX509Creds) {
-    static const int cert_type_priority[] = { GNUTLS_CRT_X509, 0 };
-    static const int protocol_priority[]= { GNUTLS_TLS1_1, GNUTLS_TLS1_0, GNUTLS_SSL3, 0 };
-    static const int kx_anon[] = {GNUTLS_KX_ANON_DH, 0};
-    static const int kx_x509[] = {GNUTLS_KX_DHE_DSS, GNUTLS_KX_RSA, GNUTLS_KX_DHE_RSA, GNUTLS_KX_SRP, 0};
 
     VNC_DEBUG("Do TLS setup\n");
     if (vnc_tls_initialize() < 0) {
@@ -310,21 +306,7 @@ int vnc_tls_client_setup(struct VncState *vs,
             return -1;
         }
 
-        if (gnutls_kx_set_priority(vs->tls.session, needX509Creds ? kx_x509 : kx_anon) < 0) {
-            gnutls_deinit(vs->tls.session);
-            vs->tls.session = NULL;
-            vnc_client_error(vs);
-            return -1;
-        }
-
-        if (gnutls_certificate_type_set_priority(vs->tls.session, cert_type_priority) < 0) {
-            gnutls_deinit(vs->tls.session);
-            vs->tls.session = NULL;
-            vnc_client_error(vs);
-            return -1;
-        }
-
-        if (gnutls_protocol_set_priority(vs->tls.session, protocol_priority) < 0) {
+        if (gnutls_priority_set_direct(vs->tls.session, needX509Creds ? "NORMAL" : "NORMAL:+ANON-DH", NULL) < 0) {
             gnutls_deinit(vs->tls.session);
             vs->tls.session = NULL;
             vnc_client_error(vs);
-- 
1.7.6

Re: [Qemu-devel] [PATCH 1/3] Avoid the use of deprecated gnutls gnutls_*_set_priority functions.

[Stefan Hajnoczi]

On Mon, Jul 4, 2011 at 11:00 PM, Raghavendra D Prabhu
<raghu.prabhu13@gmail.com> wrote:
> The gnutls_*_set_priority family of functions has been marked deprecated
> in 2.12.x. These functions have been superceded by
> gnutls_priority_set_direct().
>
> Signed-off-by: Raghavendra D Prabhu <rprabhu@wnohang.net>
> ---
>  ui/vnc-tls.c |   20 +-------------------
>  1 files changed, 1 insertions(+), 19 deletions(-)

Gerd,
You reported these F16 build failures.  I'm getting them now too and
Raghavendra's patch would be useful to fix them.

Stefan

Re: [Qemu-devel] [PATCH 1/3] Avoid the use of deprecated gnutls gnutls_*_set_priority functions.

[Gerd Hoffmann]

On 08/22/11 10:26, Stefan Hajnoczi wrote:
> On Mon, Jul 4, 2011 at 11:00 PM, Raghavendra D Prabhu
> <raghu.prabhu13@gmail.com>  wrote:
>> The gnutls_*_set_priority family of functions has been marked deprecated
>> in 2.12.x. These functions have been superceded by
>> gnutls_priority_set_direct().
>>
>> Signed-off-by: Raghavendra D Prabhu<rprabhu@wnohang.net>
>> ---
>>   ui/vnc-tls.c |   20 +-------------------
>>   1 files changed, 1 insertions(+), 19 deletions(-)
>
> Gerd,
> You reported these F16 build failures.  I'm getting them now too and
> Raghavendra's patch would be useful to fix them.

Indeed.  Patch looks good to me.

cheers,
   Gerd

Re: [Qemu-devel] [PATCH 1/3] Avoid the use of deprecated gnutls gnutls_*_set_priority functions.

[Stefan Hajnoczi]

On Mon, Jul 4, 2011 at 11:00 PM, Raghavendra D Prabhu
<raghu.prabhu13@gmail.com> wrote:
> The gnutls_*_set_priority family of functions has been marked deprecated
> in 2.12.x. These functions have been superceded by
> gnutls_priority_set_direct().
>
> Signed-off-by: Raghavendra D Prabhu <rprabhu@wnohang.net>
> ---
>  ui/vnc-tls.c |   20 +-------------------
>  1 files changed, 1 insertions(+), 19 deletions(-)
>
> diff --git a/ui/vnc-tls.c b/ui/vnc-tls.c
> index dec626c..33a5d8c 100644
> --- a/ui/vnc-tls.c
> +++ b/ui/vnc-tls.c
> @@ -286,10 +286,6 @@ int vnc_tls_validate_certificate(struct VncState *vs)
>
>  int vnc_tls_client_setup(struct VncState *vs,
>                          int needX509Creds) {
> -    static const int cert_type_priority[] = { GNUTLS_CRT_X509, 0 };
> -    static const int protocol_priority[]= { GNUTLS_TLS1_1, GNUTLS_TLS1_0, GNUTLS_SSL3, 0 };
> -    static const int kx_anon[] = {GNUTLS_KX_ANON_DH, 0};
> -    static const int kx_x509[] = {GNUTLS_KX_DHE_DSS, GNUTLS_KX_RSA, GNUTLS_KX_DHE_RSA, GNUTLS_KX_SRP, 0};
>
>     VNC_DEBUG("Do TLS setup\n");
>     if (vnc_tls_initialize() < 0) {
> @@ -310,21 +306,7 @@ int vnc_tls_client_setup(struct VncState *vs,
>             return -1;
>         }
>
> -        if (gnutls_kx_set_priority(vs->tls.session, needX509Creds ? kx_x509 : kx_anon) < 0) {
> -            gnutls_deinit(vs->tls.session);
> -            vs->tls.session = NULL;
> -            vnc_client_error(vs);
> -            return -1;
> -        }
> -
> -        if (gnutls_certificate_type_set_priority(vs->tls.session, cert_type_priority) < 0) {
> -            gnutls_deinit(vs->tls.session);
> -            vs->tls.session = NULL;
> -            vnc_client_error(vs);
> -            return -1;
> -        }
> -
> -        if (gnutls_protocol_set_priority(vs->tls.session, protocol_priority) < 0) {
> +        if (gnutls_priority_set_direct(vs->tls.session, needX509Creds ? "NORMAL" : "NORMAL:+ANON-DH", NULL) < 0) {
>             gnutls_deinit(vs->tls.session);
>             vs->tls.session = NULL;
>             vnc_client_error(vs);
> --
> 1.7.6

Daniel,
This patch looks good to me but I don't know much about gnutls or
crypto in general.  Would you be willing to review this?

Thanks,
Stefan

Re: [Qemu-devel] [PATCH 1/3] Avoid the use of deprecated gnutls gnutls_*_set_priority functions.

[Daniel P. Berrange]

On Thu, Aug 25, 2011 at 11:54:41AM +0100, Stefan Hajnoczi wrote:
> On Mon, Jul 4, 2011 at 11:00 PM, Raghavendra D Prabhu
> <raghu.prabhu13@gmail.com> wrote:
> > The gnutls_*_set_priority family of functions has been marked deprecated
> > in 2.12.x. These functions have been superceded by
> > gnutls_priority_set_direct().
> >
> > Signed-off-by: Raghavendra D Prabhu <rprabhu@wnohang.net>
> > ---
> >  ui/vnc-tls.c |   20 +-------------------
> >  1 files changed, 1 insertions(+), 19 deletions(-)
> >
> > diff --git a/ui/vnc-tls.c b/ui/vnc-tls.c
> > index dec626c..33a5d8c 100644
> > --- a/ui/vnc-tls.c
> > +++ b/ui/vnc-tls.c
> > @@ -286,10 +286,6 @@ int vnc_tls_validate_certificate(struct VncState *vs)
> >
> >  int vnc_tls_client_setup(struct VncState *vs,
> >                          int needX509Creds) {
> > -    static const int cert_type_priority[] = { GNUTLS_CRT_X509, 0 };
> > -    static const int protocol_priority[]= { GNUTLS_TLS1_1, GNUTLS_TLS1_0, GNUTLS_SSL3, 0 };
> > -    static const int kx_anon[] = {GNUTLS_KX_ANON_DH, 0};
> > -    static const int kx_x509[] = {GNUTLS_KX_DHE_DSS, GNUTLS_KX_RSA, GNUTLS_KX_DHE_RSA, GNUTLS_KX_SRP, 0};
> >
> >     VNC_DEBUG("Do TLS setup\n");
> >     if (vnc_tls_initialize() < 0) {
> > @@ -310,21 +306,7 @@ int vnc_tls_client_setup(struct VncState *vs,
> >             return -1;
> >         }
> >
> > -        if (gnutls_kx_set_priority(vs->tls.session, needX509Creds ? kx_x509 : kx_anon) < 0) {
> > -            gnutls_deinit(vs->tls.session);
> > -            vs->tls.session = NULL;
> > -            vnc_client_error(vs);
> > -            return -1;
> > -        }
> > -
> > -        if (gnutls_certificate_type_set_priority(vs->tls.session, cert_type_priority) < 0) {
> > -            gnutls_deinit(vs->tls.session);
> > -            vs->tls.session = NULL;
> > -            vnc_client_error(vs);
> > -            return -1;
> > -        }
> > -
> > -        if (gnutls_protocol_set_priority(vs->tls.session, protocol_priority) < 0) {
> > +        if (gnutls_priority_set_direct(vs->tls.session, needX509Creds ? "NORMAL" : "NORMAL:+ANON-DH", NULL) < 0) {
> >             gnutls_deinit(vs->tls.session);
> >             vs->tls.session = NULL;
> >             vnc_client_error(vs);
> > --
> > 1.7.6
> 
> Daniel,
> This patch looks good to me but I don't know much about gnutls or
> crypto in general.  Would you be willing to review this?

ACK, this approach is different from what I did in libvirt, but it matches
the recommendations in the GNUTLS manual for setting priority, so I believe
it is good.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

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

[Raghavendra D Prabhu]

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

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
 
-- 
1.7.6

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

[Peter Maydell]

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

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

[Stefan Hajnoczi]

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

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

[Stefan Hajnoczi]

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>

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

[Raghavendra D Prabhu]

* 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

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

[Stefan Hajnoczi]

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

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

[Raghavendra D Prabhu]

[-- 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 --]

[Qemu-devel] [PATCH 3/3] Avoid Wunsed-but-set warnings (or errors in case of Werror)

[Raghavendra D Prabhu]

In a few cases, variable attributed 'unused' has been added, in other cases
unused variable has been either removed or commented out.

Signed-off-by: Raghavendra D Prabhu <rprabhu@wnohang.net>
---
 hw/device-assignment.c |    6 +++---
 simpletrace.c          |    2 +-
 xen-mapcache.c         |    7 ++-----
 3 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/hw/device-assignment.c b/hw/device-assignment.c
index 36ad6b0..19a59b4 100644
--- a/hw/device-assignment.c
+++ b/hw/device-assignment.c
@@ -1654,7 +1654,7 @@ static void reset_assigned_device(DeviceState *dev)
     AssignedDevice *adev = DO_UPCAST(AssignedDevice, dev, pci_dev);
     char reset_file[64];
     const char reset[] = "1";
-    int fd, ret;
+    int fd, __attribute__((unused)) ret;
 
     snprintf(reset_file, sizeof(reset_file),
              "/sys/bus/pci/devices/%04x:%02x:%02x.%01x/reset",
@@ -1682,7 +1682,7 @@ static void reset_assigned_device(DeviceState *dev)
 static int assigned_initfn(struct PCIDevice *pci_dev)
 {
     AssignedDevice *dev = DO_UPCAST(AssignedDevice, dev, pci_dev);
-    uint8_t e_device, e_intx;
+    uint8_t e_intx;
     int r;
 
     if (!kvm_enabled()) {
@@ -1709,7 +1709,7 @@ static int assigned_initfn(struct PCIDevice *pci_dev)
         goto out;
 
     /* handle interrupt routing */
-    e_device = (dev->dev.devfn >> 3) & 0x1f;
+    /*e_device = (dev->dev.devfn >> 3) & 0x1f;*/
     e_intx = dev->dev.config[0x3d] - 1;
     dev->intpin = e_intx;
     dev->run = 0;
diff --git a/simpletrace.c b/simpletrace.c
index f1dbb5e..2ce9cff 100644
--- a/simpletrace.c
+++ b/simpletrace.c
@@ -119,7 +119,7 @@ static void *writeout_thread(void *opaque)
     TraceRecord record;
     unsigned int writeout_idx = 0;
     unsigned int num_available, idx;
-    size_t unused;
+    size_t __attribute__((unused)) unused;
 
     for (;;) {
         wait_for_trace_records_available();
diff --git a/xen-mapcache.c b/xen-mapcache.c
index fac47cd..94642bc 100644
--- a/xen-mapcache.c
+++ b/xen-mapcache.c
@@ -166,7 +166,7 @@ static void qemu_remap_bucket(MapCacheEntry *entry,
 
 uint8_t *qemu_map_cache(target_phys_addr_t phys_addr, target_phys_addr_t size, uint8_t lock)
 {
-    MapCacheEntry *entry, *pentry = NULL;
+    MapCacheEntry *entry;
     target_phys_addr_t address_index  = phys_addr >> MCACHE_BUCKET_SHIFT;
     target_phys_addr_t address_offset = phys_addr & (MCACHE_BUCKET_SIZE - 1);
     target_phys_addr_t __size = size;
@@ -192,12 +192,10 @@ uint8_t *qemu_map_cache(target_phys_addr_t phys_addr, target_phys_addr_t size, u
             (entry->paddr_index != address_index || entry->size != __size ||
              !test_bits(address_offset >> XC_PAGE_SHIFT, size >> XC_PAGE_SHIFT,
                  entry->valid_mapping))) {
-        pentry = entry;
         entry = entry->next;
     }
     if (!entry) {
         entry = qemu_mallocz(sizeof (MapCacheEntry));
-        pentry->next = entry;
         qemu_remap_bucket(entry, __size, address_index);
     } else if (!entry->lock) {
         if (!entry->vaddr_base || entry->paddr_index != address_index ||
@@ -232,7 +230,7 @@ uint8_t *qemu_map_cache(target_phys_addr_t phys_addr, target_phys_addr_t size, u
 
 ram_addr_t qemu_ram_addr_from_mapcache(void *ptr)
 {
-    MapCacheEntry *entry = NULL, *pentry = NULL;
+    MapCacheEntry *entry = NULL;
     MapCacheRev *reventry;
     target_phys_addr_t paddr_index;
     target_phys_addr_t size;
@@ -258,7 +256,6 @@ ram_addr_t qemu_ram_addr_from_mapcache(void *ptr)
 
     entry = &mapcache->entry[paddr_index % mapcache->nr_buckets];
     while (entry && (entry->paddr_index != paddr_index || entry->size != size)) {
-        pentry = entry;
         entry = entry->next;
     }
     if (!entry) {
-- 
1.7.6

Re: [Qemu-devel] [PATCH 3/3] Avoid Wunsed-but-set warnings (or errors in case of Werror)

[Markus Armbruster]

Typo in subject: "unsed".  The warning is spelled
"unused-but-set-variable", the option "-Wunused-but-set-variable".

Raghavendra D Prabhu <raghu.prabhu13@gmail.com> writes:

> In a few cases, variable attributed 'unused' has been added, in other cases
> unused variable has been either removed or commented out.
>
> Signed-off-by: Raghavendra D Prabhu <rprabhu@wnohang.net>
> ---
>  hw/device-assignment.c |    6 +++---
>  simpletrace.c          |    2 +-
>  xen-mapcache.c         |    7 ++-----
>  3 files changed, 6 insertions(+), 9 deletions(-)
>
> diff --git a/hw/device-assignment.c b/hw/device-assignment.c
> index 36ad6b0..19a59b4 100644
> --- a/hw/device-assignment.c
> +++ b/hw/device-assignment.c
> @@ -1654,7 +1654,7 @@ static void reset_assigned_device(DeviceState *dev)
>      AssignedDevice *adev = DO_UPCAST(AssignedDevice, dev, pci_dev);
>      char reset_file[64];
>      const char reset[] = "1";
> -    int fd, ret;
> +    int fd, __attribute__((unused)) ret;
>  
>      snprintf(reset_file, sizeof(reset_file),
>               "/sys/bus/pci/devices/%04x:%02x:%02x.%01x/reset",

What about (void)write() and do away with ret?

> @@ -1682,7 +1682,7 @@ static void reset_assigned_device(DeviceState *dev)
>  static int assigned_initfn(struct PCIDevice *pci_dev)
>  {
>      AssignedDevice *dev = DO_UPCAST(AssignedDevice, dev, pci_dev);
> -    uint8_t e_device, e_intx;
> +    uint8_t e_intx;
>      int r;
>  
>      if (!kvm_enabled()) {
> @@ -1709,7 +1709,7 @@ static int assigned_initfn(struct PCIDevice *pci_dev)
>          goto out;
>  
>      /* handle interrupt routing */
> -    e_device = (dev->dev.devfn >> 3) & 0x1f;
> +    /*e_device = (dev->dev.devfn >> 3) & 0x1f;*/
>      e_intx = dev->dev.config[0x3d] - 1;
>      dev->intpin = e_intx;
>      dev->run = 0;
> diff --git a/simpletrace.c b/simpletrace.c
> index f1dbb5e..2ce9cff 100644
> --- a/simpletrace.c
> +++ b/simpletrace.c
> @@ -119,7 +119,7 @@ static void *writeout_thread(void *opaque)
>      TraceRecord record;
>      unsigned int writeout_idx = 0;
>      unsigned int num_available, idx;
> -    size_t unused;
> +    size_t __attribute__((unused)) unused;
>  
>      for (;;) {
>          wait_for_trace_records_available();

Same here.

[...]

Re: [Qemu-devel] [PATCH 3/3] Avoid Wunsed-but-set warnings (or errors in case of Werror)

[Peter Maydell]

On 5 July 2011 07:15, Markus Armbruster <armbru@redhat.com> wrote:
>> +    int fd, __attribute__((unused)) ret;
>>
>>      snprintf(reset_file, sizeof(reset_file),
>>               "/sys/bus/pci/devices/%04x:%02x:%02x.%01x/reset",
>
> What about (void)write() and do away with ret?

If 'ret' has been used to silence compiler warnings about functions
which have been declared with attribute __warn_unused_result__
(eg write() and various other libc functions) then "(void)write()"
is insufficient -- gcc requires the variable.

-- PMM

Re: [Qemu-devel] [PATCH 3/3] Avoid Wunsed-but-set warnings (or errors in case of Werror)

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 5 July 2011 07:15, Markus Armbruster <armbru@redhat.com> wrote:
>>> +    int fd, __attribute__((unused)) ret;
>>>
>>>      snprintf(reset_file, sizeof(reset_file),
>>>               "/sys/bus/pci/devices/%04x:%02x:%02x.%01x/reset",
>>
>> What about (void)write() and do away with ret?
>
> If 'ret' has been used to silence compiler warnings about functions
> which have been declared with attribute __warn_unused_result__
> (eg write() and various other libc functions) then "(void)write()"
> is insufficient -- gcc requires the variable.

gcc being silly.  Oh well.

Re: [Qemu-devel] [PATCH 3/3] Avoid Wunsed-but-set warnings (or errors in case of Werror)

[Paolo Bonzini]

On 07/05/2011 09:49 AM, Markus Armbruster wrote:
> >  If 'ret' has been used to silence compiler warnings about functions
> >  which have been declared with attribute __warn_unused_result__
> >  (eg write() and various other libc functions) then "(void)write()"
> >  is insufficient -- gcc requires the variable.
>
> gcc being silly.  Oh well.

In this particular case I think that the return value should be checked. 
  It's good if something is printed in the log saying that the reset 
wasn't done for some reason---even if it is just defensive.

The really silly thing is in glibc, not gcc.  __warn_unused_result__ was 
added to fwrite, where you have no certainty that the write has been 
done, but not to either fclose or fflush.  Oh well...

Paolo

[Qemu-devel] [PATCH 0/3] Build fixes

[Juan Quintela]

This series contains all build fixes on my queue:
- fix slirp to compile with a custom CFLAGS
- Add -Wold-style-* flags only if compiler understand them
- Fix last users of TRUE/FALSE.  Now we can include <stdbool.h> anywhere.

Later, Juan.

Juan Quintela (3):
  slirp: It needs to use QEMU_CFLAGS not CFLAGS
  Add -Wold-style-* flags
  bdf: Remove last users of FALSE/TRUE

 arm-dis.c        |   60 ++++++++++++++++++++++++++---------------------------
 configure        |   12 +++++++++-
 cris-dis.c       |   18 +++++++---------
 dis-asm.h        |    4 +-
 microblaze-dis.c |    2 +-
 5 files changed, 51 insertions(+), 45 deletions(-)