diff mbox series

kbuild: clang: do not use CROSS_COMPILE for target triple

Message ID 20230401170117.1580840-1-masahiroy@kernel.org
State New
Headers
Series kbuild: clang: do not use CROSS_COMPILE for target triple |

Commit Message

Masahiro Yamada April 1, 2023, 5:01 p.m. UTC 
  The target triple is overridden by the user-supplied CROSS_COMPILE,
but I do not see a good reason to support it. Users can use a new
architecture without adding CLANG_TARGET_FLAGS_*, but that would be
a rare case.

Use the hard-coded and deterministic target triple all the time.

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

 scripts/Makefile.clang | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

Comments

Nathan Chancellor April 3, 2023, 2:47 p.m. UTC | #1 
On Sun, Apr 02, 2023 at 02:01:17AM +0900, Masahiro Yamada wrote:
> The target triple is overridden by the user-supplied CROSS_COMPILE,
> but I do not see a good reason to support it. Users can use a new
> architecture without adding CLANG_TARGET_FLAGS_*, but that would be
> a rare case.
> 
> Use the hard-coded and deterministic target triple all the time.
> 
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>

I know of one bug where the value of '--target' matters:

https://github.com/ClangBuiltLinux/linux/issues/1244

This was fixed in LLVM 12.0.0. We are not testing this in our CI though,
so we would not get bit by this (we could bump the minimum supported
version of LLVM to 12.0.0 for this, we have talked recently about doing
it for other reasons).

I guess I cannot really think of a good reason not to do this aside from
that; the target triple should only affect code generation, rather than
tool selection (i.e., this does not take away the ability to use a
custom set of binutils with clang).

However, Nick is currently OOO and I would like his opinion voiced
before we commit to this. Consider this a tentative:

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

> ---
> 
>  scripts/Makefile.clang | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/scripts/Makefile.clang b/scripts/Makefile.clang
> index 70b354fa1cb4..9076cc939e87 100644
> --- a/scripts/Makefile.clang
> +++ b/scripts/Makefile.clang
> @@ -13,15 +13,11 @@ CLANG_TARGET_FLAGS_x86		:= x86_64-linux-gnu
>  CLANG_TARGET_FLAGS_um		:= $(CLANG_TARGET_FLAGS_$(SUBARCH))
>  CLANG_TARGET_FLAGS		:= $(CLANG_TARGET_FLAGS_$(SRCARCH))
>  
> -ifeq ($(CROSS_COMPILE),)
>  ifeq ($(CLANG_TARGET_FLAGS),)
> -$(error Specify CROSS_COMPILE or add '--target=' option to scripts/Makefile.clang)
> +$(error add '--target=' option to scripts/Makefile.clang)
>  else
>  CLANG_FLAGS	+= --target=$(CLANG_TARGET_FLAGS)
> -endif # CLANG_TARGET_FLAGS
> -else
> -CLANG_FLAGS	+= --target=$(notdir $(CROSS_COMPILE:%-=%))
> -endif # CROSS_COMPILE
> +endif
>  
>  ifeq ($(LLVM_IAS),0)
>  CLANG_FLAGS	+= -fno-integrated-as
> -- 
> 2.37.2
>
Nick Desaulniers April 7, 2023, 7:54 p.m. UTC | #2 
On Mon, Apr 3, 2023 at 7:48 AM Nathan Chancellor <nathan@kernel.org> wrote:
>
> On Sun, Apr 02, 2023 at 02:01:17AM +0900, Masahiro Yamada wrote:
> > The target triple is overridden by the user-supplied CROSS_COMPILE,
> > but I do not see a good reason to support it. Users can use a new
> > architecture without adding CLANG_TARGET_FLAGS_*, but that would be
> > a rare case.
> >
> > Use the hard-coded and deterministic target triple all the time.
> >
> > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
>
> I know of one bug where the value of '--target' matters:
>
> https://github.com/ClangBuiltLinux/linux/issues/1244
>
> This was fixed in LLVM 12.0.0. We are not testing this in our CI though,
> so we would not get bit by this (we could bump the minimum supported
> version of LLVM to 12.0.0 for this, we have talked recently about doing
> it for other reasons).
>
> I guess I cannot really think of a good reason not to do this aside from
> that; the target triple should only affect code generation, rather than
> tool selection (i.e., this does not take away the ability to use a
> custom set of binutils with clang).
>
> However, Nick is currently OOO and I would like his opinion voiced
> before we commit to this. Consider this a tentative:

Yeah, nothing I could think of; at this point CROSS_COMPILE is only
necessary for LLVM_IAS=0 builds and s390 (since LLD lacks s390
support) IIUC.

A user is more likely to adjust the --target for the host, which they
can do via USERCFLAGS or USERLDFLAGS, but not for the target.  I don't
think the gnu vs musl for the target triple makes a difference; we
might even be able to omit that part of the triple but I haven't
grepped through LLVM sources to see if that would result in
differences for codegen.

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

>
> Acked-by: Nathan Chancellor <nathan@kernel.org>
>
> > ---
> >
> >  scripts/Makefile.clang | 8 ++------
> >  1 file changed, 2 insertions(+), 6 deletions(-)
> >
> > diff --git a/scripts/Makefile.clang b/scripts/Makefile.clang
> > index 70b354fa1cb4..9076cc939e87 100644
> > --- a/scripts/Makefile.clang
> > +++ b/scripts/Makefile.clang
> > @@ -13,15 +13,11 @@ CLANG_TARGET_FLAGS_x86            := x86_64-linux-gnu
> >  CLANG_TARGET_FLAGS_um                := $(CLANG_TARGET_FLAGS_$(SUBARCH))
> >  CLANG_TARGET_FLAGS           := $(CLANG_TARGET_FLAGS_$(SRCARCH))
> >
> > -ifeq ($(CROSS_COMPILE),)
> >  ifeq ($(CLANG_TARGET_FLAGS),)
> > -$(error Specify CROSS_COMPILE or add '--target=' option to scripts/Makefile.clang)
> > +$(error add '--target=' option to scripts/Makefile.clang)
> >  else
> >  CLANG_FLAGS  += --target=$(CLANG_TARGET_FLAGS)
> > -endif # CLANG_TARGET_FLAGS
> > -else
> > -CLANG_FLAGS  += --target=$(notdir $(CROSS_COMPILE:%-=%))
> > -endif # CROSS_COMPILE
> > +endif
> >
> >  ifeq ($(LLVM_IAS),0)
> >  CLANG_FLAGS  += -fno-integrated-as
> > --
> > 2.37.2
> >
Masahiro Yamada April 16, 2023, 1:02 p.m. UTC | #3 
On Mon, Apr 3, 2023 at 11:48 PM Nathan Chancellor <nathan@kernel.org> wrote:
>
> On Sun, Apr 02, 2023 at 02:01:17AM +0900, Masahiro Yamada wrote:
> > The target triple is overridden by the user-supplied CROSS_COMPILE,
> > but I do not see a good reason to support it. Users can use a new
> > architecture without adding CLANG_TARGET_FLAGS_*, but that would be
> > a rare case.
> >
> > Use the hard-coded and deterministic target triple all the time.
> >
> > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
>
> I know of one bug where the value of '--target' matters:
>
> https://github.com/ClangBuiltLinux/linux/issues/1244


I did not look into it closely, but if we say
"
Using either CROSS_COMPILE=powerpc64-linux-gnu- or
CROSS_COMPILE=powerpc-linux-gnu- fixes it.
Using KCFLAGS=-v reveals that powerpc64le-linux-gnu-as is not getting
the endianness information.
", why didn't we fix it like the following?


 diff --git a/scripts/Makefile.clang b/scripts/Makefile.clang
 index 70b354fa1cb4..8dda7dc69c93 100644
 --- a/scripts/Makefile.clang
 +++ b/scripts/Makefile.clang
 @@ -6,7 +6,7 @@ CLANG_TARGET_FLAGS_arm64        := aarch64-linux-gnu
  CLANG_TARGET_FLAGS_hexagon     := hexagon-linux-musl
  CLANG_TARGET_FLAGS_m68k                := m68k-linux-gnu
  CLANG_TARGET_FLAGS_mips                := mipsel-linux-gnu
 -CLANG_TARGET_FLAGS_powerpc     := powerpc64le-linux-gnu
 +CLANG_TARGET_FLAGS_powerpc     := powerpc64-linux-gnu
  CLANG_TARGET_FLAGS_riscv       := riscv64-linux-gnu
  CLANG_TARGET_FLAGS_s390                := s390x-linux-gnu
  CLANG_TARGET_FLAGS_x86         := x86_64-linux-gnu



We do not need to test all possible target triples.
We can just use the one that is known to work.


Anyway, I will apply this patch. Thanks.


