[PATCH] coverage: filter out lib{fdt,elf}-temp.o

Michal Orzel posted 1 patch 3 months, 1 week ago
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/20240118120641.24824-1-michal.orzel@amd.com
xen/common/libelf/Makefile | 2 +-
xen/common/libfdt/Makefile | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
[PATCH] coverage: filter out lib{fdt,elf}-temp.o
Posted by Michal Orzel 3 months, 1 week ago
At the moment, trying to run xencov read/reset (calling SYSCTL_coverage_op
under the hood) results in a crash. This is due to an attempt to
access code in the .init.* sections (libfdt for Arm and libelf for x86)
that are stripped after boot. Normally, the build system compiles any
*.init.o file without COV_FLAGS. However, these two libraries are
handled differently as sections will be renamed to init after linking.

This worked until e321576f4047 ("xen/build: start using if_changed")
that added lib{fdt,elf}-temp.o to extra-y. Any file listed there without
*.init.o suffix will be part of non-init-objects for which COV_FLAGS
will be appended. In such case, the solution is to add a file to nocov-y.

Also, for libfdt, append to nocov-y only if CONFIG_OVERLAY_DTB is not
set. Otherwise, there is no section renaming and we should be able to
run the coverage.

Fixes: e321576f4047 ("xen/build: start using if_changed")
Signed-off-by: Michal Orzel <michal.orzel@amd.com>
---
 xen/common/libelf/Makefile | 2 +-
 xen/common/libfdt/Makefile | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/common/libelf/Makefile b/xen/common/libelf/Makefile
index 8a4522e4e141..f618f70d5c8e 100644
--- a/xen/common/libelf/Makefile
+++ b/xen/common/libelf/Makefile
@@ -1,5 +1,5 @@
 obj-bin-y := libelf.o
-nocov-y += libelf.o
+nocov-y += libelf.o libelf-temp.o
 libelf-objs := libelf-tools.o libelf-loader.o libelf-dominfo.o
 
 SECTIONS := text data $(SPECIAL_DATA_SECTIONS)
diff --git a/xen/common/libfdt/Makefile b/xen/common/libfdt/Makefile
index d50487aa6e32..fb0d8a48eee2 100644
--- a/xen/common/libfdt/Makefile
+++ b/xen/common/libfdt/Makefile
@@ -5,10 +5,10 @@ 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))
+nocov-y += libfdt.o libfdt-temp.o
 endif
 
 obj-y += libfdt.o
-nocov-y += libfdt.o
 
 CFLAGS-y += -I$(srctree)/include/xen/libfdt/
 
-- 
2.25.1
Re: [PATCH] coverage: filter out lib{fdt,elf}-temp.o
Posted by Andrew Cooper 3 months, 1 week ago
On 18/01/2024 12:06 pm, Michal Orzel wrote:
> At the moment, trying to run xencov read/reset (calling SYSCTL_coverage_op
> under the hood) results in a crash. This is due to an attempt to
> access code in the .init.* sections

Minor point, but for coverage it's only data.  It's the per-basic-block
counters.

~Andrew

Re: [PATCH] coverage: filter out lib{fdt,elf}-temp.o
Posted by Jan Beulich 3 months, 1 week ago
On 18.01.2024 13:06, Michal Orzel wrote:
> At the moment, trying to run xencov read/reset (calling SYSCTL_coverage_op
> under the hood) results in a crash. This is due to an attempt to
> access code in the .init.* sections (libfdt for Arm and libelf for x86)
> that are stripped after boot. Normally, the build system compiles any
> *.init.o file without COV_FLAGS. However, these two libraries are
> handled differently as sections will be renamed to init after linking.
> 
> This worked until e321576f4047 ("xen/build: start using if_changed")
> that added lib{fdt,elf}-temp.o to extra-y. Any file listed there without
> *.init.o suffix will be part of non-init-objects for which COV_FLAGS
> will be appended.

While this is true, aiui COV_FLAGS would be empty for anything listed
in nocov-y and all of the prerequisites of those objects (iirc target-
specific variable settings propagate to prerequisites). Therefore ...

