Patchwork s390x: silence warning from GCC on uninitialized values

login
register
mail settings
Submitter Cornelia Huck
Date Feb. 5, 2013, 9:27 a.m.
Message ID <20130205102700.1f745c36@gondolin>
Download mbox | patch
Permalink /patch/218200/
State New
Headers show

Comments

Cornelia Huck - Feb. 5, 2013, 9:27 a.m.
On Mon, 04 Feb 2013 22:57:43 +0100
Stefan Weil <sw@weilnetz.de> wrote:

> Am 04.02.2013 22:23, schrieb Anthony Liguori:
> > As best I can tell, this is a false positive.
> >
> >   [aliguori@ccnode4 qemu-s390]$ make
> >     CC    s390x-softmmu/target-s390x/helper.o
> >   /home/aliguori/git/qemu/target-s390x/helper.c: In function ‘do_interrupt’:
> >   /home/aliguori/git/qemu/target-s390x/helper.c:673:17: error: ‘addr’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
> >   /home/aliguori/git/qemu/target-s390x/helper.c:620:20: note: ‘addr’ was declared here
> >   /home/aliguori/git/qemu/target-s390x/helper.c:673:17: error: ‘mask’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
> >   /home/aliguori/git/qemu/target-s390x/helper.c:620:14: note: ‘mask’ was declared here
> >   cc1: all warnings being treated as errors
> >   make[1]: *** [target-s390x/helper.o] Error 1
> >   make: *** [subdir-s390x-softmmu] Error 2
> >
> 
> Yes, this is a false positive. A better compiler will complain when your
> patch was applied because addr, mask are assigned values which are
> never used...
> 
> Would it be possible to completely eliminate variable "found" and
> move the DPRINTF, load_psw statements into the for loop (just before
> the break statement)?

We could move the lpsw. However, this made me notice another problem:
We stop scanning subsequent iscs if we found an interrupt to inject...

This is not a problem for current virtio-ccw based Linux guests since
they never use anything else than isc 3, but we'll probably want the
following patch.

From 8b2f40e8eaac16b55a72ab1e36a4c5de0b016495 Mon Sep 17 00:00:00 2001
From: Cornelia Huck <cornelia.huck@de.ibm.com>
Date: Tue, 5 Feb 2013 10:14:49 +0100
Subject: [PATCH] s390: Keep I/O interrupts enabled for all iscs.

do_io_interrupt() would stop scanning further iscs if it found
an I/O interrupt it could inject. This might cause the pending
interrupt indication for I/O interrupts to be reset although there
might be queued I/O interrupts for subsequent iscs.

Fix this by reordering the logic: Inject the I/O interrupt immediately
and continue searching all iscs for queued interrupts.

Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
---
 target-s390x/helper.c | 40 +++++++++++++++++++++-------------------
 1 file changed, 21 insertions(+), 19 deletions(-)
Cornelia Huck - Feb. 7, 2013, 8:52 a.m.
On Tue, 5 Feb 2013 10:27:00 +0100
Cornelia Huck <cornelia.huck@de.ibm.com> wrote:

> On Mon, 04 Feb 2013 22:57:43 +0100
> Stefan Weil <sw@weilnetz.de> wrote:
> 
> > Am 04.02.2013 22:23, schrieb Anthony Liguori:
> > > As best I can tell, this is a false positive.
> > >
> > >   [aliguori@ccnode4 qemu-s390]$ make
> > >     CC    s390x-softmmu/target-s390x/helper.o
> > >   /home/aliguori/git/qemu/target-s390x/helper.c: In function ‘do_interrupt’:
> > >   /home/aliguori/git/qemu/target-s390x/helper.c:673:17: error: ‘addr’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
> > >   /home/aliguori/git/qemu/target-s390x/helper.c:620:20: note: ‘addr’ was declared here
> > >   /home/aliguori/git/qemu/target-s390x/helper.c:673:17: error: ‘mask’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
> > >   /home/aliguori/git/qemu/target-s390x/helper.c:620:14: note: ‘mask’ was declared here
> > >   cc1: all warnings being treated as errors
> > >   make[1]: *** [target-s390x/helper.o] Error 1
> > >   make: *** [subdir-s390x-softmmu] Error 2
> > >
> > 
> > Yes, this is a false positive. A better compiler will complain when your
> > patch was applied because addr, mask are assigned values which are
> > never used...
> > 
> > Would it be possible to completely eliminate variable "found" and
> > move the DPRINTF, load_psw statements into the for loop (just before
> > the break statement)?
> 
> We could move the lpsw. However, this made me notice another problem:
> We stop scanning subsequent iscs if we found an interrupt to inject...
> 
> This is not a problem for current virtio-ccw based Linux guests since
> they never use anything else than isc 3, but we'll probably want the
> following patch.
> 
> From 8b2f40e8eaac16b55a72ab1e36a4c5de0b016495 Mon Sep 17 00:00:00 2001
> From: Cornelia Huck <cornelia.huck@de.ibm.com>
> Date: Tue, 5 Feb 2013 10:14:49 +0100
> Subject: [PATCH] s390: Keep I/O interrupts enabled for all iscs.
> 
> do_io_interrupt() would stop scanning further iscs if it found
> an I/O interrupt it could inject. This might cause the pending
> interrupt indication for I/O interrupts to be reset although there
> might be queued I/O interrupts for subsequent iscs.
> 
> Fix this by reordering the logic: Inject the I/O interrupt immediately
> and continue searching all iscs for queued interrupts.
> 
> Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>

Any opinions on this? I have another fix touching this code and I'd
like to base that patch on top of this one.

> ---
>  target-s390x/helper.c | 40 +++++++++++++++++++++-------------------
>  1 file changed, 21 insertions(+), 19 deletions(-)
> 
> diff --git a/target-s390x/helper.c b/target-s390x/helper.c
> index 043feb2..9f9088b 100644
> --- a/target-s390x/helper.c
> +++ b/target-s390x/helper.c
> @@ -617,7 +617,6 @@ static void do_ext_interrupt(CPUS390XState *env)
>  
>  static void do_io_interrupt(CPUS390XState *env)
>  {
> -    uint64_t mask = 0, addr = 0;
>      LowCore *lowcore;
>      IOIntQueue *q;
>      uint8_t isc;
> @@ -642,36 +641,39 @@ static void do_io_interrupt(CPUS390XState *env)
>              disable = 0;
>              continue;
>          }
> -        found = 1;
> -        lowcore = cpu_map_lowcore(env);
> +        if (!found) {
> +            uint64_t mask, addr;
>  
> -        lowcore->subchannel_id = cpu_to_be16(q->id);
> -        lowcore->subchannel_nr = cpu_to_be16(q->nr);
> -        lowcore->io_int_parm = cpu_to_be32(q->parm);
> -        lowcore->io_int_word = cpu_to_be32(q->word);
> -        lowcore->io_old_psw.mask = cpu_to_be64(get_psw_mask(env));
> -        lowcore->io_old_psw.addr = cpu_to_be64(env->psw.addr);
> -        mask = be64_to_cpu(lowcore->io_new_psw.mask);
> -        addr = be64_to_cpu(lowcore->io_new_psw.addr);
> +            found = 1;
> +            lowcore = cpu_map_lowcore(env);
>  
> -        cpu_unmap_lowcore(lowcore);
> +            lowcore->subchannel_id = cpu_to_be16(q->id);
> +            lowcore->subchannel_nr = cpu_to_be16(q->nr);
> +            lowcore->io_int_parm = cpu_to_be32(q->parm);
> +            lowcore->io_int_word = cpu_to_be32(q->word);
> +            lowcore->io_old_psw.mask = cpu_to_be64(get_psw_mask(env));
> +            lowcore->io_old_psw.addr = cpu_to_be64(env->psw.addr);
> +            mask = be64_to_cpu(lowcore->io_new_psw.mask);
> +            addr = be64_to_cpu(lowcore->io_new_psw.addr);
>  
> -        env->io_index[isc]--;
> +            cpu_unmap_lowcore(lowcore);
> +
> +            env->io_index[isc]--;
> +
> +            DPRINTF("%s: %" PRIx64 " %" PRIx64 "\n", __func__,
> +                    env->psw.mask, env->psw.addr);
> +            load_psw(env, mask, addr);
> +        }
>          if (env->io_index[isc] >= 0) {
>              disable = 0;
>          }
> -        break;
> +        continue;
>      }
>  
>      if (disable) {
>          env->pending_int &= ~INTERRUPT_IO;
>      }
>  
> -    if (found) {
> -        DPRINTF("%s: %" PRIx64 " %" PRIx64 "\n", __func__,
> -                env->psw.mask, env->psw.addr);
> -        load_psw(env, mask, addr);
> -    }
>  }
>  
>  static void do_mchk_interrupt(CPUS390XState *env)

