Re: [PATCH v4] Destructor support for C++11 thread_local variables

["Carlos O'Donell" <carlos at redhat dot com>]

On 10/30/2012 10:37 AM, Siddhesh Poyarekar wrote:
> My test run completed and I realized that I had got to sort order wrong
> in the abilist files.  Here's the updated patch.

ChangeLog looks OK.

> diff --git a/include/link.h b/include/link.h
> index f0c8ad5..2e7147f 100644
> --- a/include/link.h
> +++ b/include/link.h
> @@ -301,6 +301,9 @@ struct link_map
>      /* Index of the module in the dtv array.  */
>      size_t l_tls_modid;
>  
> +    /* Number of thread_local objects constructed by this DSO.  */
> +    size_t l_tls_dtor_count;
> +
>      /* Information used to change permission after the relocations are
>         done.  */
>      ElfW(Addr) l_relro_addr;

OK.

> diff --git a/include/stdlib.h b/include/stdlib.h
> index d45b2f0..4387394 100644
> --- a/include/stdlib.h
> +++ b/include/stdlib.h
> @@ -99,6 +99,10 @@ extern int __cxa_atexit (void (*func) (void *), void *arg, void *d);
>  extern int __cxa_atexit_internal (void (*func) (void *), void *arg, void *d)
>       attribute_hidden;
>  
> +extern int __cxa_thread_atexit_impl (void (*func) (void *), void *arg,
> +				     void *d);
> +extern void __call_tls_dtors (void);
> +
>  extern void __cxa_finalize (void *d);
>  
>  extern int __posix_memalign (void **memptr, size_t alignment, size_t size);

OK

> diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c
> index 197dfa7..34c0d30 100644
> --- a/nptl/pthread_create.c
> +++ b/nptl/pthread_create.c
> @@ -311,6 +311,9 @@ start_thread (void *arg)
>  #endif
>      }
>  
> +  /* Call destructors for the thread_local TLS variables.  */
> +  __call_tls_dtors ();
> +

OK, call this when the thread exits.

>    /* Run the destructor for the thread-local data.  */
>    __nptl_deallocate_tsd ();

You mentioned in the wiki page of the description of this patch
that because thread_local is destroyed *before* the key destructors
are called that you can't use thread_local in the key destructors.

This needs to be documented. Please create a pthreads section in
the manual, create an empty definition for pthread_key_create and
document that C++11 thread_local may not be used by those registered
destructors.

Is there any way to avoid this restriction? Why can't we destory
the thread_local variables *after* calling the key destructors?
  
> --- a/stdlib/Makefile
> +++ b/stdlib/Makefile
> @@ -33,7 +33,7 @@ routines	:=							      \
>  	bsearch qsort msort						      \
>  	getenv putenv setenv secure-getenv				      \
>  	exit on_exit atexit cxa_atexit cxa_finalize old_atexit		      \
> -	quick_exit at_quick_exit cxa_at_quick_exit			      \
> +	quick_exit at_quick_exit cxa_at_quick_exit cxa_thread_atexit_impl     \

OK.

>  	abs labs llabs							      \
>  	div ldiv lldiv							      \
>  	mblen mbstowcs mbtowc wcstombs wctomb				      \
> @@ -69,9 +69,12 @@ tests		:= tst-strtol tst-strtod testmb testrand testsort testdiv   \
>  		   tst-makecontext tst-strtod4 tst-strtod5 tst-qsort2	    \
>  		   tst-makecontext2 tst-strtod6 tst-unsetenv1		    \
>  		   tst-makecontext3 bug-getcontext bug-fmtmsg1		    \
> -		   tst-secure-getenv tst-strtod-overflow tst-strtod-round
> +		   tst-secure-getenv tst-strtod-overflow tst-strtod-round   \
> +		   tst-tls-atexit

Thanks for the test case.