>
> This was fixed in LLVM 12.0.0. We are not testing this in our CI though,
> so we would not get bit by this (we could bump the minimum supported
> version of LLVM to 12.0.0 for this, we have talked recently about doing
> it for other reasons).
>
> I guess I cannot really think of a good reason not to do this aside from
> that; the target triple should only affect code generation, rather than
> tool selection (i.e., this does not take away the ability to use a
> custom set of binutils with clang).
>
> However, Nick is currently OOO and I would like his opinion voiced
> before we commit to this. Consider this a tentative:
>
> Acked-by: Nathan Chancellor <nathan@kernel.org>
>
> > ---
> >
> >  scripts/Makefile.clang | 8 ++------
> >  1 file changed, 2 insertions(+), 6 deletions(-)
> >
> > diff --git a/scripts/Makefile.clang b/scripts/Makefile.clang
> > index 70b354fa1cb4..9076cc939e87 100644
> > --- a/scripts/Makefile.clang
> > +++ b/scripts/Makefile.clang
> > @@ -13,15 +13,11 @@ CLANG_TARGET_FLAGS_x86            := x86_64-linux-gnu
> >  CLANG_TARGET_FLAGS_um                := $(CLANG_TARGET_FLAGS_$(SUBARCH))
> >  CLANG_TARGET_FLAGS           := $(CLANG_TARGET_FLAGS_$(SRCARCH))
> >
> > -ifeq ($(CROSS_COMPILE),)
> >  ifeq ($(CLANG_TARGET_FLAGS),)
> > -$(error Specify CROSS_COMPILE or add '--target=' option to scripts/Makefile.clang)
> > +$(error add '--target=' option to scripts/Makefile.clang)
> >  else
> >  CLANG_FLAGS  += --target=$(CLANG_TARGET_FLAGS)
> > -endif # CLANG_TARGET_FLAGS
> > -else
> > -CLANG_FLAGS  += --target=$(notdir $(CROSS_COMPILE:%-=%))
> > -endif # CROSS_COMPILE
> > +endif
> >
> >  ifeq ($(LLVM_IAS),0)
> >  CLANG_FLAGS  += -fno-integrated-as
> > --
> > 2.37.2
> >
Nathan Chancellor April 19, 2023, 7:24 p.m. UTC | #4 
On Sun, Apr 16, 2023 at 10:02:12PM +0900, Masahiro Yamada wrote:
> On Mon, Apr 3, 2023 at 11:48 PM Nathan Chancellor <nathan@kernel.org> wrote:
> >
> > On Sun, Apr 02, 2023 at 02:01:17AM +0900, Masahiro Yamada wrote:
> > > The target triple is overridden by the user-supplied CROSS_COMPILE,
> > > but I do not see a good reason to support it. Users can use a new
> > > architecture without adding CLANG_TARGET_FLAGS_*, but that would be
> > > a rare case.
> > >
> > > Use the hard-coded and deterministic target triple all the time.
> > >
> > > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> >
> > I know of one bug where the value of '--target' matters:
> >
> > https://github.com/ClangBuiltLinux/linux/issues/1244
> 
> 
> I did not look into it closely, but if we say

Heh, neither did I when I wrote the initial response to this patch,
because that issue turns out to be entirely irrelevant for this patch :)

> "
> Using either CROSS_COMPILE=powerpc64-linux-gnu- or
> CROSS_COMPILE=powerpc-linux-gnu- fixes it.
> Using KCFLAGS=-v reveals that powerpc64le-linux-gnu-as is not getting
> the endianness information.
> ", why didn't we fix it like the following?

It is because this already technically happens under the hood with
clang. '--target=powerpc64le-linux-gnu -mbig-endian' is functionally
equivalent to '--target=powerpc64-linux-gnu'. The issue is that
'--target=powerpc64-linux-gnu' and '--target=powerpc-linux-gnu' were not
passing along '-mbig-endian' to the assembler, so
powerpc64le-linux-gnu-ld was expecting big endian object files due to
'-EB' in LDFLAGS but was getting little endian object files, since the
assembler's triple was little endian by default.

We could work around this in the kernel by explicitly passing
'-Wa,-mlittle-endian' for these older versions of clang but I do not
think it is worth it, especially since it will just get removed once we
no longer support clang-11, which is our next uprev.

TL;DR: Patch should be fine, issue is tangential.

