This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH roland/weak-tls-dtors] Avoid unconditional __call_tls_dtors calls in static linking.
- From: "Carlos O'Donell" <carlos at redhat dot com>
- To: Roland McGrath <roland at hack dot frob dot com>
- Cc: "GNU C. Library" <libc-alpha at sourceware dot org>
- Date: Wed, 20 Mar 2013 19:37:41 -0400
- Subject: Re: [PATCH roland/weak-tls-dtors] Avoid unconditional __call_tls_dtors calls in static linking.
- References: <20130320224406 dot BE4452C06F at topped-with-meat dot com>
On 03/20/2013 06:44 PM, Roland McGrath wrote:
> The cxa_thread_atexit_impl addition had a few issues that nobody caught in
> review. Let's take more care, folks.
>
> 1. libc_hidden_proto et al are not for internal functions.
> You don't need that complexity. Just declare it hidden and you're done.
> The only reason we have that stuff is for public functions called from
> elsewhere in the library.
What problem does this cause?
I'd be happier if there was just one way to handle this, but I know
that without context you can't solve it properly.
It might be better to say have a single macro called "libc_hidden_proto"
but whose Nth argument that distinguishes the use. That way everyone
learns to use just one macro instead of a macro vs. an attribute.
Thoughts?
> 2. Rampant inattention to unconditional cruft in static linking.
> This problem has been bad for years and this is a drop in the bucket.
> But we don't need any more drops. Always think about the static linking
> case, and think very carefully when adding calls to new code that might
> bring in a lot of other references.
Agreed.
I think static linking is an important use case and I would not
object to more patches to fix the current state of affairs.
> If nobody sees any problems with this in a day or three, I'll put it in.
>
>
> Thanks,
> Roland
>
>
> 2013-03-20 Roland McGrath <roland@hack.frob.com>
>
> * include/stdlib.h (__call_tls_dtors): Declare with attribute_hidden.
> Drop libc_hidden_proto. Declare with __attribute__ ((weak)).
> * stdlib/cxa_thread_atexit_impl.c (__call_tls_dtors): Define with
> attribute_hidden. Drop libc_hidden_def.
> * stdlib/exit.c (__libc_atexit) [!SHARED]:
> Call __call_tls_dtors only if it's not NULL.
This needs a static linking test case to prevent regression.
> nptl/
> 2013-03-20 Roland McGrath <roland@hack.frob.com>
>
> * pthread_create.c (start_thread) [!SHARED]:
> Call __call_tls_dtors only if it's not NULL.
>
> --- a/include/stdlib.h
> +++ b/include/stdlib.h
> @@ -102,8 +102,7 @@ extern int __cxa_atexit_internal (void (*func) (void *), void *arg, void *d)
>
> extern int __cxa_thread_atexit_impl (void (*func) (void *), void *arg,
> void *d);
> -extern void __call_tls_dtors (void);
> -libc_hidden_proto (__call_tls_dtors);
> +extern void __call_tls_dtors (void) attribute_hidden __attribute__ ((weak));
Wish we had one way of doing this.
> extern void __cxa_finalize (void *d);
>
> --- a/nptl/pthread_create.c
> +++ b/nptl/pthread_create.c
> @@ -312,7 +312,10 @@ start_thread (void *arg)
> }
>
> /* Call destructors for the thread_local TLS variables. */
> - __call_tls_dtors ();
> +#ifndef SHARED
> + if (&__call_tls_dtors != NULL)
> +#endif
> + __call_tls_dtors ();
OK, so because you marked this weak the linker may not pull in
the object file to resolve the reference to __call_tls_dtors, instead
it must legitimately find a reference to __cxa_thread_atexit_impl
to pull in the object file. If no such reference to __cxa_thread_atexit_impl
exists, likely because no thread_local was used then &__call_tls_dtors
is NULL.
So if thread_local is used we pull in the implementation required to
support it, otherwise we don't and skip the call.
> /* Run the destructor for the thread-local data. */
> __nptl_deallocate_tsd ();
> --- a/stdlib/cxa_thread_atexit_impl.c
> +++ b/stdlib/cxa_thread_atexit_impl.c
> @@ -76,7 +76,8 @@ __cxa_thread_atexit_impl (dtor_func func, void *obj, void *dso_symbol)
> }
>
> /* 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. */
> + initial function or when the process exits via the exit function. */
> +attribute_hidden
> void
> __call_tls_dtors (void)
> {
> @@ -99,4 +100,3 @@ __call_tls_dtors (void)
> free (cur);
> }
> }
> -libc_hidden_def (__call_tls_dtors)
OK.
> --- a/stdlib/exit.c
> +++ b/stdlib/exit.c
> @@ -34,7 +34,10 @@ __run_exit_handlers (int status, struct exit_function_list **listp,
> bool run_list_atexit)
> {
> /* First, call the TLS destructors. */
> - __call_tls_dtors ();
> +#ifndef SHARED
> + if (&__call_tls_dtors != NULL)
> +#endif
> + __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
OK if you add a test case.
Cheers,
Carlos.