[v5] y2038: nptl: Convert pthread_{clock|timed}join_np to support 64 bit time

Message ID 20200812102205.14816-1-lukma@denx.de
State Committed
Delegated to: Lukasz Majewski
Headers
Series [v5] y2038: nptl: Convert pthread_{clock|timed}join_np to support 64 bit time |

Commit Message

Lukasz Majewski Aug. 12, 2020, 10:22 a.m. UTC
  The pthread_clockjoin_np and pthread_timedjoin_np have been converted to
support 64 bit time.

This change introduces new futex_timed_wait_cancel64 function in
./sysdeps/nptl/futex-internal.h, which uses futex_time64 where possible
and tries to replace low-level preprocessor macros from
lowlevellock-futex.h
The pthread_{timed|clock}join_np only accept absolute time. Moreover,
there is no need to check for NULL passed as *abstime pointer as
clockwait_tid() always passes struct __timespec64.

For systems with __TIMESIZE != 64 && __WORDSIZE == 32:
- Conversions between 64 bit time to 32 bit are necessary
- Redirection to __pthread_{clock|timed}join_np64 will provide support
  for 64 bit time

Build tests:
./src/scripts/build-many-glibcs.py glibcs

Run-time tests:
- Run specific tests on ARM/x86 32bit systems (qemu):
  https://github.com/lmajewski/meta-y2038 and run tests:
  https://github.com/lmajewski/y2038-tests/commits/master

Above tests were performed with Y2038 redirection applied as well as without
to test the proper usage of both __pthread_{timed|clock}join_np64 and
__pthread_{timed|clock}join_np.

---
Changes for v2:
- Update the commit message

Changes for v3:
- Use const qualifier for futex_timed_wait_cancel64() timeout parameter
- Use INTERNAL_SYSCALL_CALL macro instead of INTERNAL_SYSCALL
- Use LIBC_CANCEL_{RESET|ASYNC} macro instead of CANCEL_{RESET|ASYNC}
- Add switch clause with listing possible error values returned by
  futex{_time64} syscall

Changes for v4:

- Update comment when -EOVERFLOW is returned from futex syscall
- Use INTERNAL_SYSCALL_CANCELL instead of INTERNAL_SYSCALL_CALL
  (with or without LIBC_CANCEL_ASYNC/LIBC_CANCEL_RESET) to make
  the code smaller as it hides cancellation points handling.

Changes for v5:

- Refactor the code to avoid using goto
---
 nptl/pthreadP.h               | 16 ++++++++++-
 nptl/pthread_clockjoin.c      | 19 +++++++++++--
 nptl/pthread_join_common.c    | 19 +++++++------
 nptl/pthread_timedjoin.c      | 18 ++++++++++--
 sysdeps/nptl/futex-internal.h | 53 +++++++++++++++++++++++++++++++++++
 5 files changed, 111 insertions(+), 14 deletions(-)
  

Comments

Alistair Francis Aug. 12, 2020, 9:54 p.m. UTC | #1
On Wed, Aug 12, 2020 at 3:22 AM Lukasz Majewski <lukma@denx.de> wrote:
>
> The pthread_clockjoin_np and pthread_timedjoin_np have been converted to
> support 64 bit time.
>
> This change introduces new futex_timed_wait_cancel64 function in
> ./sysdeps/nptl/futex-internal.h, which uses futex_time64 where possible
> and tries to replace low-level preprocessor macros from
> lowlevellock-futex.h
> The pthread_{timed|clock}join_np only accept absolute time. Moreover,
> there is no need to check for NULL passed as *abstime pointer as
> clockwait_tid() always passes struct __timespec64.
>
> For systems with __TIMESIZE != 64 && __WORDSIZE == 32:
> - Conversions between 64 bit time to 32 bit are necessary
> - Redirection to __pthread_{clock|timed}join_np64 will provide support
>   for 64 bit time
>
> Build tests:
> ./src/scripts/build-many-glibcs.py glibcs
>
> Run-time tests:
> - Run specific tests on ARM/x86 32bit systems (qemu):
>   https://github.com/lmajewski/meta-y2038 and run tests:
>   https://github.com/lmajewski/y2038-tests/commits/master
>
> Above tests were performed with Y2038 redirection applied as well as without
> to test the proper usage of both __pthread_{timed|clock}join_np64 and
> __pthread_{timed|clock}join_np.
>
> ---
> Changes for v2:
> - Update the commit message
>
> Changes for v3:
> - Use const qualifier for futex_timed_wait_cancel64() timeout parameter
> - Use INTERNAL_SYSCALL_CALL macro instead of INTERNAL_SYSCALL
> - Use LIBC_CANCEL_{RESET|ASYNC} macro instead of CANCEL_{RESET|ASYNC}
> - Add switch clause with listing possible error values returned by
>   futex{_time64} syscall
>
> Changes for v4:
>
> - Update comment when -EOVERFLOW is returned from futex syscall
> - Use INTERNAL_SYSCALL_CANCELL instead of INTERNAL_SYSCALL_CALL
>   (with or without LIBC_CANCEL_ASYNC/LIBC_CANCEL_RESET) to make
>   the code smaller as it hides cancellation points handling.
>
> Changes for v5:
>
> - Refactor the code to avoid using goto

