1. Patchew
  2. Xen
  3. [PATCH v3 0/3] Enable MC/DC support for GCC/GCOV
  4. View patch

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

Volodymyr Babchuk posted 3 patches 7 months ago
There is a newer version of this series
[PATCH v3 3/3] xen: debug: gcov: add condition coverage support
Posted by Volodymyr Babchuk 7 months ago 
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 3/3] xen: debug: gcov: add condition coverage support
Posted by Jan Beulich 7 months ago 
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
Posted by Volodymyr Babchuk 6 months, 4 weeks ago 
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
Posted by Anthony PERARD 6 months, 3 weeks ago 
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
Posted by Volodymyr Babchuk 6 months, 3 weeks ago 
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
Re: [PATCH v3 3/3] xen: debug: gcov: add condition coverage support
Posted by Jan Beulich 6 months, 3 weeks ago 
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
Posted by Anthony PERARD 7 months ago 
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
Posted by Jan Beulich 7 months ago 
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

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.