> In such case, the solution is to add a file to nocov-y.

... libelf.o / libfdt.o already being listed there ought to suffice.
Alternatively listing only libelf-temp.o / libfdt-temp.o ought to
suffice as well.

Since you apparently observed things not working, I must be missing
something.

Jan
Re: [PATCH] coverage: filter out lib{fdt,elf}-temp.o
Posted by Anthony PERARD 3 months, 1 week ago
On Thu, Jan 18, 2024 at 02:12:21PM +0100, Jan Beulich wrote:
> On 18.01.2024 13:06, Michal Orzel wrote:
> > At the moment, trying to run xencov read/reset (calling SYSCTL_coverage_op
> > under the hood) results in a crash. This is due to an attempt to
> > access code in the .init.* sections (libfdt for Arm and libelf for x86)
> > that are stripped after boot. Normally, the build system compiles any
> > *.init.o file without COV_FLAGS. However, these two libraries are
> > handled differently as sections will be renamed to init after linking.
> > 
> > This worked until e321576f4047 ("xen/build: start using if_changed")
> > that added lib{fdt,elf}-temp.o to extra-y. Any file listed there without
> > *.init.o suffix will be part of non-init-objects for which COV_FLAGS
> > will be appended.
> 
> While this is true, aiui COV_FLAGS would be empty for anything listed
> in nocov-y and all of the prerequisites of those objects (iirc target-
> specific variable settings propagate to prerequisites). Therefore ...
> 
> > In such case, the solution is to add a file to nocov-y.
> 
> ... libelf.o / libfdt.o already being listed there ought to suffice.
> Alternatively listing only libelf-temp.o / libfdt-temp.o ought to
> suffice as well.
> 
> Since you apparently observed things not working, I must be missing
> something.

Yes, $(extra-y) is like $(obj-y), but objects there will not be added
"built_in.o". So, make is likely building "libelf-temp.o" and deps
because it's in $(extra-y) rather than because it's a prerequisite of
"libelf.o". We could ask make to process prerequisite in a reverse
order, and suddenly, the command line to make all "libelf-*.o" is
different: `make --shuffle=reverse V=2`.

So, adding extra object to $(nocov-y) is a workaround, but I think a
better fix would be to add those objects to $(targets) instead of
$(extra-y). I think I've made a mistake by using $(extra-y) instead of
$(targets) in that original commit.

Cheers,

-- 
Anthony PERARD
Re: [PATCH] coverage: filter out lib{fdt,elf}-temp.o
Posted by Michal Orzel 3 months, 1 week ago
Hi Anthony,

On 18/01/2024 18:37, Anthony PERARD wrote:
> 
> 
> On Thu, Jan 18, 2024 at 02:12:21PM +0100, Jan Beulich wrote:
>> On 18.01.2024 13:06, Michal Orzel wrote:
>>> At the moment, trying to run xencov read/reset (calling SYSCTL_coverage_op
>>> under the hood) results in a crash. This is due to an attempt to
>>> access code in the .init.* sections (libfdt for Arm and libelf for x86)
>>> that are stripped after boot. Normally, the build system compiles any
>>> *.init.o file without COV_FLAGS. However, these two libraries are
>>> handled differently as sections will be renamed to init after linking.
>>>
>>> This worked until e321576f4047 ("xen/build: start using if_changed")
>>> that added lib{fdt,elf}-temp.o to extra-y. Any file listed there without
>>> *.init.o suffix will be part of non-init-objects for which COV_FLAGS
>>> will be appended.
>>
>> While this is true, aiui COV_FLAGS would be empty for anything listed
>> in nocov-y and all of the prerequisites of those objects (iirc target-
>> specific variable settings propagate to prerequisites). Therefore ...
>>
>>> In such case, the solution is to add a file to nocov-y.
>>
>> ... libelf.o / libfdt.o already being listed there ought to suffice.
>> Alternatively listing only libelf-temp.o / libfdt-temp.o ought to
>> suffice as well.
>>
>> Since you apparently observed things not working, I must be missing
>> something.
> 
> Yes, $(extra-y) is like $(obj-y), but objects there will not be added
> "built_in.o". So, make is likely building "libelf-temp.o" and deps
> because it's in $(extra-y) rather than because it's a prerequisite of
> "libelf.o". We could ask make to process prerequisite in a reverse
> order, and suddenly, the command line to make all "libelf-*.o" is
> different: `make --shuffle=reverse V=2`.
That's helpful.

