| Message ID | 20250327004044.2014048-3-volodymyr_babchuk@epam.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Enable MC/DC support for GCOV | expand |
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@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. 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
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@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. > 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
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@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. > >> 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
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@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;
diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c index dd8d921f18..ae7224a145 100644 --- 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; for (irq = nr_irqs_gsi; irq < nr_irqs; irq++) {
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@epam.com> --- Attempt to declare "irq" as "unsigned int" didn't changed anything, so looks like compiler infers unitialized via some other reasoning... And it is interesting that this issue can be observed only with MC/DC enabled. --- xen/arch/x86/irq.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)