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.1On 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
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
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
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
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
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
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
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
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, >
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
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
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
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
© 2016 - 2026 Red Hat, Inc.