[PATCH v3 2/2] nptl: Do not use pthread set_tid_address as state synchronization (BZ #19951)

[Adhemerval Zanella Netto]

On 25/07/25 07:16, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> diff --git a/nptl/pthread_cancel.c b/nptl/pthread_cancel.c
>> index b838273881..f0828ea870 100644
>> --- a/nptl/pthread_cancel.c
>> +++ b/nptl/pthread_cancel.c
>> @@ -60,7 +60,8 @@ __pthread_cancel (pthread_t th)
>>  {
>>    volatile struct pthread *pd = (volatile struct pthread *) th;
>>  
>> -  if (pd->tid == 0)
>> +  int state = atomic_load_acquire (&pd->joinstate);
>> +  if (state == THREAD_STATE_EXITED || state == THREAD_STATE_EXITING)
>>      /* The thread has already exited on the kernel side.  Its outcome
>>         (regular exit, other cancelation) has already been
>>         determined.  */
> 
> The “has already exited on the kernel side” part is not true anymore.

I think this comments holds, if joinstate is THREAD_STATE_EXITED it means
the kernel already acted upon the defined set_tid_addres; and
THREAD_STATE_EXITING is set atomically *before* exiting.

I think maybe I can remove the THREAD_STATE_EXITING and rely on 
__pthread_kill_implementation instead (since it will always take
the 'exit_lock' lock).

> 
> In pthread_kill, we have  this comment for a similar situation:
> 
>     /* The thread is about to exit (or has exited).  Sending the
>        signal is either not observable (the target thread has already
>        blocked signals at this point), or it will fail, or it might be
>        delivered to a new, unrelated thread that has reused the TID.
>        So do not actually send the signal.  */
> 
>>    if (__glibc_unlikely (atomic_fetch_add_relaxed (&__nptl_nthreads, -1) == 1))
>>      /* This was the last thread.  */
>>      exit (0);
>> @@ -574,20 +592,21 @@ start_thread (void *arg)
>>        pd->setxid_futex = 0;
>>      }
>>  
>> -  /* If the thread is detached free the TCB.  */
>> -  if (IS_DETACHED (pd))
>> +  if (prevstate == THREAD_STATE_DETACHED)
>>      /* Free the TCB.  */
>>      __nptl_free_tcb (pd);
>>  
>>    /* Remove the associated name from the thread stack.  */
>>    name_stack_maps (pd, false);
>>  
>> +  pd->tid = 0;
> 
> We have this:
> 
> int
> __pthread_getcpuclockid (pthread_t threadid, clockid_t *clockid)
> {
>   struct pthread *pd = (struct pthread *) threadid;
> 
>   /* Make sure the descriptor is valid.  */
>   if (INVALID_TD_P (pd))
>     /* Not a valid thread handle.  */
>     return ESRCH;
> …
> }
> 
> I asume the pd->tid assignment amove ensures that pthread_getcpuclockid
> can still be used t probe whether a thread is running?  Do we have a
> test for this?  There should be a comment on the assignment.

The tid will be continue to be set by pthread_create at the end of
thread execution, it won't done atomically anymore and it will not
consider the window between setting the 'tid' and the 'exit' syscall
issue as valid.

> 
> We anticipated a change as far as pthread_gettid_np is concerned and
> mentioned the possible behaviors in the manual.
> 
> From a debuggability point of view, it may be beneficial not to do the
> pd->tid assignment, so that it's possible to see previous TIDs in
> coredumps.  We can look at pd->joinstate in pthread_getcpuclockid
> and keep the current behavior, I think.

I think to allow it we would need to remove INVALID_TD_P, which I
had in my previous version of this fix; and use 'joinstate' otherwise.