[RFC PATCH] build: include/compat: figure out which other compat headers are needed

[RFC PATCH] build: include/compat: figure out which other compat headers are needed

[Anthony PERARD]

Some compat headers depends on other compat headers that may not have
been generated due to config option.

This would be a generic way to deal with deps, instead of
    headers-$(call or $(CONFIG_TRACEBUFFER),$(CONFIG_HVM)) += compat/trace.h

This is just an RFC, as it only deals with "hvm_op.h" and nothing is
done to have make regenerate the new file when needed.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 xen/include/Makefile | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/xen/include/Makefile b/xen/include/Makefile
index 65be310eca..5e6de97841 100644
--- a/xen/include/Makefile
+++ b/xen/include/Makefile
@@ -34,6 +34,29 @@ headers-$(CONFIG_TRACEBUFFER) += compat/trace.h
 headers-$(CONFIG_XENOPROF) += compat/xenoprof.h
 headers-$(CONFIG_XSM_FLASK) += compat/xsm/flask_op.h
 
+# Find dependencies of compat headers.
+# e.g. hvm/hvm_op.h needs trace.h; but if CONFIG_TRACEBUFFER=n, then trace.h would be missing.
+#
+# Using sed to remove ".." from path because unsure if something else is available
+# There's `realpath`, but maynot be available
+#	realpath --relative-to=. -mL compat/hvm/../trace.h -> compat/trace.h
+# `make` also have macro for that $(abspath), only recent version.
+#
+# The $(CC) line to gen deps is derived from $(cmd_compat_i)
+include $(obj)/.compat-header-deps.d
+$(obj)/.compat-header-deps.d: include/public/hvm/hvm_op.h
+	$(CC) -MM -MF $@.tmp $(filter-out -Wa$(comma)% -include %/include/xen/config.h,$(XEN_CFLAGS)) $<
+	for f in $$(cat $@.tmp | sed -r '1s/^[^:]*: //; s/ \\$$//'); do \
+	    echo $$f; \
+	done | sed -r \
+	    -e 's#.*/public#compat#; : p; s#/[^/]+/../#/#; t p; s#$$# \\#' \
+	    -e '1i headers-y-deps := \\' -e '$$a \ ' \
+	    > $@
+
+headers-y-deps := $(filter-out compat/xen-compat.h,$(headers-y-deps))
+# Add compat header dependencies and eliminates duplicates
+headers-y := $(sort $(headers-y) $(headers-y-deps))
+
 cppflags-y                := -include public/xen-compat.h -DXEN_GENERATING_COMPAT_HEADERS
 cppflags-$(CONFIG_X86)    += -m32
 
-- 
Anthony PERARD

Re:[Multiple reverts] [RFC PATCH] build: include/compat: figure out which other compat headers are needed

[Andrew Cooper]

