Re: [PATCH] Run thread shutdown functions in an explicit order

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

On 06/25/2018 01:10 PM, Florian Weimer wrote:
> This removes the __libc_thread_subfreeres hook in favor of explict
> calls.

I have never liked the freeres function pointer table registration
and infrastructure, it was always hard to debug, and the ordering
was not deterministic.

Thank you for expanding the call, making them deterministic, and
cleaning things up at the same time.

This looks good to me modulo the libc_freeres_fn issues, below, that
I think we should cleanup to use the already existing macro in
include/libc-symbols.h until we kill libc freeres.

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

> 2018-06-25  Florian Weimer  <fweimer@redhat.com>
> 
> 	Run thread shutdown functions in an explicit order.
> 	* malloc/thread-freeres.c (__libc_thread_subfreeres): Remove hook
> 	definition.
> 	(__libc_thread_freeres): Call thread shutdown functions
> 	explicitly.
> 	* include/rpc/rpc.h (__rpc_thread_destroy): Add hidden attribute.
> 	* include/string.h (__strerror_thread_freeres): Declare.
> 	* malloc/arena.c (__malloc_arena_thread_freeres): Renamed from
> 	arena_thread_freeres.  No longer static.  Remove thread shutdown
> 	hook registration.
> 	* malloc/malloc-internal.h (__malloc_arena_thread_freeres):
> 	Declare.
> 	* resolv/res-close.c (__res_thread_freeres): Renamed from
> 	res_thread_freeres.  No longer static.  Remove thread shutdown
> 	hook registration.
> 	* resolv/resolv-internal.h (__res_thread_freeres): Declare.
> 	* resolv/resolv_conf.c (freeres): Remove incorrect section
> 	attribute.
> 	* string/strerror_l.c (__strerror_thread_freeres): Renamed from
> 	strerror_thread_freeres.  No longer static.  Remove thread
> 	shutdown hook registration.
> 	* sysdeps/mach/strerror_l.c (__strerror_thread_freeres): Likewise.
> 	* sunrpc/rpc_thread.c (__rpc_thread_destroy): Remove thread
> 	shutdown hook registration.
> 	* manual/memory.texi (Basic Allocation): Update comment.
> 
> diff --git a/include/rpc/rpc.h b/include/rpc/rpc.h
> index 327d84319e..f5cee6caef 100644
> --- a/include/rpc/rpc.h
> +++ b/include/rpc/rpc.h
> @@ -45,7 +45,7 @@ extern void __rpc_thread_svc_cleanup (void) attribute_hidden;
>  extern void __rpc_thread_clnt_cleanup (void) attribute_hidden;
>  extern void __rpc_thread_key_cleanup (void) attribute_hidden;
>  
> -extern void __rpc_thread_destroy (void);
> +extern void __rpc_thread_destroy (void) attribute_hidden;

OK.

