[v3] Makefile: lld: tell clang to use lld

Message ID 20190402070858.49597-1-ndesaulniers@google.com
State New
Headers show
Series
  • [v3] Makefile: lld: tell clang to use lld
Related show

Commit Message

Nick Desaulniers April 2, 2019, 7:08 a.m.
This is needed because clang doesn't select which linker to use based on
$LD but rather -fuse-ld={bfd,gold,lld,<absolute path to linker>}.  This
is problematic especially for cc-ldoption, which checks for linker flag
support via invoking the compiler, rather than the linker.

Select the linker via absolute path from $PATH via `which`. This allows
you to build with:

$ make LD=ld.lld
$ make LD=ld.lld-8
$ make LD=/path/to/ld.lld

Add -Qunused-arguments to KBUILD_CPPFLAGS when using LLD, as otherwise
Clang likes to complain about -fuse-lld= being unused when compiling but
not linking (-c) such as when cc-option is used.

Link: https://github.com/ClangBuiltLinux/linux/issues/342
Link: https://github.com/ClangBuiltLinux/linux/issues/366
Link: https://github.com/ClangBuiltLinux/linux/issues/357
Suggested-by: Nathan Chancellor <natechancellor@gmail.com>
Suggested-by: Masahiro Yamada <yamada.masahiro@socionext.com>
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>

---
Changes V2->V3:
* Use absolute path based on `which $LD` as per Masahiro.
* Add -Qunused-arguments.
* Drop tested-by/reviewed-by tags, since this patched has changed enough
  to warrant re-testing/re-review, IMO.
* Add more info to the commit message.
Changes V1->V2:
* add reviewed and tested by tags.
* move this addition up 2 statments so that it's properly added to
  KBUILD_*FLAGS as per Nathan.

 Makefile | 4 ++++
 1 file changed, 4 insertions(+)

-- 
2.21.0.392.gf8f6787159e-goog

Comments

Nathan Chancellor April 2, 2019, 7:27 a.m. | #1
On Tue, Apr 02, 2019 at 12:08:58AM -0700, Nick Desaulniers wrote:
> This is needed because clang doesn't select which linker to use based on

> $LD but rather -fuse-ld={bfd,gold,lld,<absolute path to linker>}.  This

> is problematic especially for cc-ldoption, which checks for linker flag

> support via invoking the compiler, rather than the linker.

> 

> Select the linker via absolute path from $PATH via `which`. This allows

> you to build with:

> 

> $ make LD=ld.lld

> $ make LD=ld.lld-8

> $ make LD=/path/to/ld.lld

> 

> Add -Qunused-arguments to KBUILD_CPPFLAGS when using LLD, as otherwise

> Clang likes to complain about -fuse-lld= being unused when compiling but

> not linking (-c) such as when cc-option is used.

> 

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

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

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

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

> Suggested-by: Masahiro Yamada <yamada.masahiro@socionext.com>

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

> ---

> Changes V2->V3:

> * Use absolute path based on `which $LD` as per Masahiro.

> * Add -Qunused-arguments.

> * Drop tested-by/reviewed-by tags, since this patched has changed enough

>   to warrant re-testing/re-review, IMO.

> * Add more info to the commit message.

> Changes V1->V2:

> * add reviewed and tested by tags.

> * move this addition up 2 statments so that it's properly added to

>   KBUILD_*FLAGS as per Nathan.

> 

>  Makefile | 4 ++++

>  1 file changed, 4 insertions(+)

> 

> diff --git a/Makefile b/Makefile

> index 026fbc450906..895c076b6305 100644

> --- a/Makefile

> +++ b/Makefile

> @@ -514,6 +514,10 @@ ifneq ($(GCC_TOOLCHAIN),)

>  CLANG_FLAGS	+= --gcc-toolchain=$(GCC_TOOLCHAIN)

>  endif

>  CLANG_FLAGS	+= -no-integrated-as

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

> +CLANG_FLAGS	+= -fuse-ld=$(shell which $(LD))

> +KBUILD_CPPFLAGS += -Qunused-arguments


We should remove the -Qunused-arguments call in the second clang block
at the same time. With that change:

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

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


> +endif

>  KBUILD_CFLAGS	+= $(CLANG_FLAGS)

>  KBUILD_AFLAGS	+= $(CLANG_FLAGS)

>  export CLANG_FLAGS

> -- 

> 2.21.0.392.gf8f6787159e-goog