>  tests-static	:= tst-secure-getenv
>  
> +modules-names	= tst-tls-atexit-lib
> +
>  include ../Makeconfig
>  
>  ifeq ($(build-shared),yes)
> @@ -151,3 +154,9 @@ link-libm = $(common-objpfx)math/libm.a
>  endif
>  $(objpfx)bug-getcontext: $(link-libm)
>  $(objpfx)tst-strtod-round: $(link-libm)
> +
> +tst-tls-atexit-lib.so-no-z-defs = yes
> +
> +LDFLAGS-tst-tls-atexit = $(common-objpfx)nptl/libpthread.so \
> +			 $(common-objpfx)dlfcn/libdl.so
> +$(objpfx)tst-tls-atexit.out: $(objpfx)tst-tls-atexit-lib.so

OK.

> diff --git a/stdlib/Versions b/stdlib/Versions
> index 250bd5f..50dec5d 100644
> --- a/stdlib/Versions
> +++ b/stdlib/Versions
> @@ -105,6 +105,7 @@ libc {
>    }
>    GLIBC_2.17 {
>      secure_getenv;
> +    __cxa_thread_atexit_impl;

OK, because libstdc++ calls it, and __ because it's an internal implementation detail.

Note: I'd eventually like this to become pthread_atexit_np() which has been absent
from the standard and something users have asked for, but that's another issue.

>    }
>    GLIBC_PRIVATE {
>      # functions which have an additional interface since they are
> @@ -114,5 +115,6 @@ libc {
>      __abort_msg;
>      # Used from other libraries
>      __libc_secure_getenv;
> +    __call_tls_dtors;

OK, because libpthread calls it, but nothing else.

>    }
>  }
> diff --git a/stdlib/cxa_thread_atexit_impl.c b/stdlib/cxa_thread_atexit_impl.c
> new file mode 100644
> index 0000000..4f03126
> --- /dev/null
> +++ b/stdlib/cxa_thread_atexit_impl.c
> @@ -0,0 +1,108 @@
> +/* Register destructors for C++ TLS variables declared with thread_local.
> +   Copyright (C) 2012 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/>.  */
> +
> +#include <stdlib.h>
> +#include <ldsodefs.h>
> +
> +typedef void (*dtor_func) (void *);
> +
> +struct dtor_list
> +{
> +  dtor_func func;
> +  void *obj;
> +  struct link_map *map;
> +  struct dtor_list *next;
> +};

OK, linked list.

> +
> +static __thread struct dtor_list *tls_dtor_list;

OK, Per-thread data used to hold the lists.

> +static __thread void *dso_handle_cache;
> +static __thread struct link_map *lm_cache;

OK, Per-thread data to optimize lookups.