> 
> So, adding extra object to $(nocov-y) is a workaround, but I think a
> better fix would be to add those objects to $(targets) instead of
> $(extra-y). I think I've made a mistake by using $(extra-y) instead of
> $(targets) in that original commit.
I can confirm that by moving lib{fdt,elf}-temp.o and deps to targets, the issue is gone as well.

Is my understanding correct that by switching from extra-y to targets we are preventing these objects to
appear in non-init-objects (and thus having COV_FLAGS appended) while retaining the proper if_changed behavior?

According to docs/misc/xen-makefiles/makefiles.rst:
Any target that utilises if_changed must be listed in $(targets),
otherwise the command line check will fail, and the target will
always be built.

~Michal
Re: [PATCH] coverage: filter out lib{fdt,elf}-temp.o
Posted by Anthony PERARD 3 months, 1 week ago
On Fri, Jan 19, 2024 at 09:43:30AM +0100, Michal Orzel wrote:
> Is my understanding correct that by switching from extra-y to targets we are preventing these objects to
> appear in non-init-objects (and thus having COV_FLAGS appended) while retaining the proper if_changed behavior?
> 
> According to docs/misc/xen-makefiles/makefiles.rst:
> Any target that utilises if_changed must be listed in $(targets),
> otherwise the command line check will fail, and the target will
> always be built.

Indeed, and $(extra-y) is added to $(targets) via
$(targets-for-builtin).

While switching from $(extra-y) to $(targets) prevents the objects from
been added to $(non-init-objets), it doesn't matter because "libelf.o"
is in that variable, so $(COV_FLAGS) is added to $(_c_flags) and its
value is used in all the prerequisites of "libelf.o" which includes
"libelf-temp.o" and for example "libelf-dominfo.o". So the only thing
preventing $(COV_FLAGS) from been added when building "libelf-tools.o"
for example is that we set `COV_FLAGS:=` for "libelf.o".

Cheers,

-- 
Anthony PERARD
Re: [PATCH] coverage: filter out lib{fdt,elf}-temp.o
Posted by Jan Beulich 3 months, 1 week ago
On 19.01.2024 16:25, Anthony PERARD wrote:
> On Fri, Jan 19, 2024 at 09:43:30AM +0100, Michal Orzel wrote:
>> Is my understanding correct that by switching from extra-y to targets we are preventing these objects to
>> appear in non-init-objects (and thus having COV_FLAGS appended) while retaining the proper if_changed behavior?
>>
>> According to docs/misc/xen-makefiles/makefiles.rst:
>> Any target that utilises if_changed must be listed in $(targets),
>> otherwise the command line check will fail, and the target will
>> always be built.
> 
> Indeed, and $(extra-y) is added to $(targets) via
> $(targets-for-builtin).
> 
> While switching from $(extra-y) to $(targets) prevents the objects from
> been added to $(non-init-objets), it doesn't matter because "libelf.o"
> is in that variable, so $(COV_FLAGS) is added to $(_c_flags) and its
> value is used in all the prerequisites of "libelf.o" which includes
> "libelf-temp.o" and for example "libelf-dominfo.o". So the only thing
> preventing $(COV_FLAGS) from been added when building "libelf-tools.o"
> for example is that we set `COV_FLAGS:=` for "libelf.o".

Yet doesn't that (again) mean things should actually work as-is, yet
Michal is observing this not being the case?