On 11/01/2023 6:17 pm, Anthony PERARD wrote:
> Some compat headers depends on other compat headers that may not have
> been generated due to config option.
>
> This would be a generic way to deal with deps, instead of
>     headers-$(call or $(CONFIG_TRACEBUFFER),$(CONFIG_HVM)) += compat/trace.h
>
> This is just an RFC, as it only deals with "hvm_op.h" and nothing is
> done to have make regenerate the new file when needed.
>
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
>  xen/include/Makefile | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
>
> diff --git a/xen/include/Makefile b/xen/include/Makefile
> index 65be310eca..5e6de97841 100644
> --- a/xen/include/Makefile
> +++ b/xen/include/Makefile
> @@ -34,6 +34,29 @@ headers-$(CONFIG_TRACEBUFFER) += compat/trace.h
>  headers-$(CONFIG_XENOPROF) += compat/xenoprof.h
>  headers-$(CONFIG_XSM_FLASK) += compat/xsm/flask_op.h
>  
> +# Find dependencies of compat headers.
> +# e.g. hvm/hvm_op.h needs trace.h; but if CONFIG_TRACEBUFFER=n, then trace.h would be missing.
> +#
> +# Using sed to remove ".." from path because unsure if something else is available
> +# There's `realpath`, but maynot be available
> +#	realpath --relative-to=. -mL compat/hvm/../trace.h -> compat/trace.h
> +# `make` also have macro for that $(abspath), only recent version.
> +#
> +# The $(CC) line to gen deps is derived from $(cmd_compat_i)
> +include $(obj)/.compat-header-deps.d
> +$(obj)/.compat-header-deps.d: include/public/hvm/hvm_op.h
> +	$(CC) -MM -MF $@.tmp $(filter-out -Wa$(comma)% -include %/include/xen/config.h,$(XEN_CFLAGS)) $<
> +	for f in $$(cat $@.tmp | sed -r '1s/^[^:]*: //; s/ \\$$//'); do \
> +	    echo $$f; \
> +	done | sed -r \
> +	    -e 's#.*/public#compat#; : p; s#/[^/]+/../#/#; t p; s#$$# \\#' \
> +	    -e '1i headers-y-deps := \\' -e '$$a \ ' \
> +	    > $@
> +
> +headers-y-deps := $(filter-out compat/xen-compat.h,$(headers-y-deps))
> +# Add compat header dependencies and eliminates duplicates
> +headers-y := $(sort $(headers-y) $(headers-y-deps))
> +
>  cppflags-y                := -include public/xen-compat.h -DXEN_GENERATING_COMPAT_HEADERS
>  cppflags-$(CONFIG_X86)    += -m32
>  

For posterity,
https://gitlab.com/xen-project/people/andyhhp/xen/-/jobs/3585379553 is
the issue in question.

In file included from arch/x86/hvm/hvm.c:82:
./include/compat/hvm/hvm_op.h:6:10: fatal error: ../trace.h: No such
file or directory
    6 | #include "../trace.h"
      |          ^~~~~~~~~~~~
compilation terminated.
make[4]: *** [Rules.mk:246: arch/x86/hvm/hvm.o] Error 1
make[3]: *** [Rules.mk:320: arch/x86/hvm] Error 2
make[3]: *** Waiting for unfinished jobs....


All public headers use "../" relative includes for traversing the
public/ hierarchy.  This cannot feasibly change given our "copy this
into your project" stance, but it means the compat headers have the same
structure under compat/.

This include is supposed to be including compat/trace.h but it was
actually picking up x86's asm/trace.h, hence the build failure now that
I've deleted the file.

This demonstrates that trying to be clever with -iquote is a mistake. 
Nothing good can possibly come of having the <> and "" include paths
being different.  Therefore we must revert all uses of -iquote.


But, that isn't the only bug.

The real hvm_op.h legitimately includes the real trace.h, therefore the
compat hvm_op.h legitimately includes the compat trace.h too.  But
generation of compat trace.h was made asymmetric because of 2c8fabb223.

In hindsight, that's a public ABI breakage.  The current configuration
of this build of the hypervisor has no legitimate bearing on the headers
needing to be installed to /usr/include/xen.

Or put another way, it is a breakage to require Xen to have
CONFIG_COMPAT+CONFIG_TRACEBUFFER enabled in the build simply to get the
public API headers generated properly.

So 2c8fabb223 needs reverting too.

~Andrew

Re: [Multiple reverts] [RFC PATCH] build: include/compat: figure out which other compat headers are needed

[Jan Beulich]

On 11.01.2023 23:29, Andrew Cooper wrote:
> For posterity,
> https://gitlab.com/xen-project/people/andyhhp/xen/-/jobs/3585379553 is
> the issue in question.
> 
> In file included from arch/x86/hvm/hvm.c:82:
> ./include/compat/hvm/hvm_op.h:6:10: fatal error: ../trace.h: No such
> file or directory
>     6 | #include "../trace.h"
>       |          ^~~~~~~~~~~~
> compilation terminated.
> make[4]: *** [Rules.mk:246: arch/x86/hvm/hvm.o] Error 1
> make[3]: *** [Rules.mk:320: arch/x86/hvm] Error 2
> make[3]: *** Waiting for unfinished jobs....
> 
> 
> All public headers use "../" relative includes for traversing the
> public/ hierarchy.  This cannot feasibly change given our "copy this
> into your project" stance, but it means the compat headers have the same
> structure under compat/.
> 
> This include is supposed to be including compat/trace.h but it was
> actually picking up x86's asm/trace.h, hence the build failure now that
> I've deleted the file.
> 
> This demonstrates that trying to be clever with -iquote is a mistake. 
> Nothing good can possibly come of having the <> and "" include paths
> being different.  Therefore we must revert all uses of -iquote.