> +
> +/* Register a destructor for TLS variables declared with the 'thread_local'
> +   keyword.  This function is only called from code generated by the C++
> +   compiler.  */
> +int
> +__cxa_thread_atexit_impl (dtor_func func, void *obj, void *dso_handle)
> +{
> +  /* Prepend.  */
> +  struct dtor_list *new = calloc (1, sizeof (struct dtor_list));
> +  new->func = func;
> +  new->obj = obj;
> +  new->next = tls_dtor_list;
> +  tls_dtor_list = new;
> +
> +  /* See if we already encountered the DSO.  */
> +  __rtld_lock_lock_recursive (GL(dl_load_lock));

OK, take the load lock to prevent other threads from touching the loaded
libraries while we mark this one with DF_1_NODELETE.

> +
> +  if (__builtin_expect (dso_handle_cache != dso_handle, 0))

IIUC this is simply an optimization for registering a series of thread_local
destructors for the same library?

> +    {
> +      ElfW(Addr) caller = (ElfW(Addr)) dso_handle;
> +
> +      /* If the address is not recognized the call comes from the main
> +         program (we hope).  */
> +      lm_cache = GL(dl_ns)[LM_ID_BASE]._ns_loaded;
> +
> +      /* Find the highest-addressed object that DSO_HANDLE is not below.  */
> +      for (Lmid_t ns = 0; ns < GL(dl_nns); ++ns)
> +        for (struct link_map *l = GL(dl_ns)[ns]._ns_loaded; l != NULL;
> +             l = l->l_next)
> +          if (caller >= l->l_map_start && caller < l->l_map_end
> +              && (l->l_contiguous || _dl_addr_inside_object (l, caller)))
> +            {
> +              lm_cache = l;
> +              break;
> +            }
> +

OK, I need some clarification here, is DSO_HANDLE a handle returned by
dlopen() or simply a symbol within the DSO that the compiler has?
I assume the latter, otherwise I don't think this would work.

(1) Use a utility function.

I'm going to punish you because this is the 6th copy of this code 
I've seen. You cribbed it from do_sym.

[carlos@koi elf]$ grep 'Lmid_t ns = 0; ns < GL(dl_nns); ++ns' *
dl-addr.c:  for (Lmid_t ns = 0; ns < GL(dl_nns); ++ns)
dl-caller.c:  for (Lmid_t ns = 0; ns < GL(dl_nns); ++ns)
dl-libc.c:  for (Lmid_t ns = 0; ns < GL(dl_nns); ++ns)
dl-open.c:      for (Lmid_t ns = 0; ns < GL(dl_nns); ++ns)
dl-sym.c:  for (Lmid_t ns = 0; ns < GL(dl_nns); ++ns)
...

Could we please create a utility function and get that patch 
checked in first?

(2) Do not use the word "DSO handle" since that might confuse people.

If it's a symbol from within the DSO that the compiler is using to mark
that DSO in some way then say "DSO symbol". A handle is what is returned
from dlopen.

> +    }
> +  /* A destructor could result in a thread_local construction and the former
> +     could have cleared the flag.  */
> +  if (lm_cache->l_type == lt_loaded && lm_cache->l_tls_dtor_count == 0)
> +    lm_cache->l_flags_1 |= DF_1_NODELETE;
> +
> +  new->map = lm_cache;
> +  new->map->l_tls_dtor_count++;
> +
> +  __rtld_lock_unlock_recursive (GL(dl_load_lock));
> +
> +  return 0;
> +}
> +
> +/* Call the destructors.  This is called either when a thread returns from the
> +   initial function or when the process exits via the exit(3) function.  */
> +void
> +__call_tls_dtors (void)
> +{
> +  while (tls_dtor_list)
> +    {
> +      struct dtor_list *cur = tls_dtor_list;
> +      tls_dtor_list = tls_dtor_list->next;
> +
> +      cur->func (cur->obj);
> +
> +      __rtld_lock_lock_recursive (GL(dl_load_lock));
> +
> +      /* Allow DSO unload if count drops to zero.  */
> +      cur->map->l_tls_dtor_count--;
> +      if (cur->map->l_tls_dtor_count == 0 && cur->map->l_type == lt_loaded)
> +        cur->map->l_flags_1 &= ~DF_1_NODELETE;
> +
> +      __rtld_lock_unlock_recursive (GL(dl_load_lock));
> +
> +      free (cur);
> +    }
> +}

OK.

