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

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

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