Looks good.

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  nptl/pthreadP.h               | 16 ++++++++++-
>  nptl/pthread_clockjoin.c      | 19 +++++++++++--
>  nptl/pthread_join_common.c    | 19 +++++++------
>  nptl/pthread_timedjoin.c      | 18 ++++++++++--
>  sysdeps/nptl/futex-internal.h | 53 +++++++++++++++++++++++++++++++++++
>  5 files changed, 111 insertions(+), 14 deletions(-)
>
> diff --git a/nptl/pthreadP.h b/nptl/pthreadP.h
> index 6f94d6be31..99713c8447 100644
> --- a/nptl/pthreadP.h
> +++ b/nptl/pthreadP.h
> @@ -458,6 +458,20 @@ extern int __pthread_cond_init (pthread_cond_t *cond,
>  libc_hidden_proto (__pthread_cond_init)
>  extern int __pthread_cond_signal (pthread_cond_t *cond);
>  extern int __pthread_cond_wait (pthread_cond_t *cond, pthread_mutex_t *mutex);
> +
> +#if __TIMESIZE == 64
> +# define __pthread_clockjoin_np64 __pthread_clockjoin_np
> +# define __pthread_timedjoin_np64 __pthread_timedjoin_np
> +#else
> +extern int __pthread_clockjoin_np64 (pthread_t threadid, void **thread_return,
> +                                     clockid_t clockid,
> +                                     const struct __timespec64 *abstime);
> +libc_hidden_proto (__pthread_clockjoin_np64)
> +extern int __pthread_timedjoin_np64 (pthread_t threadid, void **thread_return,
> +                                     const struct __timespec64 *abstime);
> +libc_hidden_proto (__pthread_timedjoin_np64)
> +#endif
> +
>  extern int __pthread_cond_timedwait (pthread_cond_t *cond,
>                                      pthread_mutex_t *mutex,
>                                      const struct timespec *abstime);
> @@ -488,7 +502,7 @@ extern int __pthread_enable_asynccancel (void) attribute_hidden;
>  extern void __pthread_disable_asynccancel (int oldtype) attribute_hidden;
>  extern void __pthread_testcancel (void);
>  extern int __pthread_clockjoin_ex (pthread_t, void **, clockid_t,
> -                                  const struct timespec *, bool)
> +                                  const struct __timespec64 *, bool)
>    attribute_hidden;
>  extern int __pthread_sigmask (int, const sigset_t *, sigset_t *);
>  libc_hidden_proto (__pthread_sigmask);
> diff --git a/nptl/pthread_clockjoin.c b/nptl/pthread_clockjoin.c
> index a3e7f37e3b..3cd780f688 100644
> --- a/nptl/pthread_clockjoin.c
> +++ b/nptl/pthread_clockjoin.c
> @@ -16,14 +16,27 @@
>     License along with the GNU C Library; if not, see
>     <http://www.gnu.org/licenses/>.  */
>
> +#include <time.h>
>  #include "pthreadP.h"
>
>  int
> -__pthread_clockjoin_np (pthread_t threadid, void **thread_return,
> -                       clockid_t clockid,
> -                       const struct timespec *abstime)
> +__pthread_clockjoin_np64 (pthread_t threadid, void **thread_return,
> +                          clockid_t clockid, const struct __timespec64 *abstime)
>  {
>    return __pthread_clockjoin_ex (threadid, thread_return,
>                                   clockid, abstime, true);
>  }
> +
> +#if __TIMESIZE != 64
> +libc_hidden_def (__pthread_clockjoin_np64)
> +
> +int
> +__pthread_clockjoin_np (pthread_t threadid, void **thread_return,
> +                        clockid_t clockid, const struct timespec *abstime)
> +{
> +  struct __timespec64 ts64 = valid_timespec_to_timespec64 (*abstime);
> +
> +  return __pthread_clockjoin_np64 (threadid, thread_return, clockid, &ts64);
> +}
> +#endif
>  weak_alias (__pthread_clockjoin_np, pthread_clockjoin_np)
> diff --git a/nptl/pthread_join_common.c b/nptl/pthread_join_common.c
> index a96ceafde4..67d8e2b780 100644
> --- a/nptl/pthread_join_common.c
> +++ b/nptl/pthread_join_common.c
> @@ -20,6 +20,7 @@
>  #include <atomic.h>
>  #include <stap-probe.h>
>  #include <time.h>
> +#include <futex-internal.h>
>
>  static void
>  cleanup (void *arg)
> @@ -37,9 +38,11 @@ cleanup (void *arg)
>     afterwards.  The kernel up to version 3.16.3 does not use the private futex
>     operations for futex wake-up when the clone terminates.  */
>  static int
> -clockwait_tid (pid_t *tidp, clockid_t clockid, const struct timespec *abstime)
> +clockwait_tid (pid_t *tidp, clockid_t clockid,
> +               const struct __timespec64 *abstime)
>  {
>    pid_t tid;
> +  int ret;
>
>    if (! valid_nanoseconds (abstime->tv_nsec))
>      return EINVAL;
> @@ -47,11 +50,11 @@ clockwait_tid (pid_t *tidp, clockid_t clockid, const struct timespec *abstime)
>    /* Repeat until thread terminated.  */
>    while ((tid = *tidp) != 0)
>      {
> -      struct timespec rt;
> +      struct __timespec64 rt;
>
>        /* Get the current time. This can only fail if clockid is
>           invalid. */
> -      if (__glibc_unlikely (__clock_gettime (clockid, &rt)))
> +      if (__glibc_unlikely (__clock_gettime64 (clockid, &rt)))
>          return EINVAL;
>
>        /* Compute relative timeout.  */
> @@ -70,9 +73,9 @@ clockwait_tid (pid_t *tidp, clockid_t clockid, const struct timespec *abstime)
>        /* If *tidp == tid, wait until thread terminates or the wait times out.
>           The kernel up to version 3.16.3 does not use the private futex
>           operations for futex wake-up when the clone terminates.  */
> -      if (lll_futex_timed_wait_cancel (tidp, tid, &rt, LLL_SHARED)
> -         == -ETIMEDOUT)
> -        return ETIMEDOUT;
> +      ret = futex_timed_wait_cancel64 (tidp, tid, &rt, LLL_SHARED);
> +      if (ret == -ETIMEDOUT || ret == -EOVERFLOW)
> +        return -ret;
>      }
>
>    return 0;
> @@ -80,8 +83,8 @@ clockwait_tid (pid_t *tidp, clockid_t clockid, const struct timespec *abstime)
>
>  int
>  __pthread_clockjoin_ex (pthread_t threadid, void **thread_return,
> -                       clockid_t clockid,
> -                       const struct timespec *abstime, bool block)
> +                        clockid_t clockid,
> +                        const struct __timespec64 *abstime, bool block)
>  {
>    struct pthread *pd = (struct pthread *) threadid;
>
> diff --git a/nptl/pthread_timedjoin.c b/nptl/pthread_timedjoin.c
> index dd7038dcf7..6164ae7060 100644
> --- a/nptl/pthread_timedjoin.c
> +++ b/nptl/pthread_timedjoin.c
> @@ -16,13 +16,27 @@
>     License along with the GNU C Library; if not, see
>     <https://www.gnu.org/licenses/>.  */
>
> +#include <time.h>
>  #include "pthreadP.h"
>
>  int
> -__pthread_timedjoin_np (pthread_t threadid, void **thread_return,
> -                       const struct timespec *abstime)
> +__pthread_timedjoin_np64 (pthread_t threadid, void **thread_return,
> +                          const struct __timespec64 *abstime)
>  {
>    return __pthread_clockjoin_ex (threadid, thread_return,
>                                   CLOCK_REALTIME, abstime, true);
>  }
> +
> +#if __TIMESIZE != 64
> +libc_hidden_def (__pthread_timedjoin_np64)
> +
> +int
> +__pthread_timedjoin_np (pthread_t threadid, void **thread_return,
> +                        const struct timespec *abstime)
> +{
> +  struct __timespec64 ts64 = valid_timespec_to_timespec64 (*abstime);
> +
> +  return __pthread_timedjoin_np64 (threadid, thread_return, &ts64);
> +}
> +#endif
>  weak_alias (__pthread_timedjoin_np, pthread_timedjoin_np)
> diff --git a/sysdeps/nptl/futex-internal.h b/sysdeps/nptl/futex-internal.h
> index d622122ddc..1d038e4949 100644
> --- a/sysdeps/nptl/futex-internal.h
> +++ b/sysdeps/nptl/futex-internal.h
> @@ -467,4 +467,57 @@ futex_unlock_pi (unsigned int *futex_word, int private)
>      }
>  }
>
> +#ifndef __NR_futex_time64
> +# define __NR_futex_time64 __NR_futex
> +#endif
> +
> +static __always_inline int
> +futex_timed_wait_cancel64 (pid_t *tidp,  pid_t tid,
> +                           const struct __timespec64 *timeout, int private)
> +{
> +  int err = INTERNAL_SYSCALL_CANCEL (futex_time64, tidp,
> +                                     __lll_private_flag
> +                                     (FUTEX_WAIT, private), tid, timeout);
> +#ifndef __ASSUME_TIME64_SYSCALLS
> +  if (err == -ENOSYS)
> +    {
> +      struct timespec ts32;
> +      if (in_time_t_range (timeout->tv_sec))
> +        {
> +          ts32 = valid_timespec64_to_timespec (*timeout);
> +
> +          err = INTERNAL_SYSCALL_CANCEL (futex, tidp,
> +                                         __lll_private_flag (FUTEX_WAIT,
> +                                                             private),
> +                                         tid, &ts32);
> +        }
> +      else
> +        err = -EOVERFLOW;
> +    }
> +#endif
> +  switch (err)
> +    {
> +    case 0:
> +    case -EAGAIN:
> +    case -EINTR:
> +    case -ETIMEDOUT:
> +    case -EDEADLK:
> +    case -ENOSYS:
> +    case -EOVERFLOW:  /* Passed absolute timeout uses 64 bit time_t type, but
> +                         underlying kernel does not support 64 bit time_t futex
> +                         syscalls.  */
> +    case -EPERM:  /*  The caller is not allowed to attach itself to the futex.
> +                     Used to check if PI futexes are supported by the
> +                     kernel.  */
> +      return -err;
> +
> +    case -EINVAL: /* Either due to wrong alignment or due to the timeout not
> +                    being normalized.  Must have been caused by a glibc or
> +                    application bug.  */
> +    case -EFAULT: /* Must have been caused by a glibc or application bug.  */
> +    /* No other errors are documented at this time.  */
> +    default:
> +      futex_fatal_error ();
> +    }
> +}
>  #endif  /* futex-internal.h */
> --
> 2.20.1
>
  
