Re: [PATCH] Avoid an extra branch to PLT for -z now

["H.J. Lu" <hjl dot tools at gmail dot com>]

On Wed, May 18, 2016 at 11:36 AM, Rich Felker <dalias@libc.org> wrote:
> On Wed, May 18, 2016 at 11:02:31AM -0700, H.J. Lu wrote:
>> On Wed, May 4, 2016 at 11:17 AM, H.J. Lu <hongjiu.lu@intel.com> wrote:
>> > When --enable-bind-now is used to configure glibc build, we can avoid
>> > an extra branch to the PLT entry by using indirect branch via the GOT
>> > slot instead, which is similar to the first instructuon in the PLT
>> > entry.  Changes in the shared library sizes in text sections:
>> >
>> > Shared library    Before (bytes)   After (bytes)
>> > libm.so             1060813          1060797
>> > libmvec.so           160881           160805
>> > libpthread.so         94992            94984
>> > librt.so              25064            25048
>> >
>> > Tested on x86-64.  OK for master?
>> >
>> > H.J.
>> > ---
>> >         * config.h.in (BIND_NOW): New.
>> >         * configure.ac (BIND_NOW): New.  Defined for --enable-bind-now.
>> >         * configure: Regenerated.
>> >         * sysdeps/x86_64/sysdep.h (JUMPTARGET): Defined to indirect
>> >         branch via the GOT slot.
>> > ---
>> >  config.h.in             | 3 +++
>> >  configure               | 4 ++++
>> >  configure.ac            | 3 +++
>> >  sysdeps/x86_64/sysdep.h | 8 ++++++--
>> >  4 files changed, 16 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/config.h.in b/config.h.in
>> > index 0147ba3..2c902b0 100644
>> > --- a/config.h.in
>> > +++ b/config.h.in
>> > @@ -87,6 +87,9 @@
>> >     include/libc-symbols.h that avoid PLT slots in the shared objects.  */
>> >  #undef NO_HIDDEN
>> >
>> > +/* Define this to disable lazy relocations in DSOs.  */
>> > +#undef BIND_NOW
>> > +
>> >  /* AArch64 big endian ABI */
>> >  #undef HAVE_AARCH64_BE
>> >
>> > diff --git a/configure b/configure
>> > index 8fe5937..19a4829 100755
>> > --- a/configure
>> > +++ b/configure
>> > @@ -3416,6 +3416,10 @@ else
>> >  fi
>> >
>> >
>> > +if test "x$bindnow" = xyes; then
>> > +  $as_echo "#define BIND_NOW 1" >>confdefs.h
>> > +
>> > +fi
>> >
>> >  # Check whether --enable-static-nss was given.
>> >  if test "${enable_static_nss+set}" = set; then :
>> > diff --git a/configure.ac b/configure.ac
>> > index 3c766b7..123f0d2 100644
>> > --- a/configure.ac
>> > +++ b/configure.ac
>> > @@ -231,6 +231,9 @@ AC_ARG_ENABLE([bind-now],
>> >               [bindnow=$enableval],
>> >               [bindnow=no])
>> >  AC_SUBST(bindnow)
>> > +if test "x$bindnow" = xyes; then
>> > +  AC_DEFINE(BIND_NOW)
>> > +fi
>> >
>> >  dnl On some platforms we cannot use dynamic loading.  We must provide
>> >  dnl static NSS modules.
>> > diff --git a/sysdeps/x86_64/sysdep.h b/sysdeps/x86_64/sysdep.h
>> > index fbe3560..75ac747 100644
>> > --- a/sysdeps/x86_64/sysdep.h
>> > +++ b/sysdeps/x86_64/sysdep.h
>> > @@ -90,9 +90,13 @@ lose:                                                                              \
>> >
>> >  #undef JUMPTARGET
>> >  #ifdef PIC
>> > -#define JUMPTARGET(name)       name##@PLT
>> > +# ifdef BIND_NOW
>> > +#  define JUMPTARGET(name)     *name##@GOTPCREL(%rip)
>> > +# else
>> > +#  define JUMPTARGET(name)     name##@PLT
>> > +# endif
>> >  #else
>> > -#define JUMPTARGET(name)       name
>> > +# define JUMPTARGET(name)      name
>> >  #endif
>> >
>> >  /* Local label name for asm code. */
>> > --
>> > 2.5.5
>> >
>>
>> Any objections?
>
> Is there a reason you need to make it conditional? Just using
> *name@GOTPCREL(%rip) unconditionally would force this particular
> symbol to be resolved at load time, and doing that for the small
> number of affected symbols is not going to make any noticable
> difference to startup performance, but may noticably improve runtime
> performance.

In fact, I prefer "*name@GOTPCREL(%rip)" and I will make a proposal
to x86-64 psABI to get rid of PLT all together to enhance security,
But as the first step, I will support lazy binding for now.

-- 
H.J.