diff mbox series

[3/4] Makefile: lld: tell clang to use lld

Message ID 20190207220155.81351-3-ndesaulniers@google.com
State New
Headers show
Series [1/4] init/Kconfig: add config support for detecting linker | expand

Commit Message

Nick Desaulniers Feb. 7, 2019, 10:01 p.m. UTC
This is needed because clang doesn't select which linker to use based on
$LD but rather -fuse-ld=$(LD). This is problematic especially for
cc-ldoption, which checks for linker flag support via invoking the
compiler, rather than the linker.

Link: https://github.com/ClangBuiltLinux/linux/issues/342
Suggested-by: Nathan Chancellor <natechancellor@gmail.com>
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>

---
 Makefile | 3 +++
 1 file changed, 3 insertions(+)

-- 
2.20.1.791.gb4d0f1c61a-goog

Comments

Nathan Chancellor Feb. 8, 2019, 5:50 a.m. UTC | #1
On Thu, Feb 07, 2019 at 02:01:51PM -0800, ndesaulniers@google.com wrote:
> This is needed because clang doesn't select which linker to use based on

> $LD but rather -fuse-ld=$(LD). This is problematic especially for

> cc-ldoption, which checks for linker flag support via invoking the

> compiler, rather than the linker.

> 

> Link: https://github.com/ClangBuiltLinux/linux/issues/342

> Suggested-by: Nathan Chancellor <natechancellor@gmail.com>

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

> ---

>  Makefile | 3 +++

>  1 file changed, 3 insertions(+)

> 

> diff --git a/Makefile b/Makefile

> index 0eae4277206e..6307c17259ea 100644

> --- a/Makefile

> +++ b/Makefile

> @@ -502,6 +502,9 @@ endif

>  CLANG_FLAGS	+= -no-integrated-as

>  KBUILD_CFLAGS	+= $(CLANG_FLAGS)

>  KBUILD_AFLAGS	+= $(CLANG_FLAGS)

> +ifneq ($(shell $(LD) --version 2>&1 | head -n 1 | grep LLD),)

> +CLANG_FLAGS	+= -fuse-ld=lld

> +endif


This section needs to be moved up above KBUILD_CFLAGS otherwise it never
actually gets passed along.

With that change, please add:

Reviewed-by: Nathan Chancellor <natechancellor@gmail.com>

Tested-by: Nathan Chancellor <natechancellor@gmail.com>


>  export CLANG_FLAGS

>  endif

>  

> -- 

> 2.20.1.791.gb4d0f1c61a-goog

>
Nick Desaulniers Feb. 8, 2019, 5:27 p.m. UTC | #2
On Thu, Feb 7, 2019 at 9:51 PM Nathan Chancellor
<natechancellor@gmail.com> wrote:
>

> On Thu, Feb 07, 2019 at 02:01:51PM -0800, ndesaulniers@google.com wrote:

> > This is needed because clang doesn't select which linker to use based on

> > $LD but rather -fuse-ld=$(LD). This is problematic especially for

> > cc-ldoption, which checks for linker flag support via invoking the

> > compiler, rather than the linker.

> >

> > Link: https://github.com/ClangBuiltLinux/linux/issues/342

> > Suggested-by: Nathan Chancellor <natechancellor@gmail.com>

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

> > ---

> >  Makefile | 3 +++

> >  1 file changed, 3 insertions(+)

> >

> > diff --git a/Makefile b/Makefile

> > index 0eae4277206e..6307c17259ea 100644

> > --- a/Makefile

> > +++ b/Makefile

> > @@ -502,6 +502,9 @@ endif

> >  CLANG_FLAGS  += -no-integrated-as

> >  KBUILD_CFLAGS        += $(CLANG_FLAGS)

> >  KBUILD_AFLAGS        += $(CLANG_FLAGS)

> > +ifneq ($(shell $(LD) --version 2>&1 | head -n 1 | grep LLD),)

> > +CLANG_FLAGS  += -fuse-ld=lld


Sorry, Sedat reports [0] that this is not quite correct; if $(LD)
matches LLD, then we need to set -fuse-ld=$(LD), not -fuse-ld=lld,
since you may have lld installed as lld-7 or lld-8, for example:
https://github.com/ClangBuiltLinux/linux/issues/358

> > +endif

>

> This section needs to be moved up above KBUILD_CFLAGS otherwise it never

> actually gets passed along.

>

> With that change, please add:

>

> Reviewed-by: Nathan Chancellor <natechancellor@gmail.com>

> Tested-by: Nathan Chancellor <natechancellor@gmail.com>

>

> >  export CLANG_FLAGS

