Message-ID: <6d398f1d-6882-40fb-8ced-7fe6bee2aee7@nvidia.com>
Date: Sat, 13 Apr 2024 15:10:17 -0700
From: John Hubbard <jhubbard@...dia.com>
To: Valentin Obst <kernel@...entinobst.de>, Shuah Khan <shuah@...nel.org>
CC: Anders Roxell <anders.roxell@...aro.org>, Benjamin Poirier
	<bpoirier@...dia.com>, Guillaume Tucker <guillaume.tucker@...labora.com>,
	Marcos Paulo de Souza <mpdesouza@...e.com>, Mark Brown <broonie@...nel.org>,
	Nathan Chancellor <nathan@...nel.org>, Sasha Levin <sashal@...nel.org>,
	<linux-kernel@...r.kernel.org>, <linux-kselftest@...r.kernel.org>
Subject: Re: [PATCH] selftests: default to host arch for LLVM builds

On 3/29/24 3:49 AM, Valentin Obst wrote:
> Align the behavior for gcc and clang builds by interpreting unset
> `ARCH` and `CROSS_COMPILE` variables in `LLVM` builds as a sign that the
> user wants to build for the host architecture.
> 
> This patch preserves the properties that setting the `ARCH` variable to an
> unknown value will trigger an error that complains about insufficient
> information, and that a set `CROSS_COMPILE` variable will override the
> target triple that is determined based on presence/absence of `ARCH`.
> 
> When compiling with clang, i.e., `LLVM` is set, an unset `ARCH` variable in
> combination with an unset `CROSS_COMPILE` variable, i.e., compiling for
> the host architecture, leads to compilation failures since `lib.mk` can
> not determine the clang target triple. In this case, the following error
> message is displayed for each subsystem that does not set `ARCH` in its
> own Makefile before including `lib.mk` (lines wrapped at 75 chrs):
> 
>    make[1]: Entering directory '/mnt/build/linux/tools/testing/selftests/
>     sysctl'
>    ../lib.mk:33: *** Specify CROSS_COMPILE or add '--target=' option to
>     lib.mk.  Stop.
>    make[1]: Leaving directory '/mnt/build/linux/tools/testing/selftests/
>     sysctl'

Thanks for fixing this.

And yes, the selftests "normal" (non-cross-compile) build is *broken*
right now, for clang. I didn't realize from the patch title that this is
actually a significant fix. Maybe we should change the subject line (patch
title) to something like:

     [PATCH] selftests: fix the clang build: default to host arch for LLVM builds

?

Just a thought. The "Fixes:" tag covers it already, I realize.

Anyway, this looks correct, and fixes that aspect of the build for me, so
either way, please feel free to add:

Reviewed-by: John Hubbard <jhubbard@...dia.com>


thanks,
-- 
John Hubbard
NVIDIA

> 
> In the same scenario a gcc build would default to the host architecture,
> i.e., it would use plain `gcc`.
> 
> Fixes: 795285ef2425 ("selftests: Fix clang cross compilation")
> Reviewed-by: Mark Brown <broonie@...nel.org>
> Signed-off-by: Valentin Obst <kernel@...entinobst.de>
> ---
> I am not entirely sure whether this behavior is in fact known and intended
> and whether the way to obtain the host target triple is sufficiently
> general. The flag was introduced in llvm-8 with [1], it will be an error in
> older clang versions.
> 
> The target triple you get with `-print-target-triple` may not be the
> same that you would get when explicitly setting ARCH to you host
> architecture. For example on my x86_64 system it get
> `x86_64-pc-linux-gnu` instead of `x86_64-linux-gnu`, similar deviations
> were observed when testing other clang binaries on compiler-explorer,
> e.g., [2].
> 
> An alternative could be to simply do:
> 
>        ARCH ?= $(shell uname -m)
> 
> before using it to select the target. Possibly with some post processing,
> but at that point we would likely be replicating `scripts/subarch.include`.
> This is what some subsystem Makefiles do before including `lib.mk`. This
> change might make it possible to remove the explicit setting of `ARCH` from
> the few subsystem Makefiles that do it.
> 
> [1]: https://reviews.llvm.org/D50755
> [2]: https://godbolt.org/z/r7Gn9bvv1
> 
> Changes in v1:
> - Shortened commit message.
> - Link to RFC: https://lore.kernel.org/r/20240303-selftests-libmk-llvm-rfc-v1-1-9ab53e365e31@valentinobst.de
> ---
>   tools/testing/selftests/lib.mk | 12 ++++++++++--
>   1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/lib.mk b/tools/testing/selftests/lib.mk
> index da2cade3bab0..8ae203d8ed7f 100644
> --- a/tools/testing/selftests/lib.mk
> +++ b/tools/testing/selftests/lib.mk
> @@ -7,6 +7,8 @@ else ifneq ($(filter -%,$(LLVM)),)
>   LLVM_SUFFIX := $(LLVM)
>   endif
> 
> +CLANG := $(LLVM_PREFIX)clang$(LLVM_SUFFIX)
> +
>   CLANG_TARGET_FLAGS_arm          := arm-linux-gnueabi
>   CLANG_TARGET_FLAGS_arm64        := aarch64-linux-gnu
>   CLANG_TARGET_FLAGS_hexagon      := hexagon-linux-musl
> @@ -18,7 +20,13 @@ CLANG_TARGET_FLAGS_riscv        := riscv64-linux-gnu
>   CLANG_TARGET_FLAGS_s390         := s390x-linux-gnu
>   CLANG_TARGET_FLAGS_x86          := x86_64-linux-gnu
>   CLANG_TARGET_FLAGS_x86_64       := x86_64-linux-gnu
> -CLANG_TARGET_FLAGS              := $(CLANG_TARGET_FLAGS_$(ARCH))
> +
> +# Default to host architecture if ARCH is not explicitly given.
> +ifeq ($(ARCH),)
> +CLANG_TARGET_FLAGS := $(shell $(CLANG) -print-target-triple)
> +else
> +CLANG_TARGET_FLAGS := $(CLANG_TARGET_FLAGS_$(ARCH))
> +endif
> 
>   ifeq ($(CROSS_COMPILE),)
>   ifeq ($(CLANG_TARGET_FLAGS),)
> @@ -30,7 +38,7 @@ else
>   CLANG_FLAGS     += --target=$(notdir $(CROSS_COMPILE:%-=%))
>   endif # CROSS_COMPILE
> 
> -CC := $(LLVM_PREFIX)clang$(LLVM_SUFFIX) $(CLANG_FLAGS) -fintegrated-as
> +CC := $(CLANG) $(CLANG_FLAGS) -fintegrated-as
>   else
>   CC := $(CROSS_COMPILE)gcc
>   endif # LLVM
> 
> ---
> base-commit: 4cece764965020c22cff7665b18a012006359095
> change-id: 20240303-selftests-libmk-llvm-rfc-5fe3cfa9f094
> 
> Best regards,
> --
> Valentin Obst <kernel@...entinobst.de>
> 

Powered by blists - more mailing lists