Re: [PATCH for 6.0 v2] hw/intc/i8259: Refactor pic_read_irq() to avoid uninitialized variable

[BALATON Zoltan <balaton@eik.bme.hu>]

[-- Attachment #1: Type: text/plain, Size: 2132 bytes --]

On Thu, 18 Mar 2021, Philippe Mathieu-Daudé wrote:
> Some compiler versions are smart enough to detect a potentially
> uninitialized variable, but are not smart enough to detect that this
> cannot happen due to the code flow:
>
> ../hw/intc/i8259.c: In function ‘pic_read_irq’:
> ../hw/intc/i8259.c:203:13: error: ‘irq2’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
>   203 |         irq = irq2 + 8;
>       |         ~~~~^~~~~~~~~~
>
> Restrict irq2 variable use to the inner statement.
>
> Fixes: 78ef2b6989f ("i8259: Reorder intack in pic_read_irq")
> Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> Supersedes: <20210318154738.27094-1-borntraeger@de.ibm.com>
> ---
> hw/intc/i8259.c | 11 ++++++-----
> 1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/hw/intc/i8259.c b/hw/intc/i8259.c
> index 344fd04db14..52c039c6c03 100644
> --- a/hw/intc/i8259.c
> +++ b/hw/intc/i8259.c
> @@ -176,10 +176,12 @@ static void pic_intack(PICCommonState *s, int irq)
> int pic_read_irq(DeviceState *d)
> {
>     PICCommonState *s = PIC_COMMON(d);
> -    int irq, irq2, intno;
> +    int irq, intno;
>
>     irq = pic_get_irq(s);
>     if (irq >= 0) {
> +        int irq2;
> +
>         if (irq == 2) {
>             irq2 = pic_get_irq(slave_pic);
>             if (irq2 >= 0) {
> @@ -189,8 +191,11 @@ int pic_read_irq(DeviceState *d)
>                 irq2 = 7;
>             }
>             intno = slave_pic->irq_base + irq2;
> +            pic_intack(s, irq);
> +            irq = irq2 + 8;
>         } else {
>             intno = s->irq_base + irq;
> +            pic_intack(s, irq);
>         }
>         pic_intack(s, irq);

Do you still need this pic_intack() here or did you intend to move it in 
the if above?

Regards,
BALATON Zoltan

>     } else {
> @@ -199,10 +204,6 @@ int pic_read_irq(DeviceState *d)
>         intno = s->irq_base + irq;
>     }
>
> -    if (irq == 2) {
> -        irq = irq2 + 8;
> -    }
> -
> #ifdef DEBUG_IRQ_LATENCY
>     printf("IRQ%d latency=%0.3fus\n",
>            irq,
>