nptl: Implement pthread_self in libc.so

  All binaries use TLS and thus need a properly set up TCB, so we can
simply return its address directly, instead of forwarding to the
libpthread implementation from libc.

For versioned symbols, the dynamic linker checks that the soname matches
the name supplied by the link editor, so a compatibility symbol in
libpthread is needed.

The layout of struct pthread_functions is not changed by this commit,
although the pointer used to store the address of pthread_self is now
unused.

To avoid linking against the libpthread function in all cases, we would
have to bump the symbol version of libpthread in libc.so and supply a
compat symbol.  This commit does not do that because the function
implementation is so small, so the overhead by two active copies of the
same function might well be smaller than the increase in symbol table
size.

2017-12-14  Florian Weimer  <fweimer@redhat.com>

	nptl: Provide full implementation of pthread_self in libc.so.
	* nptl/Makefile (routines): Add pthread_self.
	(libpthread-routines): Replace pthread_self with
	compat-pthread_self.
	* nptl/forward.c (pthread_self): Remove.
	* nptl/nptl-init.c (pthread_functions): Do not initialize
	ptr_pthread_self.
	* nptl/pthread_self.c (pthread_self): Remove weak alias.
	* nptl/compat-pthread_self.c: New file.
	* sysdeps/nptl/pthread-functions.h (struct pthread_functions):
	Rename ptr_pthread_self to unused_pthread_self.
  

On 12/14/2017 10:37 AM, Florian Weimer wrote:
> All binaries use TLS and thus need a properly set up TCB, so we can
> simply return its address directly, instead of forwarding to the
> libpthread implementation from libc.

Could you expand on this a bit?

Yes, all binaries use TLS by virtue of errno.

Yes, because we need errno setup we need a TCB setup.

However, internally we never call pthread_self, we use THREAD_SELF,
and access the thread register directly. So there is no internal cost
paid for pthread_self.

The only cost paid is if the application binary calls pthread_self to
use that descriptor in some way, and to use it in some way means calling
a pthread function with it.

Are there thread functions we might use in a program not linked against
libpthread e.g. the pthread_atfork changes we have in Fedora?
 
> For versioned symbols, the dynamic linker checks that the soname matches
> the name supplied by the link editor, so a compatibility symbol in
> libpthread is needed.

I think we should drop the soname matches check, it would allow us to move
versioned symbols between shared objects :-)
 
> The layout of struct pthread_functions is not changed by this commit,
> although the pointer used to store the address of pthread_self is now
> unused.

Why?

> To avoid linking against the libpthread function in all cases, we would
> have to bump the symbol version of libpthread in libc.so and supply a
> compat symbol.  

Could you expand on what this might look like?

> This commit does not do that because the function
> implementation is so small, so the overhead by two active copies of the
> same function might well be smaller than the increase in symbol table
> size.

Can we please measure that to be sure?

