| Message ID | 20190402070858.49597-1-ndesaulniers@google.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] Makefile: lld: tell clang to use lld | expand |
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 >
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.
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.
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
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
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
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
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
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