I'm afraid I can't see the connection between use of -iquote and the bug
here.

> But, that isn't the only bug.
> 
> The real hvm_op.h legitimately includes the real trace.h, therefore the
> compat hvm_op.h legitimately includes the compat trace.h too.  But
> generation of compat trace.h was made asymmetric because of 2c8fabb223.
> 
> In hindsight, that's a public ABI breakage.  The current configuration
> of this build of the hypervisor has no legitimate bearing on the headers
> needing to be installed to /usr/include/xen.
> 
> Or put another way, it is a breakage to require Xen to have
> CONFIG_COMPAT+CONFIG_TRACEBUFFER enabled in the build simply to get the
> public API headers generated properly.

There are no public API headers which are generated. The compat headers
are generate solely for Xen's internal purposes (and hence there's also
no public ABI breakage). Since generation is slow, avoiding to generate
ones not needed during the build is helpful.

Jan

Re: [Multiple reverts] [RFC PATCH] build: include/compat: figure out which other compat headers are needed

[Jan Beulich]

On 12.01.2023 08:46, Jan Beulich wrote:
> On 11.01.2023 23:29, Andrew Cooper wrote:
>> For posterity,
>> https://gitlab.com/xen-project/people/andyhhp/xen/-/jobs/3585379553 is
>> the issue in question.
>>
>> In file included from arch/x86/hvm/hvm.c:82:
>> ./include/compat/hvm/hvm_op.h:6:10: fatal error: ../trace.h: No such
>> file or directory
>>     6 | #include "../trace.h"
>>       |          ^~~~~~~~~~~~
>> compilation terminated.
>> make[4]: *** [Rules.mk:246: arch/x86/hvm/hvm.o] Error 1
>> make[3]: *** [Rules.mk:320: arch/x86/hvm] Error 2
>> make[3]: *** Waiting for unfinished jobs....
>>
>>
>> All public headers use "../" relative includes for traversing the
>> public/ hierarchy.  This cannot feasibly change given our "copy this
>> into your project" stance, but it means the compat headers have the same
>> structure under compat/.
>>
>> This include is supposed to be including compat/trace.h but it was
>> actually picking up x86's asm/trace.h, hence the build failure now that
>> I've deleted the file.
>>
>> This demonstrates that trying to be clever with -iquote is a mistake. 
>> Nothing good can possibly come of having the <> and "" include paths
>> being different.  Therefore we must revert all uses of -iquote.
> 
> I'm afraid I can't see the connection between use of -iquote and the bug
> here.

In fact I think the issue was caused by

CFLAGS += -I$(srctree)/arch/x86/include/asm/mach-generic
CFLAGS += -I$(srctree)/arch/x86/include/asm/mach-default

which allowed the compiler when seeing "../trace.h" to pick up
arch/x86/include/asm/trace.h. No -iquote in sight here; all that
happens is that #include "..." fall back to using -I specified
paths when the file could not be found by using ""-only paths.

Jan

Re: [Multiple reverts] [RFC PATCH] build: include/compat: figure out which other compat headers are needed

[Anthony PERARD]