Jan
Re: [PATCH] coverage: filter out lib{fdt,elf}-temp.o
Posted by Anthony PERARD 3 months, 1 week ago
On Mon, Jan 22, 2024 at 11:04:41AM +0100, Jan Beulich wrote:
> On 19.01.2024 16:25, Anthony PERARD wrote:
> > On Fri, Jan 19, 2024 at 09:43:30AM +0100, Michal Orzel wrote:
> >> Is my understanding correct that by switching from extra-y to targets we are preventing these objects to
> >> appear in non-init-objects (and thus having COV_FLAGS appended) while retaining the proper if_changed behavior?
> >>
> >> According to docs/misc/xen-makefiles/makefiles.rst:
> >> Any target that utilises if_changed must be listed in $(targets),
> >> otherwise the command line check will fail, and the target will
> >> always be built.
> > 
> > Indeed, and $(extra-y) is added to $(targets) via
> > $(targets-for-builtin).
> > 
> > While switching from $(extra-y) to $(targets) prevents the objects from
> > been added to $(non-init-objets), it doesn't matter because "libelf.o"
> > is in that variable, so $(COV_FLAGS) is added to $(_c_flags) and its
> > value is used in all the prerequisites of "libelf.o" which includes
> > "libelf-temp.o" and for example "libelf-dominfo.o". So the only thing
> > preventing $(COV_FLAGS) from been added when building "libelf-tools.o"
> > for example is that we set `COV_FLAGS:=` for "libelf.o".
> 
> Yet doesn't that (again) mean things should actually work as-is, [...]

No, because I've explain how it should work, in the hypothetical world
where we have `targets += libelf-temp.o $(libelf-objs)`.

-- 
Anthony PERARD
Re: [PATCH] coverage: filter out lib{fdt,elf}-temp.o
Posted by Anthony PERARD 3 months, 1 week ago
On Mon, Jan 22, 2024 at 10:54:13AM +0000, Anthony PERARD wrote:
> On Mon, Jan 22, 2024 at 11:04:41AM +0100, Jan Beulich wrote:
> > On 19.01.2024 16:25, Anthony PERARD wrote:
> > > On Fri, Jan 19, 2024 at 09:43:30AM +0100, Michal Orzel wrote:
> > >> Is my understanding correct that by switching from extra-y to targets we are preventing these objects to
> > >> appear in non-init-objects (and thus having COV_FLAGS appended) while retaining the proper if_changed behavior?
> > >>
> > >> According to docs/misc/xen-makefiles/makefiles.rst:
> > >> Any target that utilises if_changed must be listed in $(targets),
> > >> otherwise the command line check will fail, and the target will
> > >> always be built.
> > > 
> > > Indeed, and $(extra-y) is added to $(targets) via
> > > $(targets-for-builtin).
> > > 
> > > While switching from $(extra-y) to $(targets) prevents the objects from
> > > been added to $(non-init-objets), it doesn't matter because "libelf.o"
> > > is in that variable, so $(COV_FLAGS) is added to $(_c_flags) and its
> > > value is used in all the prerequisites of "libelf.o" which includes
> > > "libelf-temp.o" and for example "libelf-dominfo.o". So the only thing
> > > preventing $(COV_FLAGS) from been added when building "libelf-tools.o"
> > > for example is that we set `COV_FLAGS:=` for "libelf.o".
> > 
> > Yet doesn't that (again) mean things should actually work as-is, [...]
> 
> No, because I've explain how it should work, in the hypothetical world
> where we have `targets += libelf-temp.o $(libelf-objs)`.

The problem is that there's currently two "paths" to build libelf-temp.o
(and even three I think for libelf-tools.o libelf-loader.o
libelf-dominfo.o):

Simplified makefile:

    obj-y := libelf.o
    extra-y := libelf-temp.o
    COV_FLAGS := -fcoverage

    __build: $(extra-y) built_in.o
    built_in.o: $(obj-y)
    libelf.o: COV_FLAGS :=
    libelf.o: libelf-temp.o

So, make can build "libelf-temp.o" as prerequisite of "__build" the
default target, or as prerequisite of "libelf.o".
In the first case, COV_FLAGS would have all the flags, and in the second
case, COV_FLAGS would be reset, but that second case is too late as make
is more likely to have already built libelf-temp.o with all the flags.

