[PATCH v3] y2038: nptl: Convert pthread_cond_{clock|timed}wait to support 64 bit time
Adhemerval Zanella
adhemerval.zanella@linaro.org
Tue Sep 1 18:40:32 GMT 2020
On 30/08/2020 19:44, Lukasz Majewski wrote:
> The pthread_cond_clockwait and pthread_cond_timedwait have been converted
> to support 64 bit time.
>
> This change introduces new futex_abstimed_wait_cancelable64 function in
> ./sysdeps/nptl/futex-helpers.c, which uses futex_time64 where possible
> and tries to replace low-level preprocessor macros from
> lowlevellock-futex.h
> The pthread_cond_{clock|timed}wait only accepts absolute time. Moreover,
> there is no need to check for NULL passed as *abstime pointer as
> __pthread_cond_wait_common() always passes non-NULL struct __timespec64
> pointer to futex_abstimed_wait_cancellable64().
>
> For systems with __TIMESIZE != 64 && __WORDSIZE == 32:
> - Conversions between 64 bit time to 32 bit are necessary
> - Redirection to __pthread_cond_{clock|timed}wait64 will provide support
> for 64 bit time
>
> The futex_abstimed_wait_cancelable64 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).
>
> In fact - new function - namely __futex_abstimed_wait_cancellable32 is
> used to reduce number of needed registers (as some in-register values are
> stored on the stack when function call is made).
>
> 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_cond_{clock|timed}wait64 and
> __pthread_cond_{clock|timed}wait.
LGTM with attribute_hidden fixes below.
Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>
>
> ---
> Changes for v3:
>
> - Change the file name from futex-helpers.c to futex-internal.c
> - Use INTERNAL_SYSCALL_CANCEL() instead of INTERNAL_SYSCALL_CALL and remove not
> necessary __pthread_{disable|enable}_asynccancel ()
> - Remove not needed blank lines
> - Do not change code formatting not related to this patch
> - Add attribute_hidden instead of libpthread_hidden_def
> - Rewrite the __futex_abstimed_wait_cancelable64 to have a separate function
> call for 32 bit time syscall fallback
> - Remove sysdeps/unix/sysv/linux/m68k/futex-helpers.c as it is not needed
> anymore
> ---
> nptl/pthreadP.h | 11 ++++
> nptl/pthread_cond_wait.c | 43 +++++++++++++---
> sysdeps/nptl/Makefile | 2 +-
> sysdeps/nptl/futex-internal.c | 96 +++++++++++++++++++++++++++++++++++
> sysdeps/nptl/futex-internal.h | 11 ++++
> 5 files changed, 154 insertions(+), 9 deletions(-)
> create mode 100644 sysdeps/nptl/futex-internal.c
>
> diff --git a/nptl/pthreadP.h b/nptl/pthreadP.h
> index 99713c8447..9bb44c8535 100644
> --- a/nptl/pthreadP.h
> +++ b/nptl/pthreadP.h
> @@ -462,6 +462,8 @@ 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
> +# define __pthread_cond_timedwait64 __pthread_cond_timedwait
> +# define __pthread_cond_clockwait64 __pthread_cond_clockwait
> #else
> extern int __pthread_clockjoin_np64 (pthread_t threadid, void **thread_return,
> clockid_t clockid,
> @@ -470,6 +472,15 @@ 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)
> +extern int __pthread_cond_timedwait64 (pthread_cond_t *cond,
> + pthread_mutex_t *mutex,
> + const struct __timespec64 *abstime);
> +libpthread_hidden_proto (__pthread_cond_timedwait64)
> +extern int __pthread_cond_clockwait64 (pthread_cond_t *cond,
> + pthread_mutex_t *mutex,
> + clockid_t clockid,
> + const struct __timespec64 *abstime);
> +libpthread_hidden_proto (__pthread_cond_clockwait64)
> #endif
>
> extern int __pthread_cond_timedwait (pthread_cond_t *cond,
Ok.
> diff --git a/nptl/pthread_cond_wait.c b/nptl/pthread_cond_wait.c
> index 85ddbc1011..7d158d553f 100644
> --- a/nptl/pthread_cond_wait.c
> +++ b/nptl/pthread_cond_wait.c
> @@ -378,8 +378,7 @@ __condvar_cleanup_waiting (void *arg)
> */
> static __always_inline int
> __pthread_cond_wait_common (pthread_cond_t *cond, pthread_mutex_t *mutex,
> - clockid_t clockid,
> - const struct timespec *abstime)
> + clockid_t clockid, const struct __timespec64 *abstime)
> {
> const int maxspin = 0;
> int err;
> @@ -517,7 +516,7 @@ __pthread_cond_wait_common (pthread_cond_t *cond, pthread_mutex_t *mutex,
> err = ETIMEDOUT;
> else
> {
> - err = futex_abstimed_wait_cancelable
> + err = __futex_abstimed_wait_cancelable64
> (cond->__data.__g_signals + g, 0, clockid, abstime,
> private);
> }
Ok.
> @@ -640,8 +639,8 @@ __pthread_cond_wait (pthread_cond_t *cond, pthread_mutex_t *mutex)
>
> /* See __pthread_cond_wait_common. */
> int
> -__pthread_cond_timedwait (pthread_cond_t *cond, pthread_mutex_t *mutex,
> - const struct timespec *abstime)
> +__pthread_cond_timedwait64 (pthread_cond_t *cond, pthread_mutex_t *mutex,
> + const struct __timespec64 *abstime)
> {
> /* Check parameter validity. This should also tell the compiler that
> it can assume that abstime is not NULL. */
Ok.
> @@ -655,6 +654,20 @@ __pthread_cond_timedwait (pthread_cond_t *cond, pthread_mutex_t *mutex,
> ? CLOCK_MONOTONIC : CLOCK_REALTIME;
> return __pthread_cond_wait_common (cond, mutex, clockid, abstime);
> }
> +
> +#if __TIMESIZE != 64
> +libpthread_hidden_def (__pthread_cond_timedwait64)
> +
> +int
> +__pthread_cond_timedwait (pthread_cond_t *cond, pthread_mutex_t *mutex,
> + const struct timespec *abstime)
> +{
> + struct __timespec64 ts64 = valid_timespec_to_timespec64 (*abstime);
> +
> + return __pthread_cond_timedwait64 (cond, mutex, &ts64);
> +}
> +#endif
> +
> versioned_symbol (libpthread, __pthread_cond_wait, pthread_cond_wait,
> GLIBC_2_3_2);
> versioned_symbol (libpthread, __pthread_cond_timedwait, pthread_cond_timedwait,
Ok.
> @@ -662,9 +675,9 @@ versioned_symbol (libpthread, __pthread_cond_timedwait, pthread_cond_timedwait,
>
> /* See __pthread_cond_wait_common. */
> int
> -__pthread_cond_clockwait (pthread_cond_t *cond, pthread_mutex_t *mutex,
> - clockid_t clockid,
> - const struct timespec *abstime)
> +__pthread_cond_clockwait64 (pthread_cond_t *cond, pthread_mutex_t *mutex,
> + clockid_t clockid,
> + const struct __timespec64 *abstime)
> {
> /* Check parameter validity. This should also tell the compiler that
> it can assume that abstime is not NULL. */
Ok.
> @@ -676,4 +689,18 @@ __pthread_cond_clockwait (pthread_cond_t *cond, pthread_mutex_t *mutex,
>
> return __pthread_cond_wait_common (cond, mutex, clockid, abstime);
> }
> +
> +#if __TIMESIZE != 64
> +libpthread_hidden_def (__pthread_cond_clockwait64)
> +
> +int
> +__pthread_cond_clockwait (pthread_cond_t *cond, pthread_mutex_t *mutex,
> + clockid_t clockid,
> + const struct timespec *abstime)
> +{
> + struct __timespec64 ts64 = valid_timespec_to_timespec64 (*abstime);
> +
> + return __pthread_cond_clockwait64 (cond, mutex, clockid, &ts64);
> +}
> +#endif
Ok.
> weak_alias (__pthread_cond_clockwait, pthread_cond_clockwait);
> diff --git a/sysdeps/nptl/Makefile b/sysdeps/nptl/Makefile
> index 0631a870c8..a65be3b7ea 100644
> --- a/sysdeps/nptl/Makefile
> +++ b/sysdeps/nptl/Makefile
> @@ -17,7 +17,7 @@
> # <https://www.gnu.org/licenses/>.
>
> ifeq ($(subdir),nptl)
> -libpthread-sysdep_routines += errno-loc
> +libpthread-sysdep_routines += errno-loc futex-internal
> endif
>
> ifeq ($(subdir),rt)
Ok.
> diff --git a/sysdeps/nptl/futex-internal.c b/sysdeps/nptl/futex-internal.c
> new file mode 100644
> index 0000000000..692820b0b0
> --- /dev/null
> +++ b/sysdeps/nptl/futex-internal.c
> @@ -0,0 +1,96 @@
> +/* futex helper functions for glibc-internal use.
> + Copyright (C) 2020 Free Software Foundation, Inc.
> + This file is part of the GNU C Library.
> +
> + The GNU C Library is free software; you can redistribute it and/or
> + modify it under the terms of the GNU Lesser General Public
> + License as published by the Free Software Foundation; either
> + version 2.1 of the License, or (at your option) any later version.
> +
> + The GNU C Library is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + Lesser General Public License for more details.
> +
> + You should have received a copy of the GNU Lesser General Public
> + License along with the GNU C Library; if not, see
> + <https://www.gnu.org/licenses/>. */
> +
> +#include <errno.h>
> +#include <sysdep.h>
> +#include <time.h>
> +#include <futex-internal.h>
> +#include <kernel-features.h>
> +
> +#ifndef __ASSUME_TIME64_SYSCALLS
> +static int
> +__futex_abstimed_wait_cancellable32 (unsigned int* futex_word,
> + unsigned int expected, clockid_t clockid,
> + const struct __timespec64* abstime,
> + int private)
> +{
> + if (! 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);
> +
> + struct timespec ts32 = valid_timespec64_to_timespec (*abstime);
> + return INTERNAL_SYSCALL_CANCEL (futex, futex_word, op, expected,
> + &ts32, NULL /* Unused. */,
> + FUTEX_BITSET_MATCH_ANY);
> +}
> +#endif
> +
> +int
> +attribute_hidden
There is no need to add this, the function prototype should handle it.
> +__futex_abstimed_wait_cancelable64 (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;
> +
> + clockbit = (clockid == CLOCK_REALTIME) ? FUTEX_CLOCK_REALTIME : 0;
> + int op = __lll_private_flag (FUTEX_WAIT_BITSET | clockbit, private);
> +
> + err = INTERNAL_SYSCALL_CANCEL (futex_time64, futex_word, op, expected,
> + abstime, NULL /* Unused. */,
> + FUTEX_BITSET_MATCH_ANY);
> +#ifndef __ASSUME_TIME64_SYSCALLS
> +if (err == -ENOSYS)
> + err = __futex_abstimed_wait_cancellable32 (futex_word, expected,
> + clockid, abstime, private);
> +#endif
> +
> + switch (err)
> + {
> + case 0:
> + case -EAGAIN:
> + case -EINTR:
> + case -ETIMEDOUT:
> + case -EOVERFLOW: /* Passed absolute timeout uses 64 bit time_t type, but
> + underlying kernel does not support 64 bit time_t futex
> + syscalls. */
> + return -err;
> +
> + case -EFAULT: /* Must have been caused by a glibc or application bug. */
> + 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 -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 159aae82dc..3c4a3edb55 100644
> --- a/sysdeps/nptl/futex-internal.h
> +++ b/sysdeps/nptl/futex-internal.h
> @@ -519,4 +519,15 @@ futex_timed_wait_cancel64 (pid_t *tidp, pid_t tid,
> futex_fatal_error ();
> }
> }
> +
> +/* The futex_abstimed_wait_cancelable64 has been moved to a separate file
> + to avoid problems with exhausting available registers on some architectures
> + - e.g. on m68k architecture. */
> +int
> +attribute_hidden
Move the attribute_hidden to the end of the function prototype (commit 30e5069c7d4
fix a issue where the __MALLOC_DEPRECATED is empty on some configurations, it
should not happen here since attribute_hidden will be always defined with supported
configurations though).
> +__futex_abstimed_wait_cancelable64 (unsigned int* futex_word,
> + unsigned int expected, clockid_t clockid,
> + const struct __timespec64* abstime,
> + int private);
> +
> #endif /* futex-internal.h */
>
More information about the Libc-alpha
mailing list