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>
---
Since v3:
- Declare irq2 really local (Richard)
Since v2:
- Remove pic_intack() call (Zoltan)
---
 hw/intc/i8259.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/hw/intc/i8259.c b/hw/intc/i8259.c
index 344fd04db14..d67c4710f15 100644
--- a/hw/intc/i8259.c
+++ b/hw/intc/i8259.c
@@ -176,12 +176,13 @@ 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) {
         if (irq == 2) {
-            irq2 = pic_get_irq(slave_pic);
+            int irq2 = pic_get_irq(slave_pic);
+
             if (irq2 >= 0) {
                 pic_intack(slave_pic, irq2);
             } else {
@@ -189,20 +190,18 @@ 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);
     } else {
         /* spurious IRQ on host controller */
         irq = 7;
         intno = s->irq_base + irq;
     }
 
-    if (irq == 2) {
-        irq = irq2 + 8;
-    }
-
 #ifdef DEBUG_IRQ_LATENCY
     printf("IRQ%d latency=%0.3fus\n",
            irq,
-- 
2.26.2

On 3/19/21 3:23 PM, 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>
> ---
> Since v3:
> - Declare irq2 really local (Richard)

I forgot to keep:
Tested-by: Christian Borntraeger <borntraeger@de.ibm.com>

On Fri, Mar 19, 2021 at 03:23:08PM +0100, 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>

Looks ok.

Acked-by: Michael S. Tsirkin <mst@redhat.com>

Paolo, you merging this?

> ---
> Since v3:
> - Declare irq2 really local (Richard)
> Since v2:
> - Remove pic_intack() call (Zoltan)
> ---
>  hw/intc/i8259.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/intc/i8259.c b/hw/intc/i8259.c
> index 344fd04db14..d67c4710f15 100644
> --- a/hw/intc/i8259.c
> +++ b/hw/intc/i8259.c
> @@ -176,12 +176,13 @@ 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) {
>          if (irq == 2) {
> -            irq2 = pic_get_irq(slave_pic);
> +            int irq2 = pic_get_irq(slave_pic);
> +
>              if (irq2 >= 0) {
>                  pic_intack(slave_pic, irq2);
>              } else {
> @@ -189,20 +190,18 @@ 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);
>      } else {
>          /* spurious IRQ on host controller */
>          irq = 7;
>          intno = s->irq_base + irq;
>      }
>  
> -    if (irq == 2) {
> -        irq = irq2 + 8;
> -    }
> -
>  #ifdef DEBUG_IRQ_LATENCY
>      printf("IRQ%d latency=%0.3fus\n",
>             irq,
> -- 
> 2.26.2