> >  endif

> >

> > --

> > 2.20.1.791.gb4d0f1c61a-goog

> >




-- 
Thanks,
~Nick Desaulniers
Nathan Chancellor Feb. 8, 2019, 5:45 p.m. UTC | #3
On Fri, Feb 08, 2019 at 09:27:19AM -0800, Nick Desaulniers wrote:
> On Thu, Feb 7, 2019 at 9:51 PM Nathan Chancellor

> <natechancellor@gmail.com> wrote:

> >

> > On Thu, Feb 07, 2019 at 02:01:51PM -0800, ndesaulniers@google.com wrote:

> > > This is needed because clang doesn't select which linker to use based on

> > > $LD but rather -fuse-ld=$(LD). This is problematic especially for

> > > cc-ldoption, which checks for linker flag support via invoking the

> > > compiler, rather than the linker.

> > >

> > > Link: https://github.com/ClangBuiltLinux/linux/issues/342

> > > Suggested-by: Nathan Chancellor <natechancellor@gmail.com>

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

> > > ---

> > >  Makefile | 3 +++

> > >  1 file changed, 3 insertions(+)

> > >

> > > diff --git a/Makefile b/Makefile

> > > index 0eae4277206e..6307c17259ea 100644

> > > --- a/Makefile

> > > +++ b/Makefile

> > > @@ -502,6 +502,9 @@ endif

> > >  CLANG_FLAGS  += -no-integrated-as

> > >  KBUILD_CFLAGS        += $(CLANG_FLAGS)

> > >  KBUILD_AFLAGS        += $(CLANG_FLAGS)

> > > +ifneq ($(shell $(LD) --version 2>&1 | head -n 1 | grep LLD),)

> > > +CLANG_FLAGS  += -fuse-ld=lld

> 

> Sorry, Sedat reports [0] that this is not quite correct; if $(LD)

> matches LLD, then we need to set -fuse-ld=$(LD), not -fuse-ld=lld,

> since you may have lld installed as lld-7 or lld-8, for example:

> https://github.com/ClangBuiltLinux/linux/issues/358

> 


We'll need to make sure that it is compatible with the way that Clang
expects arguments to '-fuse-ld=' because it looks like 'ld.' is
automatically appended.

https://github.com/llvm/llvm-project/blob/5be3dbdb082ffba4f846e3572ea802d156dd8d34/clang/lib/Driver/ToolChain.cpp#L438-L468

> > > +endif

> >

> > This section needs to be moved up above KBUILD_CFLAGS otherwise it never

> > actually gets passed along.

> >

> > With that change, please add:

> >

> > Reviewed-by: Nathan Chancellor <natechancellor@gmail.com>

> > Tested-by: Nathan Chancellor <natechancellor@gmail.com>

> >

> > >  export CLANG_FLAGS

> > >  endif

> > >

> > > --

> > > 2.20.1.791.gb4d0f1c61a-goog

> > >

> 

> 

> 

> -- 

> Thanks,

> ~Nick Desaulniers
Masahiro Yamada Feb. 11, 2019, 3:32 p.m. UTC | #4
On Sat, Feb 9, 2019 at 2:27 AM Nick Desaulniers <ndesaulniers@google.com> wrote:
>

> On Thu, Feb 7, 2019 at 9:51 PM Nathan Chancellor

> <natechancellor@gmail.com> wrote:

> >

> > On Thu, Feb 07, 2019 at 02:01:51PM -0800, ndesaulniers@google.com wrote:

> > > This is needed because clang doesn't select which linker to use based on

> > > $LD but rather -fuse-ld=$(LD). This is problematic especially for

> > > cc-ldoption, which checks for linker flag support via invoking the

> > > compiler, rather than the linker.


What do you mean by 'problematic' ?

I see no build error in
arch/arm64/kernel/vdso/Makefile, at least.

Clang uses BFD linker by default,
which may not match to the $(LD) passed in.

Is this what you mean?





> > > Link: https://github.com/ClangBuiltLinux/linux/issues/342

> > > Suggested-by: Nathan Chancellor <natechancellor@gmail.com>

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

> > > ---

> > >  Makefile | 3 +++

> > >  1 file changed, 3 insertions(+)

> > >

> > > diff --git a/Makefile b/Makefile

> > > index 0eae4277206e..6307c17259ea 100644

> > > --- a/Makefile

> > > +++ b/Makefile

> > > @@ -502,6 +502,9 @@ endif

> > >  CLANG_FLAGS  += -no-integrated-as

> > >  KBUILD_CFLAGS        += $(CLANG_FLAGS)

