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

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

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.

> 
>>> +/* 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).