[PATCH] nptl: Implement pthread_self in libc.so

[Carlos O'Donell]

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 *);
> 


-- 
Cheers,
Carlos.