Date:   Fri, 12 Apr 2019 10:24:09 -0700
From:   Nick Desaulniers <ndesaulniers@...gle.com>
To:     Will Deacon <will.deacon@....com>
Cc:     Masahiro Yamada <yamada.masahiro@...ionext.com>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>,
        Olof Johansson <olof@...om.net>, Arnd Bergmann <arnd@...db.de>,
        Kees Cook <keescook@...omium.org>,
        Catalin Marinas <catalin.marinas@....com>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] arm64: vdso: use $(LD) instead of $(CC) to link VDSO

On Fri, Apr 12, 2019 at 1:50 AM Will Deacon <will.deacon@....com> wrote:
>
> Hi all,
>
> On Fri, Apr 12, 2019 at 10:21:38AM +0900, Masahiro Yamada wrote:
> > On Fri, Apr 12, 2019 at 3:20 AM Nick Desaulniers
> > <ndesaulniers@...gle.com> wrote:
> > > On Thu, Apr 11, 2019 at 2:30 AM Masahiro Yamada
> > > <yamada.masahiro@...ionext.com> wrote:
> > > >
> > > > We use $(LD) to link vmlinux, modules, decompressors, etc.
> > > >
> > > > VDSO is the only exceptional case where $(CC) is used as the linker
> > > > driver, but I do not know why we need to do so. VDSO uses a special
> > > > linker script, and does not link standard libraries at all.
> > > >
> > > > I changed the Makefile to use $(LD) rather than $(CC). I tested this,
> > > > and VDSO worked for me.
> > > >
> > > > 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 <yamada.masahiro@...ionext.com>
> > > > ---
> > > >
> > > >  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
> > > > index a0af6bf6c11b..744b9dbaba03 100644
> > > > --- a/arch/arm64/kernel/vdso/Makefile
> > > > +++ b/arch/arm64/kernel/vdso/Makefile
> > > > @@ -12,17 +12,12 @@ obj-vdso := gettimeofday.o note.o sigreturn.o
> > > >  targets := $(obj-vdso) vdso.so vdso.so.dbg
> > > >  obj-vdso := $(addprefix $(obj)/, $(obj-vdso))
> > > >
> > > > -ccflags-y := -shared -fno-common -fno-builtin
> > >
> > > Thanks for the patch; in general LGTM. Just some small questions:
> > >
> > > Looks like -shared and -fno-common are linker flags forwarded along,
> > > but -fno-builtin is meant for the compiler, right?
> >
> > Definitely, -shared must be passed to the linker
> > to create a shared object.
> > It is listed in the linker option list:
> > https://gcc.gnu.org/onlinedocs/gcc-8.3.0/gcc/Link-Options.html#Link-Options
> >
> >
> >
> > I think -fno-common is a compiler flag instead of a linker one
> > as far as I understand this:
> > https://gcc.gnu.org/onlinedocs/gcc-8.3.0/gcc/Code-Gen-Options.html#Code-Gen-Options
> >
> >
> > -fno-builtin is a compiler flag:
> > https://gcc.gnu.org/onlinedocs/gcc-8.3.0/gcc/C-Dialect-Options.html#C-Dialect-Options
> >
> >
> >
> > > Do we want to keep
> > > that in ccflags?
> >
> > All source files in arch/arm64/kernel/vdso/
> > are written in assembly (gettimeofday.S note.S sigreturn.S).
> >
> > So, -fno-common -fno-builtin for ccflags-y
> > are not used.
> >
> > That's why I dropped them.
> >
> > If we want to keep them just in case,
> > it is fine with me too.
>
> I think dropping this is fine for now, but be aware that there is ongoing
> work to rewrite the vdso in C, in which case we may want that back. Much
> of the Makefile will probably get reworked when that happens anyway, so
> no need to worry.
>
> > >  I'm not sure why it would have been added
> > > intentionally; maybe Will knows?  Maybe it's ok to drop.
> >
> > They have been here since the initial VDSO support
> > by commit 9031fefde6f2a.
> >
> > Will, do you remember why you added these flags?
>
> Shamelessly lifted from ppc :)
>
> > > > -ccflags-y += -nostdlib -Wl,-soname=linux-vdso.so.1 \
> > > > -               $(call cc-ldoption, -Wl$(comma)--hash-style=sysv)
> > > > +ldflags-y := -shared -nostdlib -soname=linux-vdso.so.1 \
> > > > +               $(call ld-option, --hash-style=sysv) -n -T
> > >
> > > Forgive my ignorance, but the man page for ld makes it looks like the
> > > -T argument requires a subsequent argument `scriptfile`.  Is that what
> > > `$(real-prereqs)` was doing below? If so, is dropping it in this patch
> > > intentional?
> >
> > I just re-used cmd_ld in scripts/Makefile.lib
> >
> >
> > It will be expanded like follows:
> >
> > cmd_ld = $(LD) $(ld_flags) $(real-prereqs) -o $@
> >
> >  ->
> >
> >  $(LD) $(KBUILD_LDFLAGS) $(ldflags-y) $(LDFLAGS_$(@F)) $(real-prereqs) -o $@
> >
> >  ->
> >
> > $(LD) -shared -nostdlib -soname=linux-vdso.so.1 \
> >      --hash-style=sysv -n -T \
> >     $(obj)/vdso.lds $(obj-vdso) -o $(obj)/vdso.so.dbg
>
> Yeah, this still works, but boy is it ugly to have that dangling -T and
> then grabbing the linker script from extra-y! That's also my fault though,
> so I think your patch is ok.
>
> Will

Will, thanks for the additional context.
Reviewed-by: Nick Desaulniers <ndesaulniers@...gle.com>

-- 
Thanks,
~Nick Desaulniers

Powered by blists - more mailing lists