All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] selftests: default to host arch for LLVM builds
@ 2024-03-29 10:49 Valentin Obst
  2024-04-13 22:10 ` John Hubbard
  0 siblings, 1 reply; 8+ messages in thread
From: Valentin Obst @ 2024-03-29 10:49 UTC (permalink / raw)
  To: Shuah Khan
  Cc: Anders Roxell, Benjamin Poirier, Guillaume Tucker, John Hubbard,
	Marcos Paulo de Souza, Mark Brown, Nathan Chancellor,
	Sasha Levin, linux-kernel, linux-kselftest, Valentin Obst

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'

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@kernel.org>
Signed-off-by: Valentin Obst <kernel@valentinobst.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@valentinobst.de>


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH] selftests: default to host arch for LLVM builds
  2024-03-29 10:49 [PATCH] selftests: default to host arch for LLVM builds Valentin Obst
@ 2024-04-13 22:10 ` John Hubbard
  2024-04-28 12:08   ` Valentin Obst
  0 siblings, 1 reply; 8+ messages in thread
From: John Hubbard @ 2024-04-13 22:10 UTC (permalink / raw)
  To: Valentin Obst, Shuah Khan
  Cc: Anders Roxell, Benjamin Poirier, Guillaume Tucker,
	Marcos Paulo de Souza, Mark Brown, Nathan Chancellor,
	Sasha Levin, linux-kernel, linux-kselftest

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@nvidia.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@kernel.org>
> Signed-off-by: Valentin Obst <kernel@valentinobst.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@valentinobst.de>
> 




^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] selftests: default to host arch for LLVM builds
  2024-04-13 22:10 ` John Hubbard
@ 2024-04-28 12:08   ` Valentin Obst
  2024-04-28 22:04     ` John Hubbard
  2024-05-03 19:55     ` Shuah Khan
  0 siblings, 2 replies; 8+ messages in thread
From: Valentin Obst @ 2024-04-28 12:08 UTC (permalink / raw)
  To: jhubbard
  Cc: anders.roxell, bpoirier, broonie, guillaume.tucker, kernel,
	linux-kernel, linux-kselftest, mpdesouza, nathan, sashal, shuah

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

Yes, I agree that the title should contain the word 'fix' somewhere. For
me its okay if maintainers reword the title when applying the patch,
alternatively I can send a v2. (Is it still a v2 if I change the title, or
rather a new patch?).

Any thoughts on whether this also needs a 'Cc stable'? Its not quite
clear to me if this fix meets the requirements. As above, no objections if
maintainers should decide to add it.

>
> ?
>
> 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@nvidia.com>

Thanks!

	- Best Valentin
>
>
> 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@kernel.org>
> > Signed-off-by: Valentin Obst <kernel@valentinobst.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@valentinobst.de>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] selftests: default to host arch for LLVM builds
  2024-04-28 12:08   ` Valentin Obst
@ 2024-04-28 22:04     ` John Hubbard
  2024-04-30 11:44       ` Valentin Obst
  2024-05-03 19:55     ` Shuah Khan
  1 sibling, 1 reply; 8+ messages in thread
From: John Hubbard @ 2024-04-28 22:04 UTC (permalink / raw)
  To: Valentin Obst
  Cc: anders.roxell, bpoirier, broonie, guillaume.tucker, linux-kernel,
	linux-kselftest, mpdesouza, nathan, sashal, shuah

On 4/28/24 5:08 AM, Valentin Obst wrote:
...
>> 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
> 
> Yes, I agree that the title should contain the word 'fix' somewhere. For
> me its okay if maintainers reword the title when applying the patch,
> alternatively I can send a v2. (Is it still a v2 if I change the title, or
> rather a new patch?).

It would still be a v2, although the cover letter, or the section after the
"---", would need to point to v1 so that people could make the connection.

> 
> Any thoughts on whether this also needs a 'Cc stable'? Its not quite
> clear to me if this fix meets the requirements. As above, no objections if
> maintainers should decide to add it.
> 

Maybe not, because it doesn't seem urgent. But it's a judgment call.

By the way, I've been chipping away at fixing clang selftest build
failures and warnings that are only visible after clang is working again
(due to your fix here), and I'm up to 30+ patches, and probably only a
few more to go to get all of them.

I'm expecting to post the series soon, hopefully this week. And I'm
thinking maybe I should carry your patch as the first one in the series,
in order to ensure it gets picked up. Or, I can just refer to it as a
prerequisite in the cover letter.


thanks,
-- 
John Hubbard
NVIDIA


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] selftests: default to host arch for LLVM builds
  2024-04-28 22:04     ` John Hubbard
@ 2024-04-30 11:44       ` Valentin Obst
  2024-04-30 14:50         ` Mark Brown
  0 siblings, 1 reply; 8+ messages in thread
From: Valentin Obst @ 2024-04-30 11:44 UTC (permalink / raw)
  To: jhubbard
  Cc: anders.roxell, bpoirier, broonie, kernel, linux-kernel,
	linux-kselftest, mpdesouza, nathan, sashal, shuah

On 4/29/24 12:04 AM, John Hubbard wrote:
> ...
>>> 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
>>
>> Yes, I agree that the title should contain the word 'fix' somewhere. For
>> me its okay if maintainers reword the title when applying the patch,
>> alternatively I can send a v2. (Is it still a v2 if I change the
>> title, or
>> rather a new patch?).
>
> It would still be a v2, although the cover letter, or the section after the
> "---", would need to point to v1 so that people could make the connection.
>
>>
>> Any thoughts on whether this also needs a 'Cc stable'? Its not quite
>> clear to me if this fix meets the requirements. As above, no
>> objections if
>> maintainers should decide to add it.
>>
>
> Maybe not, because it doesn't seem urgent. But it's a judgment call.
>
> By the way, I've been chipping away at fixing clang selftest build
> failures and warnings that are only visible after clang is working again
> (due to your fix here), and I'm up to 30+ patches, and probably only a
> few more to go to get all of them.

