This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH roland/weak-tls-dtors] Avoid unconditional __call_tls_dtors calls in static linking.


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.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]