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

[Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>]

Hi Jan,

Jan Beulich <jbeulich@xxxxxxxx> 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