[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] s390x: silence warning from GCC on uninitialize
From: |
Cornelia Huck |
Subject: |
Re: [Qemu-devel] [PATCH] s390x: silence warning from GCC on uninitialized values |
Date: |
Thu, 7 Feb 2013 09:52:37 +0100 |
On Tue, 5 Feb 2013 10:27:00 +0100
Cornelia Huck <address@hidden> wrote:
> On Mon, 04 Feb 2013 22:57:43 +0100
> Stefan Weil <address@hidden> wrote:
>
> > Am 04.02.2013 22:23, schrieb Anthony Liguori:
> > > As best I can tell, this is a false positive.
> > >
> > > address@hidden 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 <address@hidden>
> 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 <address@hidden>
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)