Re: [PATCH] kbuild: clang: do not use CROSS_COMPILE for target triple

[Nathan Chancellor]

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