| Message ID | 20230111181703.30991-1-anthony.perard@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] build: include/compat: figure out which other compat headers are needed | expand |
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
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
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
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
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,
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
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
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
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
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(+)