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
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
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 - 2024 Red Hat, Inc.