>
Sedat Dilek April 2, 2019, 7:52 a.m. | #2
On Tue, Apr 2, 2019 at 9:09 AM 'Nick Desaulniers' via Clang Built
Linux <clang-built-linux@googlegroups.com> wrote:
>

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

> $LD but rather -fuse-ld={bfd,gold,lld,<absolute path to linker>}.  This

> is problematic especially for cc-ldoption, which checks for linker flag

> support via invoking the compiler, rather than the linker.

>

> Select the linker via absolute path from $PATH via `which`. This allows

> you to build with:

>

> $ make LD=ld.lld

> $ make LD=ld.lld-8

> $ make LD=/path/to/ld.lld

>

> Add -Qunused-arguments to KBUILD_CPPFLAGS when using LLD, as otherwise

> Clang likes to complain about -fuse-lld= being unused when compiling but

> not linking (-c) such as when cc-option is used.

>

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

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

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

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

> Suggested-by: Masahiro Yamada <yamada.masahiro@socionext.com>

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


Thanks for bringing this up again, Nick.

Suggested-by: Sedat Dilek <sedat.dilek@gmail.com> (see [1]).

I suggest to do a separate patch on the move of  "KBUILD_CPPFLAGS +=
-Qunused-arguments".
( Patch was attached in [2],[3]. )

As pointed by Nathan this needs the removal in the original code-block.

Other than that:

Reviewed-by: Sedat Dilek <sedat.dilek@gmail.com>


[1] https://github.com/ClangBuiltLinux/linux/issues/366#issuecomment-466836735
[2] https://github.com/ClangBuiltLinux/linux/issues/366#issuecomment-467118373
[3] https://github.com/ClangBuiltLinux/linux/files/2901865/kbuild-clang-lld-Move-Qunused-arguments-CPPFLAGS-patch.txt

> ---

> Changes V2->V3:

> * Use absolute path based on `which $LD` as per Masahiro.

> * Add -Qunused-arguments.

> * Drop tested-by/reviewed-by tags, since this patched has changed enough

>   to warrant re-testing/re-review, IMO.

> * Add more info to the commit message.

> Changes V1->V2:

> * add reviewed and tested by tags.

> * move this addition up 2 statments so that it's properly added to

>   KBUILD_*FLAGS as per Nathan.

>

>  Makefile | 4 ++++

>  1 file changed, 4 insertions(+)

>

> diff --git a/Makefile b/Makefile

> index 026fbc450906..895c076b6305 100644

> --- a/Makefile

> +++ b/Makefile

> @@ -514,6 +514,10 @@ ifneq ($(GCC_TOOLCHAIN),)

>  CLANG_FLAGS    += --gcc-toolchain=$(GCC_TOOLCHAIN)

>  endif

>  CLANG_FLAGS    += -no-integrated-as

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

> +CLANG_FLAGS    += -fuse-ld=$(shell which $(LD))

> +KBUILD_CPPFLAGS += -Qunused-arguments

> +endif

>  KBUILD_CFLAGS  += $(CLANG_FLAGS)

>  KBUILD_AFLAGS  += $(CLANG_FLAGS)

>  export CLANG_FLAGS

> --

> 2.21.0.392.gf8f6787159e-goog

>

> --

> You received this message because you are subscribed to the Google Groups "Clang Built Linux" group.

> To unsubscribe from this group and stop receiving emails from it, send an email to clang-built-linux+unsubscribe@googlegroups.com.

> To post to this group, send email to clang-built-linux@googlegroups.com.

> To view this discussion on the web visit https://groups.google.com/d/msgid/clang-built-linux/20190402070858.49597-1-ndesaulniers%40google.com.

> For more options, visit https://groups.google.com/d/optout.
Nathan Chancellor April 2, 2019, 7:56 a.m. | #3
On Tue, Apr 02, 2019 at 09:52:13AM +0200, Sedat Dilek wrote:
> On Tue, Apr 2, 2019 at 9:09 AM 'Nick Desaulniers' via Clang Built

> Linux <clang-built-linux@googlegroups.com> wrote:

> >

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

> > $LD but rather -fuse-ld={bfd,gold,lld,<absolute path to linker>}.  This

> > is problematic especially for cc-ldoption, which checks for linker flag

> > support via invoking the compiler, rather than the linker.

> >

> > Select the linker via absolute path from $PATH via `which`. This allows

> > you to build with:

> >

> > $ make LD=ld.lld

> > $ make LD=ld.lld-8

> > $ make LD=/path/to/ld.lld

> >

> > Add -Qunused-arguments to KBUILD_CPPFLAGS when using LLD, as otherwise

