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

[Ian Jackson <iwj@xxxxxxxxxxxxxx>]

Jan Beulich writes ("Re: [PATCH 2/2] hvmloader: do not include system headers 
for type declarations"):
> On 24.02.2021 11:26, Roger Pau Monne wrote:
> > --- /dev/null
> > +++ b/tools/firmware/hvmloader/types.h
> > @@ -0,0 +1,47 @@
> > +#ifndef _HVMLOADER_TYPES_H_
> > +#define _HVMLOADER_TYPES_H_
> > +
> > +typedef unsigned char uint8_t;
> > +typedef signed char int8_t;
> > +
> > +typedef unsigned short uint16_t;
> > +typedef signed short int16_t;
> > +
> > +typedef unsigned int uint32_t;
> > +typedef signed int int32_t;
> > +
> > +typedef unsigned long long uint64_t;
> > +typedef signed long long int64_t;
> 
> I wonder if we weren't better of not making assumptions on
> short / int / long long, and instead use
> __attribute__((__mode__(...))) or, if available, the compiler
> provided __{,U}INT*_TYPE__.

This code is only ever going to be for 32-bit x86, so I think the way
Roger did it is fine.

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.

> > +#define INT8_MIN        (-0x7f-1)
> > +#define INT16_MIN       (-0x7fff-1)
> > +#define INT32_MIN       (-0x7fffffff-1)
> > +#define INT64_MIN       (-0x7fffffffffffffffll-1)
> > +
> > +#define INT8_MAX        0x7f
> > +#define INT16_MAX       0x7fff
> > +#define INT32_MAX       0x7fffffff
> > +#define INT64_MAX       0x7fffffffffffffffll
> > +
> > +#define UINT8_MAX       0xff
> > +#define UINT16_MAX      0xffff
> > +#define UINT32_MAX      0xffffffffu
> > +#define UINT64_MAX      0xffffffffffffffffull
> 
> At least if going the above outlined route, I think we'd then
> also be better off not #define-ing any of these which we don't
> really use. Afaics it's really only UINTPTR_MAX which needs
> providing.

I disagree.  Providing the full set now gets them all properly
reviewe and reduces the burden on future work.

> > +typedef uint32_t size_t;

I would be inclined to provide ssize_t too but maybe hvmloader will
never need it.

> Like the hypervisor, we should prefer using __SIZE_TYPE__
> when available.

I disagree.

> > +typedef uint32_t uintptr_t;
> 
> Again - use __UINTPTR_TYPE__ or, like Xen,
> __attribute__((__mode__(__pointer__))).

I disagree.

> > +#define bool _Bool
> > +#define true 1
> > +#define false 0
> > +#define __bool_true_false_are_defined   1
> > +
> > +typedef __builtin_va_list va_list;
> > +#define va_copy(dest, src)    __builtin_va_copy((dest), (src))
> > +#define va_start(ap, last)    __builtin_va_start((ap), (last))
> 
> 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.

Ian.