[PATCH v3 0/3] Enable MC/DC support for GCC/GCOV

[PATCH v3 0/3] 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.

Second patch in the series ("xen: x86: irq: use do-while loop in
create_irq()") deals with unexpected GCC issue. As was discussed in
the previous version, I changed "for" loop to "do {} while".

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 GCOV.

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 (3):
  xen: gcov: add support for gcc 14
  xen: x86: irq: use do-while loop in create_irq()
  xen: debug: gcov: add condition coverage support

 xen/Kconfig                     |  5 +++++
 xen/Kconfig.debug               |  9 +++++++++
 xen/Rules.mk                    |  3 +++
 xen/arch/x86/irq.c              | 17 +++++++++++++----
 xen/common/coverage/gcc_4_7.c   |  4 +++-
 xen/common/coverage/gcov_base.c |  5 +++++
 6 files changed, 38 insertions(+), 5 deletions(-)

-- 
2.48.1

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

[Volodymyr Babchuk]

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

---

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(-)

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;
-- 
2.48.1

[PATCH v3 1/3] xen: gcov: add support for gcc 14

[Volodymyr Babchuk]

gcc 14 (with patch "Add condition coverage (MC/DC)") introduced 9th
gcov counter. Also this version can call new merge function
__gcov_merge_ior(), so we need a new stub for it.

Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>

---

Changes in v3:
 - Added Jan's R-b tag

Changes is v2:
 - Check for gcc 14, not gcc 14.1
---
 xen/common/coverage/gcc_4_7.c   | 4 +++-
 xen/common/coverage/gcov_base.c | 5 +++++
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/xen/common/coverage/gcc_4_7.c b/xen/common/coverage/gcc_4_7.c
index 1c20e35ee5..f4c1802303 100644
--- a/xen/common/coverage/gcc_4_7.c
+++ b/xen/common/coverage/gcc_4_7.c
@@ -28,8 +28,10 @@
 #define GCOV_COUNTERS 10
 #elif GCC_VERSION < 100000
 #define GCOV_COUNTERS 9
-#else
+#elif GCC_VERSION < 140000
 #define GCOV_COUNTERS 8
+#else
+#define GCOV_COUNTERS 9
 #endif
 
 #define GCOV_TAG_FUNCTION_LENGTH        3
diff --git a/xen/common/coverage/gcov_base.c b/xen/common/coverage/gcov_base.c
index d0c6d0a3f9..742034e039 100644
--- a/xen/common/coverage/gcov_base.c
+++ b/xen/common/coverage/gcov_base.c
@@ -56,6 +56,11 @@ void __gcov_merge_delta(gcov_type *counters, unsigned int n_counters)
     /* Unused. */
 }
 