>  
>  __libc_tsd_define (extern, struct rpc_thread_variables *, RPC_VARS)
>  
> diff --git a/include/string.h b/include/string.h
> index bb4922cbbe..4d622f1c03 100644
> --- a/include/string.h
> +++ b/include/string.h
> @@ -50,6 +50,9 @@ extern int __ffs (int __i) __attribute__ ((const));
>  
>  extern char *__strerror_r (int __errnum, char *__buf, size_t __buflen);
>  
> +/* Called as part of the thread shutdown sequence.  */
> +void __strerror_thread_freeres (void) attribute_hidden;
> +
>  /* Get _STRING_ARCH_unaligned.  */
>  #include <string_private.h>
>  #endif
> diff --git a/malloc/arena.c b/malloc/arena.c
> index 37183cfb6a..497ae475e7 100644
> --- a/malloc/arena.c
> +++ b/malloc/arena.c
> @@ -941,8 +941,8 @@ arena_get_retry (mstate ar_ptr, size_t bytes)
>    return ar_ptr;
>  }
>  
> -static void __attribute__ ((section ("__libc_thread_freeres_fn")))
> -arena_thread_freeres (void)
> +void
> +__malloc_arena_thread_freeres (void)
>  {
>    /* Shut down the thread cache first.  This could deallocate data for
>       the thread arena, so do this before we put the arena on the free
> @@ -966,7 +966,6 @@ arena_thread_freeres (void)
>        __libc_lock_unlock (free_list_lock);
>      }
>  }
> -text_set_element (__libc_thread_subfreeres, arena_thread_freeres);

OK.

>  
>  /*
>   * Local variables:
> diff --git a/malloc/malloc-internal.h b/malloc/malloc-internal.h
> index ad054508ee..9cee0fb2d7 100644
> --- a/malloc/malloc-internal.h
> +++ b/malloc/malloc-internal.h
> @@ -71,6 +71,9 @@ void __malloc_fork_unlock_parent (void) attribute_hidden;
>  /* Called in the child process after a fork.  */
>  void __malloc_fork_unlock_child (void) attribute_hidden;
>  
> +/* Called as part of the thread shutdown sequence.  */
> +void __malloc_arena_thread_freeres (void) attribute_hidden;
> +
>  /* Set *RESULT to LEFT * RIGHT.  Return true if the multiplication
>     overflowed.  */
>  static inline bool
> diff --git a/malloc/thread-freeres.c b/malloc/thread-freeres.c
> index 53ce41bb78..8902c845bc 100644
> --- a/malloc/thread-freeres.c
> +++ b/malloc/thread-freeres.c
> @@ -16,16 +16,24 @@
>     License along with the GNU C Library; if not, see
>     <http://www.gnu.org/licenses/>.  */
>  
> -#include <stdlib.h>
>  #include <libc-internal.h>
> -#include <set-hooks.h>
> +#include <malloc-internal.h>
> +#include <resolv/resolv-internal.h>
> +#include <rpc/rpc.h>
> +#include <string.h>
>  
> -#ifdef _LIBC_REENTRANT
> -DEFINE_HOOK (__libc_thread_subfreeres, (void));
> -
> -void __attribute__ ((section ("__libc_thread_freeres_fn")))
> +/* Thread shutdown function.  Note that this function must be called
> +   for threads during shutdown for correctness reasons.  Unlike
> +   __libc_subfreeres, skipping calls to it is not a valid
> +   optimization.  */
> +void
>  __libc_thread_freeres (void)
>  {
> -  RUN_HOOK (__libc_thread_subfreeres, ());
> +  call_function_static_weak (__rpc_thread_destroy);
> +  call_function_static_weak (__res_thread_freeres);
> +  call_function_static_weak (__strerror_thread_freeres);

OK.

> +
> +  /* This should come last because it shuts down malloc for this
> +     thread and the other shutdown functions might well call free.  */
> +  call_function_static_weak (__malloc_arena_thread_freeres);

OK.

>  }
> -#endif
> diff --git a/manual/memory.texi b/manual/memory.texi
> index b95f6aa1b9..2fac64939f 100644
> --- a/manual/memory.texi
> +++ b/manual/memory.texi
> @@ -404,7 +404,7 @@ this function is in @file{stdlib.h}.
>  @c      reads&writes next_to_use and iterates over arena next without guards
>  @c      those are harmless as long as we don't drop arenas from the
>  @c      NEXT list, and we never do; when a thread terminates,
> -@c      arena_thread_freeres prepends the arena to the free_list
> +@c      __malloc_arena_thread_freeres prepends the arena to the free_list
>  @c      NEXT_FREE list, but NEXT is never modified, so it's safe!
>  @c     mutex_trylock (arena lock) @asulock @aculock
>  @c     mutex_lock (arena lock) dup @asulock @aculock
> diff --git a/resolv/res-close.c b/resolv/res-close.c
> index e02f5afa15..38572b1d2f 100644
> --- a/resolv/res-close.c
> +++ b/resolv/res-close.c
> @@ -126,8 +126,8 @@ res_nclose (res_state statp)
>  libc_hidden_def (__res_nclose)
>  
>  /* This is called when a thread is exiting to free resources held in _res.  */
> -static void __attribute__ ((section ("__libc_thread_freeres_fn")))
> -res_thread_freeres (void)
> +void
> +__res_thread_freeres (void)
>  {
>    __resolv_context_freeres ();
>  
> @@ -140,5 +140,4 @@ res_thread_freeres (void)
>    /* Make sure we do a full re-initialization the next time.  */
>    _res.options = 0;
>  }
> -text_set_element (__libc_thread_subfreeres, res_thread_freeres);
> -text_set_element (__libc_subfreeres, res_thread_freeres);
> +text_set_element (__libc_subfreeres, __res_thread_freeres);

Until we kill __libc_subfreeres, this function should use the libc_freeres_fn() macro for now.

> diff --git a/resolv/resolv-internal.h b/resolv/resolv-internal.h
> index ac6e495927..b8e447c726 100644
> --- a/resolv/resolv-internal.h
> +++ b/resolv/resolv-internal.h
> @@ -97,4 +97,7 @@ int __res_nopt (struct resolv_context *, int n0,
>  int __inet_pton_length (int af, const char *src, size_t srclen, void *);
>  libc_hidden_proto (__inet_pton_length)
>  
> +/* Called as part of the thread shutdown sequence.  */
> +void __res_thread_freeres (void) attribute_hidden;
> +
>  #endif  /* _RESOLV_INTERNAL_H */
> diff --git a/resolv/resolv_conf.c b/resolv/resolv_conf.c
> index ab4e5dc82c..08c3a9a163 100644
> --- a/resolv/resolv_conf.c
> +++ b/resolv/resolv_conf.c
> @@ -673,7 +673,7 @@ __resolv_conf_detach (struct __res_state *resp)
>  }
>  
>  /* Deallocate the global data.  */
> -static void __attribute__ ((section ("__libc_thread_freeres_fn")))
> +static void

OK. Was this function in the wrong section? This section placement doesn't
really matter, it's just an optimization for hot/cold icache.

>  freeres (void)
>  {
>    /* No locking because this function is supposed to be called when

Until we kill __libc_subfreeres, this function should use the libc_freeres_fn() macro for now.

> diff --git a/string/strerror_l.c b/string/strerror_l.c
> index 7d90e809ce..2a9c3b5e0b 100644
> --- a/string/strerror_l.c
> +++ b/string/strerror_l.c
> @@ -57,15 +57,9 @@ strerror_l (int errnum, locale_t loc)
>  }
>  
>  
> -#ifdef _LIBC
> -# ifdef _LIBC_REENTRANT
> -/* This is called when a thread is exiting to free the last_value string.  */
> -static void __attribute__ ((section ("__libc_thread_freeres_fn")))
> -strerror_thread_freeres (void)
> +void
> +__strerror_thread_freeres (void)
>  {
>    free (last_value);
>  }
> -text_set_element (__libc_thread_subfreeres, strerror_thread_freeres);
> -text_set_element (__libc_subfreeres, strerror_thread_freeres);
> -# endif
> -#endif
> +text_set_element (__libc_subfreeres, __strerror_thread_freeres);

Until we kill __libc_subfreeres, this function should use the libc_freeres_fn() macro for now.

> diff --git a/sunrpc/rpc_thread.c b/sunrpc/rpc_thread.c
> index 3b1213aca7..31ccd89f74 100644
> --- a/sunrpc/rpc_thread.c
> +++ b/sunrpc/rpc_thread.c
> @@ -15,7 +15,7 @@ static __thread struct rpc_thread_variables *thread_rpc_vars
>  /*
>   * Task-variable destructor
>   */
> -void __attribute__ ((section ("__libc_thread_freeres_fn")))
> +void
>  __rpc_thread_destroy (void)
>  {
>  	struct rpc_thread_variables *tvp = thread_rpc_vars;
> @@ -36,12 +36,8 @@ __rpc_thread_destroy (void)
>  		thread_rpc_vars = NULL;
>  	}
>  }
> -#ifdef _LIBC_REENTRANT
> -text_set_element (__libc_thread_subfreeres, __rpc_thread_destroy);
> -#endif
>  text_set_element (__libc_subfreeres, __rpc_thread_destroy);

Until we kill __libc_subfreeres, this function should use the libc_freeres_fn() macro for now.

>  
> -
>  /*
>   * Initialize RPC multi-threaded operation
>   */
> diff --git a/sysdeps/mach/strerror_l.c b/sysdeps/mach/strerror_l.c
> index 61515a46b8..b9842ccf40 100644
> --- a/sysdeps/mach/strerror_l.c
> +++ b/sysdeps/mach/strerror_l.c
> @@ -87,15 +87,9 @@ strerror_l (int errnum, locale_t loc)
>  }
>  
>  
> -#ifdef _LIBC
> -# ifdef _LIBC_REENTRANT
> -/* This is called when a thread is exiting to free the last_value string.  */
> -static void __attribute__ ((section ("__libc_thread_freeres_fn")))
> -strerror_thread_freeres (void)
> +void
> +__strerror_thread_freeres (void)
>  {
>    free (last_value);
>  }
> -text_set_element (__libc_thread_subfreeres, strerror_thread_freeres);
> -text_set_element (__libc_subfreeres, strerror_thread_freeres);
> -# endif
> -#endif
> +text_set_element (__libc_subfreeres, __strerror_thread_freeres);

Until we kill __libc_subfreeres, this function should use the libc_freeres_fn() macro for now.

Cheers,
Carlos.