> 2017-12-14  Florian Weimer  <fweimer@redhat.com>
> 
> 	nptl: Provide full implementation of pthread_self in libc.so.
> 	* nptl/Makefile (routines): Add pthread_self.
> 	(libpthread-routines): Replace pthread_self with
> 	compat-pthread_self.
> 	* nptl/forward.c (pthread_self): Remove.
> 	* nptl/nptl-init.c (pthread_functions): Do not initialize
> 	ptr_pthread_self.
> 	* nptl/pthread_self.c (pthread_self): Remove weak alias.
> 	* nptl/compat-pthread_self.c: New file.
> 	* sysdeps/nptl/pthread-functions.h (struct pthread_functions):
> 	Rename ptr_pthread_self to unused_pthread_self.
> 
> diff --git a/nptl/Makefile b/nptl/Makefile
> index 570a42301c..ae388d1112 100644
> --- a/nptl/Makefile
> +++ b/nptl/Makefile
> @@ -30,7 +30,7 @@ install-lib-ldscripts := libpthread.so
>  
>  routines = alloca_cutoff forward libc-lowlevellock libc-cancellation \
>  	   libc-cleanup libc_pthread_init libc_multiple_threads \
> -	   register-atfork unregister-atfork
> +	   register-atfork unregister-atfork pthread_self
>  shared-only-routines = forward
>  
>  # We need to provide certain routines for compatibility with existing
> @@ -48,7 +48,7 @@ pthread-compat-wrappers = \
>  libpthread-routines = nptl-init vars events version pt-interp \
>  		      pthread_create pthread_exit pthread_detach \
>  		      pthread_join pthread_tryjoin pthread_timedjoin \
> -		      pthread_self pthread_equal pthread_yield \
> +		      compat-pthread_self pthread_equal pthread_yield \
>  		      pthread_getconcurrency pthread_setconcurrency \
>  		      pthread_getschedparam pthread_setschedparam \
>  		      pthread_setschedprio \
> diff --git a/nptl/compat-pthread_self.c b/nptl/compat-pthread_self.c
> new file mode 100644
> index 0000000000..5e9f4eb27d
> --- /dev/null
> +++ b/nptl/compat-pthread_self.c
> @@ -0,0 +1,27 @@
> +/* Compatibility version of pthread_self in libpthread.
> +   Copyright (C) 2017 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +/* Compatibility version of pthread_self for old binaries which link
> +   directly against libpthread's version.  */
> +
> +#include <shlib-compat.h>
> +
> +#if SHLIB_COMPAT (libpthread, GLIBC_2_0, GLIBC_2_27)
> +# include "pthread_self.c"
> +compat_symbol (libpthread, pthread_self, pthread_self, GLIBC_2_0);
> +#endif
> diff --git a/nptl/forward.c b/nptl/forward.c
> index ac96765f29..8abbccdf5e 100644
> --- a/nptl/forward.c
> +++ b/nptl/forward.c
> @@ -193,10 +193,6 @@ FORWARD (pthread_mutex_lock, (pthread_mutex_t *mutex), (mutex), 0)
>  
>  FORWARD (pthread_mutex_unlock, (pthread_mutex_t *mutex), (mutex), 0)
>  
> -
> -FORWARD2 (pthread_self, pthread_t, (void), (), return 0)
> -
> -
>  FORWARD (__pthread_setcancelstate, (int state, int *oldstate),
>  	 (state, oldstate), 0)
>  strong_alias (__pthread_setcancelstate, pthread_setcancelstate)
> diff --git a/nptl/nptl-init.c b/nptl/nptl-init.c
> index 869e926f17..a5979f27fd 100644
> --- a/nptl/nptl-init.c
> +++ b/nptl/nptl-init.c
> @@ -122,7 +122,6 @@ static const struct pthread_functions pthread_functions =
>      .ptr_pthread_mutex_init = __pthread_mutex_init,
>      .ptr_pthread_mutex_lock = __pthread_mutex_lock,
>      .ptr_pthread_mutex_unlock = __pthread_mutex_unlock,
> -    .ptr_pthread_self = __pthread_self,
>      .ptr___pthread_setcancelstate = __pthread_setcancelstate,
>      .ptr_pthread_setcanceltype = __pthread_setcanceltype,
>      .ptr___pthread_cleanup_upto = __pthread_cleanup_upto,
> diff --git a/nptl/pthread_self.c b/nptl/pthread_self.c
> index 8e21775e31..b75af9358e 100644
> --- a/nptl/pthread_self.c
> +++ b/nptl/pthread_self.c
> @@ -19,10 +19,8 @@
>  #include "pthreadP.h"
>  #include <tls.h>
>  
> -
>  pthread_t
> -__pthread_self (void)
> +pthread_self (void)
>  {
>    return (pthread_t) THREAD_SELF;
>  }
> -weak_alias (__pthread_self, pthread_self)
> diff --git a/sysdeps/nptl/pthread-functions.h b/sysdeps/nptl/pthread-functions.h
> index 4006fc6c25..a385ff5684 100644
> --- a/sysdeps/nptl/pthread-functions.h
> +++ b/sysdeps/nptl/pthread-functions.h
> @@ -74,7 +74,7 @@ struct pthread_functions
>  				 const pthread_mutexattr_t *);
>    int (*ptr_pthread_mutex_lock) (pthread_mutex_t *);
>    int (*ptr_pthread_mutex_unlock) (pthread_mutex_t *);
> -  pthread_t (*ptr_pthread_self) (void);
> +  pthread_t (*unused_pthread_self) (void);
>    int (*ptr___pthread_setcancelstate) (int, int *);
>    int (*ptr_pthread_setcanceltype) (int, int *);
>    void (*ptr___pthread_cleanup_upto) (__jmp_buf, char *);
>
  