> diff --git a/stdlib/exit.c b/stdlib/exit.c
> index 1ad548f..78cb9f5 100644
> --- a/stdlib/exit.c
> +++ b/stdlib/exit.c
> @@ -25,7 +25,6 @@
>  #include "set-hooks.h"
>  DEFINE_HOOK (__libc_atexit, (void))
>  
> -
>  /* Call all functions registered with `atexit' and `on_exit',
>     in the reverse of the order in which they were registered
>     perform stdio cleanup, and terminate program execution with STATUS.  */
> @@ -34,6 +33,9 @@ attribute_hidden
>  __run_exit_handlers (int status, struct exit_function_list **listp,
>  		     bool run_list_atexit)
>  {
> +  /* First, call the TLS destructors.  */
> +  __call_tls_dtors ();
> +

OK.

>    /* We do it this way to handle recursive calls to exit () made by
>       the functions registered with `atexit' and `on_exit'. We call
>       everyone on the list and use the status value in the last
> diff --git a/stdlib/tst-tls-atexit-lib.c b/stdlib/tst-tls-atexit-lib.c
> new file mode 100644
> index 0000000..45ce54d
> --- /dev/null
> +++ b/stdlib/tst-tls-atexit-lib.c
> @@ -0,0 +1,37 @@
> +/* Verify that DSO is unloaded only if its TLS objects are destroyed - the DSO.
> +   Copyright (C) 2012 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/>.  */
> +
> +extern void *__dso_handle;
> +
> +typedef struct
> +{
> +  void *val;
> +} A;
> +
> +/* We only care about the destructor.  */
> +void A_dtor (void *obj)
> +{
> +  ((A *)obj)->val = obj;
> +}
> +
> +void do_foo (void)
> +{
> +  static __thread A b;
> +  __cxa_thread_atexit_impl (A_dtor, &b, __dso_handle);
> +}
> +

OK.

> diff --git a/stdlib/tst-tls-atexit.c b/stdlib/tst-tls-atexit.c
> new file mode 100644
> index 0000000..e973cc1
> --- /dev/null
> +++ b/stdlib/tst-tls-atexit.c
> @@ -0,0 +1,110 @@
> +/* Verify that DSO is unloaded only if its TLS objects are destroyed.
> +   Copyright (C) 2012 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/>.  */
> +
> +/* There are two tests in this test case.  The first is implicit where it is
> +   assumed that the destructor call on exit of the LOAD function does not
> +   segfault.  The other is a verification that after the thread has exited, a
> +   dlclose will unload the DSO.  */
> +
> +#include <dlfcn.h>
> +#include <pthread.h>
> +#include <stdio.h>
> +#include <unistd.h>
> +#include <string.h>
> +#include <errno.h>
> +
> +void *handle;
> +pthread_mutex_t m = PTHREAD_MUTEX_INITIALIZER;
> +
> +void *
> +load (void *u)
> +{
> +  pthread_mutex_lock (&m);
> +  handle = dlopen ("$ORIGIN/tst-tls-atexit-lib.so", RTLD_LAZY);
> +  if (!handle)
> +    {
> +      printf ("Unable to load DSO: %s\n", dlerror ());
> +      return (void *) (uintptr_t) 1;
> +    }
> +
> +  void (*foo) (void) = (void (*) (void)) dlsym(handle, "do_foo");
> +
> +  if (!foo)
> +    {
> +      printf ("Unable to find symbol: %s\n", dlerror ());
> +      exit (1);
> +    }
> +
> +  foo ();
> +
> +  /* This should not unload the DSO.  If it does, then the thread exit will
> +     result in a segfault.  */
> +  dlclose (handle);
> +  pthread_mutex_unlock (&m);
> +
> +  return NULL;
> +}
> +
> +int
> +main (void)
> +{
> +  pthread_t t;
> +  int ret;
> +  void *thr_ret;
> +
> +  if ((ret = pthread_create (&t, NULL, load, NULL)) != 0)
> +    {
> +      printf ("pthread_create failed: %s\n", strerror (ret));
> +      return 1;
> +    }
> +
> +  if ((ret = pthread_join (t, &thr_ret)) != 0)
> +    {
> +      printf ("pthread_create failed: %s\n", strerror (ret));
> +      return 1;
> +    }
> +
> +  if (thr_ret != NULL)
> +    return 1;
> +
> +  /* Now this should unload the DSO.  */
> +  dlclose (handle);
> +
> +  /* Run through our maps and ensure that the DSO is unloaded.  */
> +  FILE *f = fopen ("/proc/self/maps", "r");
> +
> +  if (f == NULL)
> +    {
> +      perror ("Failed to open /proc/self/maps");
> +      fprintf (stderr, "Skipping verification of DSO unload\n");
> +      return 0;
> +    }
> +
> +  char *line;
> +  size_t s;
> +  while (getline (&line, &s, f) > 0)
> +    {
> +      if (strstr (line, "tst-tls-atexit-lib.so"))
> +        {
> +	  printf ("DSO not unloaded yet:\n%s", line);
> +	  return 1;
> +	}
> +    }
> +
> +  return 0;
> +}

OK.

ABI files look OK.

Repost the two new patches (a) utility function cleanup and (b) new patch with questions answered.

Cheers,
Carlos.