Run thread shutdown functions in an explicit order
Commit Message
On 06/25/2018 08:17 PM, Carlos O'Donell wrote:
> 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.
Hah. I had looked for that, but somehow didn't spot it. New patch
attached, still passes testing (on x86_64).
Thanks,
Florian
Comments
On 06/25/2018 03:58 PM, Florian Weimer wrote:
> On 06/25/2018 08:17 PM, Carlos O'Donell wrote:
>> 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.
>
> Hah. I had looked for that, but somehow didn't spot it. New patch attached, still passes testing (on x86_64).
v3 looks good to me. I have a follow-on patch to this
which is part of the libdl freeres work needed for DJ's
malloc hook removal. I hope you can review that too since
we are now both familiar with this code again :-)
Reviewed-by: Carlos O'Donell <carlos@redhat.com>
> 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>
>
> 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.
> * Makerules (shlib.lds): Do not provide section boundary symbols
> for __libc_thread_subfreeres.
> * manual/memory.texi (Basic Allocation): Update comment.
>
> diff --git a/Makerules b/Makerules
> index b2c2724fcb..a10a0b4d70 100644
> --- a/Makerules
> +++ b/Makerules
> @@ -648,9 +648,6 @@ $(common-objpfx)shlib.lds: $(common-objpfx)config.make $(..)Makerules
> PROVIDE(__start___libc_atexit = .);\
> __libc_atexit : { *(__libc_atexit) }\
> PROVIDE(__stop___libc_atexit = .);\
> - PROVIDE(__start___libc_thread_subfreeres = .);\
> - __libc_thread_subfreeres : { *(__libc_thread_subfreeres) }\
> - PROVIDE(__stop___libc_thread_subfreeres = .);\
OK.
> PROVIDE(__start___libc_IO_vtables = .);\
> __libc_IO_vtables : { *(__libc_IO_vtables) }\
> PROVIDE(__stop___libc_IO_vtables = .);\
> 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;
OK.
> +
> /* 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)
OK.
> {
> /* 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;
OK.
> +
> /* 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>
OK.
>
> -#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
OK.
> 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
OK.
> @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)
OK.
> {
> __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);
OK.
> 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;
OK.
> +
> #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)
OK.
> {
> /* 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);
OK.
> 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)
OK.
> {
> 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);
OK.
> 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)
OK.
> {
> 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
OK.
> 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)
OK.
> {
> 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);
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>
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.
* Makerules (shlib.lds): Do not provide section boundary symbols
for __libc_thread_subfreeres.
* manual/memory.texi (Basic Allocation): Update comment.
@@ -648,9 +648,6 @@ $(common-objpfx)shlib.lds: $(common-objpfx)config.make $(..)Makerules
PROVIDE(__start___libc_atexit = .);\
__libc_atexit : { *(__libc_atexit) }\
PROVIDE(__stop___libc_atexit = .);\
- PROVIDE(__start___libc_thread_subfreeres = .);\
- __libc_thread_subfreeres : { *(__libc_thread_subfreeres) }\
- PROVIDE(__stop___libc_thread_subfreeres = .);\
PROVIDE(__start___libc_IO_vtables = .);\
__libc_IO_vtables : { *(__libc_IO_vtables) }\
PROVIDE(__stop___libc_IO_vtables = .);\
@@ -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;
__libc_tsd_define (extern, struct rpc_thread_variables *, RPC_VARS)
@@ -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
@@ -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:
@@ -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
@@ -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
@@ -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
@@ -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);
@@ -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 */
@@ -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);
@@ -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);
@@ -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
*/
@@ -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);