Re: [PATCH 01/10] nptl: Perform signal initialization upon pthread_create

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

On 20/05/2021 17:05, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> On 20/05/2021 16:41, Florian Weimer wrote:
>>> * Adhemerval Zanella:
>>>
>>>>>  int
>>>>>  __pthread_cancel (pthread_t th)
>>>>> @@ -72,14 +129,23 @@ __pthread_cancel (pthread_t th)
>>>>>  						    oldval))
>>>>>  	    goto again;
>>>>>  
>>>>> -	  /* The cancellation handler will take care of marking the
>>>>> -	     thread as canceled.  */
>>>>> -	  pid_t pid = __getpid ();
>>>>> -
>>>>> -	  int val = INTERNAL_SYSCALL_CALL (tgkill, pid, pd->tid,
>>>>> -					   SIGCANCEL);
>>>>> -	  if (INTERNAL_SYSCALL_ERROR_P (val))
>>>>> -	    result = INTERNAL_SYSCALL_ERRNO (val);
>>>>> +	  if (pd == THREAD_SELF)
>>>>> +	    /* This is not merely an optimization: An application may
>>>>> +	       call pthread_cancel (pthread_self ()) without calling
>>>>> +	       pthread_create, so the signal handler may not have been
>>>>> +	       set up for a self-cancel.  */
>>>>> +	    sigcancel_handler ();
>>>>
>>>> I think it would be simple to just call __pthread_exit (PTHREAD_CANCELED)
>>>> here, it won't require to split the cancellation handler, it already
>>>> unwind if cancel state is enabled and asynchronous, and it does not
>>>> require add another PTHREAD_STATIC_FN_REQUIRE hack. 
>>>>
>>>> It would require an extra __libc_unwind_link_get call, but I think we
>>>> can optimize it later (I am working on a patch to simplify it).
>>>
>>> It would be correct, I think.  pthread_cancel is not a cancellation
>>> point.
>>>
>>> #include <stdio.h>
>>> #include <pthread.h>
>>>
>>> int
>>> main (void)
>>> {
>>>   pthread_cancel (pthread_self ());
>>>   puts ("about to exit");
>>> }
>>>
>>> This should print “about to exit”.
>>
>> Yes, this is essentially sysdeps/pthread/tst-cancel-self.c.
> 
> But surely this won't work if we call pthread_exit (PTHREAD_CANCELED)
> because that exits the main thread at that point, and not at the next
> cancellation point.

But you need to use the same condition as the other places:

  if (CANCEL_ENABLED_AND_CANCELED_AND_ASYNCHRONOUS (...)
    __pthread_exit (PTHREAD_CANCELLED);

I really hope to once we get the libpthread move done to simplify
the whole cancellation code.

> 
>>>>> +/* This performs the initialization necessary when going from
>>>>> +   single-threaded to multi-threaded mode for the first time.  */
>>>>> +static void
>>>>> +late_init (void)
>>>>> +{
>>>>> +  struct sigaction sa;
>>>>> +  __sigemptyset (&sa.sa_mask);
>>>>> +
>>>>> +  /* Install the cancellation signal handler (in static builds only if
>>>>> +     pthread_cancel has been linked in).  If for some reason we cannot
>>>>> +     install the handler we do not abort.  Maybe we should, but it is
>>>>> +     only asynchronous cancellation which is affected.  */
>>>>> +#ifndef SHARED
>>>>> +  extern __typeof (__nptl_sigcancel_handler) __nptl_sigcancel_handler
>>>>> +    __attribute__ ((weak));
>>>>> +  if (__nptl_sigcancel_handler != NULL)
>>>>> +#endif
>>>>
>>>> This weak symbol can be avoided if we move the cancellation setup
>>>> on pthread_cancel instead.  I still think this is best approach,
>>>> it disentangle the cancellation handling.
>>>
>>> But then we either have to introduce yet another global flag or install
>>> the signal handler unconditionally before every cancel operation.  I do
>>> not think this results in a simplification.
>>
>> The flag will be just a static bool or int only define on pthread_cancel,
>> something like:
>>
>>   int
>>   __pthread_cancel (pthread_t th) 
>>   {
>>     [...]
>>     static int init = 0;
>>     if (atomic_load_relaxed (&init) == 0)
>>       {
>>         install_sighandler ();
>>         init = 1;
>>       }
>>     [...]
>>   }
>>
>> Bu the main advantage is to move the cancellation code logically when
>> it is actually used, and it is small improvement on both static
>> linking (since the static code will be used solely is cancellation is
>> used) and on runtime (since sigaction will be set only if pthread_cancel
>> is called).
> 
> Okay, I can prepare a version along these lines.  But in general, I
> consider less data and fewer conditionals an improvement.

Thanks.