[v1] intc/i8259: avoid (false positive) gcc warning

[PATCH] intc/i8259: avoid (false positive) gcc warning

Posted by Christian Borntraeger 4 years, 8 months ago

some copiler 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;
       |         ~~~~^~~~~~~~~~

Let us initialize irq2 to -1 to avoid this warning as the most simple
solution.

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 hw/intc/i8259.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/intc/i8259.c b/hw/intc/i8259.c
index 344fd04db14d..ade6fb726faf 100644
--- a/hw/intc/i8259.c
+++ b/hw/intc/i8259.c
@@ -176,7 +176,7 @@ 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, irq2 = -1, intno;
 
     irq = pic_get_irq(s);
     if (irq >= 0) {
-- 
2.30.2

Re: [PATCH] intc/i8259: avoid (false positive) gcc warning

Posted by Paolo Bonzini 4 years, 8 months ago

On 18/03/21 16:47, Christian Borntraeger wrote:
> some copiler 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;
>         |         ~~~~^~~~~~~~~~
> 
> Let us initialize irq2 to -1 to avoid this warning as the most simple
> solution.

What about:

diff --git a/hw/intc/i8259.c b/hw/intc/i8259.c
index 344fd04db1..bf28c179de 100644
--- a/hw/intc/i8259.c
+++ b/hw/intc/i8259.c
@@ -189,20 +189,18 @@ int pic_read_irq(DeviceState *d)
                  irq2 = 7;
              }
              intno = slave_pic->irq_base + irq2;
+            irq = irq2 + 8;
+            pic_intack(s, 2);
          } 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,


?

Paolo

Re: [PATCH] intc/i8259: avoid (false positive) gcc warning

Posted by Christian Borntraeger 4 years, 8 months ago


On 18.03.21 17:03, Paolo Bonzini wrote:
> On 18/03/21 16:47, Christian Borntraeger wrote:
>> some copiler 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;
>>         |         ~~~~^~~~~~~~~~
>>
>> Let us initialize irq2 to -1 to avoid this warning as the most simple
>> solution.
> 
> What about:

This also works, but if you want to go down that path then it would be good if you
could do this patch as I do not have the testing infrastructure to do proper x86
changes.
> 
> diff --git a/hw/intc/i8259.c b/hw/intc/i8259.c
> index 344fd04db1..bf28c179de 100644
> --- a/hw/intc/i8259.c
> +++ b/hw/intc/i8259.c
> @@ -189,20 +189,18 @@ int pic_read_irq(DeviceState *d)
>                   irq2 = 7;
>               }
>               intno = slave_pic->irq_base + irq2;
> +            irq = irq2 + 8;
> +            pic_intack(s, 2);
>           } 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,
> 
> 
> ?
> 
> Paolo
>

Re: [PATCH] intc/i8259: avoid (false positive) gcc warning

Posted by Philippe Mathieu-Daudé 4 years, 8 months ago

On 3/18/21 5:11 PM, Christian Borntraeger wrote:
> On 18.03.21 17:03, Paolo Bonzini wrote:
>> On 18/03/21 16:47, Christian Borntraeger wrote:
>>> some copiler 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;
>>>         |         ~~~~^~~~~~~~~~
>>>
>>> Let us initialize irq2 to -1 to avoid this warning as the most simple
>>> solution.
>>
>> What about:
> 
> This also works, but if you want to go down that path then it would be
> good if you
> could do this patch as I do not have the testing infrastructure to do
> proper x86
> changes.
>>
>> diff --git a/hw/intc/i8259.c b/hw/intc/i8259.c
>> index 344fd04db1..bf28c179de 100644
>> --- a/hw/intc/i8259.c
>> +++ b/hw/intc/i8259.c
>> @@ -189,20 +189,18 @@ int pic_read_irq(DeviceState *d)
>>                   irq2 = 7;
>>               }
>>               intno = slave_pic->irq_base + irq2;
>> +            irq = irq2 + 8;
>> +            pic_intack(s, 2);
>>           } 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;
>> -    }
>> -

This looks like the patch I just sent :)

Re: [PATCH] intc/i8259: avoid (false positive) gcc warning

Posted by BALATON Zoltan 4 years, 8 months ago

On Thu, 18 Mar 2021, Philippe Mathieu-Daudé wrote:
> On 3/18/21 5:11 PM, Christian Borntraeger wrote:
>> On 18.03.21 17:03, Paolo Bonzini wrote:
>>> On 18/03/21 16:47, Christian Borntraeger wrote:
>>>> some copiler 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;
>>>>         |         ~~~~^~~~~~~~~~
>>>>
>>>> Let us initialize irq2 to -1 to avoid this warning as the most simple
>>>> solution.
>>>
>>> What about:
>>
>> This also works, but if you want to go down that path then it would be
>> good if you
>> could do this patch as I do not have the testing infrastructure to do
>> proper x86
>> changes.
>>>
>>> diff --git a/hw/intc/i8259.c b/hw/intc/i8259.c
>>> index 344fd04db1..bf28c179de 100644
>>> --- a/hw/intc/i8259.c
>>> +++ b/hw/intc/i8259.c
>>> @@ -189,20 +189,18 @@ int pic_read_irq(DeviceState *d)
>>>                   irq2 = 7;
>>>               }
>>>               intno = slave_pic->irq_base + irq2;
>>> +            irq = irq2 + 8;
>>> +            pic_intack(s, 2);
>>>           } 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;
>>> -    }
>>> -
>
> This looks like the patch I just sent :)

Except this is simpler and more straightforward. I like this better FWIW.

Regards,
BALATON Zoltan