> > >  KBUILD_AFLAGS        += $(CLANG_FLAGS)

> > > +ifneq ($(shell $(LD) --version 2>&1 | head -n 1 | grep LLD),)

> > > +CLANG_FLAGS  += -fuse-ld=lld

>

> Sorry, Sedat reports [0] that this is not quite correct; if $(LD)

> matches LLD, then we need to set -fuse-ld=$(LD), not -fuse-ld=lld,

> since you may have lld installed as lld-7 or lld-8, for example:

> https://github.com/ClangBuiltLinux/linux/issues/358



Hmm, $(LD) might be a full path.

Looks like Clang can take a full-path for -fuse-ld=,
but does it work for GCC?






> > > +endif

> >

> > This section needs to be moved up above KBUILD_CFLAGS otherwise it never

> > actually gets passed along.

> >

> > With that change, please add:

> >

> > Reviewed-by: Nathan Chancellor <natechancellor@gmail.com>

> > Tested-by: Nathan Chancellor <natechancellor@gmail.com>

> >

> > >  export CLANG_FLAGS

> > >  endif

> > >

> > > --

> > > 2.20.1.791.gb4d0f1c61a-goog

> > >

>

>

>

> --

> Thanks,

> ~Nick Desaulniers




-- 
Best Regards
Masahiro Yamada
Sedat Dilek Feb. 11, 2019, 4:05 p.m. UTC | #5
On Mon, Feb 11, 2019 at 4:33 PM Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
[ ... ]
> > > > Link: https://github.com/ClangBuiltLinux/linux/issues/342

> > > > Suggested-by: Nathan Chancellor <natechancellor@gmail.com>

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

> > > > ---

> > > >  Makefile | 3 +++

> > > >  1 file changed, 3 insertions(+)

> > > >

> > > > diff --git a/Makefile b/Makefile

> > > > index 0eae4277206e..6307c17259ea 100644

> > > > --- a/Makefile

> > > > +++ b/Makefile

> > > > @@ -502,6 +502,9 @@ endif

> > > >  CLANG_FLAGS  += -no-integrated-as

> > > >  KBUILD_CFLAGS        += $(CLANG_FLAGS)

> > > >  KBUILD_AFLAGS        += $(CLANG_FLAGS)

> > > > +ifneq ($(shell $(LD) --version 2>&1 | head -n 1 | grep LLD),)

> > > > +CLANG_FLAGS  += -fuse-ld=lld

> >

> > Sorry, Sedat reports [0] that this is not quite correct; if $(LD)

> > matches LLD, then we need to set -fuse-ld=$(LD), not -fuse-ld=lld,

> > since you may have lld installed as lld-7 or lld-8, for example:

> > https://github.com/ClangBuiltLinux/linux/issues/358

>

>

> Hmm, $(LD) might be a full path.

>

> Looks like Clang can take a full-path for -fuse-ld=,

> but does it work for GCC?

>


Nope. GCC expects "-fuse-ld={bfd,gold,lld} ".
Adding suffix "$-VER" was not recognized while building a
llvm-toolchain, so I guess this will be the same for the Linux kernel
side.

- Sedat -

[1] https://gcc.gnu.org/onlinedocs/gcc/Link-Options.html
[2] https://github.com/ClangBuiltLinux/linux/issues/358
Nick Desaulniers Feb. 11, 2019, 7:26 p.m. UTC | #6
On Thu, Feb 7, 2019 at 9:51 PM Nathan Chancellor
<natechancellor@gmail.com> wrote:
>

> On Thu, Feb 07, 2019 at 02:01:51PM -0800, ndesaulniers@google.com wrote:

> > This is needed because clang doesn't select which linker to use based on

> > $LD but rather -fuse-ld=$(LD). This is problematic especially for

> > cc-ldoption, which checks for linker flag support via invoking the

> > compiler, rather than the linker.

> >

> > Link: https://github.com/ClangBuiltLinux/linux/issues/342

> > Suggested-by: Nathan Chancellor <natechancellor@gmail.com>

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

> > ---

> >  Makefile | 3 +++

> >  1 file changed, 3 insertions(+)

> >

> > diff --git a/Makefile b/Makefile

> > index 0eae4277206e..6307c17259ea 100644

> > --- a/Makefile

> > +++ b/Makefile

> > @@ -502,6 +502,9 @@ endif

> >  CLANG_FLAGS  += -no-integrated-as

> >  KBUILD_CFLAGS        += $(CLANG_FLAGS)

> >  KBUILD_AFLAGS        += $(CLANG_FLAGS)

> > +ifneq ($(shell $(LD) --version 2>&1 | head -n 1 | grep LLD),)

