[PATCH v4 0/2] Enable MC/DC support for GCC/GCOV

[PATCH v4 0/2] Enable MC/DC support for GCC/GCOV

[Volodymyr Babchuk]

This series enables MC/DC for Xen when building with GCC.

Condition coverage, also known as MC/DC (modified condition/decision
coverage) is a coverage metric that tracks separate outcomes in
boolean expressions. This metric is used in critical software
components, so it natural to collect it for Xen.

First patch in the series ("xen: x86: irq: initialize irq desc in
create_irq()") deals with unexpected GCC issue, which is not exactly
related to the code coverage, as it can be reproduced just with -Og
optimization level. I have opened the bug here:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=119665

This series support only GCC, although Clang, starting with clang 18.
supports similar feature. But Clang 18 uses raw profiling format
version 10, while Xen supports only version 4, and there are quite
substantial changes in headers and structures, so adding new version
format is non-trivial. Also, Xen built for aarch64 with clang 19.1.17
and code coverage enabled, completely hangs up during boot, so there
is clearly more work required.

Another problem with clang-based MC/DC support is that it has multiple
issues which will prevent use it in a meaningfull way:

https://github.com/llvm/llvm-project/issues?q=is%3Aissue%20state%3Aopen%20MC%2FDC

But at least we can have MC/DC with GCC/GCOV.

Changes in v4:
 - one patch less, as "xen: gcov: add support for gcc 14" is merged already
 - reworked irq.c fix back to original state (but with a comment this time)
 - opened a GCC bug
 - make COV_FLAGS -> cov-flags-y convertion as part of the second patch

Changes in v3:
 - Check if gcc accepts -fcondition-coverage
 - Clarify why we use do { } loop
 - Add Jan's R-b tag for PATCH 1/3

Changes in v2:
 - Check for gcc 14, not gcc 14.1
 - Reworked irq.c patch



Volodymyr Babchuk (2):
  xen: x86: irq: initialize irq desc in create_irq()
  xen: debug: gcov: add condition coverage support

 xen/Kconfig        |  4 ++++
 xen/Kconfig.debug  |  9 +++++++++
 xen/Rules.mk       | 14 +++++++-------
 xen/arch/x86/irq.c |  2 +-
 4 files changed, 21 insertions(+), 8 deletions(-)

-- 
2.48.1

[PATCH v4 2/2] xen: debug: gcov: add condition coverage support

[Volodymyr Babchuk]

Condition coverage, also known as MC/DC (modified condition/decision
coverage) is a coverage metric that tracks separate outcomes in
boolean expressions.

This patch adds CONFIG_CONDITION_COVERAGE option to enable MC/DC for
GCC. Clang is not supported right now.

Also, use the opportunity to convert COV_FLAGS to cov_flags-y, which
reduces amount of ifeqs in Rules.mk. Otherwise this patch had to add
another nesting level with "ifeq ($(CONFIG_CONDITION_COVERAGE),y)".

Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>

---

Changes in v4:
 - Slight formatting fixes
 - COV_FLAGS  -> cov_flags-y

Changes in v3:
 - Introduced CC_HAS_MCDC that checks if compiler supports
   required feature

Changes in v2:
 - Move gcc version check from .c file to Rules.mk (I can't find
   an easy way to check GCC version at Kconfig level)
 - Check for gcc 14, not gcc 14.1
---
 xen/Kconfig       |  4 ++++
 xen/Kconfig.debug |  9 +++++++++
 xen/Rules.mk      | 14 +++++++-------
 3 files changed, 20 insertions(+), 7 deletions(-)

diff --git a/xen/Kconfig b/xen/Kconfig
index 2128f0ccfc..3a723db8ea 100644
--- a/xen/Kconfig
+++ b/xen/Kconfig
@@ -41,6 +41,10 @@ config CC_SPLIT_SECTIONS
 config CC_HAS_UBSAN
 	def_bool $(cc-option,-fsanitize=undefined)
 
+# Compiler supports -fcondition-coverage aka MC/DC
+config CC_HAS_MCDC
+	def_bool $(cc-option,-fcondition-coverage)
+
 # Set code alignment.
 #
 # Allow setting on a boolean basis, and then convert such selection to an
diff --git a/xen/Kconfig.debug b/xen/Kconfig.debug
index f7cc5ffaab..f89cbd823b 100644
--- a/xen/Kconfig.debug
+++ b/xen/Kconfig.debug
@@ -44,6 +44,15 @@ config COVERAGE
 
 	  If unsure, say N here.
 
+config CONDITION_COVERAGE
+	bool "Condition coverage support"
+	depends on COVERAGE && CC_HAS_MCDC
+	help
+	  Enable condition coverage support. Used for collecting MC/DC
+	  (Modified Condition/Decision Coverage) metrics.
+
+	  If unsure, say N here.
+
 config DEBUG_LOCK_PROFILE
 	bool "Lock Profiling"
 	select DEBUG_LOCKS
diff --git a/xen/Rules.mk b/xen/Rules.mk
index d759cccee3..e9e049368f 100644
--- a/xen/Rules.mk
+++ b/xen/Rules.mk
@@ -31,6 +31,7 @@ CFLAGS-y :=
 AFLAGS-y :=
 nocov-y :=
 noubsan-y :=
+cov-flags-y :=
 
 SPECIAL_DATA_SECTIONS := rodata $(foreach a,1 2 4 8 16, \
                                             $(foreach w,1 2 4, \
@@ -133,19 +134,18 @@ $(filter %.init.o,$(obj-y) $(obj-bin-y) $(extra-y)): CFLAGS-y += -DINIT_SECTIONS
 
 non-init-objects = $(filter-out %.init.o, $(obj-y) $(obj-bin-y) $(extra-y))
 
-ifeq ($(CONFIG_COVERAGE),y)
 ifeq ($(CONFIG_CC_IS_CLANG),y)
-    COV_FLAGS := -fprofile-instr-generate -fcoverage-mapping
+    cov-flags-$(CONFIG_COVERAGE) := -fprofile-instr-generate -fcoverage-mapping
 else
-    COV_FLAGS := -fprofile-arcs -ftest-coverage
+    cov-flags-$(CONFIG_COVERAGE) := -fprofile-arcs -ftest-coverage
+    cov-flags-$(CONFIG_CONDITION_COVERAGE) += -fcondition-coverage
 endif
 
-# Reset COV_FLAGS in cases where an objects has another one as prerequisite
+# Reset cov-flags-y in cases where an objects has another one as prerequisite
 $(nocov-y) $(filter %.init.o, $(obj-y) $(obj-bin-y) $(extra-y)): \
-    COV_FLAGS :=
+    cov-flags-y :=
 
-$(non-init-objects): _c_flags += $(COV_FLAGS)
-endif
+$(non-init-objects): _c_flags += $(cov-flags-y)
 
 ifeq ($(CONFIG_UBSAN),y)
 # Any -fno-sanitize= options need to come after any -fsanitize= options
-- 
2.48.1

Re: [PATCH v4 2/2] xen: debug: gcov: add condition coverage support

[Jan Beulich]

On 07.04.2025 18:46, Volodymyr Babchuk wrote:
> --- a/xen/Rules.mk
> +++ b/xen/Rules.mk
> @@ -31,6 +31,7 @@ CFLAGS-y :=
>  AFLAGS-y :=
>  nocov-y :=
>  noubsan-y :=
> +cov-flags-y :=

Personally I would have put this slightly higher up, at least ahead of the two
no*-y. Thinking of it only now (sorry), also maybe cov-cflags-y might be
slightly better a name?

> @@ -133,19 +134,18 @@ $(filter %.init.o,$(obj-y) $(obj-bin-y) $(extra-y)): CFLAGS-y += -DINIT_SECTIONS
>  
>  non-init-objects = $(filter-out %.init.o, $(obj-y) $(obj-bin-y) $(extra-y))
>  
> -ifeq ($(CONFIG_COVERAGE),y)
>  ifeq ($(CONFIG_CC_IS_CLANG),y)
> -    COV_FLAGS := -fprofile-instr-generate -fcoverage-mapping
> +    cov-flags-$(CONFIG_COVERAGE) := -fprofile-instr-generate -fcoverage-mapping
>  else
> -    COV_FLAGS := -fprofile-arcs -ftest-coverage
> +    cov-flags-$(CONFIG_COVERAGE) := -fprofile-arcs -ftest-coverage
> +    cov-flags-$(CONFIG_CONDITION_COVERAGE) += -fcondition-coverage

Why's this inside the remaining ifeq(,)? Surely there's at least a chance for
Clang to also support the option at some point?

Jan

Re: [PATCH v4 2/2] xen: debug: gcov: add condition coverage support

[Volodymyr Babchuk]

Hi Jan,

Jan Beulich <jbeulich@suse.com> writes:

> On 07.04.2025 18:46, Volodymyr Babchuk wrote:
>> --- a/xen/Rules.mk
>> +++ b/xen/Rules.mk
>> @@ -31,6 +31,7 @@ CFLAGS-y :=
>>  AFLAGS-y :=
>>  nocov-y :=
>>  noubsan-y :=
>> +cov-flags-y :=
>
> Personally I would have put this slightly higher up, at least ahead of the two
> no*-y. Thinking of it only now (sorry), also maybe cov-cflags-y might be
> slightly better a name?

Okay, I'll do this in the next version.

>
>> @@ -133,19 +134,18 @@ $(filter %.init.o,$(obj-y) $(obj-bin-y) $(extra-y)): CFLAGS-y += -DINIT_SECTIONS
>>  
>>  non-init-objects = $(filter-out %.init.o, $(obj-y) $(obj-bin-y) $(extra-y))
>>  
>> -ifeq ($(CONFIG_COVERAGE),y)
>>  ifeq ($(CONFIG_CC_IS_CLANG),y)
>> -    COV_FLAGS := -fprofile-instr-generate -fcoverage-mapping
>> +    cov-flags-$(CONFIG_COVERAGE) := -fprofile-instr-generate -fcoverage-mapping
>>  else
>> -    COV_FLAGS := -fprofile-arcs -ftest-coverage
>> +    cov-flags-$(CONFIG_COVERAGE) := -fprofile-arcs -ftest-coverage
>> +    cov-flags-$(CONFIG_CONDITION_COVERAGE) += -fcondition-coverage
>
> Why's this inside the remaining ifeq(,)? Surely there's at least a chance for
> Clang to also support the option at some point?

Yes, but Clang uses different option: -fcoverage-mcdc. I see no sense in
adding it right now, as Xen does not support version 10 of llvm
profiling format, in which they added MC/DC support.

-- 
WBR, Volodymyr

Re: [PATCH v4 2/2] xen: debug: gcov: add condition coverage support

[Jan Beulich]

On 08.04.2025 17:38, Volodymyr Babchuk wrote:
> Jan Beulich <jbeulich@suse.com> writes:
>> On 07.04.2025 18:46, Volodymyr Babchuk wrote:
>>> @@ -133,19 +134,18 @@ $(filter %.init.o,$(obj-y) $(obj-bin-y) $(extra-y)): CFLAGS-y += -DINIT_SECTIONS
>>>  
>>>  non-init-objects = $(filter-out %.init.o, $(obj-y) $(obj-bin-y) $(extra-y))
>>>  
>>> -ifeq ($(CONFIG_COVERAGE),y)
>>>  ifeq ($(CONFIG_CC_IS_CLANG),y)
>>> -    COV_FLAGS := -fprofile-instr-generate -fcoverage-mapping
>>> +    cov-flags-$(CONFIG_COVERAGE) := -fprofile-instr-generate -fcoverage-mapping
>>>  else
>>> -    COV_FLAGS := -fprofile-arcs -ftest-coverage
>>> +    cov-flags-$(CONFIG_COVERAGE) := -fprofile-arcs -ftest-coverage
>>> +    cov-flags-$(CONFIG_CONDITION_COVERAGE) += -fcondition-coverage
>>
>> Why's this inside the remaining ifeq(,)? Surely there's at least a chance for
>> Clang to also support the option at some point?
> 
> Yes, but Clang uses different option: -fcoverage-mcdc. I see no sense in
> adding it right now, as Xen does not support version 10 of llvm
> profiling format, in which they added MC/DC support.

Okay, but then can you amend "Clang is not supported right now" in the description
by another half sentence clarifying why that is?

Jan

[PATCH v4 1/2] xen: x86: irq: initialize irq desc in create_irq()

[Volodymyr Babchuk]

While building xen with GCC 14.2.1 with "-fcondition-coverage" option
or with "-Og", 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 due to a
bug [1] in GCC.

Initialize "desc" with NULL to make GCC happy.

[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=119665

Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>

---

Changes in v4:
 - Reverted back to initialing desc, per Jan's request
 - Added link to the corresponding GCC bug

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 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
index dd8d921f18..38ac0823d7 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 ; /* gcc14 -Og or -fcondition-coverage */
 
     for (irq = nr_irqs_gsi; irq < nr_irqs; irq++)
     {
-- 
2.48.1

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

[Jan Beulich]

On 07.04.2025 18:46, Volodymyr Babchuk wrote:
> While building xen with GCC 14.2.1 with "-fcondition-coverage" option
> or with "-Og", 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 due to a
> bug [1] in GCC.
> 
> Initialize "desc" with NULL to make GCC happy.
> 
> [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=119665
> 
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

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

[Jan Beulich]

On 07.04.2025 18:46, Volodymyr Babchuk wrote:
> While building xen with GCC 14.2.1 with "-fcondition-coverage" option
> or with "-Og", 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 due to a
> bug [1] in GCC.
> 
> Initialize "desc" with NULL to make GCC happy.
> 
> [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=119665
> 
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>

Just one other remark here: Personally I dislike the use of multiple or otherwise
excessive patch subject prefixes. xen/x86/irq: or even x86/irq: would have been
better here, imo.

Jan