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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 05.04.2025 05:30, Volodymyr Babchuk wrote:
> 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 ?

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