[v3,2/3] xen: x86: irq: use do-while loop in create_irq()

Commit Message

Volodymyr Babchuk April 1, 2025, 1:17 a.m. UTC

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

The same behavior can be observed when building Xen with "-Og"
optimization level. Fix this by using "do { } while" loop instead of
"for" loop.

Signed-off-by: Volodymyr Babchuk <[e-mail]>

---

Changes in v3:
 - Correct code style ("do {")
 - Add comment describing why we need do { } while loop.
   I prefer to leave do {} while because Nicola Vetrini
   said that this approach might help with MISRA Rule 9.1
   without needing an explicit initializer.

Changes in v2:

 - Use do { } while loop instead of initializing desc with NULL
---
 xen/arch/x86/irq.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

Comments

Jan Beulich April 1, 2025, 6:33 a.m. UTC | #1

On 01.04.2025 03:17, Volodymyr Babchuk wrote:
> Changes in v3:
>  - Correct code style ("do {")
>  - Add comment describing why we need do { } while loop.
>    I prefer to leave do {} while because Nicola Vetrini
>    said that this approach might help with MISRA Rule 9.1
>    without needing an explicit initializer.

Just to mention it here as well - I still prefer the v1 form of the fix. Plus,
for my taste, ...

> --- a/xen/arch/x86/irq.c
> +++ b/xen/arch/x86/irq.c
> @@ -264,15 +264,24 @@ void __init clear_irq_vector(int irq)
>  
>  int create_irq(nodeid_t node, bool grant_access)
>  {
> -    int irq, ret;
> +    int ret;
> +    int irq = nr_irqs_gsi;
>      struct irq_desc *desc;
>  
> -    for (irq = nr_irqs_gsi; irq < nr_irqs; irq++)
> -    {
> +    if ( irq >= nr_irqs )
> +        return -ENOSPC;
> +
> +    /*
> +     * do { } while loop is used here to convince gcc14 that 'desc' is
> +     * really assigned. Otherwise with -Og or -fcondition-coverage it
> +     * may throw an false error stating that 'desc' may be used before
> +     * initialization.
> +     */
> +    do {
>          desc = irq_to_desc(irq);
>          if (cmpxchg(&desc->arch.used, IRQ_UNUSED, IRQ_RESERVED) == IRQ_UNUSED)
>             break;
> -    }
> +    } while ( ++irq < nr_irqs );

... the comment is now to verbose. See what I suggested as a comment for the
v1 change, as a very rough example.

Furthermore the question towards reporting the issue upstream still wasn't
answered. There really would want to be a reference to the bug report in the
description (or even the code comment) here.

Jan

Patch

diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
index dd8d921f18..2f288704b5 100644
--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -264,15 +264,24 @@  void __init clear_irq_vector(int irq)
 
 int create_irq(nodeid_t node, bool grant_access)
 {
-    int irq, ret;
+    int ret;
+    int irq = nr_irqs_gsi;
     struct irq_desc *desc;
 
-    for (irq = nr_irqs_gsi; irq < nr_irqs; irq++)
-    {
+    if ( irq >= nr_irqs )
+        return -ENOSPC;
+
+    /*
+     * do { } while loop is used here to convince gcc14 that 'desc' is
+     * really assigned. Otherwise with -Og or -fcondition-coverage it
+     * may throw an false error stating that 'desc' may be used before
+     * initialization.
+     */
+    do {
         desc = irq_to_desc(irq);
         if (cmpxchg(&desc->arch.used, IRQ_UNUSED, IRQ_RESERVED) == IRQ_UNUSED)
            break;
-    }
+    } while ( ++irq < nr_irqs );
 
     if (irq >= nr_irqs)
          return -ENOSPC;