Adhemerval Zanella Aug. 12, 2020, 11:11 p.m. UTC | #2
On 12/08/2020 07:22, Lukasz Majewski wrote:
> The pthread_clockjoin_np and pthread_timedjoin_np have been converted to
> support 64 bit time.
> 
> This change introduces new futex_timed_wait_cancel64 function in
> ./sysdeps/nptl/futex-internal.h, which uses futex_time64 where possible
> and tries to replace low-level preprocessor macros from
> lowlevellock-futex.h
> The pthread_{timed|clock}join_np only accept absolute time. Moreover,
> there is no need to check for NULL passed as *abstime pointer as
> clockwait_tid() always passes struct __timespec64.
> 
> For systems with __TIMESIZE != 64 && __WORDSIZE == 32:
> - Conversions between 64 bit time to 32 bit are necessary
> - Redirection to __pthread_{clock|timed}join_np64 will provide support
>   for 64 bit time
> 
> Build tests:
> ./src/scripts/build-many-glibcs.py glibcs
> 
> Run-time tests:
> - Run specific tests on ARM/x86 32bit systems (qemu):
>   https://github.com/lmajewski/meta-y2038 and run tests:
>   https://github.com/lmajewski/y2038-tests/commits/master
> 
> Above tests were performed with Y2038 redirection applied as well as without
> to test the proper usage of both __pthread_{timed|clock}join_np64 and
> __pthread_{timed|clock}join_np.

LGTM, thanks.  Just a small suggestion below.

Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>

> 
> ---
> Changes for v2:
> - Update the commit message
> 
> Changes for v3:
> - Use const qualifier for futex_timed_wait_cancel64() timeout parameter
> - Use INTERNAL_SYSCALL_CALL macro instead of INTERNAL_SYSCALL
> - Use LIBC_CANCEL_{RESET|ASYNC} macro instead of CANCEL_{RESET|ASYNC}
> - Add switch clause with listing possible error values returned by
>   futex{_time64} syscall
> 
> Changes for v4:
> 
> - Update comment when -EOVERFLOW is returned from futex syscall
> - Use INTERNAL_SYSCALL_CANCELL instead of INTERNAL_SYSCALL_CALL
>   (with or without LIBC_CANCEL_ASYNC/LIBC_CANCEL_RESET) to make
>   the code smaller as it hides cancellation points handling.
> 
> Changes for v5:
> 
> - Refactor the code to avoid using goto
> ---
>  nptl/pthreadP.h               | 16 ++++++++++-
>  nptl/pthread_clockjoin.c      | 19 +++++++++++--
>  nptl/pthread_join_common.c    | 19 +++++++------
>  nptl/pthread_timedjoin.c      | 18 ++++++++++--
>  sysdeps/nptl/futex-internal.h | 53 +++++++++++++++++++++++++++++++++++
>  5 files changed, 111 insertions(+), 14 deletions(-)
> 
> diff --git a/nptl/pthreadP.h b/nptl/pthreadP.h
> index 6f94d6be31..99713c8447 100644
> --- a/nptl/pthreadP.h
> +++ b/nptl/pthreadP.h
> @@ -458,6 +458,20 @@ extern int __pthread_cond_init (pthread_cond_t *cond,
>  libc_hidden_proto (__pthread_cond_init)
>  extern int __pthread_cond_signal (pthread_cond_t *cond);
>  extern int __pthread_cond_wait (pthread_cond_t *cond, pthread_mutex_t *mutex);
> +
> +#if __TIMESIZE == 64
> +# define __pthread_clockjoin_np64 __pthread_clockjoin_np
> +# define __pthread_timedjoin_np64 __pthread_timedjoin_np
> +#else
> +extern int __pthread_clockjoin_np64 (pthread_t threadid, void **thread_return,
> +                                     clockid_t clockid,
> +                                     const struct __timespec64 *abstime);
> +libc_hidden_proto (__pthread_clockjoin_np64)
> +extern int __pthread_timedjoin_np64 (pthread_t threadid, void **thread_return,
> +                                     const struct __timespec64 *abstime);
> +libc_hidden_proto (__pthread_timedjoin_np64)
> +#endif
> +
>  extern int __pthread_cond_timedwait (pthread_cond_t *cond,
>  				     pthread_mutex_t *mutex,
>  				     const struct timespec *abstime);

Ok.

> @@ -488,7 +502,7 @@ extern int __pthread_enable_asynccancel (void) attribute_hidden;
>  extern void __pthread_disable_asynccancel (int oldtype) attribute_hidden;
>  extern void __pthread_testcancel (void);
>  extern int __pthread_clockjoin_ex (pthread_t, void **, clockid_t,
> -				   const struct timespec *, bool)
> +				   const struct __timespec64 *, bool)
>    attribute_hidden;
>  extern int __pthread_sigmask (int, const sigset_t *, sigset_t *);
>  libc_hidden_proto (__pthread_sigmask);

Ok.

> diff --git a/nptl/pthread_clockjoin.c b/nptl/pthread_clockjoin.c
> index a3e7f37e3b..3cd780f688 100644
> --- a/nptl/pthread_clockjoin.c
> +++ b/nptl/pthread_clockjoin.c
> @@ -16,14 +16,27 @@
>     License along with the GNU C Library; if not, see
>     <http://www.gnu.org/licenses/>.  */
>  
> +#include <time.h>
>  #include "pthreadP.h"
>  
>  int
> -__pthread_clockjoin_np (pthread_t threadid, void **thread_return,
> -			clockid_t clockid,
> -			const struct timespec *abstime)
> +__pthread_clockjoin_np64 (pthread_t threadid, void **thread_return,
> +                          clockid_t clockid, const struct __timespec64 *abstime)
>  {
>    return __pthread_clockjoin_ex (threadid, thread_return,
>                                   clockid, abstime, true);
>  }
> +
> +#if __TIMESIZE != 64
> +libc_hidden_def (__pthread_clockjoin_np64)
> +
> +int
> +__pthread_clockjoin_np (pthread_t threadid, void **thread_return,
> +                        clockid_t clockid, const struct timespec *abstime)
> +{
> +  struct __timespec64 ts64 = valid_timespec_to_timespec64 (*abstime);
> +
> +  return __pthread_clockjoin_np64 (threadid, thread_return, clockid, &ts64);
> +}
> +#endif
>  weak_alias (__pthread_clockjoin_np, pthread_clockjoin_np)

Ok.

