Re: [PATCH v3] Support LLVM raw profile versions 5, 6, 7, 8, 9, and 10

[Saman Dehghan <samaan.dehghan@xxxxxxxxx>]

On Fri, Oct 24, 2025 at 4:33 AM Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:
>
> The subject should have a "xen: " prefix, as this applies to the
> hypervisor and not other
>
> On 24/10/2025 1:16 am, Saman Dehghan wrote:
> > This change enables compatibility for measuring code coverage
> > with Clang versions 14 through 20 by supporting their
>
> Stale 14?  It looks to be 11 now.

Sorry for the mistake. I meant "with Clang versions 11 through 20 by
supporting their...".

>
> > respective raw profile formats.
> >
> > 1- Added support for LLVM raw profile versions 5, 6, 7, 8, 9, and 10.
> > 2- Initialized llvm_profile_header for all versions based on llvm source 
> > code in
> >    compiler-rt/include/profile/InstrProfData.inc for each version.
> > 3- We tested this patch for all Clang versions from 11 through 20 on x86 
> > platform.
> > 4- Fixed linking warnings related to coverage code in x86.
> >
> > Signed-off-by: Saman Dehghan <samaan.dehghan@xxxxxxxxx>
> > ---
>
> When sending multiple revisions, it's customary to put a short list here
> if what you've changed from the previous revision.

Changes since version 2:
  1- Additionally support raw profile version 5, 6, 7 in clang 11, 12, 13.
  2- Fix coverage related linking warnings in x86.
  3- Revert unnecessary type changes, casting, etc.

>
> Also, you didn't accumulate your release ack from v2.
>
> Release-Acked-By: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>

Sorry we missed this. Thanks for reminding us.

>
> >  xen/arch/x86/xen.lds.S     |  6 ++++
> >  xen/common/coverage/llvm.c | 73 ++++++++++++++++++++++++++++++++++----
> >  xen/include/xen/xen.lds.h  | 18 ++++++++++
> >  3 files changed, 91 insertions(+), 6 deletions(-)
> >
> > diff --git a/xen/common/coverage/llvm.c b/xen/common/coverage/llvm.c
> > index 517b2aa8c2..e3272a546f 100644
> > --- a/xen/common/coverage/llvm.c
> > +++ b/xen/common/coverage/llvm.c
> > @@ -107,11 +145,34 @@ static int cf_check dump(
> >      struct llvm_profile_header header = {
> >          .magic = LLVM_PROFILE_MAGIC,
> >          .version = LLVM_PROFILE_VERSION,
> > -        .data_size = (END_DATA - START_DATA) / sizeof(struct 
> > llvm_profile_data),
> > -        .counters_size = (END_COUNTERS - START_COUNTERS) / 
> > sizeof(uint64_t),
> > +#if __clang_major__ >= 13
> > +        .binary_ids_size = 0,
> > +#endif
> > +        .num_data = ((END_DATA + sizeof(struct llvm_profile_data) - 1)
> > +                - START_DATA) / sizeof(struct llvm_profile_data),
>
> There's a helper for this expression.
>
> DIV_ROUND_UP(END_DATA - START_DATA, sizeof(llvm_profile_data))
>
> > +        .padding_bytes_before_counters = 0,
> > +        .num_counters = ((END_COUNTERS + sizeof(uint64_t) - 1)
> > +                - START_COUNTERS) / sizeof(uint64_t),
>
> DIV_ROUND_UP(END_COUNTERS - START_COUNTERS, sizeof(uint64_t))
>
>
> > +        .padding_bytes_after_counters = 0,
> > +#if __clang_major__ >= 18
> > +        .num_bitmap_bytes = 0,
> > +        .padding_bytes_after_bitmap_bytes = 0,
> > +#endif
> >          .names_size = END_NAMES - START_NAMES,
> > +#if __clang_major__ >= 14
> > +        .counters_delta = START_COUNTERS - START_DATA,
> > +#else
> >          .counters_delta = (uintptr_t)START_COUNTERS,
> > +#endif
> > +
> > +#if __clang_major__ >= 18
> > +        .bitmap_delta = 0,
> > +#endif
> >          .names_delta = (uintptr_t)START_NAMES,
> > +#if __clang_major__ >= 19 && __clang_major__ <= 20
> > +        .num_vtables = 0,
> > +        .vnames_size = 0,
> > +#endif
>
> Because this is a structure initialiser, everything set explicitly to 0
> can be omitted.  This removes all #ifdef-ary except the .counters_delta
> I believe, as well as the .padding_byte_* fields.
>

Is it undefined behaviour to leave struct members uninitialized for
local variables?

> The resulting diff is far smaller.
>
> >          .value_kind_last = LLVM_PROFILE_NUM_KINDS - 1,
> >      };
> >      unsigned int off = 0;
> > diff --git a/xen/include/xen/xen.lds.h b/xen/include/xen/xen.lds.h
> > index b126dfe887..42550a85a2 100644
> > --- a/xen/include/xen/xen.lds.h
> > +++ b/xen/include/xen/xen.lds.h
> > @@ -81,6 +81,24 @@
> >    .stab.index 0 : { *(.stab.index) }         \
> >    .stab.indexstr 0 : { *(.stab.indexstr) }
> >
> > +#if defined(CONFIG_COVERAGE) && defined(CONFIG_CC_IS_CLANG)
> > +
> > +#define LLVM_COV_RW_DATA                                   \
> > +    DECL_SECTION(__llvm_prf_cnts) { *(__llvm_prf_cnts) }   \
> > +    DECL_SECTION(__llvm_prf_data) { *(__llvm_prf_data) }   \
> > +    DECL_SECTION(__llvm_prf_bits) { *(__llvm_prf_bits) }
> > +
> > +#define LLVM_COV_RO_DATA                                   \
> > +    DECL_SECTION(__llvm_prf_names) { *(__llvm_prf_names) }
> > +
> > +#define LLVM_COV_DEBUG                                     \
> > +    DECL_DEBUG(__llvm_covfun, 8)                           \
> > +    DECL_DEBUG(__llvm_covmap, 8)
> > +#else
> > +#define LLVM_COV_RW_DATA
> > +#define LLVM_COV_RO_DATA
> > +#define LLVM_COV_DEBUG
> > +#endif
>
> Newline here.
>
> But, there's no problem stating sections which are unused.  I think the
> outer #if/#else can be dropped.
>
> Otherwise, Acked-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>
> I can fix these all up on commit, seeing as it's release acked for 4.21

Thank you for offering to fix them up. Let me know how I can help or
if I need to send another version.

Thanks,
Saman

>
> ~Andrew