Re: [PATCH v2 4/9] nptl: Remove CANCELING_BITMASK

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

On 01/06/2021 10:50, Adhemerval Zanella wrote:
>>> diff --git a/nptl/pthread_cancel.c b/nptl/pthread_cancel.c
>>> index deb404600c..8dfbcff8c3 100644
>>> --- a/nptl/pthread_cancel.c
>>> +++ b/nptl/pthread_cancel.c
>>
>>> @@ -104,72 +87,33 @@ __pthread_cancel (pthread_t th)
>>>  		    " must be installed for pthread_cancel to work\n");
>>>    }
>>> +
>>> +  int oldch = atomic_fetch_or_acquire (&pd->cancelhandling, CANCELED_BITMASK);
>>> +  if ((oldch & CANCELED_BITMASK) != 0)
>>> +    return 0;
>>> +
>>> +  if (pd == THREAD_SELF)
>>>      {
>>> +      /* A single-threaded process should be able to kill itself, since there
>>> +	 is nothing in the POSIX specification that says that it cannot.  So
>>> +	 we set multiple_threads to true so that cancellation points get
>>> +	 executed.  */
>>> +      THREAD_SETMEM (THREAD_SELF, header.multiple_threads, 1);
>>>  #ifndef TLS_MULTIPLE_THREADS_IN_TCB
>>> -	__libc_multiple_threads = 1;
>>> +      __libc_multiple_threads = 1;
>>>  #endif
>>> +
>>> +      THREAD_SETMEM (pd, result, PTHREAD_CANCELED);
>>> +      if ((oldch & CANCELTYPE_BITMASK) != 0)
>>> +	__do_cancel ();
>>> +      return 0;
>>>      }
>>
>> The last part is for asynchronous self-cancel, right?  Don't we have to
>> check that cancellation is enabled, too?

Ok, so I recall why I did this originally and checking if cancel is enable
is not strictly necessary since SIGCANCEL signal handler will bail early
if cancellation is not enabled.  But it does seems the correct way to do
it.

> 
> Indeed it should be:
> 
>       if ((oldch & CANCELSTATE_BIT)

This should be '(oldch & CANCELSTATE_BIT) == 0'.

>           && (oldch & CANCELTYPE_BITMASK) != 0)          
> 
> I ended up fixing it later in the serie with the cancel type and state 
> refactor.  I have fixed it locally.
>