> diff --git a/nptl/pthread_join_common.c b/nptl/pthread_join_common.c
> index a96ceafde4..67d8e2b780 100644
> --- a/nptl/pthread_join_common.c
> +++ b/nptl/pthread_join_common.c
> @@ -20,6 +20,7 @@
>  #include <atomic.h>
>  #include <stap-probe.h>
>  #include <time.h>
> +#include <futex-internal.h>
>  
>  static void
>  cleanup (void *arg)
> @@ -37,9 +38,11 @@ cleanup (void *arg)
>     afterwards.  The kernel up to version 3.16.3 does not use the private futex
>     operations for futex wake-up when the clone terminates.  */
>  static int
> -clockwait_tid (pid_t *tidp, clockid_t clockid, const struct timespec *abstime)
> +clockwait_tid (pid_t *tidp, clockid_t clockid,
> +               const struct __timespec64 *abstime)
>  {
>    pid_t tid;
> +  int ret;
>  
>    if (! valid_nanoseconds (abstime->tv_nsec))
>      return EINVAL;
> @@ -47,11 +50,11 @@ clockwait_tid (pid_t *tidp, clockid_t clockid, const struct timespec *abstime)
>    /* Repeat until thread terminated.  */
>    while ((tid = *tidp) != 0)
>      {
> -      struct timespec rt;
> +      struct __timespec64 rt;
>  
>        /* Get the current time. This can only fail if clockid is
>           invalid. */
> -      if (__glibc_unlikely (__clock_gettime (clockid, &rt)))
> +      if (__glibc_unlikely (__clock_gettime64 (clockid, &rt)))
>          return EINVAL;
>  
>        /* Compute relative timeout.  */

Ok.

> @@ -70,9 +73,9 @@ clockwait_tid (pid_t *tidp, clockid_t clockid, const struct timespec *abstime)
>        /* If *tidp == tid, wait until thread terminates or the wait times out.
>           The kernel up to version 3.16.3 does not use the private futex
>           operations for futex wake-up when the clone terminates.  */
> -      if (lll_futex_timed_wait_cancel (tidp, tid, &rt, LLL_SHARED)
> -	  == -ETIMEDOUT)
> -        return ETIMEDOUT;
> +      ret = futex_timed_wait_cancel64 (tidp, tid, &rt, LLL_SHARED);
> +      if (ret == -ETIMEDOUT || ret == -EOVERFLOW)
> +        return -ret;
>      }
>  
>    return 0;

Ok.

> @@ -80,8 +83,8 @@ clockwait_tid (pid_t *tidp, clockid_t clockid, const struct timespec *abstime)
>  
>  int
>  __pthread_clockjoin_ex (pthread_t threadid, void **thread_return,
> -			clockid_t clockid,
> -			const struct timespec *abstime, bool block)
> +                        clockid_t clockid,
> +                        const struct __timespec64 *abstime, bool block)
>  {
>    struct pthread *pd = (struct pthread *) threadid;
>  

Ok.

> diff --git a/nptl/pthread_timedjoin.c b/nptl/pthread_timedjoin.c
> index dd7038dcf7..6164ae7060 100644
> --- a/nptl/pthread_timedjoin.c
> +++ b/nptl/pthread_timedjoin.c
> @@ -16,13 +16,27 @@
>     License along with the GNU C Library; if not, see
>     <https://www.gnu.org/licenses/>.  */
>  
> +#include <time.h>
>  #include "pthreadP.h"
>  
>  int
> -__pthread_timedjoin_np (pthread_t threadid, void **thread_return,
> -			const struct timespec *abstime)
> +__pthread_timedjoin_np64 (pthread_t threadid, void **thread_return,
> +                          const struct __timespec64 *abstime)
>  {
>    return __pthread_clockjoin_ex (threadid, thread_return,
>                                   CLOCK_REALTIME, abstime, true);
>  }
> +
> +#if __TIMESIZE != 64
> +libc_hidden_def (__pthread_timedjoin_np64)
> +
> +int
> +__pthread_timedjoin_np (pthread_t threadid, void **thread_return,
> +                        const struct timespec *abstime)
> +{
> +  struct __timespec64 ts64 = valid_timespec_to_timespec64 (*abstime);
> +
> +  return __pthread_timedjoin_np64 (threadid, thread_return, &ts64);
> +}
> +#endif
>  weak_alias (__pthread_timedjoin_np, pthread_timedjoin_np)

Ok.

> diff --git a/sysdeps/nptl/futex-internal.h b/sysdeps/nptl/futex-internal.h
> index d622122ddc..1d038e4949 100644
> --- a/sysdeps/nptl/futex-internal.h
> +++ b/sysdeps/nptl/futex-internal.h
> @@ -467,4 +467,57 @@ futex_unlock_pi (unsigned int *futex_word, int private)
>      }
>  }
>  
> +#ifndef __NR_futex_time64
> +# define __NR_futex_time64 __NR_futex
> +#endif
> +
> +static __always_inline int
> +futex_timed_wait_cancel64 (pid_t *tidp,  pid_t tid,
> +                           const struct __timespec64 *timeout, int private)
> +{
> +  int err = INTERNAL_SYSCALL_CANCEL (futex_time64, tidp,
> +                                     __lll_private_flag
> +                                     (FUTEX_WAIT, private), tid, timeout);
> +#ifndef __ASSUME_TIME64_SYSCALLS
> +  if (err == -ENOSYS)
> +    {
> +      struct timespec ts32;
> +      if (in_time_t_range (timeout->tv_sec))
> +        {
> +          ts32 = valid_timespec64_to_timespec (*timeout);

Maybe move the 'ts32' declaration to inside the brackets?

> +
> +          err = INTERNAL_SYSCALL_CANCEL (futex, tidp,
> +                                         __lll_private_flag (FUTEX_WAIT,
> +                                                             private),
> +                                         tid, &ts32);
> +        }
> +      else
> +        err = -EOVERFLOW;
> +    }
> +#endif
> +  switch (err)
> +    {
> +    case 0:
> +    case -EAGAIN:
> +    case -EINTR:
> +    case -ETIMEDOUT:
> +    case -EDEADLK:
> +    case -ENOSYS:
> +    case -EOVERFLOW:  /* Passed absolute timeout uses 64 bit time_t type, but
> +                         underlying kernel does not support 64 bit time_t futex
> +                         syscalls.  */
> +    case -EPERM:  /*  The caller is not allowed to attach itself to the futex.
> +		      Used to check if PI futexes are supported by the
> +		      kernel.  */
> +      return -err;
> +
> +    case -EINVAL: /* Either due to wrong alignment or due to the timeout not
> +		     being normalized.  Must have been caused by a glibc or
> +		     application bug.  */
> +    case -EFAULT: /* Must have been caused by a glibc or application bug.  */
> +    /* No other errors are documented at this time.  */
> +    default:
> +      futex_fatal_error ();
> +    }
> +}
>  #endif  /* futex-internal.h */
> 

Ok.
  
H.J. Lu Aug. 15, 2020, 6:11 p.m. UTC | #3
On Wed, Aug 12, 2020 at 3:22 AM Lukasz Majewski <lukma@denx.de> wrote:
>
> The pthread_clockjoin_np and pthread_timedjoin_np have been converted to
> support 64 bit time.
>
> This change introduces new futex_timed_wait_cancel64 function in
> ./sysdeps/nptl/futex-internal.h, which uses futex_time64 where possible
> and tries to replace low-level preprocessor macros from
> lowlevellock-futex.h
> The pthread_{timed|clock}join_np only accept absolute time. Moreover,
> there is no need to check for NULL passed as *abstime pointer as
> clockwait_tid() always passes struct __timespec64.
>
> For systems with __TIMESIZE != 64 && __WORDSIZE == 32:
> - Conversions between 64 bit time to 32 bit are necessary
> - Redirection to __pthread_{clock|timed}join_np64 will provide support
>   for 64 bit time
>
> Build tests:
> ./src/scripts/build-many-glibcs.py glibcs
>
> Run-time tests:
> - Run specific tests on ARM/x86 32bit systems (qemu):
>   https://github.com/lmajewski/meta-y2038 and run tests:
>   https://github.com/lmajewski/y2038-tests/commits/master
>
> Above tests were performed with Y2038 redirection applied as well as without
> to test the proper usage of both __pthread_{timed|clock}join_np64 and
> __pthread_{timed|clock}join_np.
>
> ---
> Changes for v2:
> - Update the commit message
>
> Changes for v3:
> - Use const qualifier for futex_timed_wait_cancel64() timeout parameter
> - Use INTERNAL_SYSCALL_CALL macro instead of INTERNAL_SYSCALL
> - Use LIBC_CANCEL_{RESET|ASYNC} macro instead of CANCEL_{RESET|ASYNC}
> - Add switch clause with listing possible error values returned by
>   futex{_time64} syscall
>
> Changes for v4:
>
> - Update comment when -EOVERFLOW is returned from futex syscall
> - Use INTERNAL_SYSCALL_CANCELL instead of INTERNAL_SYSCALL_CALL
>   (with or without LIBC_CANCEL_ASYNC/LIBC_CANCEL_RESET) to make
>   the code smaller as it hides cancellation points handling.
>
> Changes for v5:
>
> - Refactor the code to avoid using goto

