[PATCH 0/2] hvmloader: drop usage of system headers

[PATCH 0/2] hvmloader: drop usage of system headers

[Roger Pau Monne]

Hello,

Following two patches aim to make hvmloader standalone, so that it don't
try to use system headers. It shouldn't result in any functional
change.

Thanks, Roger.

Roger Pau Monne (2):
  hvmloader: use Xen private header for elf structs
  hvmloader: do not include system headers for type declarations

 tools/firmware/hvmloader/32bitbios_support.c |  4 +-
 tools/firmware/hvmloader/config.h            |  3 +-
 tools/firmware/hvmloader/hypercall.h         |  2 +-
 tools/firmware/hvmloader/mp_tables.c         |  2 +-
 tools/firmware/hvmloader/option_rom.h        |  2 +-
 tools/firmware/hvmloader/pir_types.h         |  2 +-
 tools/firmware/hvmloader/smbios.c            |  2 +-
 tools/firmware/hvmloader/smbios_types.h      |  2 +-
 tools/firmware/hvmloader/types.h             | 47 ++++++++++++++++++++
 tools/firmware/hvmloader/util.c              |  1 -
 tools/firmware/hvmloader/util.h              |  5 +--
 11 files changed, 57 insertions(+), 15 deletions(-)
 create mode 100644 tools/firmware/hvmloader/types.h

-- 
2.30.1

[PATCH 1/2] hvmloader: use Xen private header for elf structs

[Roger Pau Monne]

Do not use the system provided elf.h, and instead use elfstructs.h
from libelf.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 tools/firmware/hvmloader/32bitbios_support.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/firmware/hvmloader/32bitbios_support.c b/tools/firmware/hvmloader/32bitbios_support.c
index 114135022e..e726946a7b 100644
--- a/tools/firmware/hvmloader/32bitbios_support.c
+++ b/tools/firmware/hvmloader/32bitbios_support.c
@@ -21,7 +21,7 @@
  */
 
 #include <inttypes.h>
-#include <elf.h>
+#include <xen/libelf/elfstructs.h>
 #ifdef __sun__
 #include <sys/machelf.h>
 #endif
-- 
2.30.1

Re: [PATCH 1/2] hvmloader: use Xen private header for elf structs

[Jan Beulich]

On 24.02.2021 11:26, Roger Pau Monne wrote:
> Do not use the system provided elf.h, and instead use elfstructs.h
> from libelf.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Acked-by: Jan Beulich <jbeulich@suse.com>

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

[Roger Pau Monne]

Instead create a private types.h header that contains the set of types
that are required by hvmloader. Replace include occurrences of std*
headers with type.h. Note that including types.h directly is not
required in util.c because it already includes util.h which in turn
includes the newly created types.h.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 tools/firmware/hvmloader/32bitbios_support.c |  2 +-
 tools/firmware/hvmloader/config.h            |  3 +-
 tools/firmware/hvmloader/hypercall.h         |  2 +-
 tools/firmware/hvmloader/mp_tables.c         |  2 +-
 tools/firmware/hvmloader/option_rom.h        |  2 +-
 tools/firmware/hvmloader/pir_types.h         |  2 +-
 tools/firmware/hvmloader/smbios.c            |  2 +-
 tools/firmware/hvmloader/smbios_types.h      |  2 +-
 tools/firmware/hvmloader/types.h             | 47 ++++++++++++++++++++
 tools/firmware/hvmloader/util.c              |  1 -
 tools/firmware/hvmloader/util.h              |  5 +--
 11 files changed, 56 insertions(+), 14 deletions(-)
 create mode 100644 tools/firmware/hvmloader/types.h

diff --git a/tools/firmware/hvmloader/32bitbios_support.c b/tools/firmware/hvmloader/32bitbios_support.c
index e726946a7b..32b5c4c4ad 100644
--- a/tools/firmware/hvmloader/32bitbios_support.c
+++ b/tools/firmware/hvmloader/32bitbios_support.c
@@ -20,7 +20,7 @@
  * this program; If not, see <http://www.gnu.org/licenses/>.
  */
 
-#include <inttypes.h>
+#include "types.h"
 #include <xen/libelf/elfstructs.h>
 #ifdef __sun__
 #include <sys/machelf.h>
diff --git a/tools/firmware/hvmloader/config.h b/tools/firmware/hvmloader/config.h
index 844120bc87..510d5b5c79 100644
--- a/tools/firmware/hvmloader/config.h
+++ b/tools/firmware/hvmloader/config.h
@@ -1,8 +1,7 @@
 #ifndef __HVMLOADER_CONFIG_H__
 #define __HVMLOADER_CONFIG_H__
 
-#include <stdint.h>
-#include <stdbool.h>
+#include "types.h"
 
 enum virtual_vga { VGA_none, VGA_std, VGA_cirrus, VGA_pt };
 extern enum virtual_vga virtual_vga;
diff --git a/tools/firmware/hvmloader/hypercall.h b/tools/firmware/hvmloader/hypercall.h
index 5368c30720..788f699565 100644
--- a/tools/firmware/hvmloader/hypercall.h
+++ b/tools/firmware/hvmloader/hypercall.h
@@ -31,7 +31,7 @@
 #ifndef __HVMLOADER_HYPERCALL_H__
 #define __HVMLOADER_HYPERCALL_H__
 
-#include <stdint.h>
+#include "types.h"
 #include <xen/xen.h>
 #include "config.h"
 
