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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, Mar 01, 2021 at 11:05:17AM +0100, Jan Beulich wrote:
> On 01.03.2021 10:50, Roger Pau Monné wrote:
> > On Mon, Mar 01, 2021 at 10:17:32AM +0100, Jan Beulich wrote:
> >> On 01.03.2021 10:07, Roger Pau Monné wrote:
> >>> On Fri, Feb 26, 2021 at 02:24:43PM +0100, Jan Beulich wrote:
> >>>> 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 ...
> >>>
> >>> But doesn't using -nostdinc kind of defeats the purpose of
> >>> -ffreestanding, as it would remove all default paths from the include
> >>> search, and thus prevent using the gcc private headers?
> >>
> >> I guess I don't understand: It is the purpose of this change here to
> >> not use compiler provided headers (nor libc provided ones), so why
> >> would it matter to retain any kind of built-in include paths?
> > 
> > Sorry, I'm also confused.
> > 
> > It's my understanding that the point of using -ffreestanding is that
> > the compiler will set __STDC_HOSTED__ == 0, and then the built in
> > compiler headers will be used to provide a freestanding environment
> > instead of the libc ones.
> > 
> > However if -nostdinc is used the header search path becomes:
> > 
> > #include <...> search starts here:
> > End of search list.
> > 
> > At which point setting __STDC_HOSTED__ == 0 is pointless as the built
> > in compiler headers are not used, and hence the compiler will always
> > resort to the stand alone environment provided in this patch.
> > 
> > -ffreestanding also allows the program to have a non-standard main,
> > but I don't think we care much about that since we already use a custom
> > linker script.
> 
> While indeed we don't care about this specific last aspect, we
> do e.g. care about the implied -fno-builtin (which currently we
> specify explicitly, yes). So while with -nostdinc added we
> _might_ indeed be fine already, I think we're better off going
> the full step and specify what we mean, even if - right now -
> we're unaware of further effects which are relevant to us. (For
> example, I don't see why in principle we couldn't ourselves
> grow a use of __STDC_HOSTED__ somewhere.)

OK I will rebase once Andrew posts an updated version and adjust the
commit message and comment to notice what we have discussed above.

Thanks, Roger.