Re: [PATCH v2] Support LLVM raw profile versions 8, 9, and 10

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Oct 01, 2025 at 05:09:52PM -0500, Saman Dehghan wrote:
> This change enables compatibility for measuring code coverage
> with Clang versions 14 through 20 by supporting their
> respective raw profile formats.
> 
> 1- Add support for LLVM raw profile versions 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 14 through 20 on both ARM 
> and X86 platform
> 
> Signed-off-by: Saman Dehghan <samaan.dehghan@xxxxxxxxx>
> ---
>  xen/common/coverage/llvm.c | 78 +++++++++++++++++++++++++++-----------
>  xen/include/xen/types.h    |  1 +
>  2 files changed, 57 insertions(+), 22 deletions(-)
> 
> diff --git a/xen/common/coverage/llvm.c b/xen/common/coverage/llvm.c
> index 517b2aa8c2..f92f10654c 100644
> --- a/xen/common/coverage/llvm.c
> +++ b/xen/common/coverage/llvm.c
> @@ -44,27 +44,55 @@
>      ((uint64_t)'f' << 16) | ((uint64_t)'R' << 8)  | ((uint64_t)129)
>  #endif
>  
> -#define LLVM_PROFILE_VERSION    4
> +#if __clang_major__ >= 19
> +#define LLVM_PROFILE_VERSION    10
> +#define LLVM_PROFILE_NUM_KINDS  3
> +#elif __clang_major__ == 18
> +#define LLVM_PROFILE_VERSION    9
>  #define LLVM_PROFILE_NUM_KINDS  2
> +#elif __clang_major__ >= 14
> +#define LLVM_PROFILE_VERSION    8
> +#define LLVM_PROFILE_NUM_KINDS  2
> +#else
> +#error "Unsupported Clang version"

No strong opinion, but it would be nice if we could support all
profiles from clang >= 11, as that's the minimum stated clang version.
Again I don't know how much work this will involve, so not going to
insist.

It would be extremely helpful if clang could provide a set of builtin
functions for freestanding environments to handle the performance data
in an opaque way:

https://github.com/llvm/llvm-project/issues/123034

At a minimum it would be good if the compiler exported a define
signaling the profiling version it's using, as we could then fail the
build nicely if coverage support is enabled against a version of clang
we don't support.  Right now anything >= 19 will assume to be using
profiling version 10, but that will go stale sooner or later.

> +#endif
>  
>  struct llvm_profile_data {
>      uint64_t name_ref;
>      uint64_t function_hash;
> -    void *counter;
> -    void *function;
> -    void *values;
> +    intptr_t *relative_counter;
> +#if __clang_major__ >= 18
> +    intptr_t *relative_bitmap;
> +#endif

I would prefer if this was done based on LLVM_PROFILE_VERSION rather
than __clang_major__ (same for the instances below).  The field are
related to the profile version implemented by the compiler, so it's
clearer if the check is against the profile version.

> +    intptr_t *function;
> +    intptr_t *values;
>      uint32_t nr_counters;
>      uint16_t nr_value_sites[LLVM_PROFILE_NUM_KINDS];
> +#if __clang_major__ >= 18
> +    uint32_t numbitmap_bytes;
> +#endif
>  };
>  
>  struct llvm_profile_header {
>      uint64_t magic;
>      uint64_t version;
> -    uint64_t data_size;
> -    uint64_t counters_size;
> +    uint64_t binary_ids_size;
> +    uint64_t num_data;
> +    uint64_t padding_bytes_before_counters;
> +    uint64_t num_counters;
> +    uint64_t padding_bytes_after_counters;
> +    uint64_t num_bitmap_bytes;
> +    uint64_t padding_bytes_after_bitmap_bytes;
>      uint64_t names_size;
> +#if __clang_major__ >= 18
>      uint64_t counters_delta;
> +    uint64_t bitmap_delta;
> +#endif
>      uint64_t names_delta;
> +#if __clang_major__ >= 19
> +    uint64_t num_vtables;
> +    uint64_t vnames_size;
> +#endif
>      uint64_t value_kind_last;
>  };
>  
> @@ -76,19 +104,20 @@ struct llvm_profile_header {
>   */
>  int __llvm_profile_runtime;
>  
> -extern const struct llvm_profile_data __start___llvm_prf_data[];
> -extern const struct llvm_profile_data __stop___llvm_prf_data[];
> -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[];
> +extern char __start___llvm_prf_data[];
> +extern char __stop___llvm_prf_data[];
> +extern char __start___llvm_prf_names[];
> +extern char __stop___llvm_prf_names[];
> +extern char __start___llvm_prf_cnts[];
> +extern char __stop___llvm_prf_cnts[];

What's the point of defining those uniformly as char instead of the
more accurate types used previously?

> +#define START_DATA      ((const char *)__start___llvm_prf_data)
> +#define END_DATA        ((const char *)__stop___llvm_prf_data)
> +#define START_NAMES     ((const char *)__start___llvm_prf_names)
> +#define END_NAMES       ((const char *)__stop___llvm_prf_names)
> +#define START_COUNTERS  ((char *)__start___llvm_prf_cnts)
> +#define END_COUNTERS    ((char *)__stop___llvm_prf_cnts)
>  
> -#define START_DATA      ((const void *)__start___llvm_prf_data)
> -#define END_DATA        ((const void *)__stop___llvm_prf_data)
> -#define START_NAMES     ((const void *)__start___llvm_prf_names)
> -#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)

Kind of similar question here, what's the benefit of using char *
instead of void *?

>  static void cf_check reset_counters(void)
>  {
> @@ -107,10 +136,15 @@ 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),
> -        .names_size = END_NAMES - START_NAMES,
> -        .counters_delta = (uintptr_t)START_COUNTERS,
> +        .binary_ids_size = 0,

We don't usually explicitly initialize fields to 0, as that's the
default already.

> +        .num_data = (((intptr_t)END_DATA + sizeof(struct llvm_profile_data) 
> - 1)
> +                - (intptr_t)START_DATA) / sizeof(struct llvm_profile_data),
> +        .padding_bytes_before_counters = 0,
> +        .num_counters = (((intptr_t)END_COUNTERS + sizeof(uint64_t) - 1)
> +                - (intptr_t)START_COUNTERS) / sizeof(uint64_t),
> +        .padding_bytes_after_counters = 0,
> +        .names_size = (END_NAMES - START_NAMES) * sizeof(char),
> +        .counters_delta = (uintptr_t)START_COUNTERS - (uintptr_t)START_DATA,

The casting here seems not uniform.  Why is intptr_t used in some
instances while others use uintptr_t?

I also think you could just use void *, as Xen uses the GCC extension
that allows arithmetic on void pointers.

Thanks, Roger.