Cheers,

-- 
Anthony PERARD
Re: [PATCH] coverage: filter out lib{fdt,elf}-temp.o
Posted by Jan Beulich 3 months, 1 week ago
On 22.01.2024 12:05, Anthony PERARD wrote:
> On Mon, Jan 22, 2024 at 10:54:13AM +0000, Anthony PERARD wrote:
>> On Mon, Jan 22, 2024 at 11:04:41AM +0100, Jan Beulich wrote:
>>> On 19.01.2024 16:25, Anthony PERARD wrote:
>>>> On Fri, Jan 19, 2024 at 09:43:30AM +0100, Michal Orzel wrote:
>>>>> Is my understanding correct that by switching from extra-y to targets we are preventing these objects to
>>>>> appear in non-init-objects (and thus having COV_FLAGS appended) while retaining the proper if_changed behavior?
>>>>>
>>>>> According to docs/misc/xen-makefiles/makefiles.rst:
>>>>> Any target that utilises if_changed must be listed in $(targets),
>>>>> otherwise the command line check will fail, and the target will
>>>>> always be built.
>>>>
>>>> Indeed, and $(extra-y) is added to $(targets) via
>>>> $(targets-for-builtin).
>>>>
>>>> While switching from $(extra-y) to $(targets) prevents the objects from
>>>> been added to $(non-init-objets), it doesn't matter because "libelf.o"
>>>> is in that variable, so $(COV_FLAGS) is added to $(_c_flags) and its
>>>> value is used in all the prerequisites of "libelf.o" which includes
>>>> "libelf-temp.o" and for example "libelf-dominfo.o". So the only thing
>>>> preventing $(COV_FLAGS) from been added when building "libelf-tools.o"
>>>> for example is that we set `COV_FLAGS:=` for "libelf.o".
>>>
>>> Yet doesn't that (again) mean things should actually work as-is, [...]
>>
>> No, because I've explain how it should work, in the hypothetical world
>> where we have `targets += libelf-temp.o $(libelf-objs)`.
> 
> The problem is that there's currently two "paths" to build libelf-temp.o
> (and even three I think for libelf-tools.o libelf-loader.o
> libelf-dominfo.o):
> 
> Simplified makefile:
> 
>     obj-y := libelf.o
>     extra-y := libelf-temp.o
>     COV_FLAGS := -fcoverage
> 
>     __build: $(extra-y) built_in.o

Oh, okay - this is the piece I was missing. Thanks for the explanation.

Jan

>     built_in.o: $(obj-y)
>     libelf.o: COV_FLAGS :=
>     libelf.o: libelf-temp.o
> 
> So, make can build "libelf-temp.o" as prerequisite of "__build" the
> default target, or as prerequisite of "libelf.o".
> In the first case, COV_FLAGS would have all the flags, and in the second
> case, COV_FLAGS would be reset, but that second case is too late as make
> is more likely to have already built libelf-temp.o with all the flags.
> 
> Cheers,
>
Re: [PATCH] coverage: filter out lib{fdt,elf}-temp.o
Posted by Michal Orzel 3 months, 1 week ago

