* [PATCH] coverage: filter out lib{fdt,elf}-temp.o
@ 2024-01-18 12:06 Michal Orzel
2024-01-18 13:12 ` Jan Beulich
2024-01-18 13:42 ` Andrew Cooper
0 siblings, 2 replies; 14+ messages in thread
From: Michal Orzel @ 2024-01-18 12:06 UTC (permalink / raw)
To: xen-devel
Cc: Michal Orzel, Andrew Cooper, George Dunlap, Jan Beulich,
Julien Grall, Stefano Stabellini, Wei Liu, Javi Merino
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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] coverage: filter out lib{fdt,elf}-temp.o
2024-01-18 12:06 [PATCH] coverage: filter out lib{fdt,elf}-temp.o Michal Orzel
@ 2024-01-18 13:12 ` Jan Beulich
2024-01-18 14:40 ` Michal Orzel
2024-01-18 17:37 ` Anthony PERARD
2024-01-18 13:42 ` Andrew Cooper
1 sibling, 2 replies; 14+ messages in thread
From: Jan Beulich @ 2024-01-18 13:12 UTC (permalink / raw)
To: Michal Orzel
Cc: Andrew Cooper, George Dunlap, Julien Grall, Stefano Stabellini,
Wei Liu, Javi Merino, xen-devel
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] coverage: filter out lib{fdt,elf}-temp.o
2024-01-18 12:06 [PATCH] coverage: filter out lib{fdt,elf}-temp.o Michal Orzel
2024-01-18 13:12 ` Jan Beulich
@ 2024-01-18 13:42 ` Andrew Cooper
1 sibling, 0 replies; 14+ messages in thread
From: Andrew Cooper @ 2024-01-18 13:42 UTC (permalink / raw)
To: Michal Orzel, xen-devel
Cc: George Dunlap, Jan Beulich, Julien Grall, Stefano Stabellini,
Wei Liu, Javi Merino
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] coverage: filter out lib{fdt,elf}-temp.o
2024-01-18 13:12 ` Jan Beulich
@ 2024-01-18 14:40 ` Michal Orzel
2024-01-18 15:37 ` Jan Beulich
2024-01-18 17:37 ` Anthony PERARD
1 sibling, 1 reply; 14+ messages in thread
From: Michal Orzel @ 2024-01-18 14:40 UTC (permalink / raw)
To: Jan Beulich
Cc: Andrew Cooper, George Dunlap, Julien Grall, Stefano Stabellini,
Wei Liu, Javi Merino, xen-devel
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] coverage: filter out lib{fdt,elf}-temp.o
2024-01-18 14:40 ` Michal Orzel
@ 2024-01-18 15:37 ` Jan Beulich
0 siblings, 0 replies; 14+ messages in thread
From: Jan Beulich @ 2024-01-18 15:37 UTC (permalink / raw)
To: Michal Orzel
Cc: Andrew Cooper, George Dunlap, Julien Grall, Stefano Stabellini,
Wei Liu, Javi Merino, xen-devel, Anthony Perard
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] coverage: filter out lib{fdt,elf}-temp.o
2024-01-18 13:12 ` Jan Beulich
2024-01-18 14:40 ` Michal Orzel
@ 2024-01-18 17:37 ` Anthony PERARD
2024-01-19 8:43 ` Michal Orzel
1 sibling, 1 reply; 14+ messages in thread
From: Anthony PERARD @ 2024-01-18 17:37 UTC (permalink / raw)
To: Jan Beulich
Cc: Michal Orzel, Andrew Cooper, George Dunlap, Julien Grall,
Stefano Stabellini, Wei Liu, Javi Merino, xen-devel
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] coverage: filter out lib{fdt,elf}-temp.o
2024-01-18 17:37 ` Anthony PERARD
@ 2024-01-19 8:43 ` Michal Orzel
2024-01-19 15:25 ` Anthony PERARD
0 siblings, 1 reply; 14+ messages in thread
From: Michal Orzel @ 2024-01-19 8:43 UTC (permalink / raw)
To: Anthony PERARD, Jan Beulich
Cc: Andrew Cooper, George Dunlap, Julien Grall, Stefano Stabellini,
Wei Liu, Javi Merino, xen-devel
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] coverage: filter out lib{fdt,elf}-temp.o
2024-01-19 8:43 ` Michal Orzel
@ 2024-01-19 15:25 ` Anthony PERARD
2024-01-22 10:04 ` Jan Beulich
0 siblings, 1 reply; 14+ messages in thread
From: Anthony PERARD @ 2024-01-19 15:25 UTC (permalink / raw)
To: Michal Orzel
Cc: Jan Beulich, Andrew Cooper, George Dunlap, Julien Grall,
Stefano Stabellini, Wei Liu, Javi Merino, xen-devel
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] coverage: filter out lib{fdt,elf}-temp.o
2024-01-19 15:25 ` Anthony PERARD
@ 2024-01-22 10:04 ` Jan Beulich
2024-01-22 10:54 ` Anthony PERARD
0 siblings, 1 reply; 14+ messages in thread
From: Jan Beulich @ 2024-01-22 10:04 UTC (permalink / raw)
To: Anthony PERARD
Cc: Andrew Cooper, George Dunlap, Julien Grall, Stefano Stabellini,
Wei Liu, Javi Merino, xen-devel, Michal Orzel
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] coverage: filter out lib{fdt,elf}-temp.o
2024-01-22 10:04 ` Jan Beulich
@ 2024-01-22 10:54 ` Anthony PERARD
2024-01-22 11:03 ` Jan Beulich
2024-01-22 11:05 ` Anthony PERARD
0 siblings, 2 replies; 14+ messages in thread
From: Anthony PERARD @ 2024-01-22 10:54 UTC (permalink / raw)
To: Jan Beulich
Cc: Andrew Cooper, George Dunlap, Julien Grall, Stefano Stabellini,
Wei Liu, Javi Merino, xen-devel, Michal Orzel
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] coverage: filter out lib{fdt,elf}-temp.o
2024-01-22 10:54 ` Anthony PERARD
@ 2024-01-22 11:03 ` Jan Beulich
2024-01-22 11:05 ` Anthony PERARD
1 sibling, 0 replies; 14+ messages in thread
From: Jan Beulich @ 2024-01-22 11:03 UTC (permalink / raw)
To: Anthony PERARD
Cc: Andrew Cooper, George Dunlap, Julien Grall, Stefano Stabellini,
Wei Liu, Javi Merino, xen-devel, Michal Orzel
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] coverage: filter out lib{fdt,elf}-temp.o
2024-01-22 10:54 ` Anthony PERARD
2024-01-22 11:03 ` Jan Beulich
@ 2024-01-22 11:05 ` Anthony PERARD
2024-01-22 11:19 ` Michal Orzel
2024-01-22 11:27 ` Jan Beulich
1 sibling, 2 replies; 14+ messages in thread
From: Anthony PERARD @ 2024-01-22 11:05 UTC (permalink / raw)
To: Jan Beulich
Cc: Andrew Cooper, George Dunlap, Julien Grall, Stefano Stabellini,
Wei Liu, Javi Merino, xen-devel, Michal Orzel
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] coverage: filter out lib{fdt,elf}-temp.o
2024-01-22 11:05 ` Anthony PERARD
@ 2024-01-22 11:19 ` Michal Orzel
2024-01-22 11:27 ` Jan Beulich
1 sibling, 0 replies; 14+ messages in thread
From: Michal Orzel @ 2024-01-22 11:19 UTC (permalink / raw)
To: Anthony PERARD, Jan Beulich
Cc: Andrew Cooper, George Dunlap, Julien Grall, Stefano Stabellini,
Wei Liu, Javi Merino, xen-devel
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] coverage: filter out lib{fdt,elf}-temp.o
2024-01-22 11:05 ` Anthony PERARD
2024-01-22 11:19 ` Michal Orzel
@ 2024-01-22 11:27 ` Jan Beulich
1 sibling, 0 replies; 14+ messages in thread
From: Jan Beulich @ 2024-01-22 11:27 UTC (permalink / raw)
To: Anthony PERARD
Cc: Andrew Cooper, George Dunlap, Julien Grall, Stefano Stabellini,
Wei Liu, Javi Merino, xen-devel, Michal Orzel
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,
>
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2024-01-22 11:27 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-01-18 12:06 [PATCH] coverage: filter out lib{fdt,elf}-temp.o Michal Orzel
2024-01-18 13:12 ` Jan Beulich
2024-01-18 14:40 ` Michal Orzel
2024-01-18 15:37 ` Jan Beulich
2024-01-18 17:37 ` Anthony PERARD
2024-01-19 8:43 ` Michal Orzel
2024-01-19 15:25 ` Anthony PERARD
2024-01-22 10:04 ` Jan Beulich
2024-01-22 10:54 ` Anthony PERARD
2024-01-22 11:03 ` Jan Beulich
2024-01-22 11:05 ` Anthony PERARD
2024-01-22 11:19 ` Michal Orzel
2024-01-22 11:27 ` Jan Beulich
2024-01-18 13:42 ` Andrew Cooper
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.