On 12/15/2017 05:51 AM, Carlos O'Donell wrote:
> On 12/14/2017 10:37 AM, Florian Weimer wrote:
>> All binaries use TLS and thus need a properly set up TCB, so we can
>> simply return its address directly, instead of forwarding to the
>> libpthread implementation from libc.
> 
> Could you expand on this a bit?
> 
> Yes, all binaries use TLS by virtue of errno.
> 
> Yes, because we need errno setup we need a TCB setup.
> 
> However, internally we never call pthread_self, we use THREAD_SELF,
> and access the thread register directly. So there is no internal cost
> paid for pthread_self.

I don't understand this.  The existing implementation has a cost because 
of the redirect from libc.so to libpthread.so:

Dump of assembler code for function pthread_self:
    <+0>:	mov    0x2b505a(%rip),%eax # <__libc_pthread_functions_init>
    <+6>:	test   %eax,%eax
    <+8>:	je     0x7ffff76f0690 <pthread_self+32>
    <+10>:	mov    0x2b4fb7(%rip),%rax # <__libc_pthread_functions+280>
    <+17>:	ror    $0x11,%rax
    <+21>:	xor    %fs:0x30,%rax
    <+30>:	jmpq   *%rax
    <+32>:	xor    %eax,%eax
    <+34>:	retq

Compare this to the implementation in libpthread (which is what the new 
libc.so implementation looks like as well):

Dump of assembler code for function __pthread_self:
    <+0>:	mov    %fs:0x10,%rax
    <+9>:	retq

The redirect implementation returns zero as a pthread_t value, which is 
not correct if libpthread is loaded later (something this redirect 
functionality is there to support, but which is currently defective in 
various ways).

> The only cost paid is if the application binary calls pthread_self to
> use that descriptor in some way, and to use it in some way means calling
> a pthread function with it.

It's also common to use it for logging, although that is a slight abuse. 
  The range of permitted uses is certainly limited because pthread_t is 
an unsigned long, and any direct manipulation is undefined.

The stub implementation is broken, so we can't move more functionality 
from libpthread to libc until it is fixed anyway.

> Are there thread functions we might use in a program not linked against
> libpthread e.g. the pthread_atfork changes we have in Fedora?

pthread_thread_number_np depends on this change in practical terms.

>> The layout of struct pthread_functions is not changed by this commit,
>> although the pointer used to store the address of pthread_self is now
>> unused.
> 
> Why?

To preserve the internal ABI.  It seem to have very little cost.  I can 
remove the field, of course.

>> To avoid linking against the libpthread function in all cases, we would
>> have to bump the symbol version of libpthread in libc.so and supply a
>> compat symbol.
> 
> Could you expand on what this might look like?

We'd add pthread_self@@GLIBC_2.27 to glibc and turn the existing version 
into a compat symbol.  Both would be backed by the same implementation 
(at the same address).  Hopefully, this will encourage link editors not 
to use the compatibility symbol in libpthread—it then has a completely 
different version, so it should be treated as unrelated.

>> This commit does not do that because the function
>> implementation is so small, so the overhead by two active copies of the
>> same function might well be smaller than the increase in symbol table
>> size.
> 
> Can we please measure that to be sure?

I don't see how.  Is it really necessary?  The new implementation in 
libc is clearly an improvement.

The link editor should probably be fixed not to link against 
compatibility symbols in certain cases.

Thanks,
Florian
  

