Re: [XEN][PATCH v2] coverage: extend coverage on .init and lib code

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 09/12/2025 2:28 pm, Jan Beulich wrote:
> On 09.12.2025 15:21, Grygorii Strashko wrote:
>>
>> On 09.12.25 15:19, Andrew Cooper wrote:
>>> 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 <grygorii_strashko@xxxxxxxx>
>>>>>>>>>
>>>>>>>>> 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 <grygorii_strashko@xxxxxxxx>
>>>>>>>>> ---
>>>>>>>>> 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.
>> I assume the rest is ok.
>>
>>> For libelf this means we need regular __init annotations, but #undef'd
>>> outside of __XEN__ (when we're doing the userspace build).
>>>
>> Need clarification here - this are imported libraries and changing their code
>> directly was not welcome before. Therefore there is Xen specific magic in 
>> Makefiles.
>> :(
> I can't and won't speak for libfdt, but for libelf I think we should really
> consider this ours (not imported) the latest as of the re-work for XSA-55.

Agreed.  libelf was already modified for Xen, and then XSA-55 made it
entirely unrecognisable.  It's very much our code now.

If we really don't want to modify libfdt's source, we should make a
standard way of init-ing code like this, so we can move the custom logic
out of libfdt's Makefile.

The problem really is custom local logic; that's what we need to fix.

~Andrew