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

[Philippe Mathieu-Daudé]

On 3/18/21 5:24 PM, BALATON Zoltan wrote:
> 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?

I forgot to remove it indeed, thanks!

>>     } 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,
>>