[Qemu-devel] [PATCH 0/2] Fix GCC 5.0.0 build errors

[Qemu-devel] [PATCH 0/2] Fix GCC 5.0.0 build errors

[Radim Krčmář]

Future patches should address the out-of-bounds access that is left by
using proposed solution for [2/2].

Radim Krčmář (2):
  fix GCC 5.0.0 logical-not-parentheses warnings
  milkymist-pfpu: fix GCC 5.0.0 aggressive-loop-optimizations warning

 hw/misc/milkymist-pfpu.c | 2 +-
 hw/net/virtio-net.c      | 4 ++--
 kvm-all.c                | 2 +-
 qemu-img.c               | 3 ++-
 4 files changed, 6 insertions(+), 5 deletions(-)

-- 
2.3.0

[Qemu-devel] [PATCH 1/2] fix GCC 5.0.0 logical-not-parentheses warnings

[Radim Krčmář]

man gcc:
  Warn about logical not used on the left hand side operand of a
  comparison.  This option does not warn if the RHS operand is of a
  boolean type.

By preferring bool over int where sensible, but without modifying any
depending code, make GCC happy in cases like this,
  qemu-img.c: In function ‘compare_sectors’:
  qemu-img.c:992:39: error: logical not is only applied to the left hand
  side of comparison [-Werror=logical-not-parentheses]
           if (!!memcmp(buf1, buf2, 512) != res) {

hw/ide/core.c:1836 doesn't throw an error,
  assert(!!s->error == !!(s->status & ERR_STAT));
even thought the second operand is int (and first hunk of this patch has
a very similar case), maybe GCC developers still have a little faith in
C programmers.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 v2: swapped lines in hunk 1 to to avoid parentheses [Paolo]

 hw/net/virtio-net.c | 4 ++--
 kvm-all.c           | 2 +-
 qemu-img.c          | 3 ++-
 3 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 45da34ad6129..93818675588e 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -120,8 +120,8 @@ static void virtio_net_vhost_status(VirtIONet *n, uint8_t status)
         return;
     }
 