>  diff --git a/scripts/Makefile.clang b/scripts/Makefile.clang
>  index 70b354fa1cb4..8dda7dc69c93 100644
>  --- a/scripts/Makefile.clang
>  +++ b/scripts/Makefile.clang
>  @@ -6,7 +6,7 @@ CLANG_TARGET_FLAGS_arm64        := aarch64-linux-gnu
>   CLANG_TARGET_FLAGS_hexagon     := hexagon-linux-musl
>   CLANG_TARGET_FLAGS_m68k                := m68k-linux-gnu
>   CLANG_TARGET_FLAGS_mips                := mipsel-linux-gnu
>  -CLANG_TARGET_FLAGS_powerpc     := powerpc64le-linux-gnu
>  +CLANG_TARGET_FLAGS_powerpc     := powerpc64-linux-gnu
>   CLANG_TARGET_FLAGS_riscv       := riscv64-linux-gnu
>   CLANG_TARGET_FLAGS_s390                := s390x-linux-gnu
>   CLANG_TARGET_FLAGS_x86         := x86_64-linux-gnu
> 
> 
> 
> We do not need to test all possible target triples.
> 
> 
> We can just use the one that is known to work.
> Anyway, I will apply this patch. Thanks.
> 
> 
> >
> > This was fixed in LLVM 12.0.0. We are not testing this in our CI though,
> > so we would not get bit by this (we could bump the minimum supported
> > version of LLVM to 12.0.0 for this, we have talked recently about doing
> > it for other reasons).
> >
> > I guess I cannot really think of a good reason not to do this aside from
> > that; the target triple should only affect code generation, rather than
> > tool selection (i.e., this does not take away the ability to use a
> > custom set of binutils with clang).
> >
> > However, Nick is currently OOO and I would like his opinion voiced
> > before we commit to this. Consider this a tentative:
> >
> > Acked-by: Nathan Chancellor <nathan@kernel.org>
> >
> > > ---
> > >
> > >  scripts/Makefile.clang | 8 ++------
> > >  1 file changed, 2 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/scripts/Makefile.clang b/scripts/Makefile.clang
> > > index 70b354fa1cb4..9076cc939e87 100644
> > > --- a/scripts/Makefile.clang
> > > +++ b/scripts/Makefile.clang
> > > @@ -13,15 +13,11 @@ CLANG_TARGET_FLAGS_x86            := x86_64-linux-gnu
> > >  CLANG_TARGET_FLAGS_um                := $(CLANG_TARGET_FLAGS_$(SUBARCH))
> > >  CLANG_TARGET_FLAGS           := $(CLANG_TARGET_FLAGS_$(SRCARCH))
> > >
> > > -ifeq ($(CROSS_COMPILE),)
> > >  ifeq ($(CLANG_TARGET_FLAGS),)
> > > -$(error Specify CROSS_COMPILE or add '--target=' option to scripts/Makefile.clang)
> > > +$(error add '--target=' option to scripts/Makefile.clang)
> > >  else
> > >  CLANG_FLAGS  += --target=$(CLANG_TARGET_FLAGS)
> > > -endif # CLANG_TARGET_FLAGS
> > > -else
> > > -CLANG_FLAGS  += --target=$(notdir $(CROSS_COMPILE:%-=%))
> > > -endif # CROSS_COMPILE
> > > +endif
> > >
> > >  ifeq ($(LLVM_IAS),0)
> > >  CLANG_FLAGS  += -fno-integrated-as
> > > --
> > > 2.37.2
> > >
> 
> 
> 
> -- 
> Best Regards
> Masahiro Yamada
diff mbox series

Patch

diff --git a/scripts/Makefile.clang b/scripts/Makefile.clang
index 70b354fa1cb4..9076cc939e87 100644
--- a/scripts/Makefile.clang
+++ b/scripts/Makefile.clang
@@ -13,15 +13,11 @@  CLANG_TARGET_FLAGS_x86		:= x86_64-linux-gnu
 CLANG_TARGET_FLAGS_um		:= $(CLANG_TARGET_FLAGS_$(SUBARCH))
 CLANG_TARGET_FLAGS		:= $(CLANG_TARGET_FLAGS_$(SRCARCH))
 
-ifeq ($(CROSS_COMPILE),)
 ifeq ($(CLANG_TARGET_FLAGS),)
-$(error Specify CROSS_COMPILE or add '--target=' option to scripts/Makefile.clang)
+$(error add '--target=' option to scripts/Makefile.clang)
 else
 CLANG_FLAGS	+= --target=$(CLANG_TARGET_FLAGS)
-endif # CLANG_TARGET_FLAGS
-else
-CLANG_FLAGS	+= --target=$(notdir $(CROSS_COMPILE:%-=%))
-endif # CROSS_COMPILE
+endif
 
 ifeq ($(LLVM_IAS),0)
 CLANG_FLAGS	+= -fno-integrated-as