On 22/01/2024 12:05, Anthony PERARD wrote:
> 
> 
> On Mon, Jan 22, 2024 at 10:54:13AM +0000, Anthony PERARD wrote:
>> On Mon, Jan 22, 2024 at 11:04:41AM +0100, Jan Beulich wrote:
>>> On 19.01.2024 16:25, Anthony PERARD wrote:
>>>> On Fri, Jan 19, 2024 at 09:43:30AM +0100, Michal Orzel wrote:
>>>>> Is my understanding correct that by switching from extra-y to targets we are preventing these objects to
>>>>> appear in non-init-objects (and thus having COV_FLAGS appended) while retaining the proper if_changed behavior?
>>>>>
>>>>> According to docs/misc/xen-makefiles/makefiles.rst:
>>>>> Any target that utilises if_changed must be listed in $(targets),
>>>>> otherwise the command line check will fail, and the target will
>>>>> always be built.
>>>>
>>>> Indeed, and $(extra-y) is added to $(targets) via
>>>> $(targets-for-builtin).
>>>>
>>>> While switching from $(extra-y) to $(targets) prevents the objects from
>>>> been added to $(non-init-objets), it doesn't matter because "libelf.o"
>>>> is in that variable, so $(COV_FLAGS) is added to $(_c_flags) and its
>>>> value is used in all the prerequisites of "libelf.o" which includes
>>>> "libelf-temp.o" and for example "libelf-dominfo.o". So the only thing
>>>> preventing $(COV_FLAGS) from been added when building "libelf-tools.o"
>>>> for example is that we set `COV_FLAGS:=` for "libelf.o".
>>>
>>> Yet doesn't that (again) mean things should actually work as-is, [...]
>>
>> No, because I've explain how it should work, in the hypothetical world
>> where we have `targets += libelf-temp.o $(libelf-objs)`.
> 
> The problem is that there's currently two "paths" to build libelf-temp.o
> (and even three I think for libelf-tools.o libelf-loader.o
> libelf-dominfo.o):
> 
> Simplified makefile:
> 
>     obj-y := libelf.o
>     extra-y := libelf-temp.o
>     COV_FLAGS := -fcoverage
> 
>     __build: $(extra-y) built_in.o
>     built_in.o: $(obj-y)
>     libelf.o: COV_FLAGS :=
>     libelf.o: libelf-temp.o
> 
> So, make can build "libelf-temp.o" as prerequisite of "__build" the
> default target, or as prerequisite of "libelf.o".
> In the first case, COV_FLAGS would have all the flags, and in the second
> case, COV_FLAGS would be reset, but that second case is too late as make
> is more likely to have already built libelf-temp.o with all the flags.

Thanks for detailed explanation. I will follow your rationale in v2.

~Michal
Re: [PATCH] coverage: filter out lib{fdt,elf}-temp.o
Posted by Jan Beulich 3 months, 1 week ago
On 22.01.2024 11:54, Anthony PERARD wrote:
> On Mon, Jan 22, 2024 at 11:04:41AM +0100, Jan Beulich wrote:
>> On 19.01.2024 16:25, Anthony PERARD wrote:
>>> On Fri, Jan 19, 2024 at 09:43:30AM +0100, Michal Orzel wrote:
>>>> Is my understanding correct that by switching from extra-y to targets we are preventing these objects to
>>>> appear in non-init-objects (and thus having COV_FLAGS appended) while retaining the proper if_changed behavior?
>>>>
>>>> According to docs/misc/xen-makefiles/makefiles.rst:
>>>> Any target that utilises if_changed must be listed in $(targets),
>>>> otherwise the command line check will fail, and the target will
>>>> always be built.
>>>
>>> Indeed, and $(extra-y) is added to $(targets) via
>>> $(targets-for-builtin).
>>>
>>> While switching from $(extra-y) to $(targets) prevents the objects from
>>> been added to $(non-init-objets), it doesn't matter because "libelf.o"
>>> is in that variable, so $(COV_FLAGS) is added to $(_c_flags) and its
>>> value is used in all the prerequisites of "libelf.o" which includes
>>> "libelf-temp.o" and for example "libelf-dominfo.o". So the only thing
>>> preventing $(COV_FLAGS) from been added when building "libelf-tools.o"
>>> for example is that we set `COV_FLAGS:=` for "libelf.o".
>>
>> Yet doesn't that (again) mean things should actually work as-is, [...]
> 
> No, because I've explain how it should work, in the hypothetical world
> where we have `targets += libelf-temp.o $(libelf-objs)`.

Yes and no: Why would the COV_FLAGS propagation to prereqs not work today?
Whether libelf-*.o are prereqs to libelf-temp.o or to libelf.o shouldn't
matter, nor should it matter whether libelf-temp.o or libelf.o (or both)
are listed in $(targets). As soon as either libelf-temp.o or libelf.o has
COV_FLAGS overridden (to empty), libelf-*.o ought to be built with empty
COV_FLAGS. What am I missing?

