Re: [PATCH v3] Makefile: lld: tell clang to use lld
Hi Kees, Nick,
On Sat, Apr 6, 2019 at 1:52 AM Nick Desaulniers wrote:
>
> On Fri, Apr 5, 2019 at 9:11 AM Kees Cook wrote:
> >
> > On Fri, Apr 5, 2019 at 3:17 AM Masahiro Yamada
> > 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
Re: [PATCH v3] Makefile: lld: tell clang to use lld
On Fri, Apr 5, 2019 at 9:11 AM Kees Cook wrote:
>
> On Fri, Apr 5, 2019 at 3:17 AM Masahiro Yamada
> 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
Re: [PATCH v3] Makefile: lld: tell clang to use lld
On Fri, Apr 5, 2019 at 3:17 AM Masahiro Yamada
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
Re: [PATCH v3] Makefile: lld: tell clang to use lld
On Tue, Apr 2, 2019 at 4:09 PM 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,}. 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
> Suggested-by: Masahiro Yamada
> Signed-off-by: Nick Desaulniers
> ---
> 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
From 7fcb7806991d9b644173879c3c1fcbbded532435 Mon Sep 17 00:00:00 2001
From: Masahiro Yamada
Date: Fri, 5 Apr 2019 17:46:14 +0900
Subject: [PATCH 1/2] ARM: vdso: use $(LD) instead of $(CC) to link VDSO
We use $(LD) to link vmlinux, modules, decompressors, etc.
VDSO is the only exceptional case where $(CC) is used as the frontend
of linking, but I do not know why. VDSO uses a special linker script,
and does not link standard libraries at all.
I changed the Makefile to use $(LD) rahter than $(CC). I confirmed
the same vdso.so.raw was still produced.
Users will be able to use their favorite linker (e.g. lld instead of
of bfd) by passing LD= from the command line.
My plan is to rewrite all VDSO Makefiles to use $(LD), then delete
cc-ldoption.
Signed-off-by: Masahiro Yamada
---
arch/arm/vdso/Makefile | 21 -
1 file changed, 8 insertions(+), 13 deletions(-)
diff --git a/arch/arm/vdso/Makefile b/arch/arm/vdso/Makefile
index f4efff9d3afb..fadf554d9391 100644
--- a/arch/arm/vdso/Makefile
+++ b/arch/arm/vdso/Makefile
@@ -10,12 +10,12 @@ obj-vdso := $(addprefix $(obj)/, $(obj-vdso))
ccflags-y := -fPIC -fno-common -fno-builtin -fno-stack-protector
ccflags-y += -DDISABLE_BRANCH_PROFILING
-VDSO_LDFLAGS := -Wl,-Bsymbolic -Wl,--no-undefined -Wl,-soname=linux-vdso.so.1
-VDSO_LDFLAGS += -Wl,-z,max-page-size=4096 -Wl,-z,common-page-size=4096
-VDSO_LDFLAGS += -nostdlib -shared
-VDSO_LDFLAGS += $(call cc-ldoption, -Wl$(comma)--hash-style=sysv)
-VDSO_LDFLAGS += $(call cc-ldoption, -Wl$(comma)--build-id)
-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
obj-$(CONFIG_VDSO) += vdso.o
extra-$(CONFIG_VDSO) += vdso.lds
@@ -37,8 +37,8 @@ KCOV_INSTRUMENT := n
$(obj)/vdso.o : $(obj)/vdso.so
# Link rule for the .so file
-$(obj)/vdso.so.raw: $(src)/vdso.lds $(obj-vdso) FORCE
- $(call if_changed,vdsold)
+$(obj)/vdso.so.raw: $(obj)/vdso.lds $(obj-vdso) FORCE
+ $(call if_changed,ld)
$(obj)/vdso.so.dbg: $(obj)/vdso.so.raw $(obj)/vdsomunge FORCE
$(call if_changed,vdsomunge)
@@ -48,11 +48,6 @@ $(obj)/%.so: OBJCOPYFLAGS := -S
$(obj)/%.so: $(obj)/%.so.dbg FORCE
$(call if_changed,objcopy)
-# Actual build commands
-quiet_cmd_vdsold = VDSO$@
- cmd_vdsold = $(CC) $(c_flags) $(VDSO_LDFLAGS) \
- -Wl,-T $(filter %.lds,$^) $(filter %.o,$^) -o $@
-
quiet_cmd_vdsomunge = MUNGE $@
cmd_vdsomunge = $(objtree)/$(obj)/vdsomunge $< $@
--
2.17.1
From db93aba5086ebc0558c193101d56c2e5a5878dda Mon Sep 17 00:00:00 2001
From: Masahiro Yamada
Date: Thu, 4 Apr 2019 23:40:18 +0900
Subject: [PATCH 2/2] arm64: vdso: use $(LD) instead of $(CC) to link VDSO
We use $(LD) to link vmlinux, modules, decompressors, etc.
VDSO is the only exceptional case where $(CC) is used as the frontend
of linking, but I do not know why. VDSO uses a special linker script,
and does not link standard libraries at all.
I changed the Makefile to use $(LD) rahter than $(CC). I tested, and
VDSO still worked.
Users will be able to use their favorite linker (e.g. lld instead of
of bfd) by passing LD= from the command line.
My plan is to rewrite all VDSO Makefiles to use $(LD), then delete
cc-ldoption.
Signed-off-by: Masahiro Yamada
---
arch/arm64/kernel/vdso/Makefile | 13 +++--
1 file changed, 3 insertions(+), 10 deletions(-)
diff --git a/arch/arm64/kernel/vdso/Makefile b/arch/arm64/kernel/vdso/Makefile
Re: [PATCH v3] Makefile: lld: tell clang to use lld
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 wrote:
> >
> > This is needed because clang doesn't select which linker to use based on
> > $LD but rather -fuse-ld={bfd,gold,lld,}. 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
> > Suggested-by: Masahiro Yamada
> > Signed-off-by: Nick Desaulniers
>
> Thanks for bringing this up again, Nick.
>
> Suggested-by: Sedat Dilek (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
>
> [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+unsubscr...@googlegroups.com.
> > To post to this group, send email to clang-built-li...@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.
Re: [PATCH v3] Makefile: lld: tell clang to use lld
On Tue, Apr 2, 2019 at 9:09 AM 'Nick Desaulniers' via Clang Built
Linux wrote:
>
> This is needed because clang doesn't select which linker to use based on
> $LD but rather -fuse-ld={bfd,gold,lld,}. 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
> Suggested-by: Masahiro Yamada
> Signed-off-by: Nick Desaulniers
Thanks for bringing this up again, Nick.
Suggested-by: Sedat Dilek (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
[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+unsubscr...@googlegroups.com.
> To post to this group, send email to clang-built-li...@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.
Re: [PATCH v3] Makefile: lld: tell clang to use lld
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,}. 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
> Suggested-by: Masahiro Yamada
> Signed-off-by: Nick Desaulniers
> ---
> 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
Tested-by: Nathan Chancellor
> +endif
> KBUILD_CFLAGS+= $(CLANG_FLAGS)
> KBUILD_AFLAGS+= $(CLANG_FLAGS)
> export CLANG_FLAGS
> --
> 2.21.0.392.gf8f6787159e-goog
>
