Re: [XEN PATCH] xen: add paddings after bitmap section in LLVM coverage profile

[Jan Beulich <jbeulich@xxxxxxxx>]

Since you ping-ed the patch, I'll give some comments, albeit I wouldn't feel
qualified to eventually ack the change.

On 20.12.2025 12:22, Wentao Zhang wrote:
> The layout of LLVM coverage profile is like
> 
>   header
>   data section
>   (padding #1)
>   counter section
>   (padding #2)
>   bitmap section
>   (padding #3)
>   name section
>   (padding #4)
> 
> Padding areas #1 and #2 are always zeroed on 64-bit platforms,

How does zeroing (or not) matter when size is what is of interest?

> but that
> is not the case for padding area #3 and #4. See LLVM docs [1] and
> compiler-rt's own version of "get_size()" [2].
> 
> The implementation in 08c787f "xen: Enable MC/DC coverage for Clang"
> partly considers padding #4 in get_size() but not in dump(). It worked
> because in the header .padding_bytes_after_bitmap_bytes is also
> initialized to zero so a reader may still know how to parse the profile.
> But we should probably not base ourselves on such assumption. Instead
> let's be as close as possible to hosted environment generated profiles,
> i.e. those generated by compiler-rt.
> 
> In this patch, get_size() implementation is mathematically the same but
> changed to reflect the layout somewhat better. For dump(), padding #4 is
> added both in the header and in the payload.

#4 is after the name section as per the description at the top, yet code
you add in dump() is to set / use the .padding_bytes_after_bitmap_bytes
field. That's #3 as per above, though.

> --- a/xen/common/coverage/llvm.c
> +++ b/xen/common/coverage/llvm.c
> @@ -141,11 +141,11 @@ static void cf_check reset_counters(void)
>  
>  static uint32_t cf_check get_size(void)
>  {
> -    uint32_t size = ROUNDUP(sizeof(struct llvm_profile_header) + END_DATA - 
> START_DATA +
> -                   END_COUNTERS - START_COUNTERS + END_NAMES - START_NAMES, 
> 8);
> -    if ( IS_ENABLED(CONFIG_CONDITION_COVERAGE) )
> -        size += ROUNDUP(END_BITMAP - START_BITMAP, 8);
> -    return size;
> +    return sizeof(struct llvm_profile_header) +
> +           END_DATA - START_DATA +
> +           END_COUNTERS - START_COUNTERS +
> +           ROUNDUP(END_BITMAP - START_BITMAP, 8) +
> +           ROUNDUP(END_NAMES - START_NAMES, 8);
>  }

Where are these 8-s and ...

> @@ -167,6 +167,7 @@ static int cf_check dump(
>  #if defined(CONFIG_CONDITION_COVERAGE) && LLVM_PROFILE_VERSION >= 9
>          .num_bitmap_bytes = END_BITMAP - START_BITMAP,
>          .bitmap_delta = START_BITMAP - START_DATA,
> +        .padding_bytes_after_bitmap_bytes = (-(END_BITMAP - START_BITMAP)) & 
> 7,

... this 7 coming from? All I can find in your [1] reference is "Sections might
be padded to meet specific alignment requirements. For simplicity, header fields
and data sections solely for padding purposes are omitted in the data layout
graph above and the rest of this document." No other hit when searching for 
"pad"
or "align" in that doc.

Unrelated to your change but relevant for understanding: I also can't seem to be
able to figure out where the various __{start,stop}___llvm_prf_*[] symbols are
coming from. It doesn't look to be our linker script: The LLVM_COV_{RW,RO}_DATA
macros both don't define any symbols. If they did, I would have asked whether
the alignment needs couldn't be accounted for there.

Jan