[PATCH v2] kbuild: add --target to correctly cross-compile UAPI headers with Clang

[PATCH v2] kbuild: add --target to correctly cross-compile UAPI headers with Clang

[Masahiro Yamada]

When you compile-test UAPI headers (CONFIG_UAPI_HEADER_TEST=y) with
Clang, they are currently compiled for the host target (likely x86_64)
regardless of the given ARCH=.

In fact, some exported headers include libc headers. For example,
include/uapi/linux/agpgart.h includes <stdlib.h> after being exported.
The header search paths should match to the target we are compiling
them for.

Pick up the --target triple from KBUILD_CFLAGS in the same ways as
commit 7f58b487e9ff ("kbuild: make Clang build userprogs for target
architecture").

Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---

Changes in v2:
  - Reword the commit description to mention agpgart.h instead of
    asound.h because the latter is in the no-header-test list.

 usr/include/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/usr/include/Makefile b/usr/include/Makefile
index ac206fb27c65..4215801e1110 100644
--- a/usr/include/Makefile
+++ b/usr/include/Makefile
@@ -10,7 +10,7 @@ UAPI_CFLAGS := -std=c90 -Wall -Werror=implicit-function-declaration
 
 # In theory, we do not care -m32 or -m64 for header compile tests.
 # It is here just because CONFIG_CC_CAN_LINK is tested with -m32 or -m64.
-UAPI_CFLAGS += $(filter -m32 -m64, $(KBUILD_CFLAGS))
+UAPI_CFLAGS += $(filter -m32 -m64 --target=%, $(KBUILD_CFLAGS))
 
 # USERCFLAGS might contain sysroot location for CC.
 UAPI_CFLAGS += $(USERCFLAGS)
-- 
2.32.0

Re: [PATCH v2] kbuild: add --target to correctly cross-compile UAPI headers with Clang

[Nathan Chancellor]

On Sat, Mar 05, 2022 at 09:56:05PM +0900, Masahiro Yamada wrote:
> When you compile-test UAPI headers (CONFIG_UAPI_HEADER_TEST=y) with
> Clang, they are currently compiled for the host target (likely x86_64)
> regardless of the given ARCH=.
> 
> In fact, some exported headers include libc headers. For example,
> include/uapi/linux/agpgart.h includes <stdlib.h> after being exported.
> The header search paths should match to the target we are compiling
> them for.
> 
> Pick up the --target triple from KBUILD_CFLAGS in the same ways as
> commit 7f58b487e9ff ("kbuild: make Clang build userprogs for target
> architecture").
> 
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>

Reviewed-by: Nathan Chancellor <nathan@kernel.org>

I suspect that Android will still need to do something with USERCFLAGS
in its build wrapper to pick up the correct libc (Bionic vs. glibc) but
that is tangential to this patch, we should still do this change
regardless.

> ---
> 
> Changes in v2:
>   - Reword the commit description to mention agpgart.h instead of
>     asound.h because the latter is in the no-header-test list.
> 
>  usr/include/Makefile | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/usr/include/Makefile b/usr/include/Makefile
> index ac206fb27c65..4215801e1110 100644
> --- a/usr/include/Makefile
> +++ b/usr/include/Makefile
> @@ -10,7 +10,7 @@ UAPI_CFLAGS := -std=c90 -Wall -Werror=implicit-function-declaration
>  
>  # In theory, we do not care -m32 or -m64 for header compile tests.
>  # It is here just because CONFIG_CC_CAN_LINK is tested with -m32 or -m64.
> -UAPI_CFLAGS += $(filter -m32 -m64, $(KBUILD_CFLAGS))
> +UAPI_CFLAGS += $(filter -m32 -m64 --target=%, $(KBUILD_CFLAGS))
>  
>  # USERCFLAGS might contain sysroot location for CC.
>  UAPI_CFLAGS += $(USERCFLAGS)
> -- 
> 2.32.0
> 

Re: [PATCH v2] kbuild: add --target to correctly cross-compile UAPI headers with Clang

[Nick Desaulniers]

On Sat, Mar 5, 2022 at 4:56 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> When you compile-test UAPI headers (CONFIG_UAPI_HEADER_TEST=y) with
> Clang, they are currently compiled for the host target (likely x86_64)
> regardless of the given ARCH=.
>
> In fact, some exported headers include libc headers. For example,
> include/uapi/linux/agpgart.h includes <stdlib.h> after being exported.
> The header search paths should match to the target we are compiling
> them for.

Isn't that a bug in that header though? Why does it inconsistently use
size_t vs. __kernel_size_t. Shouldn't it be consistently using
__kernel_size_t? (Seeing TRUE/FALSE defined in such a low level header
is also *yikes*.) Are there platforms where sizeof(size_t) !=
sizeof(__kernel_size_t)?

Usually to bootstrap a toolchain you need to start with kernel headers
to bootstrap the libc.  It seems like some kind of circular dependency
to me if kernel headers are dependent on libc headers. Hence my
previous comment about -ffreestanding.

>
> Pick up the --target triple from KBUILD_CFLAGS in the same ways as
> commit 7f58b487e9ff ("kbuild: make Clang build userprogs for target
> architecture").

Oh boy thanks for finding+fixing this! I still suspect we shouldn't
need a cross-libc for UAPI header testing, and that the kernel headers
simply need to be cleaned up. But regardless of that it doesn't make
sense to use the wrong target when checking headers generated for the
target. Thanks for the patch (and sorry I've been falling behind on
code review lately).

Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

>
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> ---
>
> Changes in v2:
>   - Reword the commit description to mention agpgart.h instead of
>     asound.h because the latter is in the no-header-test list.
>
>  usr/include/Makefile | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/usr/include/Makefile b/usr/include/Makefile
> index ac206fb27c65..4215801e1110 100644
> --- a/usr/include/Makefile
> +++ b/usr/include/Makefile
> @@ -10,7 +10,7 @@ UAPI_CFLAGS := -std=c90 -Wall -Werror=implicit-function-declaration
>
>  # In theory, we do not care -m32 or -m64 for header compile tests.
>  # It is here just because CONFIG_CC_CAN_LINK is tested with -m32 or -m64.
> -UAPI_CFLAGS += $(filter -m32 -m64, $(KBUILD_CFLAGS))
> +UAPI_CFLAGS += $(filter -m32 -m64 --target=%, $(KBUILD_CFLAGS))
>
>  # USERCFLAGS might contain sysroot location for CC.
>  UAPI_CFLAGS += $(USERCFLAGS)
> --
> 2.32.0
>


-- 
Thanks,
~Nick Desaulniers

Re: [PATCH v2] kbuild: add --target to correctly cross-compile UAPI headers with Clang

[Masahiro Yamada]

On Fri, Mar 11, 2022 at 4:16 AM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> On Sat, Mar 5, 2022 at 4:56 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
> >
> > When you compile-test UAPI headers (CONFIG_UAPI_HEADER_TEST=y) with
> > Clang, they are currently compiled for the host target (likely x86_64)
> > regardless of the given ARCH=.
> >
> > In fact, some exported headers include libc headers. For example,
> > include/uapi/linux/agpgart.h includes <stdlib.h> after being exported.
> > The header search paths should match to the target we are compiling
> > them for.
>
> Isn't that a bug in that header though? Why does it inconsistently use
> size_t vs. __kernel_size_t. Shouldn't it be consistently using
> __kernel_size_t? (Seeing TRUE/FALSE defined in such a low level header
> is also *yikes*.) Are there platforms where sizeof(size_t) !=
> sizeof(__kernel_size_t)?

size_t and __kernel_size_t are always the same.

For uapi headers  (include/uapi, arch/*/include/uapi), __kernel_size_t
should be used.


See this series, which is already available in linux-next.

https://lore.kernel.org/all/20220210021129.3386083-1-masahiroy@kernel.org/


>
> Usually to bootstrap a toolchain you need to start with kernel headers
> to bootstrap the libc.  It seems like some kind of circular dependency
> to me if kernel headers are dependent on libc headers.

Right, ideally, UAPI headers should be self-contained.
Hence, CONFIG_UAPI_HEADER_TEST exists.
But, I am not sure how far we can obey this rule.

Arnd Bergmann is an expert in this area.

The number of "no-header-test" in usr/include/Makefile
is gradually decreasing, but there are still more than 30.
They must be checked one by one.





> Hence my
> previous comment about -ffreestanding.

To ensure that we are not including any header from the system,
-nostdinc is a more correct flag.


-ffreestanding still allows system headers to be included.


I am skeptical about adding -ffreestanding here.
Please note we are compiling the user-space on the operating
system (this is what is called "hosted environment", which is
opposed to the "freestanding environment").




>
> >
> > Pick up the --target triple from KBUILD_CFLAGS in the same ways as
> > commit 7f58b487e9ff ("kbuild: make Clang build userprogs for target
> > architecture").
>
> Oh boy thanks for finding+fixing this! I still suspect we shouldn't
> need a cross-libc for UAPI header testing, and that the kernel headers
> simply need to be cleaned up. But regardless of that it doesn't make
> sense to use the wrong target when checking headers generated for the
> target. Thanks for the patch (and sorry I've been falling behind on
> code review lately).
>
> Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
>
> >
> > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> > ---
> >
> > Changes in v2:
> >   - Reword the commit description to mention agpgart.h instead of
> >     asound.h because the latter is in the no-header-test list.
> >
> >  usr/include/Makefile | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/usr/include/Makefile b/usr/include/Makefile
> > index ac206fb27c65..4215801e1110 100644
> > --- a/usr/include/Makefile
> > +++ b/usr/include/Makefile
> > @@ -10,7 +10,7 @@ UAPI_CFLAGS := -std=c90 -Wall -Werror=implicit-function-declaration
> >
> >  # In theory, we do not care -m32 or -m64 for header compile tests.
> >  # It is here just because CONFIG_CC_CAN_LINK is tested with -m32 or -m64.
> > -UAPI_CFLAGS += $(filter -m32 -m64, $(KBUILD_CFLAGS))
> > +UAPI_CFLAGS += $(filter -m32 -m64 --target=%, $(KBUILD_CFLAGS))
> >
> >  # USERCFLAGS might contain sysroot location for CC.
> >  UAPI_CFLAGS += $(USERCFLAGS)
> > --
> > 2.32.0
> >
>
>
> --
> Thanks,
> ~Nick Desaulniers



-- 
Best Regards
Masahiro Yamada