xen/Kconfig | 9 +++++++-- xen/Rules.mk | 1 + xen/arch/x86/Makefile | 2 +- xen/common/coverage/llvm.c | 18 +++++++++++++++++- 4 files changed, 26 insertions(+), 4 deletions(-)
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 in Kconfig to check for the required set of
Clang flags for MC/DC:
'-fprofile-instr-generate -fcoverage-mapping -fcoverage-mcdc'.
This bundle is necessary because '-fcoverage-mcdc' requires
'-fcoverage-mapping', which in turn requires '-fprofile-instr-generate'.
- 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@gmail.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
xen/Kconfig | 9 +++++++--
xen/Rules.mk | 1 +
xen/arch/x86/Makefile | 2 +-
xen/common/coverage/llvm.c | 18 +++++++++++++++++-
4 files changed, 26 insertions(+), 4 deletions(-)
diff --git a/xen/Kconfig b/xen/Kconfig
index a5e5af3b76..8f2cc111cd 100644
--- a/xen/Kconfig
+++ b/xen/Kconfig
@@ -51,9 +51,14 @@ config CC_HAS_ASM_GOTO_OUTPUT
depends on !GCC_ASM_GOTO_OUTPUT_BROKEN
depends on $(success,echo 'int foo(int x) { asm goto ("": "=r"(x) ::: bar); return x; bar: return 0; }' | $(CC) -x c - -c -o /dev/null)
-# Compiler supports -fcondition-coverage aka MC/DC
+# Compiler supports Modified Condition/Decision Coverage (MC/DC).
+# MC/DC is a rigorous code coverage metric that requires every condition
+# within a decision (boolean expression) to be shown to independently
+# influence the decision's final outcome.
+#
+# Minimum toolchain baseline: GCC >= 14, or Clang >= 18.
config CC_HAS_MCDC
- def_bool $(cc-option,-fcondition-coverage)
+ def_bool $(cc-option,-fcondition-coverage) || $(cc-option,-fprofile-instr-generate -fcoverage-mapping -fcoverage-mcdc)
# Set code alignment.
#
diff --git a/xen/Rules.mk b/xen/Rules.mk
index 24f447b957..2b28d1ac3c 100644
--- a/xen/Rules.mk
+++ b/xen/Rules.mk
@@ -136,6 +136,7 @@ non-init-objects = $(filter-out %.init.o, $(obj-y) $(obj-bin-y) $(extra-y))
ifeq ($(CONFIG_CC_IS_CLANG),y)
cov-cflags-$(CONFIG_COVERAGE) := -fprofile-instr-generate -fcoverage-mapping
+ cov-cflags-$(CONFIG_CONDITION_COVERAGE) += -fcoverage-mcdc
else
cov-cflags-$(CONFIG_COVERAGE) := -fprofile-arcs -ftest-coverage
cov-cflags-$(CONFIG_CONDITION_COVERAGE) += -fcondition-coverage
diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index 407571c510..6c0ff67fa8 100644
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -98,7 +98,7 @@ $(obj)/usercopy.o: CFLAGS-y += -iquote .
ifneq ($(CONFIG_HVM),y)
$(obj)/x86_emulate.o: CFLAGS-y += -Wno-unused-label
endif
-ifeq ($(CONFIG_CONDITION_COVERAGE),y)
+ifeq ($(CONFIG_CONDITION_COVERAGE)$(CONFIG_CC_IS_GCC),yy)
$(obj)/x86_emulate.o: CFLAGS-y += -Wno-error=coverage-too-many-conditions
endif
diff --git a/xen/common/coverage/llvm.c b/xen/common/coverage/llvm.c
index 532889c857..5663fb10dd 100644
--- a/xen/common/coverage/llvm.c
+++ b/xen/common/coverage/llvm.c
@@ -120,6 +120,8 @@ 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 const char __start___llvm_prf_bits[];
+extern const char __stop___llvm_prf_bits[];
#define START_DATA ((const void *)__start___llvm_prf_data)
#define END_DATA ((const void *)__stop___llvm_prf_data)
@@ -127,16 +129,23 @@ 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);
+ if ( IS_ENABLED(CONFIG_CONDITION_COVERAGE) )
+ memset(START_BITMAP, 0, END_BITMAP - START_BITMAP);
}
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);
+ if ( IS_ENABLED(CONFIG_CONDITION_COVERAGE) )
+ size += ROUNDUP(END_BITMAP - START_BITMAP, 8);
+ return size;
}
static int cf_check dump(
@@ -155,6 +164,10 @@ static int cf_check dump(
#endif
.names_delta = (uintptr_t)START_NAMES,
.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
};
unsigned int off = 0;
@@ -168,6 +181,9 @@ static int cf_check dump(
APPEND_TO_BUFFER(&header, sizeof(header));
APPEND_TO_BUFFER(START_DATA, END_DATA - START_DATA);
APPEND_TO_BUFFER(START_COUNTERS, END_COUNTERS - START_COUNTERS);
+#if defined(CONFIG_CONDITION_COVERAGE)
+ APPEND_TO_BUFFER(START_BITMAP, END_BITMAP - START_BITMAP);
+#endif
APPEND_TO_BUFFER(START_NAMES, END_NAMES - START_NAMES);
#undef APPEND_TO_BUFFER
--
2.49.0On 24/11/2025 1:17 pm, Saman Dehghan wrote:
> diff --git a/xen/Kconfig b/xen/Kconfig
> index a5e5af3b76..8f2cc111cd 100644
> --- a/xen/Kconfig
> +++ b/xen/Kconfig
> @@ -51,9 +51,14 @@ config CC_HAS_ASM_GOTO_OUTPUT
> depends on !GCC_ASM_GOTO_OUTPUT_BROKEN
> depends on $(success,echo 'int foo(int x) { asm goto ("": "=r"(x) ::: bar); return x; bar: return 0; }' | $(CC) -x c - -c -o /dev/null)
>
> -# Compiler supports -fcondition-coverage aka MC/DC
> +# Compiler supports Modified Condition/Decision Coverage (MC/DC).
Ah sorry, I only meant for this line. Enough for someone to usefully
google.
Otherwise, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
I can trim this down on commit if you're happy.
~Andrew
> +# MC/DC is a rigorous code coverage metric that requires every condition
> +# within a decision (boolean expression) to be shown to independently
> +# influence the decision's final outcome.
> +#
> +# Minimum toolchain baseline: GCC >= 14, or Clang >= 18.
> config CC_HAS_MCDC
> - def_bool $(cc-option,-fcondition-coverage)
> + def_bool $(cc-option,-fcondition-coverage) || $(cc-option,-fprofile-instr-generate -fcoverage-mapping -fcoverage-mcdc)
>
> # Set code alignment.
> #
>
On Mon, Nov 24, 2025 at 8:19 AM Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>
> On 24/11/2025 1:17 pm, Saman Dehghan wrote:
> > diff --git a/xen/Kconfig b/xen/Kconfig
> > index a5e5af3b76..8f2cc111cd 100644
> > --- a/xen/Kconfig
> > +++ b/xen/Kconfig
> > @@ -51,9 +51,14 @@ config CC_HAS_ASM_GOTO_OUTPUT
> > depends on !GCC_ASM_GOTO_OUTPUT_BROKEN
> > depends on $(success,echo 'int foo(int x) { asm goto ("": "=r"(x) ::: bar); return x; bar: return 0; }' | $(CC) -x c - -c -o /dev/null)
> >
> > -# Compiler supports -fcondition-coverage aka MC/DC
> > +# Compiler supports Modified Condition/Decision Coverage (MC/DC).
>
> Ah sorry, I only meant for this line. Enough for someone to usefully
> google.
>
> Otherwise, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>
> I can trim this down on commit if you're happy.
>
> ~Andrew
I’m happy with it, thanks a lot Andrew for suggesting to trim that line.
~Saman
>
> > +# MC/DC is a rigorous code coverage metric that requires every condition
> > +# within a decision (boolean expression) to be shown to independently
> > +# influence the decision's final outcome.
> > +#
> > +# Minimum toolchain baseline: GCC >= 14, or Clang >= 18.
> > config CC_HAS_MCDC
> > - def_bool $(cc-option,-fcondition-coverage)
> > + def_bool $(cc-option,-fcondition-coverage) || $(cc-option,-fprofile-instr-generate -fcoverage-mapping -fcoverage-mcdc)
> >
> > # Set code alignment.
> > #
> >
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, 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.
[1] https://llvm.org/docs/InstrProfileFormat.html
[2] https://github.com/llvm/llvm-project/blob/llvmorg-20.1.8/compiler-rt/lib/profile/InstrProfilingBuffer.c#L223
Signed-off-by: Wentao Zhang <zhangwt1997@gmail.com>
---
As an aside, an alternative way that has better long-term
maintainability would be [3]. I ran it with Xen and could unofficially
confirm it works, modulo implementation nitty-gritties.
[3] https://github.com/llvm/llvm-project/pull/167998
---
xen/common/coverage/llvm.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/xen/common/coverage/llvm.c b/xen/common/coverage/llvm.c
index 5663fb1..f15ec11 100644
--- 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);
}
static int cf_check dump(
@@ -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,
#endif
};
unsigned int off = 0;
@@ -183,6 +184,7 @@ static int cf_check dump(
APPEND_TO_BUFFER(START_COUNTERS, END_COUNTERS - START_COUNTERS);
#if defined(CONFIG_CONDITION_COVERAGE)
APPEND_TO_BUFFER(START_BITMAP, END_BITMAP - START_BITMAP);
+ off += header.padding_bytes_after_bitmap_bytes;
#endif
APPEND_TO_BUFFER(START_NAMES, END_NAMES - START_NAMES);
#undef APPEND_TO_BUFFER
--
2.34.1Since 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
Thanks,
Wentao
On Sat, 20 Dec 2025 05:22:43 -0600, Wentao Zhang <zhangwt1997@gmail.com> 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, 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.
>
> [1] https://llvm.org/docs/InstrProfileFormat.html
> [2] https://github.com/llvm/llvm-project/blob/llvmorg-20.1.8/compiler-rt/lib/profile/InstrProfilingBuffer.c#L223
>
> Signed-off-by: Wentao Zhang <zhangwt1997@gmail.com>
>
> ---
>
> As an aside, an alternative way that has better long-term
> maintainability would be [3]. I ran it with Xen and could unofficially
> confirm it works, modulo implementation nitty-gritties.
>
> [3] https://github.com/llvm/llvm-project/pull/167998
> ---
> xen/common/coverage/llvm.c | 12 +++++++-----
> 1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/xen/common/coverage/llvm.c b/xen/common/coverage/llvm.c
> index 5663fb1..f15ec11 100644
> --- 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);
> }
>
> static int cf_check dump(
> @@ -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,
> #endif
> };
> unsigned int off = 0;
> @@ -183,6 +184,7 @@ static int cf_check dump(
> APPEND_TO_BUFFER(START_COUNTERS, END_COUNTERS - START_COUNTERS);
> #if defined(CONFIG_CONDITION_COVERAGE)
> APPEND_TO_BUFFER(START_BITMAP, END_BITMAP - START_BITMAP);
> + off += header.padding_bytes_after_bitmap_bytes;
> #endif
> APPEND_TO_BUFFER(START_NAMES, END_NAMES - START_NAMES);
> #undef APPEND_TO_BUFFER
> --
> 2.34.1
© 2016 - 2026 Red Hat, Inc.