+void __gcov_merge_ior(gcov_type *counters, unsigned int n_counters)
+{
+    /* Unused. */
+}
+
 /*
  * Local variables:
  * mode: C
-- 
2.48.1

[PATCH v3 3/3] 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.

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

---

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       | 5 +++++
 xen/Kconfig.debug | 9 +++++++++
 xen/Rules.mk      | 3 +++
 3 files changed, 17 insertions(+)

diff --git a/xen/Kconfig b/xen/Kconfig
index 2128f0ccfc..2bdebfc808 100644
--- a/xen/Kconfig
+++ b/xen/Kconfig
@@ -41,6 +41,11 @@ 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..0a2933cffa 100644
--- a/xen/Rules.mk
+++ b/xen/Rules.mk
@@ -138,6 +138,9 @@ ifeq ($(CONFIG_CC_IS_CLANG),y)
     COV_FLAGS := -fprofile-instr-generate -fcoverage-mapping
 else
     COV_FLAGS := -fprofile-arcs -ftest-coverage
+ifeq ($(CONFIG_CONDITION_COVERAGE),y)
+    COV_FLAGS += -fcondition-coverage
+endif
 endif
 
 # Reset COV_FLAGS in cases where an objects has another one as prerequisite
-- 
2.48.1

Re: [PATCH v3 1/3] xen: gcov: add support for gcc 14

[Jan Beulich]

On 01.04.2025 03:17, Volodymyr Babchuk wrote:
> gcc 14 (with patch "Add condition coverage (MC/DC)") introduced 9th
> gcov counter. Also this version can call new merge function
> __gcov_merge_ior(), so we need a new stub for it.
> 
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Any reason this got re-posted, when it was committed already?

Jan

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

[Jan Beulich]

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

Re: [PATCH v3 3/3] xen: debug: gcov: add condition coverage support

[Jan Beulich]

On 01.04.2025 03:17, Volodymyr Babchuk wrote:
> --- a/xen/Kconfig
> +++ b/xen/Kconfig
> @@ -41,6 +41,11 @@ 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)
> +
> +

Nit: No double blank lines please.

Also, just to clarify - until the use of Kconfig (alone) for things like
this is properly resolved one way or another, I'm not going to approve
such changes (but I'm also not going to veto them). My proposal [1] is
still pending with no resolution, nor any counter-proposals.

> --- a/xen/Rules.mk
> +++ b/xen/Rules.mk
> @@ -138,6 +138,9 @@ ifeq ($(CONFIG_CC_IS_CLANG),y)
>      COV_FLAGS := -fprofile-instr-generate -fcoverage-mapping
>  else
>      COV_FLAGS := -fprofile-arcs -ftest-coverage
> +ifeq ($(CONFIG_CONDITION_COVERAGE),y)
> +    COV_FLAGS += -fcondition-coverage
> +endif
>  endif

Personally I find ifeq() uses like this unhelpful, and would prefer

COV_FLAGS-$(CONFIG_CONDITION_COVERAGE) += -fcondition-coverage
together with an eventual

COV_FLAGS += $(COV_FLAGS-y)

(if we don't already have one).

Jan

[1] https://lists.xen.org/archives/html/xen-devel/2022-09/msg01793.html

Re: [PATCH v3 3/3] xen: debug: gcov: add condition coverage support

[Anthony PERARD]

On Thu, Apr 03, 2025 at 09:30:21AM +0200, Jan Beulich wrote:
> On 01.04.2025 03:17, Volodymyr Babchuk wrote:
> > --- a/xen/Rules.mk
> > +++ b/xen/Rules.mk
> > @@ -138,6 +138,9 @@ ifeq ($(CONFIG_CC_IS_CLANG),y)
> >      COV_FLAGS := -fprofile-instr-generate -fcoverage-mapping
> >  else
> >      COV_FLAGS := -fprofile-arcs -ftest-coverage
> > +ifeq ($(CONFIG_CONDITION_COVERAGE),y)
> > +    COV_FLAGS += -fcondition-coverage
> > +endif
> >  endif
> 
> Personally I find ifeq() uses like this unhelpful, and would prefer
> 
> COV_FLAGS-$(CONFIG_CONDITION_COVERAGE) += -fcondition-coverage
> together with an eventual
> 
> COV_FLAGS += $(COV_FLAGS-y)
> 
> (if we don't already have one).

Not we don't. About renaming $(COV_FLAGS) to $(cov-flags-y) instead? It
is simpler as we stay with a single variable for coverage flags.

-- 

Anthony Perard | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

Re: [PATCH v3 3/3] xen: debug: gcov: add condition coverage support

[Jan Beulich]

On 03.04.2025 15:15, Anthony PERARD wrote:
> On Thu, Apr 03, 2025 at 09:30:21AM +0200, Jan Beulich wrote:
>> On 01.04.2025 03:17, Volodymyr Babchuk wrote:
>>> --- a/xen/Rules.mk
>>> +++ b/xen/Rules.mk
>>> @@ -138,6 +138,9 @@ ifeq ($(CONFIG_CC_IS_CLANG),y)
>>>      COV_FLAGS := -fprofile-instr-generate -fcoverage-mapping
>>>  else
>>>      COV_FLAGS := -fprofile-arcs -ftest-coverage
>>> +ifeq ($(CONFIG_CONDITION_COVERAGE),y)
>>> +    COV_FLAGS += -fcondition-coverage
>>> +endif
>>>  endif
>>
>> Personally I find ifeq() uses like this unhelpful, and would prefer
>>
>> COV_FLAGS-$(CONFIG_CONDITION_COVERAGE) += -fcondition-coverage
>> together with an eventual
>>
>> COV_FLAGS += $(COV_FLAGS-y)
>>
>> (if we don't already have one).
> 
> Not we don't. About renaming $(COV_FLAGS) to $(cov-flags-y) instead? It
> is simpler as we stay with a single variable for coverage flags.

I'd be all for doing that.

Jan

Re: [PATCH v3 3/3] xen: debug: gcov: add condition coverage support

[Volodymyr Babchuk]

Hi Jan,

Jan Beulich <jbeulich@suse.com> writes:

> On 01.04.2025 03:17, Volodymyr Babchuk wrote:
>> --- a/xen/Kconfig
>> +++ b/xen/Kconfig
>> @@ -41,6 +41,11 @@ 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)
>> +
>> +
>
> Nit: No double blank lines please.
>
> Also, just to clarify - until the use of Kconfig (alone) for things like
> this is properly resolved one way or another, I'm not going to approve
> such changes (but I'm also not going to veto them). My proposal [1] is
> still pending with no resolution, nor any counter-proposals.

I checked your proposal, but I am not sure how it maps for this
particular use case. In your example

> config XEN_SHSTK
>        bool "Supervisor Shadow Stacks"
>        default HAS_AS_CET_SS

The default value will be "y" which is desired, but in case
of CONDITION_COVERAGE, the default value should be "n". Are you
suggesting to put

ifeq ($(CONFIG_CONDITION_COVERAGE)x$(CONFIG_CC_HAS_MCDC), yx)
   $(warning Your compiler does not support condition coverage)
endif

somewhere in Rules.mk ?


>> --- a/xen/Rules.mk
>> +++ b/xen/Rules.mk
>> @@ -138,6 +138,9 @@ ifeq ($(CONFIG_CC_IS_CLANG),y)
>>      COV_FLAGS := -fprofile-instr-generate -fcoverage-mapping
>>  else
>>      COV_FLAGS := -fprofile-arcs -ftest-coverage
>> +ifeq ($(CONFIG_CONDITION_COVERAGE),y)
>> +    COV_FLAGS += -fcondition-coverage
>> +endif
>>  endif
>
> Personally I find ifeq() uses like this unhelpful, and would prefer
>
> COV_FLAGS-$(CONFIG_CONDITION_COVERAGE) += -fcondition-coverage
> together with an eventual
>
> COV_FLAGS += $(COV_FLAGS-y)
>
> (if we don't already have one).

I did in this way:

--- a/xen/Rules.mk
+++ b/xen/Rules.mk
@@ -133,18 +133,19 @@ $(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)
+$(non-init-objects): _c_flags += $(cov-flags-y)
 endif
 

I hope you don't mind having both changes (COV_FLAGS -> cov_flags-y and
introduction of CONFIG_CONDITION_COVERAGE) in the same patch. With
correct commit message, of course.

-- 
WBR, Volodymyr

Re: [PATCH v3 3/3] xen: debug: gcov: add condition coverage support

[Jan Beulich]

On 05.04.2025 05:30, Volodymyr Babchuk wrote:
> Jan Beulich <jbeulich@suse.com> writes:
> 
>> On 01.04.2025 03:17, Volodymyr Babchuk wrote:
>>> --- a/xen/Kconfig
>>> +++ b/xen/Kconfig
>>> @@ -41,6 +41,11 @@ 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)
>>> +
>>> +
>>
>> Nit: No double blank lines please.
>>
>> Also, just to clarify - until the use of Kconfig (alone) for things like
>> this is properly resolved one way or another, I'm not going to approve
>> such changes (but I'm also not going to veto them). My proposal [1] is
>> still pending with no resolution, nor any counter-proposals.
> 
> I checked your proposal, but I am not sure how it maps for this
> particular use case. In your example
> 
>> config XEN_SHSTK
>>        bool "Supervisor Shadow Stacks"
>>        default HAS_AS_CET_SS
> 
> The default value will be "y" which is desired, but in case
> of CONDITION_COVERAGE, the default value should be "n". Are you
> suggesting to put
> 
> ifeq ($(CONFIG_CONDITION_COVERAGE)x$(CONFIG_CC_HAS_MCDC), yx)
>    $(warning Your compiler does not support condition coverage)
> endif
> 
> somewhere in Rules.mk ?

Perhaps. Ideally abstracted by a suitable, easy to use construct.

FTAOD - if you meant to include something like this in the next version of
the patch, you'll probably face resistance by Andrew (and/or maybe others).
We really need to decide on what model to use. I simply got tired of
reminding people that this discussion needs to happen (without pre-
determined outcome), for the matter to then be settled, and for the mix of
approaches presently taken to then be straightened.

>>> --- a/xen/Rules.mk
>>> +++ b/xen/Rules.mk
>>> @@ -138,6 +138,9 @@ ifeq ($(CONFIG_CC_IS_CLANG),y)
>>>      COV_FLAGS := -fprofile-instr-generate -fcoverage-mapping
>>>  else
>>>      COV_FLAGS := -fprofile-arcs -ftest-coverage
>>> +ifeq ($(CONFIG_CONDITION_COVERAGE),y)
>>> +    COV_FLAGS += -fcondition-coverage
>>> +endif
>>>  endif
>>
>> Personally I find ifeq() uses like this unhelpful, and would prefer
>>
>> COV_FLAGS-$(CONFIG_CONDITION_COVERAGE) += -fcondition-coverage
>> together with an eventual
>>
>> COV_FLAGS += $(COV_FLAGS-y)
>>
>> (if we don't already have one).
> 
> I did in this way:
> 
> --- a/xen/Rules.mk
> +++ b/xen/Rules.mk
> @@ -133,18 +133,19 @@ $(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)
> +$(non-init-objects): _c_flags += $(cov-flags-y)
>  endif
>  
> 
> I hope you don't mind having both changes (COV_FLAGS -> cov_flags-y and
> introduction of CONFIG_CONDITION_COVERAGE) in the same patch. With
> correct commit message, of course.

If that doesn't entail too many changes elsewhere, it's probably going to be
okay.

Jan

Re: [PATCH v3 3/3] xen: debug: gcov: add condition coverage support

[Anthony PERARD]

On Sat, Apr 05, 2025 at 03:30:49AM +0000, Volodymyr Babchuk wrote:
> --- a/xen/Rules.mk
> +++ b/xen/Rules.mk
> @@ -133,18 +133,19 @@ $(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)

This removes an "ifeq ()", so you probably need to remove and "endif"
somewhere else, which doesn't appear in this snippet.

>  ifeq ($(CONFIG_CC_IS_CLANG),y)
> -    COV_FLAGS := -fprofile-instr-generate -fcoverage-mapping
> +    cov-flags-$(CONFIG_COVERAGE) := -fprofile-instr-generate -fcoverage-mapping

If you do this assignment like that, it would be better to make sure
$(cov-flags-y) is initialised properly, that is have a:

  cov-flags-y :=

before the first conditional assignment, then have all conditional
assignment be +=.

>  else
> -    COV_FLAGS := -fprofile-arcs -ftest-coverage
> +    cov-flags-$(CONFIG_COVERAGE) := -fprofile-arcs -ftest-coverage
> +    cov-flags-$(CONFIG_CONDITION_COVERAGE) += -fcondition-coverage

What happen if CONFIG_CONDITION_COVERAGE=y but CONFIG_COVERAGE=n ?

>  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)
> +$(non-init-objects): _c_flags += $(cov-flags-y)
>  endif

Cheers,

-- 

Anthony Perard | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

Re: [PATCH v3 3/3] xen: debug: gcov: add condition coverage support

[Volodymyr Babchuk]

Hi Anthony,

"Anthony PERARD" <anthony.perard@vates.tech> writes:

> On Sat, Apr 05, 2025 at 03:30:49AM +0000, Volodymyr Babchuk wrote:
>> --- a/xen/Rules.mk
>> +++ b/xen/Rules.mk
>> @@ -133,18 +133,19 @@ $(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)
>
> This removes an "ifeq ()", so you probably need to remove and "endif"
> somewhere else, which doesn't appear in this snippet.

Yes, I'm sorry, it just didn't got into the snippet. I wanted to discuss
approach only, so this is not the final version.

>>  ifeq ($(CONFIG_CC_IS_CLANG),y)
>> -    COV_FLAGS := -fprofile-instr-generate -fcoverage-mapping
>> +    cov-flags-$(CONFIG_COVERAGE) := -fprofile-instr-generate -fcoverage-mapping
>
> If you do this assignment like that, it would be better to make sure
> $(cov-flags-y) is initialised properly, that is have a:
>
>   cov-flags-y :=
>
> before the first conditional assignment, then have all conditional
> assignment be +=.

Sure.

>
>>  else
>> -    COV_FLAGS := -fprofile-arcs -ftest-coverage
>> +    cov-flags-$(CONFIG_COVERAGE) := -fprofile-arcs -ftest-coverage
>> +    cov-flags-$(CONFIG_CONDITION_COVERAGE) += -fcondition-coverage
>
> What happen if CONFIG_CONDITION_COVERAGE=y but CONFIG_COVERAGE=n ?

Kconfig ensures that this is impossible:

config CONDITION_COVERAGE
	bool "Condition coverage support"
	depends on COVERAGE && CC_HAS_MCDC

I believe, this is enough, and we don't need a separate check on
Makefile level.

-- 
WBR, Volodymyr