build: include/compat: figure out which other compat headers are needed

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

Posted by Anthony PERARD 1 year, 3 months ago

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: [RFC PATCH] build: include/compat: figure out which other compat headers are needed

Posted by Jan Beulich 1 year, 3 months ago

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

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

Posted by Andrew Cooper 1 year, 3 months ago

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