Re: RFC: pthread pid accessor (BZ# 27880)

[Adhemerval Zanella <adhemerval.zanella@linaro.org>]

On 01/06/2021 11:18, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> On 31/05/2021 13:51, Florian Weimer wrote:
>>> * Adhemerval Zanella:
>>>
>>>> It seems that this is trickier than it seems, some issues we might consider 
>>>> first:
>>>>
>>>>   1. What should we do with detached threads? As for pthread_kill, issuing 
>>>>      a pthread_gettid_np might use an invalid handler (since the pthread_t 
>>>>      identifier might be reused).  This only solution I have is to define 
>>>>      it as undefined behavior, this is not great but to proper support it
>>>>      would incur to keep tracking or all possible pthread_t identifiers 
>>>>      (we already keep the glibc provided stacks, dl_stack_cache, so it 
>>>>       would be a matter to include the user provided one in the list as 
>>>>      special entries).
>>>
>>> Detached threads are fine as long as the thread is still running.  This
>>> is something the application can ensure using synchronization.
>>>
>>> There are other interfaces with this property, including pthread_kill.
>>
>> Afaik pthread_kill detaches created threads or thread that call
>> pthread_detach are not really defined (the thread ID lifetime ends
>> when detached is issued).  We even have a bug report for this, BZ
>> #19193.
> 
> Bug 19193 is about joinable threads that have exited, not detached
> threads.

Fair, this is another issue indeed.

> 
>> But currently calling pthread_kill is already undefined: it accesses 
>> the internal tid file without any extra check.  Even using the
>> INVALID_NOT_TERMINATED_TD_P/INVALID_TD_P won't really improve thing, since
>> might still access invalid memory if the thread cache was empty and the
>> resulted 'struct thread' was deallocated.
> 
> But that's an implementation bug, not an application bug.
> 
> INVALID_NOT_TERMINATED_TD_P needs synchronization (possibly blocking the
> thread from exiting) to be correct.

Right, I think detached threads are not really issue in the end.

> 
>>>>   3. How do we handle the concurrent access between pthread_join and
>>>>   pthread_gettid_np? Once a pthread_join is issued, the pthread_t
>>>>   identifier might be reused and accessing it should be
>>>>   invalid. pthread_join first synchronizes using 'joinid' to avoid
>>>>   concurrent pthread_join and then wait the kernel signal on 'tid'
>>>>   that the thread has finished.  The straightforward
>>>>   'pthread_gettid_np' implementation would do a atomic load on tid,
>>>>   however it might read a transient value between pthread_join
>>>>   'joinid' setup and the futex wait.  I am not sure how to handle it
>>>>   correctly.
>>>
>>> The application must ensure through synchronization that the lifetime of
>>> the thread handle has not ended yet.  Concurrent calls with pthread_join
>>> is fine as long as the thread has not exited yet (same as for
>>> pthread_kill).
>>>
>>> The question is what we should do after thread exit, but with a joinable
>>> thread.  I think for that we should return the original TID the kernel
>>> assigned (even though it could not be reused).  That would strongly
>>> discourage the unsafe probing behavior because the function cannot be
>>> used to probe if the thread is still running.
>>
>> Do you mean between the thread cancel/exit and kernel reset the struct
>> thread 'tid' field?  The main problem is the thread might be detached
>> between, that's why pthread_join synchronizes first using the 'joinid'
>> field.
> 
> If the thread is detached and has exited, then the lifetime of its
> handle ends, so this isn't a problem for the new interface (or
> pthread_kill).
> 
> My question was about a joinable thread which has exited but which has
> not been joined yet.  We could return the original TID in this case, and
> there wouldn't any possible for races in this case in this interface
> because that TID does not change.  It then becomes a matter of what you
> do with the TID.  If you only use it for logging, than that is fine.

Right, assuming 'struct thread' lifetime being valid I think we can 
determine whether the threads has been terminate and not joined.

I think we should return the TID while the thread is being executing
either cancellation handler or c++ destructors during the unwind 
phase. Once no more user defined callback are being executed we should
not return the TID anymore.

It means moving the EXITING_BIT setting later, and this in turn will
require to change on how cancellation are handled (the cancellation
should not act when callbacks are being executed). I move through
this direction on my cancellation refactor.

> 
>>>> Also, MacOSX signature is:
>>>>
>>>>   int pthread_gettid_np (pthread_t thread, uint64_t *thread_id)
>>>>
>>>> And it returns the current thread identification if THREAD is NULL, returns
>>>> ESRCH for invalid handle (the 1. and 2. issue below), and also consults 
>>>> the kernel if the identifier can no be obtained.
>>>
>>> Macos calls the interface pthread_threadid_np, actually.  It looks as if
>>> it returns a truly unique number that isn't reused within the process or
>>> system.  A Linux TID wouldn't be like that, so I think we should call
>>> the interface something else.
>>
>> Fair enough, bionic has 
>>
>>   pid_t pthread_gettid_np(pthread_t t)
>>
>> So I think we might be an option. It basically returns the underlying
>> kernel process identifier, no extra guarantee as done by MacOSX 
>> implementation.
> 
> Yes, I would prefer that.  Does Bionic return the TID from the
> resettable TCB field, or a copy?

Bionic seems to maintain a list of active threads, so it returns
the active thread field directly.