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

From: Nick Desaulniers <ndesaulniers@google.com>
To: Kees Cook <keescook@chromium.org>
Cc: Masahiro Yamada <yamada.masahiro@socionext.com>,
	clang-built-linux@googlegroups.com,
	Nathan Chancellor <natechancellor@gmail.com>,
	Michal Marek <michal.lkml@markovi.net>,
	Linux Kbuild mailing list <linux-kbuild@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Nathan Lynch <nathan_lynch@mentor.com>,
	Peter Smith <peter.smith@linaro.org>
Subject: Re: [PATCH v3] Makefile: lld: tell clang to use lld
Date: Fri, 5 Apr 2019 09:52:10 -0700	[thread overview]
Message-ID: <CAKwvOd=eBWm6B24soOGhxtCk+TCeKqwra0_b-V6kTYDSHLOg_w@mail.gmail.com> (raw)
In-Reply-To: <CAGXu5jKZUP33Kz=nDPkUSj0Ksz5FOYTuuKAxv4N6xGEtFT645A@mail.gmail.com>

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