On Thu, Jan 12, 2023 at 08:46:23AM +0100, Jan Beulich wrote:
> On 11.01.2023 23:29, Andrew Cooper wrote:
> > In file included from arch/x86/hvm/hvm.c:82:
> > ./include/compat/hvm/hvm_op.h:6:10: fatal error: ../trace.h: No such
> > file or directory
> >     6 | #include "../trace.h"
> >       |          ^~~~~~~~~~~~
> > compilation terminated.
> > make[4]: *** [Rules.mk:246: arch/x86/hvm/hvm.o] Error 1
> > make[3]: *** [Rules.mk:320: arch/x86/hvm] Error 2
> > make[3]: *** Waiting for unfinished jobs....
> > 
> > 
> > All public headers use "../" relative includes for traversing the
> > public/ hierarchy.  This cannot feasibly change given our "copy this
> > into your project" stance, but it means the compat headers have the same
> > structure under compat/.
> > 
> > This include is supposed to be including compat/trace.h but it was
> > actually picking up x86's asm/trace.h, hence the build failure now that
> > I've deleted the file.
> > 
> > This demonstrates that trying to be clever with -iquote is a mistake. 
> > Nothing good can possibly come of having the <> and "" include paths
> > being different.  Therefore we must revert all uses of -iquote.
> 
> I'm afraid I can't see the connection between use of -iquote and the bug
> here.

Me neither, -iquote isn't used on that object's command line.

> > But, that isn't the only bug.
> > 
> > The real hvm_op.h legitimately includes the real trace.h, therefore the
> > compat hvm_op.h legitimately includes the compat trace.h too.  But
> > generation of compat trace.h was made asymmetric because of 2c8fabb223.
> > 
> > In hindsight, that's a public ABI breakage.  The current configuration
> > of this build of the hypervisor has no legitimate bearing on the headers
> > needing to be installed to /usr/include/xen.
> > 
> > Or put another way, it is a breakage to require Xen to have
> > CONFIG_COMPAT+CONFIG_TRACEBUFFER enabled in the build simply to get the
> > public API headers generated properly.
> 
> There are no public API headers which are generated. The compat headers
> are generate solely for Xen's internal purposes (and hence there's also
> no public ABI breakage). Since generation is slow, avoiding to generate
> ones not needed during the build is helpful.

If only we could do the generation faster:
    https://lore.kernel.org/xen-devel/20220614162248.40278-5-anthony.perard@citrix.com/
    patch which takes care of the slower part of the generation (slower
    at least for some compat headers).

Cheers,

-- 
Anthony PERARD

Re: [Multiple reverts] [RFC PATCH] build: include/compat: figure out which other compat headers are needed

[Jan Beulich]

On 12.01.2023 10:27, Anthony PERARD wrote:
> On Thu, Jan 12, 2023 at 08:46:23AM +0100, Jan Beulich wrote:
>> On 11.01.2023 23:29, Andrew Cooper wrote:
>>> The real hvm_op.h legitimately includes the real trace.h, therefore the
>>> compat hvm_op.h legitimately includes the compat trace.h too.  But
>>> generation of compat trace.h was made asymmetric because of 2c8fabb223.
>>>
>>> In hindsight, that's a public ABI breakage.  The current configuration
>>> of this build of the hypervisor has no legitimate bearing on the headers
>>> needing to be installed to /usr/include/xen.
>>>
>>> Or put another way, it is a breakage to require Xen to have
>>> CONFIG_COMPAT+CONFIG_TRACEBUFFER enabled in the build simply to get the
>>> public API headers generated properly.
>>
>> There are no public API headers which are generated. The compat headers
>> are generate solely for Xen's internal purposes (and hence there's also
>> no public ABI breakage). Since generation is slow, avoiding to generate
>> ones not needed during the build is helpful.
> 
> If only we could do the generation faster:
>     https://lore.kernel.org/xen-devel/20220614162248.40278-5-anthony.perard@citrix.com/
>     patch which takes care of the slower part of the generation (slower
>     at least for some compat headers).

Right, and I still have this in my folder waiting for a review (by someone
knowing Perl better than e.g. I do). Maybe you want to put on the agenda
of today's community call an item to see whether we can nominate someone
with enough Perl knowledge?

Jan

Re: [Multiple reverts] [RFC PATCH] build: include/compat: figure out which other compat headers are needed

[Andrew Cooper]