Jan
Re: [PATCH] coverage: filter out lib{fdt,elf}-temp.o
Posted by Michal Orzel 3 months, 1 week ago
Hi Jan,

On 18/01/2024 14:12, Jan Beulich wrote:
> 
> 
> On 18.01.2024 13:06, Michal Orzel wrote:
>> At the moment, trying to run xencov read/reset (calling SYSCTL_coverage_op
>> under the hood) results in a crash. This is due to an attempt to
>> access code in the .init.* sections (libfdt for Arm and libelf for x86)
>> that are stripped after boot. Normally, the build system compiles any
>> *.init.o file without COV_FLAGS. However, these two libraries are
>> handled differently as sections will be renamed to init after linking.
>>
>> This worked until e321576f4047 ("xen/build: start using if_changed")
>> that added lib{fdt,elf}-temp.o to extra-y. Any file listed there without
>> *.init.o suffix will be part of non-init-objects for which COV_FLAGS
>> will be appended.
> 
> While this is true, aiui COV_FLAGS would be empty for anything listed
> in nocov-y and all of the prerequisites of those objects (iirc target-
> specific variable settings propagate to prerequisites). Therefore ...
I'm not sure about this propagation.

> 
>> In such case, the solution is to add a file to nocov-y.
> 
> ... libelf.o / libfdt.o already being listed there ought to suffice.
> Alternatively listing only libelf-temp.o / libfdt-temp.o ought to
> suffice as well.
> 
> Since you apparently observed things not working, I must be missing
> something.
As I wrote on Matrix, I'm not a build system expert so it might be that the issue
is due to something else. I managed to find a commit after which building the libfdt/libelf
with coverage enabled resulted in .gcno files being present. This commit added libfdt-temp.o
(same as libfdt.o but without sections renaming) to extra-y, hence my fix.

~Michal
Re: [PATCH] coverage: filter out lib{fdt,elf}-temp.o
Posted by Jan Beulich 3 months, 1 week ago
On 18.01.2024 15:40, Michal Orzel wrote:
> On 18/01/2024 14:12, Jan Beulich wrote:
>> On 18.01.2024 13:06, Michal Orzel wrote:
>>> At the moment, trying to run xencov read/reset (calling SYSCTL_coverage_op
>>> under the hood) results in a crash. This is due to an attempt to
>>> access code in the .init.* sections (libfdt for Arm and libelf for x86)
>>> that are stripped after boot. Normally, the build system compiles any
>>> *.init.o file without COV_FLAGS. However, these two libraries are
>>> handled differently as sections will be renamed to init after linking.
>>>
>>> This worked until e321576f4047 ("xen/build: start using if_changed")
>>> that added lib{fdt,elf}-temp.o to extra-y. Any file listed there without
>>> *.init.o suffix will be part of non-init-objects for which COV_FLAGS
>>> will be appended.
>>
>> While this is true, aiui COV_FLAGS would be empty for anything listed
>> in nocov-y and all of the prerequisites of those objects (iirc target-
>> specific variable settings propagate to prerequisites). Therefore ...
> I'm not sure about this propagation.
> 
>>
>>> In such case, the solution is to add a file to nocov-y.
>>
>> ... libelf.o / libfdt.o already being listed there ought to suffice.
>> Alternatively listing only libelf-temp.o / libfdt-temp.o ought to
>> suffice as well.
>>
>> Since you apparently observed things not working, I must be missing
>> something.
> As I wrote on Matrix, I'm not a build system expert so it might be that the issue
> is due to something else. I managed to find a commit after which building the libfdt/libelf
> with coverage enabled resulted in .gcno files being present. This commit added libfdt-temp.o
> (same as libfdt.o but without sections renaming) to extra-y, hence my fix.

You should probably have Cc-ed Anthony on the submission. Doing so now,
so at least he's aware (and may then also look at the patch).

Jan