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

[Nick Desaulniers]

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 <natecha...@gmail.com>
Suggested-by: Masahiro Yamada <yamada....@socionext.com>
Signed-off-by: Nick Desaulniers <ndesau...@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
+endif
KBUILD_CFLAGS += $(CLANG_FLAGS)
KBUILD_AFLAGS += $(CLANG_FLAGS)
export CLANG_FLAGS
--
2.21.0.392.gf8f6787159e-goog

[Nathan Chancellor]

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

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

[Nick Desaulniers]

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 sooner, as otherwise

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

not linking (-c) such as when cc-option is used. There's no need to
guard with cc-option.

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 <natecha...@gmail.com>
Suggested-by: Masahiro Yamada <yamada....@socionext.com>
Signed-off-by: Nick Desaulniers <ndesau...@google.com>
---

Changes V3->V4:
* Unconditionally add -Qunused-arguments sooners, as per Nathan.
* Slight modification to commit message for that point.

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 | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index 026fbc450906..b290e76e1ca5 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))

+endif
+KBUILD_CPPFLAGS += -Qunused-arguments

KBUILD_CFLAGS += $(CLANG_FLAGS)
KBUILD_AFLAGS += $(CLANG_FLAGS)
export CLANG_FLAGS

@@ -716,7 +720,6 @@ stackp-flags-$(CONFIG_STACKPROTECTOR_STRONG) := -fstack-protector-strong
KBUILD_CFLAGS += $(stackp-flags-y)

ifdef CONFIG_CC_IS_CLANG
-KBUILD_CPPFLAGS += $(call cc-option,-Qunused-arguments,)
KBUILD_CFLAGS += $(call cc-disable-warning, format-invalid-specifier)
KBUILD_CFLAGS += $(call cc-disable-warning, gnu)
KBUILD_CFLAGS += $(call cc-disable-warning, address-of-packed-member)
--
2.21.0.392.gf8f6787159e-goog

[Sedat Dilek]

On Tue, Apr 2, 2019 at 9:09 AM 'Nick Desaulniers' via Clang Built
Linux <clang-bu...@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 <natecha...@gmail.com>
> Suggested-by: Masahiro Yamada <yamada....@socionext.com>
> Signed-off-by: Nick Desaulniers <ndesau...@google.com>

Thanks for bringing this up again, Nick.

Suggested-by: Sedat Dilek <sedat...@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...@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

> --
> 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-li...@googlegroups.com.
> To post to this group, send email to clang-bu...@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]

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

[Nathan Chancellor]

On Tue, Apr 02, 2019 at 12:33:17AM -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 sooner, as otherwise
> Clang likes to complain about -fuse-lld= being unused when compiling but
> not linking (-c) such as when cc-option is used. There's no need to
> guard with cc-option.
>
> 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 <natecha...@gmail.com>
> Suggested-by: Masahiro Yamada <yamada....@socionext.com>
> Signed-off-by: Nick Desaulniers <ndesau...@google.com>

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

[Masahiro Yamada]

On Tue, Apr 2, 2019 at 4:09 PM Nick Desaulniers <ndesau...@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 <natecha...@gmail.com>
> Suggested-by: Masahiro Yamada <yamada....@socionext.com>
> Signed-off-by: Nick Desaulniers <ndesau...@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]

On Fri, Apr 5, 2019 at 3:17 AM Masahiro Yamada
<yamada....@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]

On Fri, Apr 5, 2019 at 9:11 AM Kees Cook <kees...@chromium.org> wrote:
>
> On Fri, Apr 5, 2019 at 3:17 AM Masahiro Yamada
> <yamada....@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]

Hi Kees, Nick,

On Sat, Apr 6, 2019 at 1:52 AM Nick Desaulniers <ndesau...@google.com> wrote:
>
> On Fri, Apr 5, 2019 at 9:11 AM Kees Cook <kees...@chromium.org> wrote:
> >
> > On Fri, Apr 5, 2019 at 3:17 AM Masahiro Yamada
> > <yamada....@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.