* [PATCH v5] y2038: nptl: Convert pthread_{clock|timed}join_np to support 64 bit time
@ 2020-08-12 10:22 Lukasz Majewski
2020-08-12 21:54 ` Alistair Francis via Libc-alpha
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Lukasz Majewski @ 2020-08-12 10:22 UTC (permalink / raw)
To: Joseph Myers, Paul Eggert, Adhemerval Zanella
Cc: Florian Weimer, GNU C Library, Andreas Schwab, Stepan Golosunov,
Alistair Francis
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(-)
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v5] y2038: nptl: Convert pthread_{clock|timed}join_np to support 64 bit time
2020-08-12 10:22 [PATCH v5] y2038: nptl: Convert pthread_{clock|timed}join_np to support 64 bit time Lukasz Majewski
@ 2020-08-12 21:54 ` Alistair Francis via Libc-alpha
2020-08-12 23:11 ` Adhemerval Zanella via Libc-alpha
2020-08-15 18:11 ` [PATCH] nptl: Handle NULL abstime [BZ #26394] H.J. Lu via Libc-alpha
2 siblings, 0 replies; 10+ messages in thread
From: Alistair Francis via Libc-alpha @ 2020-08-12 21:54 UTC (permalink / raw)
To: Lukasz Majewski
Cc: Florian Weimer, GNU C Library, Andreas Schwab, Stepan Golosunov,
Alistair Francis, Joseph Myers
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
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v5] y2038: nptl: Convert pthread_{clock|timed}join_np to support 64 bit time
2020-08-12 10:22 [PATCH v5] y2038: nptl: Convert pthread_{clock|timed}join_np to support 64 bit time Lukasz Majewski
2020-08-12 21:54 ` Alistair Francis via Libc-alpha
@ 2020-08-12 23:11 ` Adhemerval Zanella via Libc-alpha
2020-08-15 18:11 ` [PATCH] nptl: Handle NULL abstime [BZ #26394] H.J. Lu via Libc-alpha
2 siblings, 0 replies; 10+ messages in thread
From: Adhemerval Zanella via Libc-alpha @ 2020-08-12 23:11 UTC (permalink / raw)
To: Lukasz Majewski, Joseph Myers, Paul Eggert
Cc: Florian Weimer, GNU C Library, Andreas Schwab, Stepan Golosunov,
Alistair Francis
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] nptl: Handle NULL abstime [BZ #26394]
2020-08-12 10:22 [PATCH v5] y2038: nptl: Convert pthread_{clock|timed}join_np to support 64 bit time Lukasz Majewski
2020-08-12 21:54 ` Alistair Francis via Libc-alpha
2020-08-12 23:11 ` Adhemerval Zanella via Libc-alpha
@ 2020-08-15 18:11 ` H.J. Lu via Libc-alpha
2020-08-17 10:22 ` Andreas Schwab
2 siblings, 1 reply; 10+ messages in thread
From: H.J. Lu via Libc-alpha @ 2020-08-15 18:11 UTC (permalink / raw)
To: Lukasz Majewski
Cc: Florian Weimer, GNU C Library, Andreas Schwab, Stepan Golosunov,
Alistair Francis, Joseph Myers
[-- Attachment #1: Type: text/plain, Size: 3787 bytes --]
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.
--
H.J.
[-- Attachment #2: 0001-nptl-Handle-NULL-abstime-BZ-26394.patch --]
[-- Type: application/x-patch, Size: 2111 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] nptl: Handle NULL abstime [BZ #26394]
2020-08-15 18:11 ` [PATCH] nptl: Handle NULL abstime [BZ #26394] H.J. Lu via Libc-alpha
@ 2020-08-17 10:22 ` Andreas Schwab
2020-08-17 12:04 ` H.J. Lu via Libc-alpha
2020-08-17 13:30 ` Lukasz Majewski
0 siblings, 2 replies; 10+ messages in thread
From: Andreas Schwab @ 2020-08-17 10:22 UTC (permalink / raw)
To: H.J. Lu
Cc: Florian Weimer, GNU C Library, Stepan Golosunov, Alistair Francis,
Joseph Myers
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.
--
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] nptl: Handle NULL abstime [BZ #26394]
2020-08-17 10:22 ` Andreas Schwab
@ 2020-08-17 12:04 ` H.J. Lu via Libc-alpha
2020-08-17 13:30 ` Lukasz Majewski
1 sibling, 0 replies; 10+ messages in thread
From: H.J. Lu via Libc-alpha @ 2020-08-17 12:04 UTC (permalink / raw)
To: Andreas Schwab
Cc: Florian Weimer, GNU C Library, Stepan Golosunov, Alistair Francis,
Joseph Myers
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)
--
2.26.2
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] nptl: Handle NULL abstime [BZ #26394]
2020-08-17 10:22 ` Andreas Schwab
2020-08-17 12:04 ` H.J. Lu via Libc-alpha
@ 2020-08-17 13:30 ` Lukasz Majewski
2020-08-17 13:52 ` H.J. Lu via Libc-alpha
1 sibling, 1 reply; 10+ messages in thread
From: Lukasz Majewski @ 2020-08-17 13:30 UTC (permalink / raw)
To: Andreas Schwab, H.J. Lu, Paul Eggert
Cc: Florian Weimer, GNU C Library, Stepan Golosunov, Alistair Francis,
Joseph Myers
[-- Attachment #1: Type: text/plain, Size: 3578 bytes --]
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
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] nptl: Handle NULL abstime [BZ #26394]
2020-08-17 13:30 ` Lukasz Majewski
@ 2020-08-17 13:52 ` H.J. Lu via Libc-alpha
2020-08-17 13:58 ` Adhemerval Zanella via Libc-alpha
0 siblings, 1 reply; 10+ messages in thread
From: H.J. Lu via Libc-alpha @ 2020-08-17 13:52 UTC (permalink / raw)
To: Lukasz Majewski
Cc: Florian Weimer, GNU C Library, Andreas Schwab, Stepan Golosunov,
Alistair Francis, Joseph Myers
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;
}
--
H.J.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] nptl: Handle NULL abstime [BZ #26394]
2020-08-17 13:52 ` H.J. Lu via Libc-alpha
@ 2020-08-17 13:58 ` Adhemerval Zanella via Libc-alpha
2020-08-17 14:24 ` Lukasz Majewski
0 siblings, 1 reply; 10+ messages in thread
From: Adhemerval Zanella via Libc-alpha @ 2020-08-17 13:58 UTC (permalink / raw)
To: H.J. Lu, Lukasz Majewski
Cc: Florian Weimer, GNU C Library, Andreas Schwab, Stepan Golosunov,
Alistair Francis, Joseph Myers
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?
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] nptl: Handle NULL abstime [BZ #26394]
2020-08-17 13:58 ` Adhemerval Zanella via Libc-alpha
@ 2020-08-17 14:24 ` Lukasz Majewski
0 siblings, 0 replies; 10+ messages in thread
From: Lukasz Majewski @ 2020-08-17 14:24 UTC (permalink / raw)
To: Adhemerval Zanella
Cc: Florian Weimer, GNU C Library, Andreas Schwab, Stepan Golosunov,
Alistair Francis, Joseph Myers
[-- Attachment #1: Type: text/plain, Size: 2587 bytes --]
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
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-08-17 14:24 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-12 10:22 [PATCH v5] y2038: nptl: Convert pthread_{clock|timed}join_np to support 64 bit time Lukasz Majewski
2020-08-12 21:54 ` Alistair Francis via Libc-alpha
2020-08-12 23:11 ` Adhemerval Zanella via Libc-alpha
2020-08-15 18:11 ` [PATCH] nptl: Handle NULL abstime [BZ #26394] H.J. Lu via Libc-alpha
2020-08-17 10:22 ` Andreas Schwab
2020-08-17 12:04 ` H.J. Lu via Libc-alpha
2020-08-17 13:30 ` Lukasz Majewski
2020-08-17 13:52 ` H.J. Lu via Libc-alpha
2020-08-17 13:58 ` Adhemerval Zanella via Libc-alpha
2020-08-17 14:24 ` Lukasz Majewski
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).