Run thread shutdown functions in an explicit order

  On 06/25/2018 07:37 PM, Carlos O'Donell wrote:

>> -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.

libc_freeres_fn defines the function as static, so it does not work here.

>>   /* 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.

I made a copy-paste error when I added it.

>>   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.

I could use libc_freefres_fn for this one, that's right.  New patch 
attached.

Thanks,
Florian
  

On 06/25/2018 01:52 PM, Florian Weimer wrote:
> On 06/25/2018 07:37 PM, Carlos O'Donell wrote:
> 

v2 looks better except I found Makerules needs a review. See below.

>>> -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.
> 
> libc_freeres_fn defines the function as static, so it does not work here.

Ah! Right, because you now need them defined with the appropriate linkage
so you can call them from the central __libc_thread_freeres function. Sorry,
I was being dense. Only those which are now called indirectly can be made
static.
 
>>>   /* 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.
> 
> I made a copy-paste error when I added it.

OK.

>>>   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.
> 
> I could use libc_freefres_fn for this one, that's right.  New patch attached.

OK.

> Thanks,
> Florian
> 
> thread-freeres.patch
> 
> 
> Subject: [PATCH] Run thread shutdown functions in an explicit order
> To: libc-alpha@sourceware.org
> 
> This removes the __libc_thread_subfreeres hook in favor of explict
> calls.
> 
> 2018-06-25  Florian Weimer  <fweimer@redhat.com>
> 

It looks like we missed a cleanup in Makerules ($(common-objpfx)shlib.lds),
can you review that please?

> 	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 and use libc_freeres_fn.
> 	* 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);
>  
>  /*
>   * 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);
> +
> +  /* 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);
>  }
> -#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);
> 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..b4021ab735 100644
> --- a/resolv/resolv_conf.c
> +++ b/resolv/resolv_conf.c
> @@ -673,8 +673,7 @@ __resolv_conf_detach (struct __res_state *resp)
>  }
>  
>  /* Deallocate the global data.  */
> -static void __attribute__ ((section ("__libc_thread_freeres_fn")))
> -freeres (void)
> +libc_freeres_fn (freeres)
>  {
>    /* No locking because this function is supposed to be called when
>       the process has turned single-threaded.  */
> @@ -698,4 +697,3 @@ freeres (void)
>       deallocated memory.  */
>    global = NULL;
>  }
> -text_set_element (__libc_subfreeres, freeres);
> 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);
> 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);
>  
> -
>  /*
>   * 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);

Cheers,
Carlos.