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_babc...@epam.com>
>>>
>>> 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;
