Re: pthread_key_create bug?

[Rich Felker <dalias@libc.org>]

On Mon, Jan 07, 2019 at 06:13:28PM +0100, Markus Wichmann wrote:
> On Sun, Jan 06, 2019 at 09:11:28PM -0500, Rich Felker wrote:
> > See commit 84d061d5a31c9c773e29e1e2b1ffe8cb9557bc58.
> > 
> > Rich
> 
> Speaking of that commit: Am I missing something? The log message of that
> commit says that pthread_key_delete was stuffed into another file to
> avoid __synccall() being pulled into programs not referencing
> pthread_key_delete(). Yet pthread_key_create.c contains
> clean_dirty_tsd(), which contains a hard dependency on
> __pthread_key_delete_synccall(), which will do the synccall, and thus
> pull everything in.
> 
> I appreciate that the function in question can never be called unless
> pthread_key_delete() is used, but the linker can't know that. The
> compiler can't see code of the other compilation units and thus has to
> generate code containing a reference to __pthread_key_delete_synccall(),
> and the linker can't see that the reference is unreachable unless
> __pthread_key_delete() is linked in...
> 
> Wait a minute. If we made that a weak reference, that would already
> suffice. Wouldn't even necessarily need an alias, since it is
> unreachable without pthread_key_delete() anyway.
> 
> Is the attached patch acceptable?
> 
> Ciao,
> Markus

> >From 851f8bb89d9fae664a0d6018fce251ab64d737b7 Mon Sep 17 00:00:00 2001
> From: Markus Wichmann <nullplan@gmx.net>
> Date: Mon, 7 Jan 2019 18:07:34 +0100
> Subject: [PATCH 4/4] Add weak reference to reduce static size.
> 
> Commit 84d061d5a31c9c773e29e1e2b1ffe8cb9557bc58 claimed to try and
> reduce the static size for programs referencing pthread_key_create() but
> not pthread_key_delete(). Unfortunately, the critical reference was
> still strong, so the __synccall() mechanism was pulled in, anyway. This
> commit makes the reference weak, so it is resolved to NULL if
> pthread_key_delete() is not used, but in that case the function is
> unreachable, anyway.
> ---
>  src/thread/pthread_key_create.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/src/thread/pthread_key_create.c b/src/thread/pthread_key_create.c
> index e26f199c..31ed0826 100644
> --- a/src/thread/pthread_key_create.c
> +++ b/src/thread/pthread_key_create.c
> @@ -35,6 +35,8 @@ static void clean_dirty_tsd_callback(void *p)
>  	if (args->caller == self) args->ret = 0;
>  }
>  
> +extern hidden weak void __pthread_key_delete_synccall(void (*f)(void *), void *p);
> +
>  static int clean_dirty_tsd(void)
>  {
>  	struct cleanup_args args = {
> -- 
> 2.19.1
> 

I think you're right, though we don't generally use weak references in
musl on the basis (perhaps somewhat dubious) that they're an
additional toolchain feature that might cause problems reusing the
code in non-ELF contexts (this may affect midipix; I'm not sure).
That's why weak aliases to dummy functions are used for this purpose
everywhere else.

Also: weak references to hidden functions historically had some
breakage with respect to generating unresolvable-at-ld-time
pc-relative references to the symbol. Thus their use might break some
gcc/binutils versions too.

Rich