[PATCH] Makefile: Fix GCC_TOOLCHAIN_DIR prefix for Clang cross compilation

[PATCH] Makefile: Fix GCC_TOOLCHAIN_DIR prefix for Clang cross compilation

[Fangrui Song]

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

Re: [PATCH] Makefile: Fix GCC_TOOLCHAIN_DIR prefix for Clang cross compilation

[Nathan Chancellor]

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
> 

Re: [PATCH] Makefile: Fix GCC_TOOLCHAIN_DIR prefix for Clang cross compilation

[Nick Desaulniers]

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

-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] Makefile: Fix GCC_TOOLCHAIN_DIR prefix for Clang cross compilation

[Fangrui Song]

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