On 12/01/2023 7:46 am, Jan Beulich wrote:
> On 11.01.2023 23:29, Andrew Cooper wrote:
>> For posterity,
>> https://gitlab.com/xen-project/people/andyhhp/xen/-/jobs/3585379553 is
>> the issue in question.
>>
>> In file included from arch/x86/hvm/hvm.c:82:
>> ./include/compat/hvm/hvm_op.h:6:10: fatal error: ../trace.h: No such
>> file or directory
>>     6 | #include "../trace.h"
>>       |          ^~~~~~~~~~~~
>> compilation terminated.
>> make[4]: *** [Rules.mk:246: arch/x86/hvm/hvm.o] Error 1
>> make[3]: *** [Rules.mk:320: arch/x86/hvm] Error 2
>> make[3]: *** Waiting for unfinished jobs....
>>
>>
>> All public headers use "../" relative includes for traversing the
>> public/ hierarchy.  This cannot feasibly change given our "copy this
>> into your project" stance, but it means the compat headers have the same
>> structure under compat/.
>>
>> This include is supposed to be including compat/trace.h but it was
>> actually picking up x86's asm/trace.h, hence the build failure now that
>> I've deleted the file.
>>
>> This demonstrates that trying to be clever with -iquote is a mistake. 
>> Nothing good can possibly come of having the <> and "" include paths
>> being different.  Therefore we must revert all uses of -iquote.
> I'm afraid I can't see the connection between use of -iquote and the bug
> here.

So I had concluded (wrongly) that it was to do with an asymmetry of
include paths, but it's not.  <../$x> would behave the same, even if it
is a bit more obviously wrong.

The actual problem is the use of ./ or ../ because, despite how they
read, they are never relative to the current file.  The contents between
the "" or <> is treated as a string literal and not interpreted by CPP.

So furthermore, the public headers are buggy in their use of ../ because
it is an implicit dependency on -I/path/to/xen/headers/dir/ being
earlier on the include path than other dirs with these fairly generic
and not-xen-prefixed file names.

I still think that include path asymmetry is bad idea and wants
reverting, but I agree it's not the source of this bug.

>> But, that isn't the only bug.
>>
>> The real hvm_op.h legitimately includes the real trace.h, therefore the
>> compat hvm_op.h legitimately includes the compat trace.h too.  But
>> generation of compat trace.h was made asymmetric because of 2c8fabb223.
>>
>> In hindsight, that's a public ABI breakage.  The current configuration
>> of this build of the hypervisor has no legitimate bearing on the headers
>> needing to be installed to /usr/include/xen.
>>
>> Or put another way, it is a breakage to require Xen to have
>> CONFIG_COMPAT+CONFIG_TRACEBUFFER enabled in the build simply to get the
>> public API headers generated properly.
> There are no public API headers which are generated. The compat headers
> are generate solely for Xen's internal purposes (and hence there's also
> no public ABI breakage).

Wow, I really was decaffeinated when working on this...

Yeah, it's not that either.

~Andrew

Re: [Multiple reverts] [RFC PATCH] build: include/compat: figure out which other compat headers are needed

[Jan Beulich]

On 12.01.2023 12:02, Andrew Cooper wrote:
> On 12/01/2023 7:46 am, Jan Beulich wrote:
>> On 11.01.2023 23:29, Andrew Cooper wrote:
>>> For posterity,
>>> https://gitlab.com/xen-project/people/andyhhp/xen/-/jobs/3585379553 is
>>> the issue in question.
>>>
>>> In file included from arch/x86/hvm/hvm.c:82:
>>> ./include/compat/hvm/hvm_op.h:6:10: fatal error: ../trace.h: No such
>>> file or directory
>>>     6 | #include "../trace.h"
>>>       |          ^~~~~~~~~~~~
>>> compilation terminated.
>>> make[4]: *** [Rules.mk:246: arch/x86/hvm/hvm.o] Error 1
>>> make[3]: *** [Rules.mk:320: arch/x86/hvm] Error 2
>>> make[3]: *** Waiting for unfinished jobs....
>>>
>>>
>>> All public headers use "../" relative includes for traversing the
>>> public/ hierarchy.  This cannot feasibly change given our "copy this
>>> into your project" stance, but it means the compat headers have the same
>>> structure under compat/.
>>>
>>> This include is supposed to be including compat/trace.h but it was
>>> actually picking up x86's asm/trace.h, hence the build failure now that
>>> I've deleted the file.
>>>
>>> This demonstrates that trying to be clever with -iquote is a mistake. 
>>> Nothing good can possibly come of having the <> and "" include paths
>>> being different.  Therefore we must revert all uses of -iquote.
>> I'm afraid I can't see the connection between use of -iquote and the bug
>> here.
> 
> So I had concluded (wrongly) that it was to do with an asymmetry of
> include paths, but it's not.  <../$x> would behave the same, even if it
> is a bit more obviously wrong.
> 
> The actual problem is the use of ./ or ../ because, despite how they
> read, they are never relative to the current file.  The contents between
> the "" or <> is treated as a string literal and not interpreted by CPP.

