Re: [PATCH 2/2] hvmloader: do not include system headers for type declarations [and 1 more messages]

[Ian Jackson <iwj@xxxxxxxxxxxxxx>]

Andrew Cooper writes ("Re: [PATCH 2/2] hvmloader: do not include system headers 
for type declarations"):
> At what point do we just declare Alpine broken?  These are all
> freestanding headers, an explicitly available for use, in the way we use
> them.

There is IMO nothing wrong with Alpine here.  Alpine amd64 simply does
not support compilation of 32-bit x86 userland binaries.

But that's OK for us.  Xen doe not require the execution of any 32-bit
userland binaries.  hvmloader is not a userland binary.

As Roger said on irc

13:35 <royger> but requiring a compiler that supports generating
               i386 code doens't imply that we also have a libc for it?
               
> There are substantial portability costs for making changes like this,
> which takes us from standards compliant C to GCC-isms-only.

Since we are defining our out standalone environment for hvmloader, we
are in the position of the C *implementor*.  Compilers have features
(like __builtin_va*) that are helpful for implementing standard C
features like stdarg.h and indeed stdint.h.

Or to put it another way, GCC does not, by itself, provide (in
Standard C terms) a "freestanding implementation".  Arguably GCC ought
to provide stdint.h et al but in practice it doing so causes more
trouble as it gets in the way of the implentors of hosted
implementations.

The conclusion is simply that we must provide for ourselves any
apsects of a "freestanding implementation" that we care about.  (And
then we get to decide for ourselves how much the internal API should
look like Standard C.  Defining the Standard C type names is
definitely IMO advisable as it makes the bulk of the code sensible to
read.)

Jan Beulich writes ("Re: [PATCH 2/2] hvmloader: do not include system headers 
for type declarations"):
> On 24.02.2021 12:07, Ian Jackson wrote:
> > This code is only ever going to be for 32-bit x86, so I think the way
> > Roger did it is fine.
> 
> It is technically correct at this point in time, from all we can
> tell. I can't see any reason though why a compiler might not
> support wider int or, in particular, long long.

Our requirement for hvmloader is that we have an ILP32 LL64 compiler
which generates 32-bit x86 machine code.  That is what "gcc -m32"
means.  Whether future compiler(s) might exist which can provide ILP32
LLP64 (and what type uint64_t is on such a compiler) is not relevant.

> > Doing it the other way, to cope with this file being used with
> > compiler settings where the above set of types is wrong, would also
> > imply more complex definitions of INT32_MIN et al.
> 
> Well, that's only as far as the use of number suffixes goes. The
> values used won't change, as these constants describe fixed width
> types.

So the definitions would need to contain casts.

> >> Like the hypervisor, we should prefer using __SIZE_TYPE__
> >> when available.
> > 
> > I disagree.
> 
> May I ask why? There is a reason providing of these types did get
> added to (at least) gcc.

__SIZE_TYPE__ is provided by the compiler to the libc implementor.  It
is one of those facilities like __builtin_va*.  The bulk of the code
in hvmloader should not use this kind of thing.  It should use plain
size_t.

As for the new header in hvmloader, it does not matter whether it uses
__SIZE_TYPE__ or some other type which is known to be 32-bit, since
this code is definitely only ever for 32-bit x86.

> One argument against this would be above mentioned independence
> on any ABI the compiler would be built for, but I'd buy that only
> if above we indeed used __attribute__((__mode__())), as that's
> the only way to achieve such independence.

We don't want or need to support building hvmloader with a differnet
ABI.

> >> Nit: Perhaps better omit the unnecessary inner parentheses?
> > 
> > We should definitely keep the inner parentheses.  I don't want to
> > start carefully reasoning about precisely which inner parentheses are
> > necesary for macro argument parsing correctness.
> 
> Can you give me an example of when the inner parentheses would be
> needed? I don't think they're needed no matter whether (taking the
> example here) __builtin_va_...() were functions or macros. They
> would of course be needed if the identifiers were part of
> expressions beyond the mere function invocation.

You mention the situation where the parentheses would be needed
yourself.

> We've been trying to eliminate such in the hypervisor part of the
> tree,

Really ?  Well I think that is in exceedingly poor taste.  I can't
claim that it's objectively wrong and this is a question of style so I
won't insist on it.

> The primary reason why I've been advocating to avoid them is that,
> as long as they're not needed for anything, they harm readability
> and increase the risk of mistakes like the one that had led to
> XSA-316.

I looked again at XSA-316.  I don't want to open another can of worms.
It will suffice to say that I don't share your view on the root cause.

Ian.