Patch

diff --git a/target-s390x/helper.c b/target-s390x/helper.c
index 043feb2..9f9088b 100644
--- a/target-s390x/helper.c
+++ b/target-s390x/helper.c
@@ -617,7 +617,6 @@  static void do_ext_interrupt(CPUS390XState *env)
 
 static void do_io_interrupt(CPUS390XState *env)
 {
-    uint64_t mask = 0, addr = 0;
     LowCore *lowcore;
     IOIntQueue *q;
     uint8_t isc;
@@ -642,36 +641,39 @@  static void do_io_interrupt(CPUS390XState *env)
             disable = 0;
             continue;
         }
-        found = 1;
-        lowcore = cpu_map_lowcore(env);
+        if (!found) {
+            uint64_t mask, addr;
 
-        lowcore->subchannel_id = cpu_to_be16(q->id);
-        lowcore->subchannel_nr = cpu_to_be16(q->nr);
-        lowcore->io_int_parm = cpu_to_be32(q->parm);
-        lowcore->io_int_word = cpu_to_be32(q->word);
-        lowcore->io_old_psw.mask = cpu_to_be64(get_psw_mask(env));
-        lowcore->io_old_psw.addr = cpu_to_be64(env->psw.addr);
-        mask = be64_to_cpu(lowcore->io_new_psw.mask);
-        addr = be64_to_cpu(lowcore->io_new_psw.addr);
+            found = 1;
+            lowcore = cpu_map_lowcore(env);
 
-        cpu_unmap_lowcore(lowcore);
+            lowcore->subchannel_id = cpu_to_be16(q->id);
+            lowcore->subchannel_nr = cpu_to_be16(q->nr);
+            lowcore->io_int_parm = cpu_to_be32(q->parm);
+            lowcore->io_int_word = cpu_to_be32(q->word);
+            lowcore->io_old_psw.mask = cpu_to_be64(get_psw_mask(env));
+            lowcore->io_old_psw.addr = cpu_to_be64(env->psw.addr);
+            mask = be64_to_cpu(lowcore->io_new_psw.mask);
+            addr = be64_to_cpu(lowcore->io_new_psw.addr);
 
-        env->io_index[isc]--;
+            cpu_unmap_lowcore(lowcore);
+
+            env->io_index[isc]--;
+
+            DPRINTF("%s: %" PRIx64 " %" PRIx64 "\n", __func__,
+                    env->psw.mask, env->psw.addr);
+            load_psw(env, mask, addr);
+        }
         if (env->io_index[isc] >= 0) {
             disable = 0;
         }
-        break;
+        continue;
     }
 
     if (disable) {
         env->pending_int &= ~INTERRUPT_IO;
     }
 
-    if (found) {
-        DPRINTF("%s: %" PRIx64 " %" PRIx64 "\n", __func__,
-                env->psw.mask, env->psw.addr);
-        load_psw(env, mask, addr);
-    }
 }
 
 static void do_mchk_interrupt(CPUS390XState *env)