First of all the C spec says nothing about how searching is performed.
It's all implementation defined.

Gcc documentation in turn is quite explicit: "By default, the
preprocessor looks for header files included by the quote form of the
directive #include "file" first relative to the directory of the
current file, and then ..." This is behavior I know from all compilers
I've ever worked with, so while not part of the C standard it is
(hopefully) something we can rely upon (or specify as a requirement
for people wanting to use the headers unmodified).

So I agree using ../ inside angle backets would be bogus at best, but
I think using such inside double quotes is sufficient generically okay.
Hence ...

> So furthermore, the public headers are buggy in their use of ../ because
> it is an implicit dependency on -I/path/to/xen/headers/dir/ being
> earlier on the include path than other dirs with these fairly generic
> and not-xen-prefixed file names.

... I don't see any bugginess here.

Jan

Re: [RFC PATCH] build: include/compat: figure out which other compat headers are needed

[Jan Beulich]

On 11.01.2023 19:17, Anthony PERARD wrote:
> Some compat headers depends on other compat headers that may not have
> been generated due to config option.
> 
> This would be a generic way to deal with deps, instead of
>     headers-$(call or $(CONFIG_TRACEBUFFER),$(CONFIG_HVM)) += compat/trace.h

But it would generate dependency headers even if there's only a fake dependency,
like is specifically the case for hvm_op.h vs trace.h (the compat header only
really needs public/trace.h, which it gets from the inclusion of the original
hvm_op.h). Avoiding the generation of unnecessary compat headers is solely to
speed up the build. If that wasn't an issue, I'd say we simply generate all
headers at al times. In particular ...

> --- a/xen/include/Makefile
> +++ b/xen/include/Makefile
> @@ -34,6 +34,29 @@ headers-$(CONFIG_TRACEBUFFER) += compat/trace.h
>  headers-$(CONFIG_XENOPROF) += compat/xenoprof.h
>  headers-$(CONFIG_XSM_FLASK) += compat/xsm/flask_op.h
>  
> +# Find dependencies of compat headers.
> +# e.g. hvm/hvm_op.h needs trace.h; but if CONFIG_TRACEBUFFER=n, then trace.h would be missing.
> +#
> +# Using sed to remove ".." from path because unsure if something else is available
> +# There's `realpath`, but maynot be available
> +#	realpath --relative-to=. -mL compat/hvm/../trace.h -> compat/trace.h
> +# `make` also have macro for that $(abspath), only recent version.
> +#
> +# The $(CC) line to gen deps is derived from $(cmd_compat_i)
> +include $(obj)/.compat-header-deps.d
> +$(obj)/.compat-header-deps.d: include/public/hvm/hvm_op.h
> +	$(CC) -MM -MF $@.tmp $(filter-out -Wa$(comma)% -include %/include/xen/config.h,$(XEN_CFLAGS)) $<

... this removal of the config.h inclusion is to avoid introducing any
dependencies on CONFIG_* in the public headers (of course we'd expect such
to be caught during review).

I'll try my alternative approach next, and post a patch if successful. I am,
however, aware that this also won't deal with all theoretically possible
cases; I think though that the remaining cases might then better be dealt
with by manually recorded dependencies (kind of along the lines of your

headers-$(call or $(CONFIG_TRACEBUFFER),$(CONFIG_HVM)) += compat/trace.h

in the description).

> +	for f in $$(cat $@.tmp | sed -r '1s/^[^:]*: //; s/ \\$$//'); do \

I'm curious: Why "cat" instead of passing the file as argument to "sed"?

Jan