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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.02.2021 09:59, 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>

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
with possibly small adjustments:

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

I agree (in particular given the observations), but I don't view
this as an argument against use of -ffreestanding. In fact I'd
rather see this change re-based on top of Andrew's changes. Then ...

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

... the initial part of the comment here would want re-wording.

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

Could I talk you into extending this to also cover __P64__? (The
alternative I see would be to omit this altogether.)

Jan