> > +CLANG_FLAGS  += -fuse-ld=lld

> > +endif

>

> This section needs to be moved up above KBUILD_CFLAGS otherwise it never

> actually gets passed along.


Great catch! I had that correct in my Pixel kernel patches; messed
that up forward porting them.  Included in v2 (sending imminently).

-- 
Thanks,
~Nick Desaulniers
Nick Desaulniers Feb. 11, 2019, 7:27 p.m. UTC | #7
On Mon, Feb 11, 2019 at 7:33 AM Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
>

> On Sat, Feb 9, 2019 at 2:27 AM Nick Desaulniers <ndesaulniers@google.com> wrote:

> >

> > On Thu, Feb 7, 2019 at 9:51 PM Nathan Chancellor

> > <natechancellor@gmail.com> wrote:

> > >

> > > On Thu, Feb 07, 2019 at 02:01:51PM -0800, ndesaulniers@google.com wrote:

> > > > This is needed because clang doesn't select which linker to use based on

> > > > $LD but rather -fuse-ld=$(LD). This is problematic especially for

> > > > cc-ldoption, which checks for linker flag support via invoking the

> > > > compiler, rather than the linker.

>

> What do you mean by 'problematic' ?

>

> I see no build error in

> arch/arm64/kernel/vdso/Makefile, at least.

>

> Clang uses BFD linker by default,

> which may not match to the $(LD) passed in.

>

> Is this what you mean?


Right, when using a linker other than BFD, we want cc-ldoption to tell
us whether that linker supports a linker flag or not.

-- 
Thanks,
~Nick Desaulniers
Nick Desaulniers Feb. 11, 2019, 7:30 p.m. UTC | #8
On Mon, Feb 11, 2019 at 8:05 AM Sedat Dilek <sedat.dilek@gmail.com> wrote:
>

> On Mon, Feb 11, 2019 at 4:33 PM Masahiro Yamada

> <yamada.masahiro@socionext.com> wrote:

> [ ... ]

> > > > > Link: https://github.com/ClangBuiltLinux/linux/issues/342

> > > > > Suggested-by: Nathan Chancellor <natechancellor@gmail.com>

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

> > > > > ---

> > > > >  Makefile | 3 +++

> > > > >  1 file changed, 3 insertions(+)

> > > > >

> > > > > diff --git a/Makefile b/Makefile

> > > > > index 0eae4277206e..6307c17259ea 100644

> > > > > --- a/Makefile

> > > > > +++ b/Makefile

> > > > > @@ -502,6 +502,9 @@ endif

> > > > >  CLANG_FLAGS  += -no-integrated-as

> > > > >  KBUILD_CFLAGS        += $(CLANG_FLAGS)

> > > > >  KBUILD_AFLAGS        += $(CLANG_FLAGS)

> > > > > +ifneq ($(shell $(LD) --version 2>&1 | head -n 1 | grep LLD),)

> > > > > +CLANG_FLAGS  += -fuse-ld=lld

> > >

> > > Sorry, Sedat reports [0] that this is not quite correct; if $(LD)

> > > matches LLD, then we need to set -fuse-ld=$(LD), not -fuse-ld=lld,

> > > since you may have lld installed as lld-7 or lld-8, for example:

> > > https://github.com/ClangBuiltLinux/linux/issues/358

> >

> >

> > Hmm, $(LD) might be a full path.

> >

> > Looks like Clang can take a full-path for -fuse-ld=,

> > but does it work for GCC?

> >

>

> Nope. GCC expects "-fuse-ld={bfd,gold,lld} ".

> Adding suffix "$-VER" was not recognized while building a

> llvm-toolchain, so I guess this will be the same for the Linux kernel

> side.


Well for now, let's add support for just `ld.lld` since a build of
llvm will create such a symlink.  If folks really need to
differentiate between different versions of lld, let's cross that
bridge when we get there.
-- 
Thanks,
~Nick Desaulniers
diff mbox series

Patch

diff --git a/Makefile b/Makefile
index 0eae4277206e..6307c17259ea 100644
--- a/Makefile
+++ b/Makefile
@@ -502,6 +502,9 @@  endif
 CLANG_FLAGS	+= -no-integrated-as
 KBUILD_CFLAGS	+= $(CLANG_FLAGS)
 KBUILD_AFLAGS	+= $(CLANG_FLAGS)
+ifneq ($(shell $(LD) --version 2>&1 | head -n 1 | grep LLD),)
+CLANG_FLAGS	+= -fuse-ld=lld
+endif
 export CLANG_FLAGS
 endif