On 08/12/2025 6:49 pm, Grygorii Strashko wrote: > Hi Andrew, > > On 06.12.25 16:21, Andrew Cooper wrote: >> On 06/12/2025 2:15 pm, Andrew Cooper wrote: >>> On 06/12/2025 9:10 am, Grygorii Strashko wrote: >>>> >>>> On 05.12.25 22:00, Andrew Cooper wrote: >>>>> On 05/12/2025 7:34 pm, Grygorii Strashko wrote: >>>>>> From: Grygorii Strashko <[email protected]> >>>>>> >>>>>> Extend coverage support on .init and lib code. >>>>>> Add two hidden Kconfig options: >>>>>> - RELAX_INIT_CHECK "Relax strict check for .init sections only in >>>>>> %.init.o >>>>>> files" >>>>>> - DO_NOT_FREE_INIT_MEMORY "Prevent freeing of .init sections at the >>>>>> end of >>>>>> Xen boot." >>>>>> >>>>>> Both selected selected when COVERAGE=y, as getting coverage >>>>>> report for >>>>>> ".init" code is required: >>>>>> - to bypass strict check for .init sections only in %.init.o files; >>>>>> - the .init code stay in memory after Xen boot. >>>>>> >>>>>> RELAX_INIT_CHECK/DO_NOT_FREE_INIT_MEMORY could be used by other >>>>>> debug >>>>>> features in the future. >>>>>> >>>>>> Signed-off-by: Grygorii Strashko <[email protected]> >>>>>> --- >>>>>> changes in v2: >>>>>> - add RELAX_INIT_CHECK and DO_NOT_FREE_INIT_MEMORY, those are two >>>>>> different things, >>>>>> both potentially reusable >>>>>> - enable coverage for libfdt/libelf always >>>>>> - enable colverage for .init always >>>>> This is a lot nicer (i.e. more simple). >>>>> >>>>> But, I still don't know why we need to avoid freeing init memory >>>>> to make >>>>> this work. What explodes if we dont? >>>>> >>>> It will just crash when coverage data is collected. >>>> >>>> First I made changes in make file to get .init covered >>>> then I hit a crash >>>> then I checked %.init.o >>>> conclusion was obvious. >>>> >>>> For example: >>>> objdump -x bzimage.init.o | grep gcov >>>> >>>> 0000000000000010 l O .bss 0000000000000028 >>>> __gcov0.bzimage_check >>>> 0000000000000040 l O .bss 0000000000000040 >>>> __gcov0.bzimage_headroom >>>> 0000000000000000 l O .bss 0000000000000008 >>>> __gcov0.output_length >>>> 0000000000000080 l O .bss 0000000000000060 >>>> __gcov0.bzimage_parse >>>> 0000000000000098 l O .init.data.rel.local 0000000000000028 >>>> __gcov_.bzimage_parse >>>> 0000000000000070 l O .init.data.rel.local 0000000000000028 >>>> __gcov_.bzimage_headroom >>>> 0000000000000048 l O .init.data.rel.local 0000000000000028 >>>> __gcov_.bzimage_check >>>> 0000000000000020 l O .init.data.rel.local 0000000000000028 >>>> __gcov_.output_length >>>> 0000000000000000 *UND* 0000000000000000 __gcov_init >>>> 0000000000000000 *UND* 0000000000000000 __gcov_exit >>>> 0000000000000000 *UND* 0000000000000000 __gcov_merge_add >>>> 0000000000000008 R_X86_64_PLT32 __gcov_init-0x0000000000000004 >>>> 0000000000000012 R_X86_64_PLT32 __gcov_exit-0x0000000000000004 >>>> 0000000000000020 R_X86_64_64 __gcov_merge_add >>>> >>> Aah, we should exclude the OJBCOPY too. That's what's moving >>> .data.rel.local amongst other sections we target with attributes >>> directly. >> >> we can't target. > > I've come up with below diff - seems it's working without > DO_NOT_FREE_INIT_MEMORY. > Is this what you have in mind? > > diff --git a/xen/Kconfig.debug b/xen/Kconfig.debug > index 8fc201d12c2c..16b1a82db46e 100644 > --- a/xen/Kconfig.debug > +++ b/xen/Kconfig.debug > @@ -40,7 +40,6 @@ config COVERAGE > depends on SYSCTL && !LIVEPATCH > select SUPPRESS_DUPLICATE_SYMBOL_WARNINGS if > !ENFORCE_UNIQUE_SYMBOLS > select RELAX_INIT_CHECK > - select DO_NOT_FREE_INIT_MEMORY > help > Enable code coverage support. > > diff --git a/xen/Rules.mk b/xen/Rules.mk > index 8c4861a427e6..47fdcc1d23b5 100644 > --- a/xen/Rules.mk > +++ b/xen/Rules.mk > @@ -33,11 +33,15 @@ cov-cflags-y := > nocov-y := > noubsan-y := > > +# when coverage is enabled the gcc internal section should stay in > memory > +# after Xen boot > +ifneq ($(CONFIG_COVERAGE),y) > SPECIAL_DATA_SECTIONS := rodata $(foreach a,1 2 4 8 16, \ > $(foreach w,1 2 4, \ > > rodata.str$(w).$(a)) \ > rodata.cst$(a)) \ > $(foreach r,rel rel.ro,data.$(r).local) > +endif > > # The filename build.mk has precedence over Makefile > include $(firstword $(wildcard $(srcdir)/build.mk) $(srcdir)/Makefile) > diff --git a/xen/common/libelf/Makefile b/xen/common/libelf/Makefile > index 60b3ae40728f..8180c78f1510 100644 > --- a/xen/common/libelf/Makefile > +++ b/xen/common/libelf/Makefile > @@ -1,8 +1,10 @@ > obj-bin-y := libelf.o > libelf-objs := libelf-tools.o libelf-loader.o libelf-dominfo.o > > +ifneq ($(CONFIG_COVERAGE),y) > SECTIONS := text data $(SPECIAL_DATA_SECTIONS) > OBJCOPYFLAGS := $(foreach s,$(SECTIONS),--rename-section > .$(s)=.init.$(s)) > +endif > > CFLAGS-y += -Wno-pointer-sign > > diff --git a/xen/common/libfdt/Makefile b/xen/common/libfdt/Makefile > index ae0f69c01373..fb26e5bff0fd 100644 > --- a/xen/common/libfdt/Makefile > +++ b/xen/common/libfdt/Makefile > @@ -4,7 +4,9 @@ SECTIONS := text data $(SPECIAL_DATA_SECTIONS) > > # For CONFIG_OVERLAY_DTB, libfdt functionalities will be needed > during runtime. > ifneq ($(CONFIG_OVERLAY_DTB),y) > -OBJCOPYFLAGS := $(foreach s,$(SECTIONS),--rename-section > .$(s)=.init.$(s)) > + ifneq ($(CONFIG_COVERAGE),y) > + OBJCOPYFLAGS := $(foreach > s,$(SECTIONS),--rename-section .$(s)=.init.$(s)) > + endif > endif
This is the (aforementioned) non-standard way of doing .init.o, which is why it doesn't play nicely. I suggest that we first convert libelf and libfdt to the standard way of doing .init. For libelf this means we need regular __init annotations, but #undef'd outside of __XEN__ (when we're doing the userspace build). For libfdt, this will need some init_or_$FOO things (matching init_or_livepatch). Once the custom init has been made standard, this code becomes easier to move into lib, and we no longer have special cases when trying to extend coverage. ~Andrew