Thanks! There're really a lot of those.

>
> I'm expecting to post the series soon, hopefully this week. And I'm
> thinking maybe I should carry your patch as the first one in the series,
> in order to ensure it gets picked up. Or, I can just refer to it as a
> prerequisite in the cover letter.

Correct me if I'm wrong, but intuitively 30+ patches that touch selftests
from many different subsystems do not sound like something that is going
to be merged fast. Since I'm also planning to send a separate series that
removes explicit setting of `ARCH` from subsystem Makefiles (if they do it
for the sole purpose of working around this issue) I'd prefer to leave this
patch separate for easier reference and potentially faster merging.

Referring to it should hopefully be sufficient to prevent it from being
forgotten.

	- Best Valentin

>
>
> thanks,


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] selftests: default to host arch for LLVM builds
  2024-04-30 11:44       ` Valentin Obst
@ 2024-04-30 14:50         ` Mark Brown
  2024-04-30 16:58           ` John Hubbard
  0 siblings, 1 reply; 8+ messages in thread
From: Mark Brown @ 2024-04-30 14:50 UTC (permalink / raw)
  To: Valentin Obst
  Cc: jhubbard, anders.roxell, bpoirier, linux-kernel, linux-kselftest,
	mpdesouza, nathan, sashal, shuah

[-- Attachment #1: Type: text/plain, Size: 848 bytes --]

On Tue, Apr 30, 2024 at 01:44:52PM +0200, Valentin Obst wrote:
> On 4/29/24 12:04 AM, John Hubbard wrote:

> > I'm expecting to post the series soon, hopefully this week. And I'm
> > thinking maybe I should carry your patch as the first one in the series,
> > in order to ensure it gets picked up. Or, I can just refer to it as a
> > prerequisite in the cover letter.

> Correct me if I'm wrong, but intuitively 30+ patches that touch selftests
> from many different subsystems do not sound like something that is going
> to be merged fast. Since I'm also planning to send a separate series that

It just seems unhelpful to lump everything into a single series - a
large portion of the selftests go with the subystems so you'd be
creating a bunch of cross subsystem issues and large serieses with big
CC lists tend to be both offputting and noisy.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] selftests: default to host arch for LLVM builds
  2024-04-30 14:50         ` Mark Brown
@ 2024-04-30 16:58           ` John Hubbard
  0 siblings, 0 replies; 8+ messages in thread
From: John Hubbard @ 2024-04-30 16:58 UTC (permalink / raw)
  To: Mark Brown, Valentin Obst
  Cc: anders.roxell, bpoirier, linux-kernel, linux-kselftest,
	mpdesouza, nathan, sashal, shuah

On 4/30/24 7:50 AM, Mark Brown wrote:
> On Tue, Apr 30, 2024 at 01:44:52PM +0200, Valentin Obst wrote:
>> On 4/29/24 12:04 AM, John Hubbard wrote:
>> Correct me if I'm wrong, but intuitively 30+ patches that touch selftests
>> from many different subsystems do not sound like something that is going
>> to be merged fast. Since I'm also planning to send a separate series that
> 
> It just seems unhelpful to lump everything into a single series - a
> large portion of the selftests go with the subystems so you'd be
> creating a bunch of cross subsystem issues and large serieses with big
> CC lists tend to be both offputting and noisy.

Thanks for this timely advice! I was on the fence about the best way to
send these out. I'll break it up into subsystem patchsets (that don't
directly include Valentin's patch, but do refer to it) instead, which as
you say, should go *much* more smoothly.

In fact, just yesterday I fixed the last warning, so I'll start posting
some much smaller patchsets later today.


thanks,
-- 
John Hubbard
NVIDIA


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] selftests: default to host arch for LLVM builds
  2024-04-28 12:08   ` Valentin Obst
  2024-04-28 22:04     ` John Hubbard
@ 2024-05-03 19:55     ` Shuah Khan
  1 sibling, 0 replies; 8+ messages in thread
From: Shuah Khan @ 2024-05-03 19:55 UTC (permalink / raw)
  To: Valentin Obst, jhubbard
  Cc: anders.roxell, bpoirier, broonie, guillaume.tucker, linux-kernel,
	linux-kselftest, mpdesouza, nathan, sashal, shuah, Shuah Khan

On 4/28/24 06:08, 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
> 
> Yes, I agree that the title should contain the word 'fix' somewhere. For
> me its okay if maintainers reword the title when applying the patch,
> alternatively I can send a v2. (Is it still a v2 if I change the title, or
> rather a new patch?).
> 
> Any thoughts on whether this also needs a 'Cc stable'? Its not quite
> clear to me if this fix meets the requirements. As above, no objections if
> maintainers should decide to add it.
> 
>>
>> ?
>>
>> 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@nvidia.com>
> 

Thanks for the patch. Applied to linux-kselftest next for Linux 6.10-rc1

thanks,
-- Shuah


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2024-05-03 19:55 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-03-29 10:49 [PATCH] selftests: default to host arch for LLVM builds Valentin Obst
2024-04-13 22:10 ` John Hubbard
2024-04-28 12:08   ` Valentin Obst
2024-04-28 22:04     ` John Hubbard
2024-04-30 11:44       ` Valentin Obst
2024-04-30 14:50         ` Mark Brown
2024-04-30 16:58           ` John Hubbard
2024-05-03 19:55     ` Shuah Khan

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.