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

[Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>]

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.
> > Irrespective of that I think such a seemingly unnecessary initializer 
> > wants
> > to come with a justifying comment, e.g.
> >
> >     struct irq_desc *desc = NULL /* gcc14 with -fcondition-coverage 
> > */;
> > here.
> >
> > Finally, did you report this to upstream gcc? It's probably too late 
> > to
> > fix in gcc15 (if still present), but it would be nice to have it fixed 
> > in
> > later versions (maybe including a late 14.x).
> >
> > Jan
--
Nicola Vetrini, B.Sc.
Software Engineer
BUGSENG (https://bugseng.com)
LinkedIn: https://www.linkedin.com/in/nicola-vetrini-a42471253