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

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

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@xxxxxxxxxx>
> ---
>  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