On 12/14/2017 09:50 PM, Florian Weimer wrote:
> On 12/15/2017 05:51 AM, Carlos O'Donell wrote:
>> On 12/14/2017 10:37 AM, Florian Weimer wrote:
>>> All binaries use TLS and thus need a properly set up TCB, so we can
>>> simply return its address directly, instead of forwarding to the
>>> libpthread implementation from libc.
>>
>> Could you expand on this a bit?
>>
>> Yes, all binaries use TLS by virtue of errno.
>>
>> Yes, because we need errno setup we need a TCB setup.
>>
>> However, internally we never call pthread_self, we use THREAD_SELF,
>> and access the thread register directly. So there is no internal cost
>> paid for pthread_self.
> 
> I don't understand this.  The existing implementation has a cost
> because of the redirect from libc.so to libpthread.so:
> 
> Dump of assembler code for function pthread_self:
>    <+0>:    mov    0x2b505a(%rip),%eax # <__libc_pthread_functions_init>
>    <+6>:    test   %eax,%eax
>    <+8>:    je     0x7ffff76f0690 <pthread_self+32>
>    <+10>:    mov    0x2b4fb7(%rip),%rax # <__libc_pthread_functions+280>
>    <+17>:    ror    $0x11,%rax
>    <+21>:    xor    %fs:0x30,%rax
>    <+30>:    jmpq   *%rax
>    <+32>:    xor    %eax,%eax
>    <+34>:    retq
> 
> Compare this to the implementation in libpthread (which is what the
> new libc.so implementation looks like as well):
> 
> Dump of assembler code for function __pthread_self:
>    <+0>:    mov    %fs:0x10,%rax
>    <+9>:    retq
> 
> The redirect implementation returns zero as a pthread_t value, which
> is not correct if libpthread is loaded later (something this redirect
> functionality is there to support, but which is currently defective
> in various ways).

Thanks.

Let me clarify.

Your change has no performance implications for libc.so, since we never
call pthread_self, we always use THREAD_SELF, the macro.

Your change *does* have a performance implication for applications calling
pthread_self without having linked against libpthread.so.

This is, as you say, an optimization.
 
>> The only cost paid is if the application binary calls pthread_self to
>> use that descriptor in some way, and to use it in some way means calling
>> a pthread function with it.
> 
> It's also common to use it for logging, although that is a slight
> abuse.  The range of permitted uses is certainly limited because
> pthread_t is an unsigned long, and any direct manipulation is
> undefined.

Good point.

> The stub implementation is broken, so we can't move more
> functionality from libpthread to libc until it is fixed anyway.

That could be fixed for just this case?
 
>> Are there thread functions we might use in a program not linked against
>> libpthread e.g. the pthread_atfork changes we have in Fedora?
> 
> pthread_thread_number_np depends on this change in practical terms.

Why? In pthread_thread_number_np you can call THREAD_SELF directly?

>>> The layout of struct pthread_functions is not changed by this commit,
>>> although the pointer used to store the address of pthread_self is now
>>> unused.
>>
>> Why?
> 
> To preserve the internal ABI.  It seem to have very little cost.  I
> can remove the field, of course.

I would prefer the removal. Please submit a v2.
 
>>> To avoid linking against the libpthread function in all cases, we would
>>> have to bump the symbol version of libpthread in libc.so and supply a
>>> compat symbol.
>>
>> Could you expand on what this might look like?
> 
> We'd add pthread_self@@GLIBC_2.27 to glibc and turn the existing
> version into a compat symbol.  Both would be backed by the same
> implementation (at the same address).  Hopefully, this will encourage
> link editors not to use the compatibility symbol in libpthread—it
> then has a completely different version, so it should be treated as
> unrelated.

I agree this is a little excessive when we already have the forwarders
in place, and can keep dlopen(pthread_self) working.
 
>>> This commit does not do that because the function
>>> implementation is so small, so the overhead by two active copies of the
>>> same function might well be smaller than the increase in symbol table
>>> size.
>>
>> Can we please measure that to be sure?
> 
> I don't see how.  Is it really necessary?  The new implementation in
> libc is clearly an improvement.

I think I see your point better now that you explained it in more detail.
The solution you propose is simpler.
 
> The link editor should probably be fixed not to link against
> compatibility symbols in certain cases.

Well, I think we should remove the symbol<->DSO error that prevents us
from migrating versioned symbols across DSO boundaries. This should be
more flexible.