> diff --git a/nptl/pthread_clockjoin.c b/nptl/pthread_clockjoin.c
> index a3e7f37e3b..3cd780f688 100644
> --- a/nptl/pthread_clockjoin.c
> +++ b/nptl/pthread_clockjoin.c
> @@ -16,14 +16,27 @@
>     License along with the GNU C Library; if not, see
>     <http://www.gnu.org/licenses/>.  */
>
> +#include <time.h>
>  #include "pthreadP.h"
>
>  int
> -__pthread_clockjoin_np (pthread_t threadid, void **thread_return,
> -                       clockid_t clockid,
> -                       const struct timespec *abstime)
> +__pthread_clockjoin_np64 (pthread_t threadid, void **thread_return,
> +                          clockid_t clockid, const struct __timespec64 *abstime)
>  {
>    return __pthread_clockjoin_ex (threadid, thread_return,
>                                   clockid, abstime, true);
>  }
> +
> +#if __TIMESIZE != 64
> +libc_hidden_def (__pthread_clockjoin_np64)
> +
> +int
> +__pthread_clockjoin_np (pthread_t threadid, void **thread_return,
> +                        clockid_t clockid, const struct timespec *abstime)
> +{
> +  struct __timespec64 ts64 = valid_timespec_to_timespec64 (*abstime);
> +
> +  return __pthread_clockjoin_np64 (threadid, thread_return, clockid, &ts64);
> +}
> +#endif
>  weak_alias (__pthread_clockjoin_np, pthread_clockjoin_np)

> +#if __TIMESIZE != 64
> +libc_hidden_def (__pthread_timedjoin_np64)
> +
> +int
> +__pthread_timedjoin_np (pthread_t threadid, void **thread_return,
> +                        const struct timespec *abstime)
> +{
> +  struct __timespec64 ts64 = valid_timespec_to_timespec64 (*abstime);
> +
> +  return __pthread_timedjoin_np64 (threadid, thread_return, &ts64);
> +}
> +#endif
>  weak_alias (__pthread_timedjoin_np, pthread_timedjoin_np)

This caused:

https://sourceware.org/bugzilla/show_bug.cgi?id=26394

I am testing this.
  
Andreas Schwab Aug. 17, 2020, 10:22 a.m. UTC | #4
On Aug 15 2020, H.J. Lu wrote:

> From fa1f97680fca290a378c449f2b63682ee348fd2c Mon Sep 17 00:00:00 2001
> From: "H.J. Lu" <hjl.tools@gmail.com>
> Date: Sat, 15 Aug 2020 11:06:35 -0700
> Subject: [PATCH] nptl: Handle NULL abstime [BZ #26394]
>
> Since abstime passed to pthread_{clock|timed}join_np may be NULL, convert
> to 64 bit abstime only if abstime isn't NULL.
> ---
>  nptl/pthread_clockjoin.c | 12 +++++++++---
>  nptl/pthread_timedjoin.c | 10 +++++++---
>  2 files changed, 16 insertions(+), 6 deletions(-)
>
> diff --git a/nptl/pthread_clockjoin.c b/nptl/pthread_clockjoin.c
> index 3cd780f688..0d780d6f4b 100644
> --- a/nptl/pthread_clockjoin.c
> +++ b/nptl/pthread_clockjoin.c
> @@ -34,9 +34,15 @@ int
>  __pthread_clockjoin_np (pthread_t threadid, void **thread_return,
>                          clockid_t clockid, const struct timespec *abstime)
>  {
> -  struct __timespec64 ts64 = valid_timespec_to_timespec64 (*abstime);
> -
> -  return __pthread_clockjoin_np64 (threadid, thread_return, clockid, &ts64);
> +  if (abstime)

        (abstime != NULL)

> +    {
> +      struct __timespec64 ts64 = valid_timespec_to_timespec64 (*abstime);
> +      return __pthread_clockjoin_np64 (threadid, thread_return, clockid,
> +				       &ts64);
> +    }
> +  else
> +      return __pthread_clockjoin_np64 (threadid, thread_return, clockid,
> +				       NULL);
>  }
>  #endif
>  weak_alias (__pthread_clockjoin_np, pthread_clockjoin_np)
> diff --git a/nptl/pthread_timedjoin.c b/nptl/pthread_timedjoin.c
> index 6164ae7060..a7b3cb35d2 100644
> --- a/nptl/pthread_timedjoin.c
> +++ b/nptl/pthread_timedjoin.c
> @@ -34,9 +34,13 @@ int
>  __pthread_timedjoin_np (pthread_t threadid, void **thread_return,
>                          const struct timespec *abstime)
>  {
> -  struct __timespec64 ts64 = valid_timespec_to_timespec64 (*abstime);
> -
> -  return __pthread_timedjoin_np64 (threadid, thread_return, &ts64);
> +  if (abstime)

        (abstime != NULL)

> +    {
> +      struct __timespec64 ts64 = valid_timespec_to_timespec64 (*abstime);
> +      return __pthread_timedjoin_np64 (threadid, thread_return, &ts64);
> +    }
> +  else
> +    return __pthread_timedjoin_np64 (threadid, thread_return, NULL);
>  }
>  #endif
>  weak_alias (__pthread_timedjoin_np, pthread_timedjoin_np)

Andreas.
  
H.J. Lu Aug. 17, 2020, 12:04 p.m. UTC | #5
On Mon, Aug 17, 2020 at 12:22:16PM +0200, Andreas Schwab wrote:
> On Aug 15 2020, H.J. Lu wrote:
> 
> > From fa1f97680fca290a378c449f2b63682ee348fd2c Mon Sep 17 00:00:00 2001
> > From: "H.J. Lu" <hjl.tools@gmail.com>
> > Date: Sat, 15 Aug 2020 11:06:35 -0700
> > Subject: [PATCH] nptl: Handle NULL abstime [BZ #26394]
> >
> > Since abstime passed to pthread_{clock|timed}join_np may be NULL, convert
> > to 64 bit abstime only if abstime isn't NULL.
> > ---
> >  nptl/pthread_clockjoin.c | 12 +++++++++---
> >  nptl/pthread_timedjoin.c | 10 +++++++---
> >  2 files changed, 16 insertions(+), 6 deletions(-)
> >
> > diff --git a/nptl/pthread_clockjoin.c b/nptl/pthread_clockjoin.c
> > index 3cd780f688..0d780d6f4b 100644
> > --- a/nptl/pthread_clockjoin.c
> > +++ b/nptl/pthread_clockjoin.c
> > @@ -34,9 +34,15 @@ int
> >  __pthread_clockjoin_np (pthread_t threadid, void **thread_return,
> >                          clockid_t clockid, const struct timespec *abstime)
> >  {
> > -  struct __timespec64 ts64 = valid_timespec_to_timespec64 (*abstime);
> > -
> > -  return __pthread_clockjoin_np64 (threadid, thread_return, clockid, &ts64);
> > +  if (abstime)
> 
>         (abstime != NULL)
> 
> > +    {
> > +      struct __timespec64 ts64 = valid_timespec_to_timespec64 (*abstime);
> > +      return __pthread_clockjoin_np64 (threadid, thread_return, clockid,
> > +				       &ts64);
> > +    }
> > +  else
> > +      return __pthread_clockjoin_np64 (threadid, thread_return, clockid,
> > +				       NULL);
> >  }
> >  #endif
> >  weak_alias (__pthread_clockjoin_np, pthread_clockjoin_np)
> > diff --git a/nptl/pthread_timedjoin.c b/nptl/pthread_timedjoin.c
> > index 6164ae7060..a7b3cb35d2 100644
> > --- a/nptl/pthread_timedjoin.c
> > +++ b/nptl/pthread_timedjoin.c
> > @@ -34,9 +34,13 @@ int
> >  __pthread_timedjoin_np (pthread_t threadid, void **thread_return,
> >                          const struct timespec *abstime)
> >  {
> > -  struct __timespec64 ts64 = valid_timespec_to_timespec64 (*abstime);
> > -
> > -  return __pthread_timedjoin_np64 (threadid, thread_return, &ts64);
> > +  if (abstime)
> 
>         (abstime != NULL)
> 
> > +    {
> > +      struct __timespec64 ts64 = valid_timespec_to_timespec64 (*abstime);
> > +      return __pthread_timedjoin_np64 (threadid, thread_return, &ts64);
> > +    }
> > +  else
> > +    return __pthread_timedjoin_np64 (threadid, thread_return, NULL);
> >  }
> >  #endif
> >  weak_alias (__pthread_timedjoin_np, pthread_timedjoin_np)
> 
> Andreas.
> 

Here is the updated patch I am checking in.

H.J.
---
Since abstime passed to pthread_{clock|timed}join_np may be NULL, convert
to 64 bit abstime only if abstime isn't NULL.
---
 nptl/pthread_clockjoin.c | 12 +++++++++---
 nptl/pthread_timedjoin.c | 10 +++++++---
 2 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/nptl/pthread_clockjoin.c b/nptl/pthread_clockjoin.c
index 3cd780f688..0baba1e83d 100644
--- a/nptl/pthread_clockjoin.c
+++ b/nptl/pthread_clockjoin.c
@@ -34,9 +34,15 @@ int
 __pthread_clockjoin_np (pthread_t threadid, void **thread_return,
                         clockid_t clockid, const struct timespec *abstime)
 {
-  struct __timespec64 ts64 = valid_timespec_to_timespec64 (*abstime);
-
-  return __pthread_clockjoin_np64 (threadid, thread_return, clockid, &ts64);
+  if (abstime != NULL)
+    {
+      struct __timespec64 ts64 = valid_timespec_to_timespec64 (*abstime);
+      return __pthread_clockjoin_np64 (threadid, thread_return, clockid,
+				       &ts64);
+    }
+  else
+      return __pthread_clockjoin_np64 (threadid, thread_return, clockid,
+				       NULL);
 }
 #endif
 weak_alias (__pthread_clockjoin_np, pthread_clockjoin_np)
diff --git a/nptl/pthread_timedjoin.c b/nptl/pthread_timedjoin.c
index 6164ae7060..6ade58853c 100644
--- a/nptl/pthread_timedjoin.c
+++ b/nptl/pthread_timedjoin.c
@@ -34,9 +34,13 @@ int
 __pthread_timedjoin_np (pthread_t threadid, void **thread_return,
                         const struct timespec *abstime)
 {
-  struct __timespec64 ts64 = valid_timespec_to_timespec64 (*abstime);
-
-  return __pthread_timedjoin_np64 (threadid, thread_return, &ts64);
+  if (abstime != NULL)
+    {
+      struct __timespec64 ts64 = valid_timespec_to_timespec64 (*abstime);
+      return __pthread_timedjoin_np64 (threadid, thread_return, &ts64);
+    }
+  else
+    return __pthread_timedjoin_np64 (threadid, thread_return, NULL);
 }
 #endif
 weak_alias (__pthread_timedjoin_np, pthread_timedjoin_np)
  
Lukasz Majewski Aug. 17, 2020, 1:30 p.m. UTC | #6
Hi Andreas, H.J.,

> On Aug 15 2020, H.J. Lu wrote:
> 
> > From fa1f97680fca290a378c449f2b63682ee348fd2c Mon Sep 17 00:00:00
> > 2001 From: "H.J. Lu" <hjl.tools@gmail.com>
> > Date: Sat, 15 Aug 2020 11:06:35 -0700
> > Subject: [PATCH] nptl: Handle NULL abstime [BZ #26394]
> >
> > Since abstime passed to pthread_{clock|timed}join_np may be NULL,

Could you point me to the exact reference that it is allowed (or
required) to pass NULL to this syscall?

The one which I've found on the web:
https://linux.die.net/man/3/pthread_timedjoin_np

doesn't mention about NULL pointer passed as the absolute time.
It says explicitly:
"The abstime argument is a structure of the following form, specifying
an absolute time measured since the Epoch"


As fair as I remember [1] glibc only handles the NULL pointer case when
it is explicitly written in the documentation/spec that NULL is passed
(like here: https://linux.die.net/man/2/timerfd_settime or here:
https://linux.die.net/man/2/utimensat).

Link:
[1] -
https://sourceware.org/pipermail/libc-alpha/2019-November/108072.html

> > convert to 64 bit abstime only if abstime isn't NULL.
> > ---
> >  nptl/pthread_clockjoin.c | 12 +++++++++---
> >  nptl/pthread_timedjoin.c | 10 +++++++---
> >  2 files changed, 16 insertions(+), 6 deletions(-)
> >
> > diff --git a/nptl/pthread_clockjoin.c b/nptl/pthread_clockjoin.c
> > index 3cd780f688..0d780d6f4b 100644
> > --- a/nptl/pthread_clockjoin.c
> > +++ b/nptl/pthread_clockjoin.c
> > @@ -34,9 +34,15 @@ int
> >  __pthread_clockjoin_np (pthread_t threadid, void **thread_return,
> >                          clockid_t clockid, const struct timespec
> > *abstime) {
> > -  struct __timespec64 ts64 = valid_timespec_to_timespec64
> > (*abstime); -
> > -  return __pthread_clockjoin_np64 (threadid, thread_return,
> > clockid, &ts64);
> > +  if (abstime)  
> 
>         (abstime != NULL)
> 
> > +    {
> > +      struct __timespec64 ts64 = valid_timespec_to_timespec64
> > (*abstime);
> > +      return __pthread_clockjoin_np64 (threadid, thread_return,
> > clockid,
> > +				       &ts64);
> > +    }
> > +  else
> > +      return __pthread_clockjoin_np64 (threadid, thread_return,
> > clockid,
> > +				       NULL);
> >  }
> >  #endif
> >  weak_alias (__pthread_clockjoin_np, pthread_clockjoin_np)
> > diff --git a/nptl/pthread_timedjoin.c b/nptl/pthread_timedjoin.c
> > index 6164ae7060..a7b3cb35d2 100644
> > --- a/nptl/pthread_timedjoin.c
> > +++ b/nptl/pthread_timedjoin.c
> > @@ -34,9 +34,13 @@ int
> >  __pthread_timedjoin_np (pthread_t threadid, void **thread_return,
> >                          const struct timespec *abstime)
> >  {
> > -  struct __timespec64 ts64 = valid_timespec_to_timespec64
> > (*abstime); -
> > -  return __pthread_timedjoin_np64 (threadid, thread_return, &ts64);
> > +  if (abstime)  
> 
>         (abstime != NULL)
> 
> > +    {
> > +      struct __timespec64 ts64 = valid_timespec_to_timespec64
> > (*abstime);
> > +      return __pthread_timedjoin_np64 (threadid, thread_return,
> > &ts64);
> > +    }
> > +  else
> > +    return __pthread_timedjoin_np64 (threadid, thread_return,
> > NULL); }
> >  #endif
> >  weak_alias (__pthread_timedjoin_np, pthread_timedjoin_np)  
> 
> Andreas.
> 




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
  
H.J. Lu Aug. 17, 2020, 1:52 p.m. UTC | #7
On Mon, Aug 17, 2020 at 6:30 AM Lukasz Majewski <lukma@denx.de> wrote:
>
> Hi Andreas, H.J.,
>
> > On Aug 15 2020, H.J. Lu wrote:
> >
> > > From fa1f97680fca290a378c449f2b63682ee348fd2c Mon Sep 17 00:00:00
> > > 2001 From: "H.J. Lu" <hjl.tools@gmail.com>
> > > Date: Sat, 15 Aug 2020 11:06:35 -0700
> > > Subject: [PATCH] nptl: Handle NULL abstime [BZ #26394]
> > >
> > > Since abstime passed to pthread_{clock|timed}join_np may be NULL,
>
> Could you point me to the exact reference that it is allowed (or
> required) to pass NULL to this syscall?
>
> The one which I've found on the web:
> https://linux.die.net/man/3/pthread_timedjoin_np
>
> doesn't mention about NULL pointer passed as the absolute time.
> It says explicitly:
> "The abstime argument is a structure of the following form, specifying
> an absolute time measured since the Epoch"
>
>
> As fair as I remember [1] glibc only handles the NULL pointer case when
> it is explicitly written in the documentation/spec that NULL is passed
> (like here: https://linux.die.net/man/2/timerfd_settime or here:
> https://linux.die.net/man/2/utimensat).
>
> Link:
> [1] -
> https://sourceware.org/pipermail/libc-alpha/2019-November/108072.html
>

sysdeps/pthread/tst-join14.c explicitly passes NULL:

static int
do_test_clock (clockid_t clockid)
{
  pthread_t th = xpthread_create (NULL, tf, NULL);

  void *status;
  int val = (clockid == CLOCK_USE_TIMEDJOIN)
    ? pthread_timedjoin_np (th, &status, NULL)
    : pthread_clockjoin_np (th, &status, clockid, NULL);
  TEST_COMPARE (val, 0);

  if (status != (void *) 42l)
    FAIL_EXIT1 ("return value %p, expected %p\n", status, (void *) 42l);

  return 0;
}
  
Adhemerval Zanella Aug. 17, 2020, 1:58 p.m. UTC | #8
On 17/08/2020 10:52, H.J. Lu wrote:
> On Mon, Aug 17, 2020 at 6:30 AM Lukasz Majewski <lukma@denx.de> wrote:
>>
>> Hi Andreas, H.J.,
>>
>>> On Aug 15 2020, H.J. Lu wrote:
>>>
>>>> From fa1f97680fca290a378c449f2b63682ee348fd2c Mon Sep 17 00:00:00
>>>> 2001 From: "H.J. Lu" <hjl.tools@gmail.com>
>>>> Date: Sat, 15 Aug 2020 11:06:35 -0700
>>>> Subject: [PATCH] nptl: Handle NULL abstime [BZ #26394]
>>>>
>>>> Since abstime passed to pthread_{clock|timed}join_np may be NULL,
>>
>> Could you point me to the exact reference that it is allowed (or
>> required) to pass NULL to this syscall?
>>
>> The one which I've found on the web:
>> https://linux.die.net/man/3/pthread_timedjoin_np
>>
>> doesn't mention about NULL pointer passed as the absolute time.
>> It says explicitly:
>> "The abstime argument is a structure of the following form, specifying
>> an absolute time measured since the Epoch"
>>
>>
>> As fair as I remember [1] glibc only handles the NULL pointer case when
>> it is explicitly written in the documentation/spec that NULL is passed
>> (like here: https://linux.die.net/man/2/timerfd_settime or here:
>> https://linux.die.net/man/2/utimensat).
>>
>> Link:
>> [1] -
>> https://sourceware.org/pipermail/libc-alpha/2019-November/108072.html
>>
> 
> sysdeps/pthread/tst-join14.c explicitly passes NULL:
> 
> static int
> do_test_clock (clockid_t clockid)
> {
>   pthread_t th = xpthread_create (NULL, tf, NULL);
> 
>   void *status;
>   int val = (clockid == CLOCK_USE_TIMEDJOIN)
>     ? pthread_timedjoin_np (th, &status, NULL)
>     : pthread_clockjoin_np (th, &status, clockid, NULL);
>   TEST_COMPARE (val, 0);
> 
>   if (status != (void *) 42l)
>     FAIL_EXIT1 ("return value %p, expected %p\n", status, (void *) 42l);
> 
>   return 0;
> }
> 

And it is a red-flag that you did not catch it running the testsuite.  Did you
check the patch with a full make check to certify there is no regressions?
  
Lukasz Majewski Aug. 17, 2020, 2:24 p.m. UTC | #9
Hi Adhemerval, H.J. Liu,

> On 17/08/2020 10:52, H.J. Lu wrote:
> > On Mon, Aug 17, 2020 at 6:30 AM Lukasz Majewski <lukma@denx.de>
> > wrote:  
> >>
> >> Hi Andreas, H.J.,
> >>  
> >>> On Aug 15 2020, H.J. Lu wrote:
> >>>  
> >>>> From fa1f97680fca290a378c449f2b63682ee348fd2c Mon Sep 17 00:00:00
> >>>> 2001 From: "H.J. Lu" <hjl.tools@gmail.com>
> >>>> Date: Sat, 15 Aug 2020 11:06:35 -0700
> >>>> Subject: [PATCH] nptl: Handle NULL abstime [BZ #26394]
> >>>>
> >>>> Since abstime passed to pthread_{clock|timed}join_np may be
> >>>> NULL,  
> >>
> >> Could you point me to the exact reference that it is allowed (or
> >> required) to pass NULL to this syscall?
> >>
> >> The one which I've found on the web:
> >> https://linux.die.net/man/3/pthread_timedjoin_np
> >>
> >> doesn't mention about NULL pointer passed as the absolute time.
> >> It says explicitly:
> >> "The abstime argument is a structure of the following form,
> >> specifying an absolute time measured since the Epoch"
> >>
> >>
> >> As fair as I remember [1] glibc only handles the NULL pointer case
> >> when it is explicitly written in the documentation/spec that NULL
> >> is passed (like here: https://linux.die.net/man/2/timerfd_settime
> >> or here: https://linux.die.net/man/2/utimensat).
> >>
> >> Link:
> >> [1] -
> >> https://sourceware.org/pipermail/libc-alpha/2019-November/108072.html
> >>  
> > 
> > sysdeps/pthread/tst-join14.c explicitly passes NULL:
> > 
> > static int
> > do_test_clock (clockid_t clockid)
> > {
> >   pthread_t th = xpthread_create (NULL, tf, NULL);
> > 
> >   void *status;
> >   int val = (clockid == CLOCK_USE_TIMEDJOIN)
> >     ? pthread_timedjoin_np (th, &status, NULL)
> >     : pthread_clockjoin_np (th, &status, clockid, NULL);
> >   TEST_COMPARE (val, 0);
> > 
> >   if (status != (void *) 42l)
> >     FAIL_EXIT1 ("return value %p, expected %p\n", status, (void *)
> > 42l);
> > 
> >   return 0;
> > }
> >   
> 
> And it is a red-flag that you did not catch it running the testsuite.
>  Did you check the patch with a full make check to certify there is
> no regressions?

I must admit that I've run the build-many-glibcs and also my tests (on
yocto) on which NULL was not passed.

I must have forgotten to run the make check - sorry .....


Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
  

Patch

diff --git a/nptl/pthreadP.h b/nptl/pthreadP.h
index 6f94d6be31..99713c8447 100644
--- a/nptl/pthreadP.h
+++ b/nptl/pthreadP.h
@@ -458,6 +458,20 @@  extern int __pthread_cond_init (pthread_cond_t *cond,
 libc_hidden_proto (__pthread_cond_init)
 extern int __pthread_cond_signal (pthread_cond_t *cond);
 extern int __pthread_cond_wait (pthread_cond_t *cond, pthread_mutex_t *mutex);
+
+#if __TIMESIZE == 64
+# define __pthread_clockjoin_np64 __pthread_clockjoin_np
+# define __pthread_timedjoin_np64 __pthread_timedjoin_np
+#else
+extern int __pthread_clockjoin_np64 (pthread_t threadid, void **thread_return,
+                                     clockid_t clockid,
+                                     const struct __timespec64 *abstime);
+libc_hidden_proto (__pthread_clockjoin_np64)
+extern int __pthread_timedjoin_np64 (pthread_t threadid, void **thread_return,
+                                     const struct __timespec64 *abstime);
+libc_hidden_proto (__pthread_timedjoin_np64)
+#endif
+
 extern int __pthread_cond_timedwait (pthread_cond_t *cond,
 				     pthread_mutex_t *mutex,
 				     const struct timespec *abstime);
@@ -488,7 +502,7 @@  extern int __pthread_enable_asynccancel (void) attribute_hidden;
 extern void __pthread_disable_asynccancel (int oldtype) attribute_hidden;
 extern void __pthread_testcancel (void);
 extern int __pthread_clockjoin_ex (pthread_t, void **, clockid_t,
-				   const struct timespec *, bool)
+				   const struct __timespec64 *, bool)
   attribute_hidden;
 extern int __pthread_sigmask (int, const sigset_t *, sigset_t *);
 libc_hidden_proto (__pthread_sigmask);
diff --git a/nptl/pthread_clockjoin.c b/nptl/pthread_clockjoin.c
index a3e7f37e3b..3cd780f688 100644
--- a/nptl/pthread_clockjoin.c
+++ b/nptl/pthread_clockjoin.c
@@ -16,14 +16,27 @@ 
    License along with the GNU C Library; if not, see
    <http://www.gnu.org/licenses/>.  */
 
+#include <time.h>
 #include "pthreadP.h"
 
 int
-__pthread_clockjoin_np (pthread_t threadid, void **thread_return,
-			clockid_t clockid,
-			const struct timespec *abstime)
+__pthread_clockjoin_np64 (pthread_t threadid, void **thread_return,
+                          clockid_t clockid, const struct __timespec64 *abstime)
 {
   return __pthread_clockjoin_ex (threadid, thread_return,
                                  clockid, abstime, true);
 }
+
+#if __TIMESIZE != 64
+libc_hidden_def (__pthread_clockjoin_np64)
+
+int
+__pthread_clockjoin_np (pthread_t threadid, void **thread_return,
+                        clockid_t clockid, const struct timespec *abstime)
+{
+  struct __timespec64 ts64 = valid_timespec_to_timespec64 (*abstime);
+
+  return __pthread_clockjoin_np64 (threadid, thread_return, clockid, &ts64);
+}
+#endif
 weak_alias (__pthread_clockjoin_np, pthread_clockjoin_np)
diff --git a/nptl/pthread_join_common.c b/nptl/pthread_join_common.c
index a96ceafde4..67d8e2b780 100644
--- a/nptl/pthread_join_common.c
+++ b/nptl/pthread_join_common.c
@@ -20,6 +20,7 @@ 
 #include <atomic.h>
 #include <stap-probe.h>
 #include <time.h>
+#include <futex-internal.h>
 
 static void
 cleanup (void *arg)
@@ -37,9 +38,11 @@  cleanup (void *arg)
    afterwards.  The kernel up to version 3.16.3 does not use the private futex
    operations for futex wake-up when the clone terminates.  */
 static int
-clockwait_tid (pid_t *tidp, clockid_t clockid, const struct timespec *abstime)
+clockwait_tid (pid_t *tidp, clockid_t clockid,
+               const struct __timespec64 *abstime)
 {
   pid_t tid;
+  int ret;
 
   if (! valid_nanoseconds (abstime->tv_nsec))
     return EINVAL;
@@ -47,11 +50,11 @@  clockwait_tid (pid_t *tidp, clockid_t clockid, const struct timespec *abstime)
   /* Repeat until thread terminated.  */
   while ((tid = *tidp) != 0)
     {
-      struct timespec rt;
+      struct __timespec64 rt;
 
       /* Get the current time. This can only fail if clockid is
          invalid. */
-      if (__glibc_unlikely (__clock_gettime (clockid, &rt)))
+      if (__glibc_unlikely (__clock_gettime64 (clockid, &rt)))
         return EINVAL;
 
       /* Compute relative timeout.  */
@@ -70,9 +73,9 @@  clockwait_tid (pid_t *tidp, clockid_t clockid, const struct timespec *abstime)
       /* If *tidp == tid, wait until thread terminates or the wait times out.
          The kernel up to version 3.16.3 does not use the private futex
          operations for futex wake-up when the clone terminates.  */
-      if (lll_futex_timed_wait_cancel (tidp, tid, &rt, LLL_SHARED)
-	  == -ETIMEDOUT)
-        return ETIMEDOUT;
+      ret = futex_timed_wait_cancel64 (tidp, tid, &rt, LLL_SHARED);
+      if (ret == -ETIMEDOUT || ret == -EOVERFLOW)
+        return -ret;
     }
 
   return 0;
@@ -80,8 +83,8 @@  clockwait_tid (pid_t *tidp, clockid_t clockid, const struct timespec *abstime)
 
 int
 __pthread_clockjoin_ex (pthread_t threadid, void **thread_return,
-			clockid_t clockid,
-			const struct timespec *abstime, bool block)
+                        clockid_t clockid,
+                        const struct __timespec64 *abstime, bool block)
 {
   struct pthread *pd = (struct pthread *) threadid;
 
diff --git a/nptl/pthread_timedjoin.c b/nptl/pthread_timedjoin.c
index dd7038dcf7..6164ae7060 100644
--- a/nptl/pthread_timedjoin.c
+++ b/nptl/pthread_timedjoin.c
@@ -16,13 +16,27 @@ 
    License along with the GNU C Library; if not, see
    <https://www.gnu.org/licenses/>.  */
 
+#include <time.h>
 #include "pthreadP.h"
 
 int
-__pthread_timedjoin_np (pthread_t threadid, void **thread_return,
-			const struct timespec *abstime)
+__pthread_timedjoin_np64 (pthread_t threadid, void **thread_return,
+                          const struct __timespec64 *abstime)
 {
   return __pthread_clockjoin_ex (threadid, thread_return,
                                  CLOCK_REALTIME, abstime, true);
 }
+
+#if __TIMESIZE != 64
+libc_hidden_def (__pthread_timedjoin_np64)
+
+int
+__pthread_timedjoin_np (pthread_t threadid, void **thread_return,
+                        const struct timespec *abstime)
+{
+  struct __timespec64 ts64 = valid_timespec_to_timespec64 (*abstime);
+
+  return __pthread_timedjoin_np64 (threadid, thread_return, &ts64);
+}
+#endif
 weak_alias (__pthread_timedjoin_np, pthread_timedjoin_np)
diff --git a/sysdeps/nptl/futex-internal.h b/sysdeps/nptl/futex-internal.h
index d622122ddc..1d038e4949 100644
--- a/sysdeps/nptl/futex-internal.h
+++ b/sysdeps/nptl/futex-internal.h
@@ -467,4 +467,57 @@  futex_unlock_pi (unsigned int *futex_word, int private)
     }
 }
 
+#ifndef __NR_futex_time64
+# define __NR_futex_time64 __NR_futex
+#endif
+
+static __always_inline int
+futex_timed_wait_cancel64 (pid_t *tidp,  pid_t tid,
+                           const struct __timespec64 *timeout, int private)
+{
+  int err = INTERNAL_SYSCALL_CANCEL (futex_time64, tidp,
+                                     __lll_private_flag
+                                     (FUTEX_WAIT, private), tid, timeout);
+#ifndef __ASSUME_TIME64_SYSCALLS
+  if (err == -ENOSYS)
+    {
+      struct timespec ts32;
+      if (in_time_t_range (timeout->tv_sec))
+        {
+          ts32 = valid_timespec64_to_timespec (*timeout);
+
+          err = INTERNAL_SYSCALL_CANCEL (futex, tidp,
+                                         __lll_private_flag (FUTEX_WAIT,
+                                                             private),
+                                         tid, &ts32);
+        }
+      else
+        err = -EOVERFLOW;
+    }
+#endif
+  switch (err)
+    {
+    case 0:
+    case -EAGAIN:
+    case -EINTR:
+    case -ETIMEDOUT:
+    case -EDEADLK:
+    case -ENOSYS:
+    case -EOVERFLOW:  /* Passed absolute timeout uses 64 bit time_t type, but
+                         underlying kernel does not support 64 bit time_t futex
+                         syscalls.  */
+    case -EPERM:  /*  The caller is not allowed to attach itself to the futex.
+		      Used to check if PI futexes are supported by the
+		      kernel.  */
+      return -err;
+
+    case -EINVAL: /* Either due to wrong alignment or due to the timeout not
+		     being normalized.  Must have been caused by a glibc or
+		     application bug.  */
+    case -EFAULT: /* Must have been caused by a glibc or application bug.  */
+    /* No other errors are documented at this time.  */
+    default:
+      futex_fatal_error ();
+    }
+}
 #endif  /* futex-internal.h */