Re: [PATCH 00/10] nptl: Fix Race conditions in pthread cancellation (BZ#12683)

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

On 17-09-2015 17:43, Joseph Myers wrote:
> On Thu, 17 Sep 2015, Adhemerval Zanella wrote:
> 
>> It will require some help for alpha, hppa, ia64, m68k, microblaze, nios2, sh,
>> sparc, and tile. I summarized in wiki page [2] the steps required to adjust 
>> the remaining architectures, but based on arm/aarch64 the minimal adjustments
>> required are:
> 
> Will you put proper instructions at 
> <https://sourceware.org/glibc/wiki/PortStatus> (describing what's required 
> for architecture-specific changes, not other parts of the patch series), 
> with a list of architectures needing help?  And do you need help for MIPS 
> or not (it's not in either of your architecture lists)?

I have updated the PostStatus wiki the required steps plus one I have forgotten
to include in my previous email:

5. Make INTERNAL_SYSCALL_NCS accept 6 argument syscall.

And I will require help to for MIPS.

> 
>> 1. Write a new syscall implementation at sysdeps/unix/sysv/linux/<arch>/syscall_cancel.S
>>    that basically do:
>>
>> long int __syscall_cancel_arch (volatile unsigned int *cancelhandling,
>>   __syscall_arg_t nr, __syscall_arg_t arg1, __syscall_arg_t arg2, 
>>   __syscall_arg_t arg3, __syscall_arg_t arg4, __syscall_arg_t arg5,
>>   __syscall_arg_t arg6)
>> {
>>   if (*cancelhandling & CANCELED_BITMASK)
>>     __syscall_do_cancel()
>>
>>   return INLINE_SYSCALL (nr, 6, arg1, arg2, arg3, arg4, arg5, arg6);
> 
> Do you mean INLINE_SYSCALL (setting errno, returning -1 on error) or 
> INTERNAL_SYSCALL?  If INTERNAL_SYSCALL, what exactly are the semantics for 
> architectures such as MIPS that use two registers for returns (with a 
> separate register for an error flag, and the normal return register having 
> the non-negated syscall number in that case)?  I didn't spot any such 
> architectures in the examples you'd already converted - if I missed any, 
> it would be helpful for your instructions for architecture maintainers to 
> point explicitly to the already-converted architectures that provide the 
> most generic examples for each of the two syscall return conventions.

This is mistake, the correct macro is indeed INTERNAL_SYSCALL_NCS and the
correct semantic is the follow (I corrected it in my last patch, but forgot
to update the email documentation):

long int __syscall_cancel_arch (volatile unsigned int *cancelhandling,
  __syscall_arg_t nr, __syscall_arg_t arg1, __syscall_arg_t arg2, 
  __syscall_arg_t arg3, __syscall_arg_t arg4, __syscall_arg_t arg5,
  __syscall_arg_t arg6)
{
  if (*cancelhandling & CANCELED_BITMASK)
    __syscall_do_cancel()

  INTERNAL_SYSCALL_DECL (err);
  result = INTERNAL_SYSCALL_NCS (nr, err, 6, a1, a2, a3, a4, a5, a6);
  if (INTERNAL_SYSCALL_ERROR_P (result, err))
    return -INTERNAL_SYSCALL_ERRNO (result, err);
  return result;
}

PowerPC also have the same architecture trait as MIPS to return the error
in a different register.

> 
>> 4. Define both SYSCALL_CANCEL_ERROR(__val) and SYSCALL_CANCEL_ERRNO(__val)
>>    macros.
> 
> Likewise, what are the semantics of these for architectures where syscall 
> return uses two registers?

These are to be used in the new SYSCALL_CANCEL definition:

#define SYSCALL_CANCEL(name, args...)                                   \
  ({                                                                    \
    long int sc_ret = SYSCALL_CANCEL_NCS (name, args);                  \
    if (SYSCALL_CANCEL_ERROR (sc_ret))                                  \
      {                                                                 \
        __set_errno (SYSCALL_CANCEL_ERRNO (sc_ret));                    \
        sc_ret = -1L;                                                   \
      }                                                                 \
    sc_ret;                                                             \
  })

So it should be modelled using similar semantics as INTERNAL_SYSCALL_ERROR_P
and INTERNAL_SYSCALL_ERRNO.

> 
> I note that you have __syscall_cancel, in the IS_IN (rtld) case, returning 
> the raw result of INTERNAL_SYSCALL_NCS, having discarded error 
> information.  I don't see how that is supposed to work in the two-register 
> case.
> 
> I think there should be a comment in the sources (maybe on a default 
> definition of the above interfaces, or on some code that uses them) 
> explaining the semantics, in addition to details on or linked from the 
> PortStatus wiki page at the point the changes go in.
> 

This should be the same as the non-thread version for non-rtld:

long int
__syscall_cancel (__syscall_arg_t nr, __syscall_arg_t arg1,
                  __syscall_arg_t arg2, __syscall_arg_t arg3,
                  __syscall_arg_t arg4, __syscall_arg_t arg5,
                  __syscall_arg_t arg6)
{
  INTERNAL_SYSCALL_DECL (err);
  result = INTERNAL_SYSCALL_NCS (nr, err, 6, a1, a2, a3, a4, a5, a6);
  if (INTERNAL_SYSCALL_ERROR_P (result, err))
    return -INTERNAL_SYSCALL_ERRNO (result, err);
  return result;
}

I will change it.