-    if (!!n->vhost_started ==
-        (virtio_net_started(n, status) && !nc->peer->link_down)) {
+    if ((virtio_net_started(n, status) && !nc->peer->link_down) ==
+        !!n->vhost_started) {
         return;
     }
     if (!n->vhost_started) {
diff --git a/kvm-all.c b/kvm-all.c
index 05a79c20e0bb..07ef62cb3227 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -366,7 +366,7 @@ static void kvm_log_stop(MemoryListener *listener,
     }
 }
 
-static int kvm_set_migration_log(int enable)
+static int kvm_set_migration_log(bool enable)
 {
     KVMState *s = kvm_state;
     KVMSlot *mem;
diff --git a/qemu-img.c b/qemu-img.c
index e148af8a3e64..21fff2ad53d5 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -977,7 +977,8 @@ static int is_allocated_sectors_min(const uint8_t *buf, int n, int *pnum,
 static int compare_sectors(const uint8_t *buf1, const uint8_t *buf2, int n,
     int *pnum)
 {
-    int res, i;
+    bool res;
+    int i;
 
     if (n <= 0) {
         *pnum = 0;
-- 
2.3.0

[Qemu-devel] [PATCH 2/2] milkymist-pfpu: fix GCC 5.0.0 aggressive-loop-optimizations warning

[Radim Krčmář]

man gcc:
  Warn if in a loop with constant number of iterations the compiler
  detects undefined behavior in some statement during one or more of
  the iterations.

Milkymist pfpu has no jump instructions, so checking for MICROCODE_WORDS
instructions should have kept us in bounds of s->microcode, but i++
allowed one loop too many,

  hw/misc/milkymist-pfpu.c: In function ‘pfpu_write’:
  hw/misc/milkymist-pfpu.c:365:20: error: loop exit may only be reached after undefined behavior [-Werror=aggressive-loop-optimizations]
                   if (i++ >= MICROCODE_WORDS) {
                      ^
  hw/misc/milkymist-pfpu.c:167:14: note: possible undefined statement is here
       uint32_t insn = s->microcode[pc];
                ^

The code can still access out of bounds, because it presumes that PC register
always begins at 0, and we allow writing to it.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 v2: used a simpler solution [Paolo, Peter]

 hw/misc/milkymist-pfpu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/misc/milkymist-pfpu.c b/hw/misc/milkymist-pfpu.c
index 609f33f9cd14..08b604f13f4b 100644
--- a/hw/misc/milkymist-pfpu.c
+++ b/hw/misc/milkymist-pfpu.c
@@ -362,7 +362,7 @@ static void pfpu_start(MilkymistPFPUState *s)
             i = 0;
             while (pfpu_decode_insn(s)) {
                 /* decode at most MICROCODE_WORDS instructions */
-                if (i++ >= MICROCODE_WORDS) {
+                if (++i >= MICROCODE_WORDS) {
                     error_report("milkymist_pfpu: too many instructions "
                             "executed in microcode. No VECTOUT?");
                     break;
-- 
2.3.0

Re: [Qemu-devel] [PATCH 0/2] Fix GCC 5.0.0 build errors

[Radim Krčmář]

Cc: qemu-trivial@nongnu.org

Re: [Qemu-devel] [PATCH 2/2] milkymist-pfpu: fix GCC 5.0.0 aggressive-loop-optimizations warning

[Michael Walle]

Am 2015-02-20 17:06, schrieb Radim Krčmář:
> man gcc:
>   Warn if in a loop with constant number of iterations the compiler
>   detects undefined behavior in some statement during one or more of
>   the iterations.
> 
> Milkymist pfpu has no jump instructions, so checking for 
> MICROCODE_WORDS
> instructions should have kept us in bounds of s->microcode, but i++
> allowed one loop too many,
> 
>   hw/misc/milkymist-pfpu.c: In function ‘pfpu_write’:
>   hw/misc/milkymist-pfpu.c:365:20: error: loop exit may only be
> reached after undefined behavior
> [-Werror=aggressive-loop-optimizations]
>                    if (i++ >= MICROCODE_WORDS) {
>                       ^
>   hw/misc/milkymist-pfpu.c:167:14: note: possible undefined statement 
> is here
>        uint32_t insn = s->microcode[pc];
>                 ^
> 
> The code can still access out of bounds, because it presumes that PC 
> register
> always begins at 0, and we allow writing to it.
> 
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> ---
>  v2: used a simpler solution [Paolo, Peter]
> 
>  hw/misc/milkymist-pfpu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/misc/milkymist-pfpu.c b/hw/misc/milkymist-pfpu.c
> index 609f33f9cd14..08b604f13f4b 100644
> --- a/hw/misc/milkymist-pfpu.c
> +++ b/hw/misc/milkymist-pfpu.c
> @@ -362,7 +362,7 @@ static void pfpu_start(MilkymistPFPUState *s)
>              i = 0;
>              while (pfpu_decode_insn(s)) {
>                  /* decode at most MICROCODE_WORDS instructions */
> -                if (i++ >= MICROCODE_WORDS) {
> +                if (++i >= MICROCODE_WORDS) {
>                      error_report("milkymist_pfpu: too many 
> instructions "
>                              "executed in microcode. No VECTOUT?");
>                      break;

Acked-by: Michael Walle <michael@walle.cc>

Re: [Qemu-devel] [PATCH 1/2] fix GCC 5.0.0 logical-not-parentheses warnings

[Michael Tokarev]

20.02.2015 19:06, Radim Krčmář wrote:
[]
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 45da34ad6129..93818675588e 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -120,8 +120,8 @@ static void virtio_net_vhost_status(VirtIONet *n, uint8_t status)
>          return;
>      }
>  
> -    if (!!n->vhost_started ==
> -        (virtio_net_started(n, status) && !nc->peer->link_down)) {
> +    if ((virtio_net_started(n, status) && !nc->peer->link_down) ==
> +        !!n->vhost_started) {

btw, can this be rewritten as
  (bool)n->vhost_started
instead of
  !!n->vhos_started

?

Not questioning the patch itself, just wondering, as these
double-negatives look ugly...

Thanks,

/mjt

Re: [Qemu-devel] [PATCH 0/2] Fix GCC 5.0.0 build errors

[Michael Tokarev]

20.02.2015 19:06, Radim Krčmář wrote:
> Future patches should address the out-of-bounds access that is left by
> using proposed solution for [2/2].
> 
> Radim Krčmář (2):
>   fix GCC 5.0.0 logical-not-parentheses warnings
>   milkymist-pfpu: fix GCC 5.0.0 aggressive-loop-optimizations warning
> 
>  hw/misc/milkymist-pfpu.c | 2 +-
>  hw/net/virtio-net.c      | 4 ++--
>  kvm-all.c                | 2 +-
>  qemu-img.c               | 3 ++-
>  4 files changed, 6 insertions(+), 5 deletions(-)

Applied both to -trivial finally.  Thank you!

/mjt

Re: [Qemu-devel] [PATCH 1/2] fix GCC 5.0.0 logical-not-parentheses warnings

[Radim Krčmář]

2015-03-04 17:36+0300, Michael Tokarev:
> 20.02.2015 19:06, Radim Krčmář wrote:
> > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > -    if (!!n->vhost_started ==
> > -        (virtio_net_started(n, status) && !nc->peer->link_down)) {
> > +    if ((virtio_net_started(n, status) && !nc->peer->link_down) ==
> > +        !!n->vhost_started) {
> 
> btw, can this be rewritten as
>   (bool)n->vhost_started
> instead of
>   !!n->vhos_started

Yes.  (It's the same as long as we use bool from stdbool.h.)

> Not questioning the patch itself, just wondering, as these
> double-negatives look ugly...

Casting to bool looks better to me as well, yet the QEMU design guide,
`grep $x | wc -l`, greatly prefers bangs.

I think that it is best to define it as bool in struct VirtIONet,
but I prefer not to change decision that I don't understand ...

Thanks for accepting the patches.

[Qemu-devel] [PATCH 0/2] Fix GCC 5.0.0 build errors

[Radim Krčmář]

GCC is more strict now.

Radim Krčmář (2):
  fix GCC 5.0.0 logical-not-parentheses warnings
  milkymist-pfpu: fix GCC 5.0.0 aggressive-loop-optimizations warning

 hw/misc/milkymist-pfpu.c | 18 ++++++++----------
 hw/net/virtio-net.c      |  2 +-
 kvm-all.c                |  2 +-
 qemu-img.c               |  3 ++-
 4 files changed, 12 insertions(+), 13 deletions(-)

-- 
2.3.0