| Message ID | 20230401170117.1580840-1-masahiroy@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | kbuild: clang: do not use CROSS_COMPILE for target triple | expand |
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 >
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 > >
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 > >
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 --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
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(-)