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

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

[Sedat Dilek]

Hi Nick,

why don't you simply check for CONFIG_LD_IS_LLD existing (when [1] applied)?

If you want a specific version of ld.lld (you mentioned kernel module
linking failure was fixed recently) why not transfer this into a
scripts/lld-version.sh file?

Greetings,
- Sedat -

[1] https://lore.kernel.org/patchwork/patch/1039719/ ("[1/4]
init/Kconfig: add config support for detecting linker")

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

[Nathan Chancellor]

On Fri, Feb 08, 2019 at 03:21:15PM +0100, Sedat Dilek wrote:
> Hi Nick,
> 
> why don't you simply check for CONFIG_LD_IS_LLD existing (when [1] applied)?
> 

Hi Sedat,

This section runs before Kconfig so we can't rely on the symbols it
generates. See commit 995167420797 ("kbuild: remove cc-name variable")
for Masahiro's explanation.

> If you want a specific version of ld.lld (you mentioned kernel module
> linking failure was fixed recently) why not transfer this into a
> scripts/lld-version.sh file?
> 

We'll probably need that eventually but given that lld isn't ready for
widespread use, I don't think it's necessary.

> Greetings,
> - Sedat -
> 
> [1] https://lore.kernel.org/patchwork/patch/1039719/ ("[1/4]
> init/Kconfig: add config support for detecting linker")

Nathan

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

[ndesaulniers]

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
 export CLANG_FLAGS
 endif
 
-- 
2.20.1.791.gb4d0f1c61a-goog

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

[Nathan Chancellor]

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
> 

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

[Nick Desaulniers]

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

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

[Nathan Chancellor]

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

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

[Masahiro Yamada]

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

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

[Sedat Dilek]

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

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

[Nick Desaulniers]

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

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

[Nick Desaulniers]

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

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

[Nick Desaulniers]

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