diff --git a/tools/firmware/hvmloader/mp_tables.c b/tools/firmware/hvmloader/mp_tables.c
index d207ecbf00..76790a9a1e 100644
--- a/tools/firmware/hvmloader/mp_tables.c
+++ b/tools/firmware/hvmloader/mp_tables.c
@@ -27,7 +27,7 @@
  * this program; If not, see <http://www.gnu.org/licenses/>.
  */
 
-#include <stdint.h>
+#include "types.h"
 #include "config.h"
 
 /* number of non-processor MP table entries */
diff --git a/tools/firmware/hvmloader/option_rom.h b/tools/firmware/hvmloader/option_rom.h
index 0fefe0812a..7988aa29ec 100644
--- a/tools/firmware/hvmloader/option_rom.h
+++ b/tools/firmware/hvmloader/option_rom.h
@@ -1,7 +1,7 @@
 #ifndef __HVMLOADER_OPTION_ROM_H__
 #define __HVMLOADER_OPTION_ROM_H__
 
-#include <stdint.h>
+#include "types.h"
 
 struct option_rom_header {
     uint8_t signature[2]; /* "\x55\xaa" */
diff --git a/tools/firmware/hvmloader/pir_types.h b/tools/firmware/hvmloader/pir_types.h
index 9f9259c2e1..9efcdcf94b 100644
--- a/tools/firmware/hvmloader/pir_types.h
+++ b/tools/firmware/hvmloader/pir_types.h
@@ -23,7 +23,7 @@
 #ifndef PIR_TYPES_H
 #define PIR_TYPES_H
 
-#include <stdint.h>
+#include "types.h"
 
 #define NR_PIR_SLOTS 6
 
diff --git a/tools/firmware/hvmloader/smbios.c b/tools/firmware/hvmloader/smbios.c
index 97a054e9e3..5821c85c30 100644
--- a/tools/firmware/hvmloader/smbios.c
+++ b/tools/firmware/hvmloader/smbios.c
@@ -19,7 +19,7 @@
  * Authors: Andrew D. Ball <aball@us.ibm.com>
  */
 
-#include <stdint.h>
+#include "types.h"
 #include <xen/xen.h>
 #include <xen/version.h>
 #include "smbios_types.h"
diff --git a/tools/firmware/hvmloader/smbios_types.h b/tools/firmware/hvmloader/smbios_types.h
index 7c648ece71..439c3fb247 100644
--- a/tools/firmware/hvmloader/smbios_types.h
+++ b/tools/firmware/hvmloader/smbios_types.h
@@ -25,7 +25,7 @@
 #ifndef SMBIOS_TYPES_H
 #define SMBIOS_TYPES_H
 
-#include <stdint.h>
+#include "types.h"
 
 /* SMBIOS entry point -- must be written to a 16-bit aligned address
    between 0xf0000 and 0xfffff. 
diff --git a/tools/firmware/hvmloader/types.h b/tools/firmware/hvmloader/types.h
new file mode 100644
index 0000000000..3d765f2c60
--- /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;
+
+#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
+
+typedef uint32_t size_t;
+typedef uint32_t uintptr_t;
+
+#define UINTPTR_MAX UINT32_MAX
+
+#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))
+#define va_end(ap)            __builtin_va_end(ap)
+#define va_arg                __builtin_va_arg
+
+#endif
diff --git a/tools/firmware/hvmloader/util.c b/tools/firmware/hvmloader/util.c
index 7da144b0bb..2df84482ab 100644
--- a/tools/firmware/hvmloader/util.c
+++ b/tools/firmware/hvmloader/util.c
@@ -24,7 +24,6 @@
 #include "vnuma.h"
 #include <acpi2_0.h>
 #include <libacpi.h>
-#include <stdint.h>
 #include <xen/xen.h>
 #include <xen/memory.h>
 #include <xen/sched.h>
diff --git a/tools/firmware/hvmloader/util.h b/tools/firmware/hvmloader/util.h
index 4f0baade0e..285a1d23c4 100644
--- a/tools/firmware/hvmloader/util.h
+++ b/tools/firmware/hvmloader/util.h
@@ -1,10 +1,7 @@
 #ifndef __HVMLOADER_UTIL_H__
 #define __HVMLOADER_UTIL_H__
 
-#include <stdarg.h>
-#include <stdint.h>
-#include <stddef.h>
-#include <stdbool.h>
+#include "types.h"
 #include <xen/xen.h>
 #include <xen/hvm/hvm_info_table.h>
 #include "e820.h"
-- 
2.30.1

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

[Jan Beulich]

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__.

> +#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.

> +typedef uint32_t size_t;

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

> +typedef uint32_t uintptr_t;

Again - use __UINTPTR_TYPE__ or, like Xen,
__attribute__((__mode__(__pointer__))).

> +#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?

Jan

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

[Ian Jackson]

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.

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

[Jan Beulich]

On 24.02.2021 12:07, Ian Jackson wrote:
> 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.

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. hvmloader, unlike
most of the rest of the tools, is a freestanding binary and hence
not tied to any particular ABI the compiler used may have been
built for.

> 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.

>>> +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.

May I ask why? There is a reason providing of these types did get
added to (at least) gcc.

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.

IOW imo if we stick to what is there now for {,u}int<N>_t, we
should use __SIZE_TYPE__ here. If we used the mode attribute
approach there, using uint32_t here would indeed be better.

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

The same question / considerations apply here then.

>>> +#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.

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. We've been trying
to eliminate such in the hypervisor part of the tree, and since
hvmloader is more closely related to the hypervisor than the tools
(see also its maintainership), I think we would want to do so
here, too. But of course if there are cases where such
parentheses are really needed, we'd want (need) to change our
approach in hypervisor code as well.

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.

Jan

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

[Ian Jackson]

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.

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

[Jan Beulich]

On 24.02.2021 15:33, Ian Jackson wrote:
> 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.

But gcc _does_ provide a stdint.h.

> 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.

I'm not sure about the last statement; I'm pretty sure we don't
check that we have such a compiler (in tools/configure).

>  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.

Which they can't, as that would make them unusable in preprocessor
directives.

>>>> 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.

From a compiler perspective, "32-bit" and "x86" needs further pairing
with an OS, as it's the OS which defines the ABI. This is why further
up I did say "It is technically correct at this point in time, from
all we can tell" - we imply that all OSes we want to be able to build
on provide a suitable ABI, so we can use their compilers.

>> 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.

Okay, if that would have been your example, then since there are
no further expressions involved here you agree parentheses aren't
needed here?

JAn

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

[Andrew Cooper]

On 24/02/2021 11:07, Ian Jackson wrote:
> Jan Beulich writes ("Re: [PATCH 2/2] hvmloader: do not include system headers for type declarations"):
>> Like the hypervisor, we should prefer using __SIZE_TYPE__
>> when available.
> I disagree.

size_t is obnoxious in the C spec.  It might not be the largest native
word size on the processor, and in some 64bit environments, it really is
32 bits.

It cannot be correctly derived from a standard type, and must come from
the compiler, because it critical that it matches what the compiler
generates for the sizeof operator.

Posix being fairly sane prohibits environments where the maximum object
size is smaller than the address size, which is why aliasing it to
unsigned long works in the common case.

~Andrew

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

[Roger Pau Monné]

On Wed, Feb 24, 2021 at 11:51:39AM +0100, Jan Beulich wrote:
> 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__.

Oh, didn't know about all this fancy stuff.

Clang doens't seem to support the same mode attributes, for example
QImode is unknown, and that's just on one version of clang that I
happened to test on.

Using __{,U}INT*_TYPE__ does seem to be supported on the clang version
I've tested with, so that might be an option if it's supported
everywhere we care about. If we still need to keep the current typedef
chunk for fallback purposes then I see no real usefulness of using
__{,U}INT*_TYPE__.

> > +#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've assumed that for consistency we would like to provide those
already. I can switch them to using __{U}INT*_{MAX,MIN}__ if we agree
that it's supported on all compilers we care about, but I would rather
not drop them. I think those might be useful in the future, and having
them already does no harm.

> > +typedef uint32_t size_t;
> 
> Like the hypervisor, we should prefer using __SIZE_TYPE__
> when available.
> 
> > +typedef uint32_t uintptr_t;
> 
> Again - use __UINTPTR_TYPE__ or, like Xen,
> __attribute__((__mode__(__pointer__))).

Let me run a gitlab test suite using the __{,U}INT*_TYPE__ stuff and
if that's fine everywhere we test then I think we can go for that if
you prefer over the current proposal?

I still think that coding them like I've done above should be fine as
we don't expect hvmloader to ever be built in a mode different than
i386?

Thanks, Roger.

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

[Jan Beulich]

On 24.02.2021 13:01, Roger Pau Monné wrote:
> On Wed, Feb 24, 2021 at 11:51:39AM +0100, Jan Beulich wrote:
>> 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__.
> 
> Oh, didn't know about all this fancy stuff.
> 
> Clang doens't seem to support the same mode attributes, for example
> QImode is unknown, and that's just on one version of clang that I
> happened to test on.

Oh, these modes have been available even in gcc 3.x. I thought
Clang was claiming to be 4.4(?) compatible.

> Using __{,U}INT*_TYPE__ does seem to be supported on the clang version
> I've tested with, so that might be an option if it's supported
> everywhere we care about. If we still need to keep the current typedef
> chunk for fallback purposes then I see no real usefulness of using
> __{,U}INT*_TYPE__.

Fair point. And they're available from 4.5 onwards only. So
just __SIZE_TYPE__ has been available for long enough. As said
in reply to Ian I think we at least want to use that one (and
I guess in the hypervisor we may want to drop the fallback).

Jan

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

[Andrew Cooper]

On 24/02/2021 10:26, Roger Pau Monne wrote:
> Instead create a private types.h header that contains the set of types
> that are required by hvmloader. Replace include occurrences of std*
> headers with type.h. Note that including types.h directly is not
> required in util.c because it already includes util.h which in turn
> includes the newly created types.h.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

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 are substantial portability costs for making changes like this,
which takes us from standards compliant C to GCC-isms-only.

~Andrew

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

[Roger Pau Monné]

On Wed, Feb 24, 2021 at 12:11:45PM +0000, Andrew Cooper wrote:
> On 24/02/2021 10:26, Roger Pau Monne wrote:
> > Instead create a private types.h header that contains the set of types
> > that are required by hvmloader. Replace include occurrences of std*
> > headers with type.h. Note that including types.h directly is not
> > required in util.c because it already includes util.h which in turn
> > includes the newly created types.h.
> >
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> 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.

The headers are available in Alpine, but they seem to be specifically
tied to the native bitness of the OS, which is causing us the issues.

So they are freestanding AFAICT, it's just that the bitness they use
is not portable.

Thanks, Roger.

Re: [PATCH 0/2] hvmloader: drop usage of system headers

[Ian Jackson]

Roger Pau Monne writes ("[PATCH 0/2] hvmloader: drop usage of system headers"):
> Following two patches aim to make hvmloader standalone, so that it don't
> try to use system headers. It shouldn't result in any functional
> change.

Both patches:

Reviewed-by: Ian Jackson <iwj@xenproject.org>

Given its status as a build fix,

Release-Acked-by: Ian Jackson <iwj@xenproject.org>

Thanks,
Ian.

Re: [PATCH 0/2] hvmloader: drop usage of system headers

[Andrew Cooper]

On 24/02/2021 10:26, Roger Pau Monne wrote:
> Hello,
>
> Following two patches aim to make hvmloader standalone, so that it don't
> try to use system headers. It shouldn't result in any functional
> change.
>
> Thanks, Roger.

After some experimentation in the arch container

Given foo.c as:

#include <stdint.h>

extern uint64_t bar;
uint64_t foo(void)
{
    return bar;
}

int main(void)
{
    return 0;
}

The preprocessed form with `gcc -m32 -E` is:

# 1 "foo.c"
# 1 "<built-in>"
# 1 "<command-line>"
# 31 "<command-line>"
# 1 "/usr/include/stdc-predef.h" 1 3 4
# 32 "<command-line>" 2
# 1 "foo.c"
# 1 "/usr/lib/gcc/x86_64-pc-linux-gnu/8.3.0/include/stdint.h" 1 3 4
# 9 "/usr/lib/gcc/x86_64-pc-linux-gnu/8.3.0/include/stdint.h" 3 4
# 1 "/usr/include/stdint.h" 1 3 4
# 26 "/usr/include/stdint.h" 3 4
# 1 "/usr/include/bits/libc-header-start.h" 1 3 4
# 33 "/usr/include/bits/libc-header-start.h" 3 4
# 1 "/usr/include/features.h" 1 3 4
# 450 "/usr/include/features.h" 3 4
# 1 "/usr/include/sys/cdefs.h" 1 3 4
# 452 "/usr/include/sys/cdefs.h" 3 4
# 1 "/usr/include/bits/wordsize.h" 1 3 4
# 453 "/usr/include/sys/cdefs.h" 2 3 4
# 1 "/usr/include/bits/long-double.h" 1 3 4
# 454 "/usr/include/sys/cdefs.h" 2 3 4
# 451 "/usr/include/features.h" 2 3 4
# 474 "/usr/include/features.h" 3 4
# 1 "/usr/include/gnu/stubs.h" 1 3 4

# 1 "/usr/include/gnu/stubs-32.h" 1 3 4
# 8 "/usr/include/gnu/stubs.h" 2 3 4
# 475 "/usr/include/features.h" 2 3 4
# 34 "/usr/include/bits/libc-header-start.h" 2 3 4
# 27 "/usr/include/stdint.h" 2 3 4
# 1 "/usr/include/bits/types.h" 1 3 4
# 27 "/usr/include/bits/types.h" 3 4
# 1 "/usr/include/bits/wordsize.h" 1 3 4
# 28 "/usr/include/bits/types.h" 2 3 4
# 1 "/usr/include/bits/timesize.h" 1 3 4
# 29 "/usr/include/bits/types.h" 2 3 4

# 31 "/usr/include/bits/types.h" 3 4
typedef unsigned char __u_char;
...

while the freestanding form with `gcc -ffreestanding -m32 -E` is:

# 1 "foo.c"
# 1 "<built-in>"
# 1 "<command-line>"
# 1 "foo.c"
# 1 "/usr/lib/gcc/x86_64-pc-linux-gnu/8.3.0/include/stdint.h" 1 3 4
# 11 "/usr/lib/gcc/x86_64-pc-linux-gnu/8.3.0/include/stdint.h" 3 4
# 1 "/usr/lib/gcc/x86_64-pc-linux-gnu/8.3.0/include/stdint-gcc.h" 1 3 4
# 34 "/usr/lib/gcc/x86_64-pc-linux-gnu/8.3.0/include/stdint-gcc.h" 3 4

# 34 "/usr/lib/gcc/x86_64-pc-linux-gnu/8.3.0/include/stdint-gcc.h" 3 4
typedef signed char int8_t;
...


I can compile and link trivial programs using -m32 and stdint.h without
any issue.

Clearly something more subtle is going on with our choice of options
when compiling hvmloader, but it certainly looks like stdint.h is fine
to use in the way we want to use it.

~Andrew

Re: [PATCH 0/2] hvmloader: drop usage of system headers

[Jan Beulich]

On 24.02.2021 21:08, Andrew Cooper wrote:
> On 24/02/2021 10:26, Roger Pau Monne wrote:
>> Hello,
>>
>> Following two patches aim to make hvmloader standalone, so that it don't
>> try to use system headers. It shouldn't result in any functional
>> change.
>>
>> Thanks, Roger.
> 
> After some experimentation in the arch container
> 
> Given foo.c as:
> 
> #include <stdint.h>
> 
> extern uint64_t bar;
> uint64_t foo(void)
> {
>     return bar;
> }
> 
> int main(void)
> {
>     return 0;
> }
> 
> The preprocessed form with `gcc -m32 -E` is:
> 
> # 1 "foo.c"
> # 1 "<built-in>"
> # 1 "<command-line>"
> # 31 "<command-line>"
> # 1 "/usr/include/stdc-predef.h" 1 3 4
> # 32 "<command-line>" 2
> # 1 "foo.c"
> # 1 "/usr/lib/gcc/x86_64-pc-linux-gnu/8.3.0/include/stdint.h" 1 3 4
> # 9 "/usr/lib/gcc/x86_64-pc-linux-gnu/8.3.0/include/stdint.h" 3 4
> # 1 "/usr/include/stdint.h" 1 3 4
> # 26 "/usr/include/stdint.h" 3 4
> # 1 "/usr/include/bits/libc-header-start.h" 1 3 4
> # 33 "/usr/include/bits/libc-header-start.h" 3 4
> # 1 "/usr/include/features.h" 1 3 4
> # 450 "/usr/include/features.h" 3 4
> # 1 "/usr/include/sys/cdefs.h" 1 3 4
> # 452 "/usr/include/sys/cdefs.h" 3 4
> # 1 "/usr/include/bits/wordsize.h" 1 3 4
> # 453 "/usr/include/sys/cdefs.h" 2 3 4
> # 1 "/usr/include/bits/long-double.h" 1 3 4
> # 454 "/usr/include/sys/cdefs.h" 2 3 4
> # 451 "/usr/include/features.h" 2 3 4
> # 474 "/usr/include/features.h" 3 4
> # 1 "/usr/include/gnu/stubs.h" 1 3 4
> 
> # 1 "/usr/include/gnu/stubs-32.h" 1 3 4
> # 8 "/usr/include/gnu/stubs.h" 2 3 4
> # 475 "/usr/include/features.h" 2 3 4
> # 34 "/usr/include/bits/libc-header-start.h" 2 3 4
> # 27 "/usr/include/stdint.h" 2 3 4
> # 1 "/usr/include/bits/types.h" 1 3 4
> # 27 "/usr/include/bits/types.h" 3 4
> # 1 "/usr/include/bits/wordsize.h" 1 3 4
> # 28 "/usr/include/bits/types.h" 2 3 4
> # 1 "/usr/include/bits/timesize.h" 1 3 4
> # 29 "/usr/include/bits/types.h" 2 3 4
> 
> # 31 "/usr/include/bits/types.h" 3 4
> typedef unsigned char __u_char;
> ...
> 
> while the freestanding form with `gcc -ffreestanding -m32 -E` is:
> 
> # 1 "foo.c"
> # 1 "<built-in>"
> # 1 "<command-line>"
> # 1 "foo.c"
> # 1 "/usr/lib/gcc/x86_64-pc-linux-gnu/8.3.0/include/stdint.h" 1 3 4
> # 11 "/usr/lib/gcc/x86_64-pc-linux-gnu/8.3.0/include/stdint.h" 3 4
> # 1 "/usr/lib/gcc/x86_64-pc-linux-gnu/8.3.0/include/stdint-gcc.h" 1 3 4
> # 34 "/usr/lib/gcc/x86_64-pc-linux-gnu/8.3.0/include/stdint-gcc.h" 3 4
> 
> # 34 "/usr/lib/gcc/x86_64-pc-linux-gnu/8.3.0/include/stdint-gcc.h" 3 4
> typedef signed char int8_t;
> ...
> 
> 
> I can compile and link trivial programs using -m32 and stdint.h without
> any issue.
> 
> Clearly something more subtle is going on with our choice of options
> when compiling hvmloader, but it certainly looks like stdint.h is fine
> to use in the way we want to use it.

Why "more subtle"? All we're lacking is -ffreestanding. The
question is whether it is an acceptably risky thing to do at
this point in the release cycle to add the option.

Jan

Re: [PATCH 0/2] hvmloader: drop usage of system headers

[Ian Jackson]

Jan Beulich writes ("Re: [PATCH 0/2] hvmloader: drop usage of system headers"):
> On 24.02.2021 21:08, Andrew Cooper wrote:
> > After some experimentation in the arch container
...
> > while the freestanding form with `gcc -ffreestanding -m32 -E` is:
...
> > # 34 "/usr/lib/gcc/x86_64-pc-linux-gnu/8.3.0/include/stdint-gcc.h" 3 4
> > typedef signed char int8_t;
> > ...

Um, but what size is size_t ?

In particular, note that that path contains nothing to do with 32-bit
so I think it is probably the wrong bitness.

> Why "more subtle"? All we're lacking is -ffreestanding. The
> question is whether it is an acceptably risky thing to do at
> this point in the release cycle to add the option.

If -ffreestanding DTRT then I think it's about as risky as the fix I
already approved and we have merged...

Ian.

Re: [PATCH 0/2] hvmloader: drop usage of system headers

[Jan Beulich]

On 25.02.2021 10:50, Ian Jackson wrote:
> Jan Beulich writes ("Re: [PATCH 0/2] hvmloader: drop usage of system headers"):
>> On 24.02.2021 21:08, Andrew Cooper wrote:
>>> After some experimentation in the arch container
> ...
>>> while the freestanding form with `gcc -ffreestanding -m32 -E` is:
> ...
>>> # 34 "/usr/lib/gcc/x86_64-pc-linux-gnu/8.3.0/include/stdint-gcc.h" 3 4
>>> typedef signed char int8_t;
>>> ...
> 
> Um, but what size is size_t ?
> 
> In particular, note that that path contains nothing to do with 32-bit
> so I think it is probably the wrong bitness.

The path doesn't need to include anything bitness specific, when
the headers can deal with both flavors.

>> Why "more subtle"? All we're lacking is -ffreestanding. The
>> question is whether it is an acceptably risky thing to do at
>> this point in the release cycle to add the option.
> 
> If -ffreestanding DTRT then I think it's about as risky as the fix I
> already approved and we have merged...

It would do the right thing, except Roger found Alpine has another
anomaly undermining this (and breaking -ffreestanding itself).

As an aside I'm not sure what you refer to with "we have merged":
So far only patch 1 of this series (plus its fixup) has gone in.

Jan

Re: [PATCH 0/2] hvmloader: drop usage of system headers

[Roger Pau Monné]

On Wed, Feb 24, 2021 at 08:08:25PM +0000, Andrew Cooper wrote:
> On 24/02/2021 10:26, Roger Pau Monne wrote:
> > Hello,
> >
> > Following two patches aim to make hvmloader standalone, so that it don't
> > try to use system headers. It shouldn't result in any functional
> > change.
> >
> > Thanks, Roger.
> 
> After some experimentation in the arch container

I'm afraid it's the alpine container the one that gives those errors,
not the arch one.

So I've done some testing on alpine and I think there's something
broken. Given the following snipped:

---
#include <stdint.h>

int main(void)
{
        _Static_assert(sizeof(uint64_t) == 8, "");
}
---

This is the output of running `gcc -E -m32 -ffreestanding test.c` on
an alpine chroot that has just the 'gcc' package installed:

---
# 1 "test.c"
# 1 "<built-in>"
# 1 "<command-line>"
# 1 "test.c"

# 1 "/usr/lib/gcc/x86_64-alpine-linux-musl/10.2.1/include/stdint.h" 1 3
# 11 "/usr/lib/gcc/x86_64-alpine-linux-musl/10.2.1/include/stdint.h" 3
# 1 "/usr/lib/gcc/x86_64-alpine-linux-musl/10.2.1/include/stdint-gcc.h" 1 3
# 34 "/usr/lib/gcc/x86_64-alpine-linux-musl/10.2.1/include/stdint-gcc.h" 3

# 34 "/usr/lib/gcc/x86_64-alpine-linux-musl/10.2.1/include/stdint-gcc.h" 3
typedef signed char int8_t;


typedef short int int16_t;


typedef int int32_t;


typedef long long int int64_t;


typedef unsigned char uint8_t;


typedef short unsigned int uint16_t;


typedef unsigned int uint32_t;


typedef long long unsigned int uint64_t;




typedef signed char int_least8_t;
typedef short int int_least16_t;
typedef int int_least32_t;
typedef long long int int_least64_t;
typedef unsigned char uint_least8_t;
typedef short unsigned int uint_least16_t;
typedef unsigned int uint_least32_t;
typedef long long unsigned int uint_least64_t;



typedef signed char int_fast8_t;
typedef int int_fast16_t;
typedef int int_fast32_t;
typedef long long int int_fast64_t;
typedef unsigned char uint_fast8_t;
typedef unsigned int uint_fast16_t;
typedef unsigned int uint_fast32_t;
typedef long long unsigned int uint_fast64_t;




typedef int intptr_t;


typedef unsigned int uintptr_t;




typedef long long int intmax_t;
typedef long long unsigned int uintmax_t;
# 12 "/usr/lib/gcc/x86_64-alpine-linux-musl/10.2.1/include/stdint.h" 2 3
# 3 "test.c" 2


# 4 "test.c"
int main(void)
{
 _Static_assert(sizeof(uint64_t) == 8, "");
}
---

OTOH, this is the output of the same command run on a chroot that has
the full list of packages required to build Xen (from
automation/build/alpine/3.12.dockerfile):

---
# 1 "test.c"
# 1 "<built-in>"
# 1 "<command-line>"
# 1 "test.c"

# 1 "/usr/include/stdint.h" 1 3 4
# 20 "/usr/include/stdint.h" 3 4
# 1 "/usr/include/bits/alltypes.h" 1 3 4
# 55 "/usr/include/bits/alltypes.h" 3 4

# 55 "/usr/include/bits/alltypes.h" 3 4
typedef unsigned long uintptr_t;
# 70 "/usr/include/bits/alltypes.h" 3 4
typedef long intptr_t;
# 96 "/usr/include/bits/alltypes.h" 3 4
typedef signed char int8_t;




typedef signed short int16_t;




typedef signed int int32_t;




typedef signed long int64_t;




typedef signed long intmax_t;




typedef unsigned char uint8_t;




typedef unsigned short uint16_t;




typedef unsigned int uint32_t;




typedef unsigned long uint64_t;
# 146 "/usr/include/bits/alltypes.h" 3 4
typedef unsigned long uintmax_t;
# 21 "/usr/include/stdint.h" 2 3 4

typedef int8_t int_fast8_t;
typedef int64_t int_fast64_t;

typedef int8_t int_least8_t;
typedef int16_t int_least16_t;
typedef int32_t int_least32_t;
typedef int64_t int_least64_t;

typedef uint8_t uint_fast8_t;
typedef uint64_t uint_fast64_t;

typedef uint8_t uint_least8_t;
typedef uint16_t uint_least16_t;
typedef uint32_t uint_least32_t;
typedef uint64_t uint_least64_t;
# 95 "/usr/include/stdint.h" 3 4
# 1 "/usr/include/bits/stdint.h" 1 3 4
typedef int32_t int_fast16_t;
typedef int32_t int_fast32_t;
typedef uint32_t uint_fast16_t;
typedef uint32_t uint_fast32_t;
# 96 "/usr/include/stdint.h" 2 3 4
# 3 "test.c" 2


# 4 "test.c"
int main(void)
{
 _Static_assert(sizeof(uint64_t) == 8, "");
}
---

This is caused by the include path order of gcc on alpine, ie:

---
# cpp -v /dev/null -o /dev/null
Using built-in specs.
COLLECT_GCC=cpp
Target: x86_64-alpine-linux-musl
Configured with: /home/buildozer/aports/main/gcc/src/gcc-10.2.1_pre1/configure --prefix=/usr --mandir=/usr/share/man --infodir=/usr/share/info --build=x86_64-alpine-linux-musl --host=x86_64-alpine-linux-musl --target=x86_64-alpine-linux-musl --with-pkgversion='Alpine 10.2.1_pre1' --enable-checking=release --disable-fixed-point --disable-libstdcxx-pch --disable-multilib --disable-nls --disable-werror --disable-symvers --enable-__cxa_atexit --enable-default-pie --enable-default-ssp --enable-cloog-backend --enable-languages=c,c++,d,objc,go,fortran,ada --disable-libssp --disable-libmpx --disable-libmudflap --disable-libsanitizer --enable-shared --enable-threads --enable-tls --with-system-zlib --with-linker-hash-style=gnu
Thread model: posix
Supported LTO compression algorithms: zlib
gcc version 10.2.1 20201203 (Alpine 10.2.1_pre1)
COLLECT_GCC_OPTIONS='-E' '-v' '-o' '/dev/null' '-mtune=generic' '-march=x86-64'
 /usr/libexec/gcc/x86_64-alpine-linux-musl/10.2.1/cc1 -E -quiet -v /dev/null -o /dev/null -mtune=generic -march=x86-64
ignoring nonexistent directory "/usr/local/include"
ignoring nonexistent directory "/usr/lib/gcc/x86_64-alpine-linux-musl/10.2.1/../../../../x86_64-alpine-linux-musl/include"
ignoring nonexistent directory "/usr/include/fortify"
#include "..." search starts here:
#include <...> search starts here:
 /usr/include
 /usr/lib/gcc/x86_64-alpine-linux-musl/10.2.1/include
End of search list.
COMPILER_PATH=/usr/libexec/gcc/x86_64-alpine-linux-musl/10.2.1/:/usr/libexec/gcc/x86_64-alpine-linux-musl/10.2.1/:/usr/libexec/gcc/x86_64-alpine-linux-musl/:/usr/lib/gcc/x86_64-alpine-linux-musl/10.2.1/:/usr/lib/gcc/x86_64-alpine-linux-musl/:/usr/lib/gcc/x86_64-alpine-linux-musl/10.2.1/../../../../x86_64-alpine-linux-musl/bin/
LIBRARY_PATH=/usr/lib/gcc/x86_64-alpine-linux-musl/10.2.1/:/usr/lib/gcc/x86_64-alpine-linux-musl/10.2.1/../../../../x86_64-alpine-linux-musl/lib/../lib/:/usr/lib/gcc/x86_64-alpine-linux-musl/10.2.1/../../../../lib/:/lib/../lib/:/usr/lib/../lib/:/usr/lib/gcc/x86_64-alpine-linux-musl/10.2.1/../../../../x86_64-alpine-linux-musl/lib/:/usr/lib/gcc/x86_64-alpine-linux-musl/10.2.1/../../../:/lib/:/usr/lib/
COLLECT_GCC_OPTIONS='-E' '-v' '-o' '/dev/null' '-mtune=generic' '-march=x86-64'
---

/usr/include takes precedence over the gcc private headers, and thus
the usage of the -ffreestanding option is broken.

On my Debian system this is:

---
# cpp -v /dev/null -o /dev/null
Using built-in specs.
COLLECT_GCC=cpp
Target: x86_64-linux-gnu
Configured with: ../src/configure -v --with-pkgversion='Debian 6.3.0-18+deb9u1' --with-bugurl=file:///usr/share/doc/gcc-6/README.Bugs --enable-languages=c,ada,c++,java,go,d,fortran,objc,obj-c++ --prefix=/usr --program-suffix=-6 --program-prefix=x86_64-linux-gnu- --enable-shared --enable-linker-build-id --libexecdir=/usr/lib --without-included-gettext --enable-threads=posix --libdir=/usr/lib --enable-nls --with-sysroot=/ --enable-clocale=gnu --enable-libstdcxx-debug --enable-libstdcxx-time=yes --with-default-libstdcxx-abi=new --enable-gnu-unique-object --disable-vtable-verify --enable-libmpx --enable-plugin --enable-default-pie --with-system-zlib --disable-browser-plugin --enable-java-awt=gtk --enable-gtk-cairo --with-java-home=/usr/lib/jvm/java-1.5.0-gcj-6-amd64/jre --enable-java-home --with-jvm-root-dir=/usr/lib/jvm/java-1.5.0-gcj-6-amd64 --with-jvm-jar-dir=/usr/lib/jvm-exports/java-1.5.0-gcj-6-amd64 --with-arch-directory=amd64 --with-ecj-jar=/usr/share/java/eclipse-ecj.jar --with-target-system-zlib --enable-objc-gc=auto --enable-multiarch --with-arch-32=i686 --with-abi=m64 --with-multilib-list=m32,m64,mx32 --enable-multilib --with-tune=generic --enable-checking=release --build=x86_64-linux-gnu --host=x86_64-linux-gnu --target=x86_64-linux-gnu
Thread model: posix
gcc version 6.3.0 20170516 (Debian 6.3.0-18+deb9u1)
COLLECT_GCC_OPTIONS='-E' '-v' '-o' '/dev/null' '-mtune=generic' '-march=x86-64'
 /usr/lib/gcc/x86_64-linux-gnu/6/cc1 -E -quiet -v -imultiarch x86_64-linux-gnu /dev/null -o /dev/null -mtune=generic -march=x86-64
ignoring nonexistent directory "/usr/local/include/x86_64-linux-gnu"
ignoring nonexistent directory "/usr/lib/gcc/x86_64-linux-gnu/6/../../../../x86_64-linux-gnu/include"
#include "..." search starts here:
#include <...> search starts here:
 /usr/lib/gcc/x86_64-linux-gnu/6/include
 /usr/local/include
 /usr/lib/gcc/x86_64-linux-gnu/6/include-fixed
 /usr/include/x86_64-linux-gnu
 /usr/include
End of search list.
COMPILER_PATH=/usr/lib/gcc/x86_64-linux-gnu/6/:/usr/lib/gcc/x86_64-linux-gnu/6/:/usr/lib/gcc/x86_64-linux-gnu/:/usr/lib/gcc/x86_64-linux-gnu/6/:/usr/lib/gcc/x86_64-linux-gnu/
LIBRARY_PATH=/usr/lib/gcc/x86_64-linux-gnu/6/:/usr/lib/gcc/x86_64-linux-gnu/6/../../../x86_64-linux-gnu/:/usr/lib/gcc/x86_64-linux-gnu/6/../../../../lib/:/lib/x86_64-linux-gnu/:/lib/../lib/:/usr/lib/x86_64-linux-gnu/:/usr/lib/../lib/:/usr/lib/gcc/x86_64-linux-gnu/6/../../../:/lib/:/usr/lib/
COLLECT_GCC_OPTIONS='-E' '-v' '-o' '/dev/null' '-mtune=generic' '-march=x86-64'
---

Which seems fine as the gcc private include path takes precedence over
/usr/{,local/}include.

Will try to figure out if there's a way to fix or workaround this
brokenness.

Roger.

Re: [PATCH 0/2] hvmloader: drop usage of system headers

[Stefano Stabellini]

Hi Roger,

These seems to be genuine breakages:

https://gitlab.com/xen-project/patchew/xen/-/pipelines/260986219

FYI keep an eye on the patchew gitlab-ci as you should be able to see
the alpine linux tests pass once your series is fixed.

Cheers,

Stefano


On Wed, 24 Feb 2021, no-reply@patchew.org wrote:
> Hi,
> 
> Patchew automatically ran gitlab-ci pipeline with this patch (series) applied, but the job failed. Maybe there's a bug in the patches?
> 
> You can find the link to the pipeline near the end of the report below:
> 
> Type: series
> Message-id: 20210224102641.89455-1-roger.pau@citrix.com
> Subject: [PATCH 0/2] hvmloader: drop usage of system headers
> 
> === TEST SCRIPT BEGIN ===
> #!/bin/bash
> sleep 10
> patchew gitlab-pipeline-check -p xen-project/patchew/xen
> === TEST SCRIPT END ===
> 
> warning: redirecting to https://gitlab.com/xen-project/patchew/xen.git/
> warning: redirecting to https://gitlab.com/xen-project/patchew/xen.git/
> From https://gitlab.com/xen-project/patchew/xen
>  * [new tag]               patchew/20210224102641.89455-1-roger.pau@citrix.com -> patchew/20210224102641.89455-1-roger.pau@citrix.com
> Switched to a new branch 'test'
> 471a8f9278 hvmloader: do not include system headers for type declarations
> f826f3a198 hvmloader: use Xen private header for elf structs
> 
> === OUTPUT BEGIN ===
> [2021-02-24 11:27:25] Looking up pipeline...
> [2021-02-24 11:27:27] Found pipeline 260986219:
> 
> https://gitlab.com/xen-project/patchew/xen/-/pipelines/260986219
> 
> [2021-02-24 11:27:27] Waiting for pipeline to finish...
> [2021-02-24 11:42:41] Still waiting...
> [2021-02-24 11:57:46] Still waiting...
> [2021-02-24 12:12:51] Still waiting...
> [2021-02-24 12:20:56] Pipeline failed
> [2021-02-24 12:20:58] Job 'qemu-smoke-x86-64-clang-pvh' in stage 'test' is skipped
> [2021-02-24 12:20:58] Job 'qemu-smoke-x86-64-gcc-pvh' in stage 'test' is skipped
> [2021-02-24 12:20:58] Job 'qemu-smoke-x86-64-clang' in stage 'test' is skipped
> [2021-02-24 12:20:58] Job 'qemu-smoke-x86-64-gcc' in stage 'test' is skipped
> [2021-02-24 12:20:58] Job 'qemu-smoke-arm64-gcc' in stage 'test' is skipped
> [2021-02-24 12:20:58] Job 'qemu-alpine-arm64-gcc' in stage 'test' is skipped
> [2021-02-24 12:20:58] Job 'build-each-commit-gcc' in stage 'test' is skipped
> [2021-02-24 12:20:58] Job 'alpine-3.12-clang-debug' in stage 'build' is failed
> [2021-02-24 12:20:58] Job 'alpine-3.12-clang' in stage 'build' is failed
> [2021-02-24 12:20:58] Job 'alpine-3.12-gcc-debug' in stage 'build' is failed
> [2021-02-24 12:20:58] Job 'alpine-3.12-gcc' in stage 'build' is failed
> === OUTPUT END ===
> 
> Test command exited with code: 1