> > Clang likes to complain about -fuse-lld= being unused when compiling but

> > not linking (-c) such as when cc-option is used.

> >

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

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

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

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

> > Suggested-by: Masahiro Yamada <yamada.masahiro@socionext.com>

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

> 

> Thanks for bringing this up again, Nick.

> 

> Suggested-by: Sedat Dilek <sedat.dilek@gmail.com> (see [1]).

> 

> I suggest to do a separate patch on the move of  "KBUILD_CPPFLAGS +=

> -Qunused-arguments".


I think it makes sense to do it all in one patch as it doesn't make much
sense to do the move without this change. Nick already sent a v4 doing
this.

Cheers,
Nathan

> ( Patch was attached in [2],[3]. )

> 

> As pointed by Nathan this needs the removal in the original code-block.

> 

> Other than that:

> 

> Reviewed-by: Sedat Dilek <sedat.dilek@gmail.com>

> 

> [1] https://github.com/ClangBuiltLinux/linux/issues/366#issuecomment-466836735

> [2] https://github.com/ClangBuiltLinux/linux/issues/366#issuecomment-467118373

> [3] https://github.com/ClangBuiltLinux/linux/files/2901865/kbuild-clang-lld-Move-Qunused-arguments-CPPFLAGS-patch.txt

> 

> > ---

> > Changes V2->V3:

> > * Use absolute path based on `which $LD` as per Masahiro.

> > * Add -Qunused-arguments.

> > * Drop tested-by/reviewed-by tags, since this patched has changed enough

> >   to warrant re-testing/re-review, IMO.

> > * Add more info to the commit message.

> > Changes V1->V2:

> > * add reviewed and tested by tags.

> > * move this addition up 2 statments so that it's properly added to

> >   KBUILD_*FLAGS as per Nathan.

> >

> >  Makefile | 4 ++++

> >  1 file changed, 4 insertions(+)

> >

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

> > index 026fbc450906..895c076b6305 100644

> > --- a/Makefile

> > +++ b/Makefile

> > @@ -514,6 +514,10 @@ ifneq ($(GCC_TOOLCHAIN),)

> >  CLANG_FLAGS    += --gcc-toolchain=$(GCC_TOOLCHAIN)

> >  endif

> >  CLANG_FLAGS    += -no-integrated-as

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

> > +CLANG_FLAGS    += -fuse-ld=$(shell which $(LD))

> > +KBUILD_CPPFLAGS += -Qunused-arguments

> > +endif

> >  KBUILD_CFLAGS  += $(CLANG_FLAGS)

> >  KBUILD_AFLAGS  += $(CLANG_FLAGS)

> >  export CLANG_FLAGS

> > --

> > 2.21.0.392.gf8f6787159e-goog

> >

> > --

> > You received this message because you are subscribed to the Google Groups "Clang Built Linux" group.

> > To unsubscribe from this group and stop receiving emails from it, send an email to clang-built-linux+unsubscribe@googlegroups.com.

> > To post to this group, send email to clang-built-linux@googlegroups.com.

> > To view this discussion on the web visit https://groups.google.com/d/msgid/clang-built-linux/20190402070858.49597-1-ndesaulniers%40google.com.

> > For more options, visit https://groups.google.com/d/optout.
Masahiro Yamada April 5, 2019, 10:16 a.m. | #4
On Tue, Apr 2, 2019 at 4:09 PM Nick Desaulniers <ndesaulniers@google.com> wrote:
>

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

> $LD but rather -fuse-ld={bfd,gold,lld,<absolute path to linker>}.  This

> is problematic especially for cc-ldoption, which checks for linker flag

> support via invoking the compiler, rather than the linker.

>

> Select the linker via absolute path from $PATH via `which`. This allows

> you to build with:

>

> $ make LD=ld.lld

> $ make LD=ld.lld-8

> $ make LD=/path/to/ld.lld

>

> Add -Qunused-arguments to KBUILD_CPPFLAGS when using LLD, as otherwise

> Clang likes to complain about -fuse-lld= being unused when compiling but

> not linking (-c) such as when cc-option is used.

>

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

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

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

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

> Suggested-by: Masahiro Yamada <yamada.masahiro@socionext.com>

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

> ---

> Changes V2->V3:

> * Use absolute path based on `which $LD` as per Masahiro.


This is not what I suggested.  I wanted to say:
"You cannot do this for GCC, so do not do it at all".

I want to propose alternative solution.
Please check the attached patches.

To apply 0002, you need the following as a prerequisite:

https://lore.kernel.org/patchwork/patch/1057685/



--
Best Regards
Masahiro Yamada
Kees Cook April 5, 2019, 4:11 p.m. | #5
On Fri, Apr 5, 2019 at 3:17 AM Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
> I want to propose alternative solution.

> Please check the attached patches.


In both patches:
> I changed the Makefile to use $(LD) rahter than $(CC). I confirmed

> the same vdso.so.raw was still produced.


typo: rahter -> rather

In the 0001 patch of arch/arm/vdso/Makefile:
> ...

> -VDSO_LDFLAGS += $(call cc-ldoption, -fuse-ld=bfd)

> +ldflags-y = -Bsymbolic --no-undefined -soname=linux-vdso.so.1 \

> +           -z max-page-size=4096 -z common-page-size=4096 \

> +           -nostdlib -shared \

> +           $(call ld-option, --hash-style=sysv) \

> +           $(call ld-option, --build-id) \

> +           -T


I see that "-fuse-ld=bfd" is explicitly dropped here, which could
reintroduce the problem fixed in d2b30cd4b722 ("ARM: 8384/1: VDSO:
force use of BFD linker") (and 3473f26592c1c). Does ld.gold still
exhibit this problem? If so, we'll need to detect gold and force bfd
still maybe?

-- 
Kees Cook
Nick Desaulniers April 5, 2019, 4:52 p.m. | #6
On Fri, Apr 5, 2019 at 9:11 AM Kees Cook <keescook@chromium.org> wrote:
>

> On Fri, Apr 5, 2019 at 3:17 AM Masahiro Yamada

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

> > I want to propose alternative solution.

> > Please check the attached patches.


```
My plan is to rewrite all VDSO Makefiles to use $(LD), then delete cc-ldoption.
```

I agree with that as a possible solution as well; I think it's best to
just invoke the linker directly rather than use the compiler as the
driver.  Just small nits with the 2 attached patches, but I think they
will also work for us.

Looks like there's 15 call sites across 12 files, plus Documentation/
and scripts/Kbuild.include; removing it seems feasible and I'd be
happy to help if you'd welcome the patches?

Let's start with that series of changes, but I suspect we will
ultimately need to revisit -fuse-ld=.  I was just thinking that even
my proposed patch doesn't do anything for KBUILD_HOSTCFLAGS, for
example.  I worry that bfd may still be invoked by Clang at some point
in the build.  I will try to test in an environment with no GNU
binutils.

> In the 0001 patch of arch/arm/vdso/Makefile:

> > ...

> > -VDSO_LDFLAGS += $(call cc-ldoption, -fuse-ld=bfd)

> > +ldflags-y = -Bsymbolic --no-undefined -soname=linux-vdso.so.1 \

> > +           -z max-page-size=4096 -z common-page-size=4096 \

> > +           -nostdlib -shared \

> > +           $(call ld-option, --hash-style=sysv) \


I think the vdso's should all REQUIRE --hash-style=sysv; the kselftest
for vdso's will fail otherwise.  I could always follow up with patches
for that if we didn't want to conflate changes.

> > +           $(call ld-option, --build-id) \

> > +           -T


Was it intentional to add -T standalone?  The man page for `ld` says
it needs an additional filename to follow -T.  Or rather is optional
if a -L flag is specified?

>

> I see that "-fuse-ld=bfd" is explicitly dropped here, which could

> reintroduce the problem fixed in d2b30cd4b722 ("ARM: 8384/1: VDSO:

> force use of BFD linker") (and 3473f26592c1c). Does ld.gold still

> exhibit this problem? If so, we'll need to detect gold and force bfd

> still maybe?


The arm32 vdso Makefile is an oddity in this regard.  I think what
Kees described is best.  Rather than always set the linker to bfd
(which harms linkers like lld), detect if gold is being used and if so
set the linker back to bfd.

For arm64, lld has this issue with -n,
https://github.com/ClangBuiltLinux/linux/issues/340, but I think we
can fix that in a follow up patch.  Same thoughts on --hash-style and
-T.

-- 
Thanks,
~Nick Desaulniers
Masahiro Yamada April 7, 2019, 2:20 a.m. | #7
Hi Kees, Nick,


On Sat, Apr 6, 2019 at 1:52 AM Nick Desaulniers <ndesaulniers@google.com> wrote:
>

> On Fri, Apr 5, 2019 at 9:11 AM Kees Cook <keescook@chromium.org> wrote:

> >

> > On Fri, Apr 5, 2019 at 3:17 AM Masahiro Yamada

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

> > > I want to propose alternative solution.

> > > Please check the attached patches.

>

> ```

> My plan is to rewrite all VDSO Makefiles to use $(LD), then delete cc-ldoption.

> ```

>

> I agree with that as a possible solution as well; I think it's best to

> just invoke the linker directly rather than use the compiler as the

> driver.  Just small nits with the 2 attached patches, but I think they

> will also work for us.

>

> Looks like there's 15 call sites across 12 files, plus Documentation/

> and scripts/Kbuild.include; removing it seems feasible and I'd be

> happy to help if you'd welcome the patches?



Yeah, your help is appreciated.

I just worked on two Makefiles
because I wanted to hear opinions
before investing more efforts.


Basically, you are positive to this approach,
so let's go this way.


> Let's start with that series of changes, but I suspect we will

> ultimately need to revisit -fuse-ld=.  I was just thinking that even

> my proposed patch doesn't do anything for KBUILD_HOSTCFLAGS, for

> example.


Host tools are not so important, but I agree.

BTW, why does clang invoke bfd linker instead of
lld by default?

If we want to make llvm self-contained,
I believe clang should use ld.lld by default.




>  I worry that bfd may still be invoked by Clang at some point

> in the build.  I will try to test in an environment with no GNU

> binutils.

>

> > In the 0001 patch of arch/arm/vdso/Makefile:

> > > ...

> > > -VDSO_LDFLAGS += $(call cc-ldoption, -fuse-ld=bfd)

> > > +ldflags-y = -Bsymbolic --no-undefined -soname=linux-vdso.so.1 \

> > > +           -z max-page-size=4096 -z common-page-size=4096 \

> > > +           -nostdlib -shared \

> > > +           $(call ld-option, --hash-style=sysv) \

>

> I think the vdso's should all REQUIRE --hash-style=sysv; the kselftest

> for vdso's will fail otherwise.  I could always follow up with patches

> for that if we didn't want to conflate changes.

>

> > > +           $(call ld-option, --build-id) \

> > > +           -T

>

> Was it intentional to add -T standalone?  The man page for `ld` says

> it needs an additional filename to follow -T.


This is because I wanted to re-use cmd_ld
defined in scripts/Makefile.lib

$(real-prereqs) contains the linker scripts and objects.


>  Or rather is optional

> if a -L flag is specified?

>

> >

> > I see that "-fuse-ld=bfd" is explicitly dropped here, which could

> > reintroduce the problem fixed in d2b30cd4b722 ("ARM: 8384/1: VDSO:

> > force use of BFD linker") (and 3473f26592c1c). Does ld.gold still

> > exhibit this problem? If so, we'll need to detect gold and force bfd

> > still maybe?


I intentionally dropped -fuse-ld=bfd.

With my patch applied, users have full control
of the linker by passing LD=.

Since we cannot link vmlinux with gold,
users should definitely pass a bfd linker.



> The arm32 vdso Makefile is an oddity in this regard.  I think what

> Kees described is best.  Rather than always set the linker to bfd

> (which harms linkers like lld), detect if gold is being used and if so

> set the linker back to bfd.

>

> For arm64, lld has this issue with -n,

> https://github.com/ClangBuiltLinux/linux/issues/340, but I think we

> can fix that in a follow up patch.  Same thoughts on --hash-style and

> -T.



If we need to add something depending on
the linker type,

I do not mind doing like this.

ldflags-$(CONFIG_LD_IS_GOLD) += ...
ldflags-$(CONFIG_LD_IS_LLD)  += ...

But, it is a different issue.






--
Best Regards
Masahiro Yamada

Patch

diff --git a/Makefile b/Makefile
index 026fbc450906..895c076b6305 100644
--- a/Makefile
+++ b/Makefile
@@ -514,6 +514,10 @@  ifneq ($(GCC_TOOLCHAIN),)
 CLANG_FLAGS	+= --gcc-toolchain=$(GCC_TOOLCHAIN)
 endif
 CLANG_FLAGS	+= -no-integrated-as
+ifneq ($(shell $(LD) --version 2>&1 | head -n 1 | grep LLD),)
+CLANG_FLAGS	+= -fuse-ld=$(shell which $(LD))
+KBUILD_CPPFLAGS += -Qunused-arguments
+endif
 KBUILD_CFLAGS	+= $(CLANG_FLAGS)
 KBUILD_AFLAGS	+= $(CLANG_FLAGS)
 export CLANG_FLAGS