Re: [PATCH] XEN: enable MC/DC coverage for Clang

[Saman Dehghan <samaan.dehghan@xxxxxxxxx>]

On Mon, Nov 24, 2025 at 5:15 AM Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:
>
> On 24/11/2025 2:18 am, Saman Dehghan wrote:
> > Clang >= 18 supports Modified Condition/Decision Coverage (MC/DC).
> > This patch enables the detection and usage of this feature when
> > compiling Xen with Clang.
> >
> > - Update detection logic to check for '-fcoverage-mcdc' when using Clang.
> > - Update llvm.c to handle the profile format changes (bitmap section)
> >   required for MC/DC.
> > - Guard -Wno-error=coverage-too-many-conditions with CONFIG_CC_IS_GCC
> >   to avoid passing a GCC-only warning option to Clang
> >
> > Signed-off-by: Saman Dehghan <samaan.dehghan@xxxxxxxxx>
> > ---
> >  xen/Kconfig                |  2 +-
> >  xen/Rules.mk               |  1 +
> >  xen/arch/x86/Makefile      |  4 +++-
> >  xen/common/coverage/llvm.c | 24 +++++++++++++++++++++++-
> >  4 files changed, 28 insertions(+), 3 deletions(-)
> >
> > diff --git a/xen/Kconfig b/xen/Kconfig
> > index a5e5af3b76..5508993f02 100644
> > --- a/xen/Kconfig
> > +++ b/xen/Kconfig
> > @@ -53,7 +53,7 @@ config CC_HAS_ASM_GOTO_OUTPUT
> >
> >  # Compiler supports -fcondition-coverage aka MC/DC
>
> While you're improving these comments, please drop -fcondition-coverage
> (as it's no longer accurate), and expand MC/DC for the benefit of people
> who don't know what it is.
>
> >  config CC_HAS_MCDC
>
> Also, # GCC >= 14, or Clang >= 18
>
> It's important for toolchain versions to be given in comments, so we can
> figure out what to clean up when upgrading the toolchain baselines.
>
> > diff --git a/xen/common/coverage/llvm.c b/xen/common/coverage/llvm.c
> > index 532889c857..a8c7e7e8d2 100644
> > --- a/xen/common/coverage/llvm.c
> > +++ b/xen/common/coverage/llvm.c
> > @@ -120,6 +120,10 @@ extern const char __start___llvm_prf_names[];
> >  extern const char __stop___llvm_prf_names[];
> >  extern uint64_t __start___llvm_prf_cnts[];
> >  extern uint64_t __stop___llvm_prf_cnts[];
> > +#ifdef CONFIG_CONDITION_COVERAGE
> > +extern const char __start___llvm_prf_bits[];
> > +extern const char __stop___llvm_prf_bits[];
> > +#endif
>
> No need for these to be #ifdef'd.  In turn, it lets you do ...
>
> >
> >  #define START_DATA      ((const void *)__start___llvm_prf_data)
> >  #define END_DATA        ((const void *)__stop___llvm_prf_data)
> > @@ -127,16 +131,25 @@ extern uint64_t __stop___llvm_prf_cnts[];
> >  #define END_NAMES       ((const void *)__stop___llvm_prf_names)
> >  #define START_COUNTERS  ((void *)__start___llvm_prf_cnts)
> >  #define END_COUNTERS    ((void *)__stop___llvm_prf_cnts)
> > +#define START_BITMAP    ((void *)__start___llvm_prf_bits)
> > +#define END_BITMAP      ((void *)__stop___llvm_prf_bits)
> >
> >  static void cf_check reset_counters(void)
> >  {
> >      memset(START_COUNTERS, 0, END_COUNTERS - START_COUNTERS);
> > +#ifdef CONFIG_CONDITION_COVERAGE
> > +    memset(START_BITMAP, 0, END_BITMAP - START_BITMAP);
> > +#endif
>
> ... this:
>
>     if ( IS_ENABLED(CONFIG_CONDITION_COVERAGE) )
>         memset(START_BITMAP, 0, END_BITMAP - START_BITMAP);
>
> >  }

Thanks Andrew.

IS_ENABLED(CONFIG_CONDITION_COVERAGE) is not the same as #ifdef
CONFIG_CONDITION_COVERAGE.
When the option is completely undefined, IS_ENABLED() returns 1 (enabled).
So even with no CONFIG_CONDITION_COVERAGE defined, the code takes the
"enabled" path, which is not what we want here.

> >
> >  static uint32_t cf_check get_size(void)
> >  {
> > -    return ROUNDUP(sizeof(struct llvm_profile_header) + END_DATA - 
> > START_DATA +
> > +    uint32_t size = ROUNDUP(sizeof(struct llvm_profile_header) + END_DATA 
> > - START_DATA +
> >                     END_COUNTERS - START_COUNTERS + END_NAMES - 
> > START_NAMES, 8);
> > +#ifdef CONFIG_CONDITION_COVERAGE
> > +    size += ROUNDUP(END_BITMAP - START_BITMAP, 8);
> > +#endif
>
> and similar here.
>
> > +    return size;
> >  }
> >
> >  static int cf_check dump(
> > @@ -147,11 +160,17 @@ static int cf_check dump(
> >          .version = LLVM_PROFILE_VERSION,
> >          .num_data = DIV_ROUND_UP(END_DATA - START_DATA, sizeof(struct 
> > llvm_profile_data)),
> >          .num_counters = DIV_ROUND_UP(END_COUNTERS - START_COUNTERS, 
> > sizeof(uint64_t)),
> > +#if defined(CONFIG_CONDITION_COVERAGE) && LLVM_PROFILE_VERSION >= 9
> > +        .num_bitmap_bytes = END_BITMAP - START_BITMAP,
> > +#endif
> >          .names_size = END_NAMES - START_NAMES,
> >  #if LLVM_PROFILE_VERSION >= 8
> >          .counters_delta = START_COUNTERS - START_DATA,
> >  #else
> >          .counters_delta = (uintptr_t)START_COUNTERS,
> > +#endif
> > +#if defined(CONFIG_CONDITION_COVERAGE) && LLVM_PROFILE_VERSION >= 9
> > +        .bitmap_delta = START_BITMAP - START_DATA,
> >  #endif
> >          .names_delta = (uintptr_t)START_NAMES,
> >          .value_kind_last = LLVM_PROFILE_NUM_KINDS - 1,
>
> With structure initialisation, you do not need to have the fields in
> declaration order.  Therefore, you want to do something like this:
>
>          .value_kind_last = LLVM_PROFILE_NUM_KINDS - 1,
> +#if defined(CONFIG_CONDITION_COVERAGE) && LLVM_PROFILE_VERSION >= 9
> +        .num_bitmap_bytes = END_BITMAP - START_BITMAP,
> +        .bitmap_delta = START_BITMAP - START_DATA,
> +#endif
>  };
>
>
> to keep the ifdefary more simple.
>
> ~Andrew