| Message ID | 20200720181237.3015826-1-maskray@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Makefile: Fix GCC_TOOLCHAIN_DIR prefix for Clang cross compilation | expand |
On Mon, Jul 20, 2020 at 11:12:22AM -0700, Fangrui Song wrote: > When CROSS_COMPILE is set (e.g. aarch64-linux-gnu-), if > $(CROSS_COMPILE)elfedit is found at /usr/bin/aarch64-linux-gnu-, > GCC_TOOLCHAIN_DIR will be set to /usr/bin/. --prefix= will be set to > /usr/bin/ and Clang as of 11 will search for both > $(prefix)aarch64-linux-gnu-$needle and $(prefix)$needle. > > GCC searchs for $(prefix)aarch64-linux-gnu/$version/$needle, > $(prefix)aarch64-linux-gnu/$needle and $(prefix)$needle. In practice, > $(prefix)aarch64-linux-gnu/$needle rarely contains executables. > > To better model how GCC's -B/--prefix takes in effect in practice, newer > Clang only searches for $(prefix)$needle and for example it will find > /usr/bin/as instead of /usr/bin/aarch64-linux-gnu-as. > > Set --prefix= to $(GCC_TOOLCHAIN_DIR)$(CROSS_COMPILE) > (/usr/bin/aarch64-linux-gnu-) so that newer Clang can find the > appropriate cross compiling GNU as (when -no-integrated-as is in > effect). > > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> > Signed-off-by: Fangrui Song <maskray@google.com> > Link: https://github.com/ClangBuiltLinux/linux/issues/1099 Sorry that I did not pay attention before but this needs Cc: stable@vger.kernel.org in the body so that it gets automatically backported into all of our stable branches. I am not sure if Masahiro is okay with adding that after the fact or if he will want a v2. I am fine with having my signed-off-by on the patch but I did not really do much :) I am fine with having that downgraded to Reviewed-by: Nathan Chancellor <natechancellor@gmail.com> Tested-by: Nathan Chancellor <natechancellor@gmail.com> if people find it odd. Thanks for sending this along! Cheers, Nathan > --- > Makefile | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/Makefile b/Makefile > index 0b5f8538bde5..3ac83e375b61 100644 > --- a/Makefile > +++ b/Makefile > @@ -567,7 +567,7 @@ ifneq ($(shell $(CC) --version 2>&1 | head -n 1 | grep clang),) > ifneq ($(CROSS_COMPILE),) > CLANG_FLAGS += --target=$(notdir $(CROSS_COMPILE:%-=%)) > GCC_TOOLCHAIN_DIR := $(dir $(shell which $(CROSS_COMPILE)elfedit)) > -CLANG_FLAGS += --prefix=$(GCC_TOOLCHAIN_DIR) > +CLANG_FLAGS += --prefix=$(GCC_TOOLCHAIN_DIR)$(CROSS_COMPILE) > GCC_TOOLCHAIN := $(realpath $(GCC_TOOLCHAIN_DIR)/..) > endif > ifneq ($(GCC_TOOLCHAIN),) > -- > 2.28.0.rc0.105.gf9edc3c819-goog >
On Mon, Jul 20, 2020 at 11:16 AM Nathan Chancellor <natechancellor@gmail.com> wrote: > > On Mon, Jul 20, 2020 at 11:12:22AM -0700, Fangrui Song wrote: > > When CROSS_COMPILE is set (e.g. aarch64-linux-gnu-), if > > $(CROSS_COMPILE)elfedit is found at /usr/bin/aarch64-linux-gnu-, > > GCC_TOOLCHAIN_DIR will be set to /usr/bin/. --prefix= will be set to > > /usr/bin/ and Clang as of 11 will search for both > > $(prefix)aarch64-linux-gnu-$needle and $(prefix)$needle. > > > > GCC searchs for $(prefix)aarch64-linux-gnu/$version/$needle, > > $(prefix)aarch64-linux-gnu/$needle and $(prefix)$needle. In practice, > > $(prefix)aarch64-linux-gnu/$needle rarely contains executables. > > > > To better model how GCC's -B/--prefix takes in effect in practice, newer > > Clang only searches for $(prefix)$needle and for example it will find "newer Clang" requires the reader to recall that "Clang as of 11" was the previous frame of reference. I think it would be clearer to: 1. call of clang-12 as having a difference in behavior. 2. explicitly link to the commit, ie: Link: https://github.com/llvm/llvm-project/commit/3452a0d8c17f7166f479706b293caf6ac76ffd90 > > /usr/bin/as instead of /usr/bin/aarch64-linux-gnu-as. That's a common source of pain (for example, when cross compiling without having the proper cross binutils installed, it's common to get spooky errors about unsupported configs or host binutils not recognizing flags specific to cross building). /usr/bin/as: unrecognized option '-EL' being the most common. So in that case, I'm actually very happy with the llvm change if it solves that particularly common pain point. > > > > Set --prefix= to $(GCC_TOOLCHAIN_DIR)$(CROSS_COMPILE) > > (/usr/bin/aarch64-linux-gnu-) so that newer Clang can find the > > appropriate cross compiling GNU as (when -no-integrated-as is in > > effect). > > > > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> > > Signed-off-by: Fangrui Song <maskray@google.com> > > Link: https://github.com/ClangBuiltLinux/linux/issues/1099 > > Sorry that I did not pay attention before but this needs > > Cc: stable@vger.kernel.org Agreed. This change to llvm will blow up all of our CI jobs that cross compile if not backported to stable. > > in the body so that it gets automatically backported into all of our > stable branches. I am not sure if Masahiro is okay with adding that > after the fact or if he will want a v2. > > I am fine with having my signed-off-by on the patch but I did not really > do much :) I am fine with having that downgraded to Not a big deal, but there's only really two cases I can think of where it's appropriate to attach someone else's "SOB" to a patch: 1. It's their patch that you're resending on their behalf, possibly as part of a larger series. 2. You're a maintainer, and...well I guess that's also case 1 above. Reported-by is more appropriate, and you can include the tags collected from this thread. Please ping me internally for help sending a v2, if needed. > > Reviewed-by: Nathan Chancellor <natechancellor@gmail.com> > Tested-by: Nathan Chancellor <natechancellor@gmail.com> I tested with this llvm pre- and post- https://github.com/llvm/llvm-project/commit/3452a0d8c17f7166f479706b293caf6ac76ffd90. I also tested downstream Android kernel builds with 3452a0d8c17f7166f479706b293caf6ac76ffd90. Builds that don't make use of CROSS_COMPILE (native host targets) are obviously unaffected. We might see this issue pop up a few more times internally if the patch isn't picked up by stable, or if those downstream kernel trees don't rebase on stable kernel trees as often as they refresh their toolchain. Tested-by: Nick Desaulniers <ndesaulniers@google.com> > > if people find it odd. > > Thanks for sending this along! > > Cheers, > Nathan > > > --- > > Makefile | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/Makefile b/Makefile > > index 0b5f8538bde5..3ac83e375b61 100644 > > --- a/Makefile > > +++ b/Makefile > > @@ -567,7 +567,7 @@ ifneq ($(shell $(CC) --version 2>&1 | head -n 1 | grep clang),) > > ifneq ($(CROSS_COMPILE),) > > CLANG_FLAGS += --target=$(notdir $(CROSS_COMPILE:%-=%)) > > GCC_TOOLCHAIN_DIR := $(dir $(shell which $(CROSS_COMPILE)elfedit)) > > -CLANG_FLAGS += --prefix=$(GCC_TOOLCHAIN_DIR) > > +CLANG_FLAGS += --prefix=$(GCC_TOOLCHAIN_DIR)$(CROSS_COMPILE) > > GCC_TOOLCHAIN := $(realpath $(GCC_TOOLCHAIN_DIR)/..) > > endif > > ifneq ($(GCC_TOOLCHAIN),) > > -- > > 2.28.0.rc0.105.gf9edc3c819-goog > > > > --
On 2020-07-20, Nick Desaulniers wrote: >On Mon, Jul 20, 2020 at 11:16 AM Nathan Chancellor ><natechancellor@gmail.com> wrote: >> >> On Mon, Jul 20, 2020 at 11:12:22AM -0700, Fangrui Song wrote: >> > When CROSS_COMPILE is set (e.g. aarch64-linux-gnu-), if >> > $(CROSS_COMPILE)elfedit is found at /usr/bin/aarch64-linux-gnu-, >> > GCC_TOOLCHAIN_DIR will be set to /usr/bin/. --prefix= will be set to >> > /usr/bin/ and Clang as of 11 will search for both >> > $(prefix)aarch64-linux-gnu-$needle and $(prefix)$needle. >> > >> > GCC searchs for $(prefix)aarch64-linux-gnu/$version/$needle, >> > $(prefix)aarch64-linux-gnu/$needle and $(prefix)$needle. In practice, >> > $(prefix)aarch64-linux-gnu/$needle rarely contains executables. >> > >> > To better model how GCC's -B/--prefix takes in effect in practice, newer >> > Clang only searches for $(prefix)$needle and for example it will find > >"newer Clang" requires the reader to recall that "Clang as of 11" was >the previous frame of reference. I think it would be clearer to: >1. call of clang-12 as having a difference in behavior. >2. explicitly link to the commit, ie: >Link: https://github.com/llvm/llvm-project/commit/3452a0d8c17f7166f479706b293caf6ac76ffd90 > >> > /usr/bin/as instead of /usr/bin/aarch64-linux-gnu-as. > >That's a common source of pain (for example, when cross compiling >without having the proper cross binutils installed, it's common to get >spooky errors about unsupported configs or host binutils not >recognizing flags specific to cross building). > >/usr/bin/as: unrecognized option '-EL' > >being the most common. So in that case, I'm actually very happy with >the llvm change if it solves that particularly common pain point. > >> > >> > Set --prefix= to $(GCC_TOOLCHAIN_DIR)$(CROSS_COMPILE) >> > (/usr/bin/aarch64-linux-gnu-) so that newer Clang can find the >> > appropriate cross compiling GNU as (when -no-integrated-as is in >> > effect). >> > >> > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> >> > Signed-off-by: Fangrui Song <maskray@google.com> >> > Link: https://github.com/ClangBuiltLinux/linux/issues/1099 >> >> Sorry that I did not pay attention before but this needs >> >> Cc: stable@vger.kernel.org > >Agreed. This change to llvm will blow up all of our CI jobs that >cross compile if not backported to stable. Thanks. I did not know this. >> >> in the body so that it gets automatically backported into all of our >> stable branches. I am not sure if Masahiro is okay with adding that >> after the fact or if he will want a v2. >> >> I am fine with having my signed-off-by on the patch but I did not really >> do much :) I am fine with having that downgraded to > >Not a big deal, but there's only really two cases I can think of where >it's appropriate to attach someone else's "SOB" to a patch: >1. It's their patch that you're resending on their behalf, possibly as >part of a larger series. >2. You're a maintainer, and...well I guess that's also case 1 above. > >Reported-by is more appropriate, and you can include the tags >collected from this thread. Please ping me internally for help >sending a v2, if needed. Nathan's draft patch on https://github.com/ClangBuiltLinux/linux/issues/1099 actually works. I removed the slash. The words are my own. Since Nathan explicitly requested a downgrade of his tag, I'll do that for V2. I'll do that anyway because I need to fix a typo in the description: $(CROSS_COMPILE)elfedit is found at /usr/bin/aarch64-linux-gnu- $(CROSS_COMPILE)elfedit is found at /usr/bin/aarch64-linux-gnu-elfedit >> >> Reviewed-by: Nathan Chancellor <natechancellor@gmail.com> >> Tested-by: Nathan Chancellor <natechancellor@gmail.com> > >I tested with this llvm pre- and post- >https://github.com/llvm/llvm-project/commit/3452a0d8c17f7166f479706b293caf6ac76ffd90. >I also tested downstream Android kernel builds with >3452a0d8c17f7166f479706b293caf6ac76ffd90. Builds that don't make use >of CROSS_COMPILE (native host targets) are obviously unaffected. We >might see this issue pop up a few more times internally if the patch >isn't picked up by stable, or if those downstream kernel trees don't >rebase on stable kernel trees as often as they refresh their >toolchain. > >Tested-by: Nick Desaulniers <ndesaulniers@google.com> Thanks for offerring proofreading service! I'm working on the description... >> >> if people find it odd. >> >> Thanks for sending this along! >> >> Cheers, >> Nathan >> >> > --- >> > Makefile | 2 +- >> > 1 file changed, 1 insertion(+), 1 deletion(-) >> > >> > diff --git a/Makefile b/Makefile >> > index 0b5f8538bde5..3ac83e375b61 100644 >> > --- a/Makefile >> > +++ b/Makefile >> > @@ -567,7 +567,7 @@ ifneq ($(shell $(CC) --version 2>&1 | head -n 1 | grep clang),) >> > ifneq ($(CROSS_COMPILE),) >> > CLANG_FLAGS += --target=$(notdir $(CROSS_COMPILE:%-=%)) >> > GCC_TOOLCHAIN_DIR := $(dir $(shell which $(CROSS_COMPILE)elfedit)) >> > -CLANG_FLAGS += --prefix=$(GCC_TOOLCHAIN_DIR) >> > +CLANG_FLAGS += --prefix=$(GCC_TOOLCHAIN_DIR)$(CROSS_COMPILE) >> > GCC_TOOLCHAIN := $(realpath $(GCC_TOOLCHAIN_DIR)/..) >> > endif >> > ifneq ($(GCC_TOOLCHAIN),) >> > -- >> > 2.28.0.rc0.105.gf9edc3c819-goog >> > >> >> -- > >-- >Thanks, >~Nick Desaulniers
diff --git a/Makefile b/Makefile index 0b5f8538bde5..3ac83e375b61 100644 --- a/Makefile +++ b/Makefile @@ -567,7 +567,7 @@ ifneq ($(shell $(CC) --version 2>&1 | head -n 1 | grep clang),) ifneq ($(CROSS_COMPILE),) CLANG_FLAGS += --target=$(notdir $(CROSS_COMPILE:%-=%)) GCC_TOOLCHAIN_DIR := $(dir $(shell which $(CROSS_COMPILE)elfedit)) -CLANG_FLAGS += --prefix=$(GCC_TOOLCHAIN_DIR) +CLANG_FLAGS += --prefix=$(GCC_TOOLCHAIN_DIR)$(CROSS_COMPILE) GCC_TOOLCHAIN := $(realpath $(GCC_TOOLCHAIN_DIR)/..) endif ifneq ($(GCC_TOOLCHAIN),)