Re: [PATCH v2 2/3] Y2038: nptl: Provide futex_abstimed_wait64 supporting 64 bit time

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

On 19/09/2020 10:07, Lukasz Majewski wrote:
> This is the helper function, which uses struct __timespec64
> to provide 64 bit absolute time to futex syscalls.
> 
> The aim of this function is to move convoluted pre-processor
> macro code from sysdeps/nptl/lowlevellock-futex.h to C
> function in futex-internal.c
> 
> The futex_abstimed_wait64 function has been put into a separate
> file on the purpose - to avoid issues apparent on the m68k
> architecture related to small number of available registers (there
> is not enough registers to put all necessary arguments in them if
> the above function would be added to futex-internal.h with
> __always_inline attribute).
> 
> Additional precautions for m68k port have been taken - the
> futex-internal.c file will be compiled with -fno-inline flag.

LGTM with a nit regarding style and a clarification about 
'-fno-inline'.

> 
> ---
> Changes for v2:
> - Handle the case when *abstime pointer is NULL
> ---
>  sysdeps/nptl/futex-internal.c         | 70 +++++++++++++++++++++++++++
>  sysdeps/nptl/futex-internal.h         |  6 +++
>  sysdeps/unix/sysv/linux/m68k/Makefile |  2 +
>  3 files changed, 78 insertions(+)
> 
> diff --git a/sysdeps/nptl/futex-internal.c b/sysdeps/nptl/futex-internal.c
> index 3366aac162..3211b4c94f 100644
> --- a/sysdeps/nptl/futex-internal.c
> +++ b/sysdeps/nptl/futex-internal.c
> @@ -45,6 +45,29 @@ __futex_abstimed_wait_cancelable32 (unsigned int* futex_word,
>                                    abstime != NULL ? &ts32 : NULL,
>                                    NULL /* Unused.  */, FUTEX_BITSET_MATCH_ANY);
>  }
> +
> +static int
> +__futex_abstimed_wait32 (unsigned int* futex_word,
> +                         unsigned int expected, clockid_t clockid,
> +                         const struct __timespec64* abstime,
> +                         int private)
> +{
> +  struct timespec ts32;
> +
> +  if (abstime != NULL && ! in_time_t_range (abstime->tv_sec))
> +    return -EOVERFLOW;
> +
> +  unsigned int clockbit = (clockid == CLOCK_REALTIME) ?
> +    FUTEX_CLOCK_REALTIME : 0;
> +  int op = __lll_private_flag (FUTEX_WAIT_BITSET | clockbit, private);
> +
> +  if (abstime != NULL)
> +    ts32 = valid_timespec64_to_timespec (*abstime);
> +
> +  return INTERNAL_SYSCALL_CALL (futex, futex_word, op, expected,
> +                                abstime != NULL ? &ts32 : NULL,
> +                                NULL /* Unused.  */, FUTEX_BITSET_MATCH_ANY);
> +}
>  #endif
>  
>  int

Ok.

> @@ -97,3 +120,50 @@ __futex_abstimed_wait_cancelable64 (unsigned int* futex_word,
>        futex_fatal_error ();
>      }
>  }
> +
> +int
> +__futex_abstimed_wait64 (unsigned int* futex_word, unsigned int expected,
> +                         clockid_t clockid,
> +                         const struct __timespec64* abstime, int private)
> +{
> +  unsigned int clockbit;
> +  int err;
> +
> +  /* Work around the fact that the kernel rejects negative timeout values
> +     despite them being valid.  */
> +  if (__glibc_unlikely ((abstime != NULL) && (abstime->tv_sec < 0)))
> +    return ETIMEDOUT;
> +
> +  if (! lll_futex_supported_clockid(clockid))
> +    return EINVAL;

Space before parentheses. 

> +
> +  clockbit = (clockid == CLOCK_REALTIME) ? FUTEX_CLOCK_REALTIME : 0;
> +  int op = __lll_private_flag (FUTEX_WAIT_BITSET | clockbit, private);
> +
> +  err = INTERNAL_SYSCALL_CALL (futex_time64, futex_word, op, expected,
> +                               abstime, NULL /* Unused.  */,
> +                               FUTEX_BITSET_MATCH_ANY);
> +#ifndef __ASSUME_TIME64_SYSCALLS
> +  if (err == -ENOSYS)
> +    err = __futex_abstimed_wait32 (futex_word, expected,
> +                                   clockid, abstime, private);
> +#endif
> +  switch (err)
> +    {
> +    case 0:
> +    case -EAGAIN:
> +    case -EINTR:
> +    case -ETIMEDOUT:
> +      return -err;
> +
> +    case -EFAULT: /* Must have been caused by a glibc or application bug.  */
> +    case -EINVAL: /* Either due to wrong alignment, unsupported
> +		     clockid or due to the timeout not being
> +		     normalized. Must have been caused by a glibc or
> +		     application bug.  */
> +    case -ENOSYS: /* Must have been caused by a glibc bug.  */
> +    /* No other errors are documented at this time.  */
> +    default:
> +      futex_fatal_error ();
> +    }
> +}

Ok.

> diff --git a/sysdeps/nptl/futex-internal.h b/sysdeps/nptl/futex-internal.h
> index 7f3910ad98..1ba0d61938 100644
> --- a/sysdeps/nptl/futex-internal.h
> +++ b/sysdeps/nptl/futex-internal.h
> @@ -529,4 +529,10 @@ __futex_abstimed_wait_cancelable64 (unsigned int* futex_word,
>                                      const struct __timespec64* abstime,
>                                      int private) attribute_hidden;
>  
> +int
> +__futex_abstimed_wait64 (unsigned int* futex_word, unsigned int expected,
> +                         clockid_t clockid,
> +                         const struct __timespec64* abstime,
> +                         int private) attribute_hidden;
> +
>  #endif  /* futex-internal.h */

Ok.

> diff --git a/sysdeps/unix/sysv/linux/m68k/Makefile b/sysdeps/unix/sysv/linux/m68k/Makefile
> index be40fae68a..65164c5752 100644
> --- a/sysdeps/unix/sysv/linux/m68k/Makefile
> +++ b/sysdeps/unix/sysv/linux/m68k/Makefile
> @@ -21,3 +21,5 @@ sysdep-dl-routines += dl-static
>  sysdep-others += lddlibc4
>  install-bin += lddlibc4
>  endif
> +
> +CFLAGS-futex-internal.c += -fno-inline
> 

Do we still need it?