Re: [PATCH for-4.15 2/3] firmware: provide a stand alone set of headers

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 26 Feb 2021, Roger Pau Monne wrote:
> The current build of the firmware relies on having 32bit compatible
> headers installed in order to build some of the 32bit firmware, but
> that usually requires multilib support and installing a i386 libc when
> building from an amd64 environment which is cumbersome just to get
> some headers.
> 
> Usually this could be solved by using the -ffreestanding compiler
> option which drops the usage of the system headers in favor of a
> private set of freestanding headers provided by the compiler itself
> that are not tied to libc. However such option is broken at least
> in the gcc compiler provided in Alpine Linux, as the system include
> path (ie: /usr/include) takes precedence over the gcc private include
> path:
> 
> #include <...> search starts here:
>  /usr/include
>  /usr/lib/gcc/x86_64-alpine-linux-musl/10.2.1/include
> 
> Since -ffreestanding is currently broken on at least that distro, and
> for resilience against future compilers also having the option broken
> provide a set of stand alone 32bit headers required for the firmware
> build.
> 
> This allows to drop the build time dependency on having a i386
> compatible set of libc headers on amd64.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> ---
> There's the argument of fixing gcc in Alpine and instead just use
> -ffreestanding. I think that's more fragile than providing our own set
> of stand alone headers for the firmware bits. Having the include paths
> wrongly sorted can easily make the system headers being picked up
> instead of the gcc ones, and then building can randomly fail because
> the system headers could be amd64 only (like the musl ones).
> 
> I've also seen clang-9 on Debian with the following include paths:
> 
> #include "..." search starts here:
> #include <...> search starts here:
>  /usr/local/include
>  /usr/lib/llvm-9/lib/clang/9.0.1/include
>  /usr/include/x86_64-linux-gnu
>  /usr/include
> 
> Which also seems slightly dangerous as local comes before the compiler
> private path.
> 
> IMO using our own set of stand alone headers is more resilient.
> 
> Regarding the release risks, the main one would be breaking the build
> (as it's currently broken on Alpine). I think there's a very low risk
> of this change successfully producing a binary image that's broken,
> and hence with enough build testing it should be safe to merge.

This patch is a lot nicer and smaller than I thought it would be. It
looks like the best approach.

In terms of testing, gitlab-ci has a pretty wide build test coverage, so
if we can pass those (and you have already provided a link with all
tests green in patch #0) then I am in favor of getting this in for 4.15.


> ---
>  README                                        |  3 --
>  tools/firmware/Rules.mk                       | 11 ++++++
>  tools/firmware/include/stdarg.h               | 10 +++++
>  tools/firmware/include/stdbool.h              |  9 +++++
>  tools/firmware/include/stddef.h               | 10 +++++
>  tools/firmware/include/stdint.h               | 39 +++++++++++++++++++
>  tools/firmware/rombios/32bit/rombios_compat.h |  4 +-
>  7 files changed, 80 insertions(+), 6 deletions(-)
>  create mode 100644 tools/firmware/include/stdarg.h
>  create mode 100644 tools/firmware/include/stdbool.h
>  create mode 100644 tools/firmware/include/stddef.h
>  create mode 100644 tools/firmware/include/stdint.h
> 
> diff --git a/README b/README
> index 33cdf6b826..5167bb1708 100644
> --- a/README
> +++ b/README
> @@ -62,9 +62,6 @@ provided by your OS distributor:
>      * GNU bison and GNU flex
>      * GNU gettext
>      * ACPI ASL compiler (iasl)
> -    * Libc multiarch package (e.g. libc6-dev-i386 / glibc-devel.i686).
> -      Required when building on a 64-bit platform to build
> -      32-bit components which are enabled on a default build.
>  
>  In addition to the above there are a number of optional build
>  prerequisites. Omitting these will cause the related features to be
> diff --git a/tools/firmware/Rules.mk b/tools/firmware/Rules.mk
> index 26bbddccd4..5d09ab06df 100644
> --- a/tools/firmware/Rules.mk
> +++ b/tools/firmware/Rules.mk
> @@ -17,3 +17,14 @@ $(call cc-options-add,CFLAGS,CC,$(EMBEDDED_EXTRA_CFLAGS))
>  
>  # Extra CFLAGS suitable for an embedded type of environment.
>  CFLAGS += -fno-builtin -msoft-float
> +
> +# Use our own set of library headers to build firmware.
> +#
> +# Ideally we would instead use -ffreestanding, but that relies on the 
> compiler
> +# having the right order for include paths (ie: compiler private headers 
> before
> +# system ones). This is not the case in Alpine at least which searches system
> +# headers before compiler ones, and has been reported upstream:
> +# https://gitlab.alpinelinux.org/alpine/aports/-/issues/12477
> +# In the meantime (and for resilience against broken compilers) use our own 
> set
> +# of headers that provide what's needed for the firmware build.
> +CFLAGS += -nostdinc -I$(XEN_ROOT)/tools/firmware/include
> diff --git a/tools/firmware/include/stdarg.h b/tools/firmware/include/stdarg.h
> new file mode 100644
> index 0000000000..c5e3761cd2
> --- /dev/null
> +++ b/tools/firmware/include/stdarg.h
> @@ -0,0 +1,10 @@
> +#ifndef _STDARG_H_
> +#define _STDARG_H_
> +
> +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/include/stdbool.h 
> b/tools/firmware/include/stdbool.h
> new file mode 100644
> index 0000000000..0cf76b106c
> --- /dev/null
> +++ b/tools/firmware/include/stdbool.h
> @@ -0,0 +1,9 @@
> +#ifndef _STDBOOL_H_
> +#define _STDBOOL_H_
> +
> +#define bool _Bool
> +#define true 1
> +#define false 0
> +#define __bool_true_false_are_defined 1
> +
> +#endif
> diff --git a/tools/firmware/include/stddef.h b/tools/firmware/include/stddef.h
> new file mode 100644
> index 0000000000..c7f974608a
> --- /dev/null
> +++ b/tools/firmware/include/stddef.h
> @@ -0,0 +1,10 @@
> +#ifndef _STDDEF_H_
> +#define _STDDEF_H_
> +
> +typedef __SIZE_TYPE__ size_t;
> +
> +#define NULL ((void*)0)
> +
> +#define offsetof(t, m) __builtin_offsetof(t, m)
> +
> +#endif
> diff --git a/tools/firmware/include/stdint.h b/tools/firmware/include/stdint.h
> new file mode 100644
> index 0000000000..7514096846
> --- /dev/null
> +++ b/tools/firmware/include/stdint.h
> @@ -0,0 +1,39 @@
> +#ifndef _STDINT_H_
> +#define _STDINT_H_
> +
> +#ifdef __LP64__
> +#error "32bit only header"
> +#endif
> +
> +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 uintptr_t;
> +
> +#define UINTPTR_MAX     UINT32_MAX
> +
> +#endif
> diff --git a/tools/firmware/rombios/32bit/rombios_compat.h 
> b/tools/firmware/rombios/32bit/rombios_compat.h
> index 3fe7d67721..8ba4c17ffd 100644
> --- a/tools/firmware/rombios/32bit/rombios_compat.h
> +++ b/tools/firmware/rombios/32bit/rombios_compat.h
> @@ -8,9 +8,7 @@
>  
>  #define ADDR_FROM_SEG_OFF(seg, off)  (void *)((((uint32_t)(seg)) << 4) + 
> (off))
>  
> -typedef unsigned char uint8_t;
> -typedef unsigned short int uint16_t;
> -typedef unsigned int uint32_t;
> +#include <stdint.h>
>  
>  typedef uint8_t  Bit8u;
>  typedef uint16_t Bit16u;
> -- 
> 2.30.1
>