Re: [PATCH 10/10] nptl: Use <unwind-link.h> for accessing the libgcc_s unwinder

[Carlos O'Donell <carlos@redhat.com>]

On 2/17/21 11:03 AM, Florian Weimer via Libc-alpha wrote:

LGTM. Uses __libc_unwind_link_get@@GLIBC_PRIVATE as the new
interface to access the unwinder. This creates an upgrade
hazard for long running processes that don't link against
libpthread but load libpthread later. There isn't anything
we can do about that except accelerate your work to fold
libpthread into libc.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

> ---
>  nptl/nptlfreeres.c                            |   1 -
>  nptl/pthreadP.h                               |   6 +-
>  nptl/pthread_cancel.c                         |   3 +-
>  sysdeps/arm/nptl/unwind-forcedunwind.c        |  25 ++++
>  sysdeps/arm/pt-arm-unwind-resume.S            |  30 +----
>  sysdeps/nptl/unwind-forcedunwind.c            | 115 +++---------------
>  .../sysv/linux/ia64/unwind-forcedunwind.c     |  16 +--
>  7 files changed, 50 insertions(+), 146 deletions(-)
>  create mode 100644 sysdeps/arm/nptl/unwind-forcedunwind.c
> 
> diff --git a/nptl/nptlfreeres.c b/nptl/nptlfreeres.c
> index d295bcb3c3..4833f04714 100644
> --- a/nptl/nptlfreeres.c
> +++ b/nptl/nptlfreeres.c
> @@ -27,5 +27,4 @@ __libpthread_freeres (void)
>  {
>    call_function_static_weak (__default_pthread_attr_freeres);
>    call_function_static_weak (__nptl_stacks_freeres);
> -  call_function_static_weak (__nptl_unwind_freeres);

OK. Remove one because we don't need to free this resource anymore.

>  }
> diff --git a/nptl/pthreadP.h b/nptl/pthreadP.h
> index d078128230..d2fd0826fe 100644
> --- a/nptl/pthreadP.h
> +++ b/nptl/pthreadP.h
> @@ -287,9 +287,11 @@ hidden_proto (__pthread_unwind_next)
>  hidden_proto (__pthread_register_cancel)
>  hidden_proto (__pthread_unregister_cancel)
>  # ifdef SHARED
> -extern void attribute_hidden pthread_cancel_init (void);
> +/* The difference from __libc_unwind_link_get is that here, errors
> +   terminate the process.  */
> +struct unwind_link ;
> +struct unwind_link *__pthread_unwind_link_get (void) attribute_hidden;
>  # endif
> -extern void __nptl_unwind_freeres (void) attribute_hidden;
>  #endif
>  
>  
> diff --git a/nptl/pthread_cancel.c b/nptl/pthread_cancel.c
> index f88fa24e19..a011d72fa1 100644
> --- a/nptl/pthread_cancel.c
> +++ b/nptl/pthread_cancel.c
> @@ -35,7 +35,8 @@ __pthread_cancel (pthread_t th)
>      return ESRCH;
>  
>  #ifdef SHARED
> -  pthread_cancel_init ();
> +  /* Trigger an error if libgcc_s cannot be loaded.  */
> +  __pthread_unwind_link_get ();
>  #endif
>    int result = 0;
>    int oldval;
> diff --git a/sysdeps/arm/nptl/unwind-forcedunwind.c b/sysdeps/arm/nptl/unwind-forcedunwind.c
> new file mode 100644
> index 0000000000..61db34c0b5
> --- /dev/null
> +++ b/sysdeps/arm/nptl/unwind-forcedunwind.c
> @@ -0,0 +1,25 @@
> +/* Unwinder function forwarders for libpthread.  Arm version.
> +   Copyright (C) 2021 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; see the file COPYING.LIB.  If
> +   not, see <https://www.gnu.org/licenses/>.  */
> +
> +#include <sysdeps/nptl/unwind-forcedunwind.c>
> +
> +void *
> +__unwind_link_get_resume (void)
> +{
> +  return UNWIND_LINK_PTR (__pthread_unwind_link_get (), _Unwind_Resume);
> +}

OK.

> diff --git a/sysdeps/arm/pt-arm-unwind-resume.S b/sysdeps/arm/pt-arm-unwind-resume.S
> index d579848696..c056eb38d0 100644
> --- a/sysdeps/arm/pt-arm-unwind-resume.S
> +++ b/sysdeps/arm/pt-arm-unwind-resume.S
> @@ -16,31 +16,5 @@
>     License along with the GNU C Library.  If not, see
>     <https://www.gnu.org/licenses/>.  */
>  
> -#include <sysdep.h>
> -
> -/* This is just implementing exactly what the C version does.
> -   We do it in assembly just to ensure that we get an unmolested tail
> -   call to the libgcc function, which is necessary for the ARM unwinder.  */
> -
> -ENTRY (_Unwind_Resume)
> -	LDR_HIDDEN (ip, ip, __libgcc_s_resume, 0)
> -	cmp	ip, #0
> -	beq	1f
> -0:	PTR_DEMANGLE (ip, ip, r2, r3)
> -	bx	ip
> -
> -	/* We need to save and restore LR (for our own return address)
> -	   and R0 (for the argument to _Unwind_Resume) around the call.  */
> -1:	push	{r0, lr}
> -	cfi_adjust_cfa_offset (8)
> -	cfi_rel_offset (r0, 0)
> -	cfi_rel_offset (lr, 4)
> -	bl	pthread_cancel_init
> -	pop	{r0, lr}
> -	cfi_adjust_cfa_offset (-8)
> -	cfi_restore (r0)
> -	cfi_restore (lr)
> -
> -	LDR_HIDDEN (ip, ip, __libgcc_s_resume, 0)
> -	b	0b
> -END (_Unwind_Resume)
> +/* The implementation in libpthread is identical to the one in libc.  */
> +#include <sysdeps/arm/arm-unwind-resume.S>

OK.

> diff --git a/sysdeps/nptl/unwind-forcedunwind.c b/sysdeps/nptl/unwind-forcedunwind.c
> index b70aae06ae..c0234670cf 100644
> --- a/sysdeps/nptl/unwind-forcedunwind.c
> +++ b/sysdeps/nptl/unwind-forcedunwind.c
> @@ -16,134 +16,49 @@
>     License along with the GNU C Library; see the file COPYING.LIB.  If
>     not, see <https://www.gnu.org/licenses/>.  */
>  
> -#include <dlfcn.h>
>  #include <stdio.h>
> -#include <unwind.h>
> +#include <unwind-link.h>
>  #include <pthreadP.h>
>  #include <sysdep.h>
>  #include <gnu/lib-names.h>
>  #include <unwind-resume.h>
>  
> -static void *libgcc_s_handle;
> -void (*__libgcc_s_resume) (struct _Unwind_Exception *exc)
> -  attribute_hidden __attribute__ ((noreturn));
> -static _Unwind_Reason_Code (*libgcc_s_personality) PERSONALITY_PROTO;
> -static _Unwind_Reason_Code (*libgcc_s_forcedunwind)
> -  (struct _Unwind_Exception *, _Unwind_Stop_Fn, void *);
> -static _Unwind_Word (*libgcc_s_getcfa) (struct _Unwind_Context *);
> -
> -void
> -__attribute_noinline__
> -pthread_cancel_init (void)
> +struct unwind_link *
> +__pthread_unwind_link_get (void)
>  {
> -  void *resume;
> -  void *personality;
> -  void *forcedunwind;
> -  void *getcfa;
> -  void *handle;
> -
> -  if (__glibc_likely (libgcc_s_handle != NULL))
> -    {
> -      /* Force gcc to reload all values.  */
> -      asm volatile ("" ::: "memory");
> -      return;
> -    }
> -
> -  /* See include/dlfcn.h. Use of __libc_dlopen requires RTLD_NOW.  */
> -  handle = __libc_dlopen (LIBGCC_S_SO);
> -
> -  if (handle == NULL
> -      || (resume = __libc_dlsym (handle, "_Unwind_Resume")) == NULL
> -      || (personality = __libc_dlsym (handle, "__gcc_personality_v0")) == NULL
> -      || (forcedunwind = __libc_dlsym (handle, "_Unwind_ForcedUnwind"))
> -	 == NULL
> -      || (getcfa = __libc_dlsym (handle, "_Unwind_GetCFA")) == NULL
> -#ifdef ARCH_CANCEL_INIT
> -      || ARCH_CANCEL_INIT (handle)
> -#endif
> -      )
> -    __libc_fatal (LIBGCC_S_SO " must be installed for pthread_cancel to work\n");
> -
> -  PTR_MANGLE (resume);
> -  __libgcc_s_resume = resume;
> -  PTR_MANGLE (personality);
> -  libgcc_s_personality = personality;
> -  PTR_MANGLE (forcedunwind);
> -  libgcc_s_forcedunwind = forcedunwind;
> -  PTR_MANGLE (getcfa);
> -  libgcc_s_getcfa = getcfa;
> -  /* Make sure libgcc_s_handle is written last.  Otherwise,
> -     pthread_cancel_init might return early even when the pointer the
> -     caller is interested in is not initialized yet.  */
> -  atomic_write_barrier ();
> -  libgcc_s_handle = handle;
> -}
> -
> -/* Register for cleanup in libpthread.so.  */
> -void
> -__nptl_unwind_freeres (void)
> -{
> -  void *handle = libgcc_s_handle;
> -  if (handle != NULL)
> -    {
> -      libgcc_s_handle = NULL;
> -      __libc_dlclose (handle);
> -    }
> +  struct unwind_link *unwind_link = __libc_unwind_link_get ();
> +  if (unwind_link == NULL)
> +    __libc_fatal (LIBGCC_S_SO
> +		  " must be installed for pthread_cancel to work\n");
> +  return unwind_link;
>  }
>  
>  #if !HAVE_ARCH_UNWIND_RESUME
>  void
>  _Unwind_Resume (struct _Unwind_Exception *exc)
>  {
> -  if (__glibc_unlikely (libgcc_s_handle == NULL))
> -    pthread_cancel_init ();
> -  else
> -    atomic_read_barrier ();
> -
> -  void (*resume) (struct _Unwind_Exception *exc) = __libgcc_s_resume;
> -  PTR_DEMANGLE (resume);
> -  resume (exc);
> +  UNWIND_LINK_PTR (__pthread_unwind_link_get (), _Unwind_Resume) (exc);
>  }
>  #endif
>  
>  _Unwind_Reason_Code
>  __gcc_personality_v0 PERSONALITY_PROTO
>  {
> -  if (__glibc_unlikely (libgcc_s_handle == NULL))
> -    pthread_cancel_init ();
> -  else
> -    atomic_read_barrier ();
> -
> -  __typeof (libgcc_s_personality) personality = libgcc_s_personality;
> -  PTR_DEMANGLE (personality);
> -  return (*personality) PERSONALITY_ARGS;
> +  return UNWIND_LINK_PTR (__pthread_unwind_link_get (), personality)
> +    PERSONALITY_ARGS;
>  }
>  
>  _Unwind_Reason_Code
>  _Unwind_ForcedUnwind (struct _Unwind_Exception *exc, _Unwind_Stop_Fn stop,
>  		      void *stop_argument)
>  {
> -  if (__glibc_unlikely (libgcc_s_handle == NULL))
> -    pthread_cancel_init ();
> -  else
> -    atomic_read_barrier ();
> -
> -  _Unwind_Reason_Code (*forcedunwind)
> -    (struct _Unwind_Exception *, _Unwind_Stop_Fn, void *)
> -    = libgcc_s_forcedunwind;
> -  PTR_DEMANGLE (forcedunwind);
> -  return forcedunwind (exc, stop, stop_argument);
> +  return UNWIND_LINK_PTR (__pthread_unwind_link_get (), _Unwind_ForcedUnwind)
> +    (exc, stop, stop_argument);
>  }
>  
>  _Unwind_Word
>  _Unwind_GetCFA (struct _Unwind_Context *context)
>  {
> -  if (__glibc_unlikely (libgcc_s_handle == NULL))
> -    pthread_cancel_init ();
> -  else
> -    atomic_read_barrier ();
> -
> -  _Unwind_Word (*getcfa) (struct _Unwind_Context *) = libgcc_s_getcfa;
> -  PTR_DEMANGLE (getcfa);
> -  return getcfa (context);
> +  return UNWIND_LINK_PTR (__pthread_unwind_link_get (), _Unwind_GetCFA)
> +    (context);
>  }

OK. All just wrappers.

> diff --git a/sysdeps/unix/sysv/linux/ia64/unwind-forcedunwind.c b/sysdeps/unix/sysv/linux/ia64/unwind-forcedunwind.c
> index e797ea22aa..eaed6cf2ef 100644
> --- a/sysdeps/unix/sysv/linux/ia64/unwind-forcedunwind.c
> +++ b/sysdeps/unix/sysv/linux/ia64/unwind-forcedunwind.c
> @@ -16,23 +16,11 @@
>     License along with the GNU C Library; if not, see
>     <https://www.gnu.org/licenses/>.  */
>  
> -#include <dlfcn.h>
> -#include <stdio.h>
> -#include <unwind.h>
> -#include <pthreadP.h>
> -
> -static _Unwind_Word (*libgcc_s_getbsp) (struct _Unwind_Context *);
> -
> -#define ARCH_CANCEL_INIT(handle) \
> -  ((libgcc_s_getbsp = __libc_dlsym (handle, "_Unwind_GetBSP")) == NULL)
> -
>  #include <sysdeps/nptl/unwind-forcedunwind.c>
>  
>  _Unwind_Word
>  _Unwind_GetBSP (struct _Unwind_Context *context)
>  {
> -  if (__builtin_expect (libgcc_s_getbsp == NULL, 0))
> -    pthread_cancel_init ();
> -
> -  return libgcc_s_getbsp (context);
> +  return UNWIND_LINK_PTR (__pthread_unwind_link_get (), _Unwind_GetBSP)
> +    (context);
>  }

OK. Likewise a wrapper.

> 


-- 
Cheers,
Carlos.