Re: [PATCH] nptl: Implement pthread_self in libc.so

[Carlos O'Donell <carlos at redhat dot com>]

On 12/14/2017 09:50 PM, Florian Weimer wrote:
> On 12/15/2017 05:51 AM, Carlos O'Donell wrote:
>> On 12/14/2017 10:37 AM, Florian Weimer wrote:
>>> All binaries use TLS and thus need a properly set up TCB, so we can
>>> simply return its address directly, instead of forwarding to the
>>> libpthread implementation from libc.
>>
>> Could you expand on this a bit?
>>
>> Yes, all binaries use TLS by virtue of errno.
>>
>> Yes, because we need errno setup we need a TCB setup.
>>
>> However, internally we never call pthread_self, we use THREAD_SELF,
>> and access the thread register directly. So there is no internal cost
>> paid for pthread_self.
> 
> I don't understand this.  The existing implementation has a cost
> because of the redirect from libc.so to libpthread.so:
> 
> Dump of assembler code for function pthread_self:
>    <+0>:    mov    0x2b505a(%rip),%eax # <__libc_pthread_functions_init>
>    <+6>:    test   %eax,%eax
>    <+8>:    je     0x7ffff76f0690 <pthread_self+32>
>    <+10>:    mov    0x2b4fb7(%rip),%rax # <__libc_pthread_functions+280>
>    <+17>:    ror    $0x11,%rax
>    <+21>:    xor    %fs:0x30,%rax
>    <+30>:    jmpq   *%rax
>    <+32>:    xor    %eax,%eax
>    <+34>:    retq
> 
> Compare this to the implementation in libpthread (which is what the
> new libc.so implementation looks like as well):
> 
> Dump of assembler code for function __pthread_self:
>    <+0>:    mov    %fs:0x10,%rax
>    <+9>:    retq
> 
> The redirect implementation returns zero as a pthread_t value, which
> is not correct if libpthread is loaded later (something this redirect
> functionality is there to support, but which is currently defective
> in various ways).

Thanks.

Let me clarify.

Your change has no performance implications for libc.so, since we never
call pthread_self, we always use THREAD_SELF, the macro.

Your change *does* have a performance implication for applications calling
pthread_self without having linked against libpthread.so.

This is, as you say, an optimization.
 
>> The only cost paid is if the application binary calls pthread_self to
>> use that descriptor in some way, and to use it in some way means calling
>> a pthread function with it.
> 
> It's also common to use it for logging, although that is a slight
> abuse.  The range of permitted uses is certainly limited because
> pthread_t is an unsigned long, and any direct manipulation is
> undefined.

Good point.

> The stub implementation is broken, so we can't move more
> functionality from libpthread to libc until it is fixed anyway.

That could be fixed for just this case?
 
>> Are there thread functions we might use in a program not linked against
>> libpthread e.g. the pthread_atfork changes we have in Fedora?
> 
> pthread_thread_number_np depends on this change in practical terms.

Why? In pthread_thread_number_np you can call THREAD_SELF directly?

>>> The layout of struct pthread_functions is not changed by this commit,
>>> although the pointer used to store the address of pthread_self is now
>>> unused.
>>
>> Why?
> 
> To preserve the internal ABI.  It seem to have very little cost.  I
> can remove the field, of course.

I would prefer the removal. Please submit a v2.
 
>>> To avoid linking against the libpthread function in all cases, we would
>>> have to bump the symbol version of libpthread in libc.so and supply a
>>> compat symbol.
>>
>> Could you expand on what this might look like?
> 
> We'd add pthread_self@@GLIBC_2.27 to glibc and turn the existing
> version into a compat symbol.  Both would be backed by the same
> implementation (at the same address).  Hopefully, this will encourage
> link editors not to use the compatibility symbol in libpthread—it
> then has a completely different version, so it should be treated as
> unrelated.

I agree this is a little excessive when we already have the forwarders
in place, and can keep dlopen(pthread_self) working.
 
>>> This commit does not do that because the function
>>> implementation is so small, so the overhead by two active copies of the
>>> same function might well be smaller than the increase in symbol table
>>> size.
>>
>> Can we please measure that to be sure?
> 
> I don't see how.  Is it really necessary?  The new implementation in
> libc is clearly an improvement.

I think I see your point better now that you explained it in more detail.
The solution you propose is simpler.
 
> The link editor should probably be fixed not to link against
> compatibility symbols in certain cases.

Well, I think we should remove the symbol<->DSO error that prevents us
from migrating versioned symbols across DSO boundaries. This should be
more flexible.

-- 
Cheers,
Carlos.