Re: [PATCH v1 2/3] xen: x86: irq: initialize irq desc in create_irq()

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

On 3/27/25 05:00, Nicola Vetrini wrote:
> On 2025-03-27 09:37, Nicola Vetrini wrote:
>> On 2025-03-27 09:03, Jan Beulich wrote:
>>> On 27.03.2025 01:40, Volodymyr Babchuk wrote:
>>>> While building xen with GCC 14.2.1 with "-fcondition-coverage" option,
>>>> the compiler produces a false positive warning:
>>>>
>>>>   arch/x86/irq.c: In function ‘create_irq’:
>>>>   arch/x86/irq.c:281:11: error: ‘desc’ may be used uninitialized 
>>>> [-Werror=maybe-uninitialized]
>>>>     281 |     ret = init_one_irq_desc(desc);
>>>>         |           ^~~~~~~~~~~~~~~~~~~~~~~
>>>>   arch/x86/irq.c:269:22: note: ‘desc’ was declared here
>>>>     269 |     struct irq_desc *desc;
>>>>         |                      ^~~~
>>>>   cc1: all warnings being treated as errors
>>>>   make[2]: *** [Rules.mk:252: arch/x86/irq.o] Error 1
>>>>
>>>> While we have signed/unsigned comparison both in "for" loop and in
>>>> "if" statement, this still can't lead to use of uninitialized "desc",
>>>> as either loop will be executed at least once, or the function will
>>>> return early. So this is a clearly false positive warning. Anyways,
>>>> initialize "desc" with NULL to make GCC happy.
>>>>
>>>> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@xxxxxxxx>
>>>
>>> Hmm, this puts us in an interesting conflict, I think. Misra, aiui, will ...
>>>
>>>> --- a/xen/arch/x86/irq.c
>>>> +++ b/xen/arch/x86/irq.c
>>>> @@ -265,7 +265,7 @@ void __init clear_irq_vector(int irq)
>>>>  int create_irq(nodeid_t node, bool grant_access)
>>>>  {
>>>>      int irq, ret;
>>>> -    struct irq_desc *desc;
>>>> +    struct irq_desc *desc = NULL;
>>>
>>> ... consider such an assignment useless (and hence potentially confusing)
>>> code. I'm curious what BugsEng folks are going to say here.
>>>
> 
> Just to mention it: having a "do { } while" loop instead of a for (just out 
> of context) probably avoid tripping gcc's false positive and also help with 
> MISRA Rule 9.1 without needing an explicit initializer.
> 
>>
>> It is quite odd to see this only in coverage builds, but the side effects of 
>> coverage options might trigger some of gcc's internal analyzer thresholds. 
>> Anyway, since there are no concerns about dead code (see 
>> https://gitlab.com/xen-project/xen/-/blob/staging/docs/misra/deviations.rst: 
>> R2.2, "There shall be no dead code", is globally deviated) and that this 
>> might actually be beneficial to remove some caution reports for R9.1 ("The 
>> value of an object with automatic storage duration shall not be read before 
>> it has been set") I think the overall effect is positive.

I tried running an "-Og default for debug builds" change through CI, and
I ran into *almost* the same error with -Og and certain version(s) of
GCC:

arch/x86/irq.c: In function 'create_irq':
arch/x86/irq.c:298:25: error: 'desc' may be used uninitialized 
[-Werror=maybe-uninitialized]
  298 |         desc->arch.used = IRQ_UNUSED;
arch/x86/irq.c:268:22: note: 'desc' was declared here
  268 |     struct irq_desc *desc;
      |                      ^~~~

The do { } while loop Nicola suggested indeed fixes it in the case of
-Og:

diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
index dd8d921f18f6..812f9eb91453 100644
--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -267,12 +267,18 @@ int create_irq(nodeid_t node, bool grant_access)
     int irq, ret;
     struct irq_desc *desc;
 
-    for (irq = nr_irqs_gsi; irq < nr_irqs; irq++)
+    irq = nr_irqs_gsi;
+
+    if ( irq >= nr_irqs )
+        return -1;
+
+    do
     {
         desc = irq_to_desc(irq);
         if (cmpxchg(&desc->arch.used, IRQ_UNUSED, IRQ_RESERVED) == IRQ_UNUSED)
            break;
-    }
+        irq++;
+    } while ( irq < nr_irqs );
 
     if (irq >= nr_irqs)
          return -ENOSPC;