| Message ID | 20191024234106.27081-1-alistair.francis@wdc.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] sysdeps/nanosleep: Use clock_nanosleep_time64 if avaliable | expand |
On Thu, 24 Oct 2019, Alistair Francis wrote: > +# ifdef __NR_clock_nanosleep_time64 > + long int ret_64; > + > + ret_64 = INTERNAL_SYSCALL_CANCEL (clock_nanosleep_time64, err, CLOCK_REALTIME, > + 0, time_point, remaining); > + > + if (ret_64 == 0 || errno != ENOSYS) > + ret = ret_64; > +# endif /* __NR_clock_nanosleep_time64 */ > + if (ret < 0) > + { > + struct timespec tp32, tr32; > + > + if (! in_time_t_range (time_point->tv_sec)) > + { > + __set_errno (EOVERFLOW); > + return -1; > + } > + > + tp32 = valid_timespec64_to_timespec (*time_point); > + ret = INTERNAL_SYSCALL_CANCEL (nanosleep, err, &tp32, &tr32); This looks like it would call the nanosleep syscall if clock_nanosleep_time64 failed for some non-ENOSYS reason. That doesn't seem right. > + if ((ret == 0 || errno != ENOSYS) && remaining) > + *remaining = valid_timespec_to_timespec64 (tr32); And if nanosleep fails for a non-EINTR reason, it won't have filled in tr32, so this could be copying uninitialized data. > +#if __TIMESIZE != 64 > +int > +thrd_sleep (const struct timespec* time_point, struct timespec* remaining) > +{ > + int ret; > + struct __timespec64 tp64, tr64; > + > + tp64 = valid_timespec_to_timespec64 (*time_point); > + ret = __thrd_sleep_time64 (&tp64, &tr64); > + > + if (ret == 0 || errno != ENOSYS) > + { > + if (! in_time_t_range (tr64.tv_sec)) > + { > + __set_errno (EOVERFLOW); > + return -1; This could not only be copying uninitialized data for such a failure (e.g. a failure for an invalid nanoseconds value), it could actually produce a spurious EOVERFLOW error if that uninitialized data is out of range. As far as I know the remaining amount can't exceed the amount passed in, so when it's copied it shouldn't actually be necessary to check for overflow. I think similar issues apply for other functions in the patch. Do we have test coverage in the testsuite for invalid nanoseconds arguments to all these functions, making sure they produce proper EINVAL errors? That might not cover many of these bugs (it wouldn't detect simply reading uninitialized memory, or calling more syscalls than necessary, and detecting the spurious EOVERFLOW would depend on what values were in that memory). But it would seem a good idea to make sure we do have such test coverage for functions using time arguments.
On 24/10/2019 20:41, Alistair Francis wrote: > The nanosleep syscall is not supported on newer 32-bit platforms (such > as RV32). To fix this issue let's use clock_nanosleep_time64 if it is > avaliable. > > Let's use CLOCK_REALTIME when calling clock_nanosleep_time64 as the > Linux specification says: > "POSIX.1 specifies that nanosleep() should measure time against the > CLOCK_REALTIME clock. However, Linux measures the time using the > CLOCK_MONOTONIC clock. This probably does not matter, since the POSIX.1 > specification for clock_settime(2) says that discontinuous changes in > CLOCK_REALTIME should not affect nanosleep()" > --- > This was patch was runtime tested with RV32 and RV64 > It was build tested using the ./scripts/build-many-glibcs.py script. > > I am currently running: > $ make ; make install ; make check > tests on native ARM (32-bit) with the following three confiugrations: > - 4.19 Kernel and 4.19 Headers > - 5.2 Kernel and 4.19 Headers > - 5.2 Kernel and 5.2 Headers > I should have the results tomorrow. > > v2: > - Explicitly include `#include <kernel-features.h>` > > include/time.h | 20 ++++++ > nptl/thrd_sleep.c | 70 +++++++++++++++++-- > sysdeps/unix/sysv/linux/clock_nanosleep.c | 73 ++++++++++++++++++-- > sysdeps/unix/sysv/linux/nanosleep.c | 65 ++++++++++++++++- > sysdeps/unix/sysv/linux/nanosleep_nocancel.c | 65 ++++++++++++++++- > 5 files changed, 280 insertions(+), 13 deletions(-) > > diff --git a/include/time.h b/include/time.h > index d93b16a7810..0e703f87cef 100644 > --- a/include/time.h > +++ b/include/time.h > @@ -174,6 +174,26 @@ libc_hidden_proto (__difftime64) > > extern double __difftime (time_t time1, time_t time0); > > +#if __TIMESIZE == 64 > +# define __thrd_sleep_time64 thrd_sleep > +# define __clock_nanosleep_time64 __clock_nanosleep > +# define __nanosleep_time64 __nanosleep > +# define __nanosleep_nocancel_time64 __nanosleep_nocancel > +#else > +extern int __thrd_sleep_time64 (const struct __timespec64* time_point, > + struct __timespec64* remaining); > +libc_hidden_proto (__thrd_sleep_time64) > +extern int __clock_nanosleep_time64 (clockid_t clock_id, > + int flags, const struct __timespec64 *req, > + struct __timespec64 *rem); > +libc_hidden_proto (__clock_nanosleep_time64) > +extern int __nanosleep_time64 (const struct __timespec64 *requested_time, > + struct __timespec64 *remaining); > +libc_hidden_proto (__nanosleep_time64) > +extern int __nanosleep_nocancel_time64 (const struct __timespec64 *requested_time, > + struct __timespec64 *remaining); > +libc_hidden_proto (__nanosleep_nocancel_time64) > +#endif > > /* Use in the clock_* functions. Size of the field representing the > actual clock ID. */ > diff --git a/nptl/thrd_sleep.c b/nptl/thrd_sleep.c > index 2e185dd748e..2a6bb1f9e3f 100644 > --- a/nptl/thrd_sleep.c > +++ b/nptl/thrd_sleep.c > @@ -17,23 +17,85 @@ > <https://www.gnu.org/licenses/>. */ > > #include <time.h> > +#include <kernel-features.h> > #include <sysdep-cancel.h> > > #include "thrd_priv.h" > > int > -thrd_sleep (const struct timespec* time_point, struct timespec* remaining) > +__thrd_sleep_time64 (const struct __timespec64* time_point, struct __timespec64* remaining) > { > INTERNAL_SYSCALL_DECL (err); > - int ret = INTERNAL_SYSCALL_CANCEL (nanosleep, err, time_point, remaining); > + int ret = -1; > + > +#ifdef __ASSUME_TIME64_SYSCALLS > +# ifndef __NR_clock_nanosleep_time64 > +# define __NR_clock_nanosleep_time64 __NR_clock_nanosleep > +# endif > + ret = INTERNAL_SYSCALL_CANCEL (clock_nanosleep_time64, err, CLOCK_REALTIME, > + 0, time_point, remaining); > +#else > +# ifdef __NR_clock_nanosleep_time64 > + long int ret_64; > + > + ret_64 = INTERNAL_SYSCALL_CANCEL (clock_nanosleep_time64, err, CLOCK_REALTIME, > + 0, time_point, remaining); > + > + if (ret_64 == 0 || errno != ENOSYS) > + ret = ret_64; > +# endif /* __NR_clock_nanosleep_time64 */ > + if (ret < 0) > + { > + struct timespec tp32, tr32; > + > + if (! in_time_t_range (time_point->tv_sec)) > + { > + __set_errno (EOVERFLOW); > + return -1; > + } > + > + tp32 = valid_timespec64_to_timespec (*time_point); > + ret = INTERNAL_SYSCALL_CANCEL (nanosleep, err, &tp32, &tr32); > + > + if ((ret == 0 || errno != ENOSYS) && remaining) > + *remaining = valid_timespec_to_timespec64 (tr32); > + } > +#endif /* __ASSUME_TIME64_SYSCALLS */ > + > if (INTERNAL_SYSCALL_ERROR_P (ret, err)) > { > /* C11 states thrd_sleep function returns -1 if it has been interrupted > - by a signal, or a negative value if it fails. */ > + by a signal, or a negative value if it fails. */ > ret = INTERNAL_SYSCALL_ERRNO (ret, err); > if (ret == EINTR) > - return -1; > + return -1; > return -2; > } > return 0; > } Wouldn't be simple to just call __clock_nanosleep_time64 instead of duplicate the syscall handling and the required error code for c11 thread? Something like: int __thrd_sleep_time64 (const struct __timespec64* time_point, struct __timespec64* remaining) { int ret = __clock_nanosleep (CLOCK_REALTIME, 0, time_point, remaining); /* C11 states thrd_sleep function returns -1 if it has been interrupted by a signal, or a negative value if it fails. */ switch (ret) { case 0: return 0; case EINTR: return -1; default: return -2; } } > + > +#if __TIMESIZE != 64 > +int > +thrd_sleep (const struct timespec* time_point, struct timespec* remaining) > +{ > + int ret; > + struct __timespec64 tp64, tr64; > + > + tp64 = valid_timespec_to_timespec64 (*time_point); > + ret = __thrd_sleep_time64 (&tp64, &tr64); > + > + if (ret == 0 || errno != ENOSYS) > + { > + if (! in_time_t_range (tr64.tv_sec)) > + { > + __set_errno (EOVERFLOW); > + return -1; > + } > + > + if (remaining) > + *remaining = valid_timespec64_to_timespec (tr64); > + } > + > + return ret; > +} > +#endif > diff --git a/sysdeps/unix/sysv/linux/clock_nanosleep.c b/sysdeps/unix/sysv/linux/clock_nanosleep.c > index 1f240b8720a..d257ea57fb0 100644 > --- a/sysdeps/unix/sysv/linux/clock_nanosleep.c > +++ b/sysdeps/unix/sysv/linux/clock_nanosleep.c > @@ -16,6 +16,7 @@ > <https://www.gnu.org/licenses/>. */ > > #include <time.h> > +#include <kernel-features.h> > #include <errno.h> > > #include <sysdep-cancel.h> > @@ -26,9 +27,11 @@ > /* We can simply use the syscall. The CPU clocks are not supported > with this function. */ > int > -__clock_nanosleep (clockid_t clock_id, int flags, const struct timespec *req, > - struct timespec *rem) > +__clock_nanosleep_time64 (clockid_t clock_id, int flags, const struct __timespec64 *req, > + struct __timespec64 *rem) > { > + int r = -1; > + > if (clock_id == CLOCK_THREAD_CPUTIME_ID) > return EINVAL; > if (clock_id == CLOCK_PROCESS_CPUTIME_ID) > @@ -37,12 +40,72 @@ __clock_nanosleep (clockid_t clock_id, int flags, const struct timespec *req, > /* If the call is interrupted by a signal handler or encounters an error, > it returns a positive value similar to errno. */ > INTERNAL_SYSCALL_DECL (err); > - int r = INTERNAL_SYSCALL_CANCEL (clock_nanosleep, err, clock_id, flags, > - req, rem); > + > +#ifdef __ASSUME_TIME64_SYSCALLS > +# ifndef __NR_clock_nanosleep_time64 > +# define __NR_clock_nanosleep_time64 __NR_clock_nanosleep > +# endif > + r = INTERNAL_SYSCALL_CANCEL (clock_nanosleep_time64, err, clock_id, > + flags, req, rem); > +#else > +# ifdef __NR_clock_nanosleep_time64 > + long int ret_64; > + > + ret_64 = INTERNAL_SYSCALL_CANCEL (clock_nanosleep_time64, err, clock_id, > + flags, req, rem); > + > + if (ret_64 == 0 || errno != ENOSYS) > + r = ret_64; > +# endif /* __NR_clock_nanosleep_time64 */ > + if (r < 0) > + { > + struct timespec ts32, tr32; > + > + if (! in_time_t_range (req->tv_sec)) > + { > + __set_errno (EOVERFLOW); > + return -1; > + } > + > + ts32 = valid_timespec64_to_timespec (*req); > + r = INTERNAL_SYSCALL_CANCEL (clock_nanosleep, err, &ts32, &tr32); > + > + if ((r == 0 || errno != ENOSYS) && rem) > + *rem = valid_timespec_to_timespec64 (tr32); > + } > +#endif /* __ASSUME_TIME64_SYSCALLS */ > + > return (INTERNAL_SYSCALL_ERROR_P (r, err) > - ? INTERNAL_SYSCALL_ERRNO (r, err) : 0); > + ? INTERNAL_SYSCALL_ERRNO (r, err) : 0); > } > > +#if __TIMESIZE != 64 > +int > +__clock_nanosleep (clockid_t clock_id, int flags, const struct timespec *req, > + struct timespec *rem) > +{ > + int r; > + struct __timespec64 treq64, trem64; > + > + treq64 = valid_timespec_to_timespec64 (*req); > + r = __clock_nanosleep_time64 (clock_id, flags, &treq64, &trem64); > + > + if (r == 0 || errno != ENOSYS) > + { > + if (! in_time_t_range (trem64.tv_sec)) > + { > + __set_errno (EOVERFLOW); > + return -1; > + } > + > + if (rem) > + *rem = valid_timespec64_to_timespec (trem64); > + } > + > + return r; > +} > +#endif > + > versioned_symbol (libc, __clock_nanosleep, clock_nanosleep, GLIBC_2_17); > /* clock_nanosleep moved to libc in version 2.17; > old binaries may expect the symbol version it had in librt. */ > diff --git a/sysdeps/unix/sysv/linux/nanosleep.c b/sysdeps/unix/sysv/linux/nanosleep.c > index 6787909248f..10a6d57a1de 100644 > --- a/sysdeps/unix/sysv/linux/nanosleep.c > +++ b/sysdeps/unix/sysv/linux/nanosleep.c > @@ -17,15 +17,76 @@ > <https://www.gnu.org/licenses/>. */ > > #include <time.h> > +#include <kernel-features.h> > #include <sysdep-cancel.h> > #include <not-cancel.h> > > /* Pause execution for a number of nanoseconds. */ > int > +__nanosleep_time64 (const struct __timespec64 *requested_time, > + struct __timespec64 *remaining) > +{ > +#if defined(__ASSUME_TIME64_SYSCALLS) > +# ifndef __NR_clock_nanosleep_time64 > +# define __NR_clock_nanosleep_time64 __NR_clock_nanosleep > +# endif > + return SYSCALL_CANCEL (clock_nanosleep_time64, CLOCK_REALTIME, 0, > + requested_time, remaining); > +#else > +# ifdef __NR_clock_nanosleep_time64 > + long int ret_64; > + > + ret_64 = SYSCALL_CANCEL (clock_nanosleep_time64, CLOCK_REALTIME, 0, > + requested_time, remaining); > + > + if (ret_64 == 0 || errno != ENOSYS) > + return ret_64; > +# endif /* __NR_clock_nanosleep_time64 */ > + int ret; > + struct timespec ts32, tr32; > + > + if (! in_time_t_range (requested_time->tv_sec)) > + { > + __set_errno (EOVERFLOW); > + return -1; > + } > + > + ts32 = valid_timespec64_to_timespec (*requested_time); > + ret = SYSCALL_CANCEL (nanosleep, &ts32, &tr32); > + > + if ((ret == 0 || errno != ENOSYS) && remaining) > + *remaining = valid_timespec_to_timespec64 (tr32); > + > + return ret; > +#endif /* __ASSUME_TIME64_SYSCALLS */ > +} > + > +#if __TIMESIZE != 64 > +int > __nanosleep (const struct timespec *requested_time, > - struct timespec *remaining) > + struct timespec *remaining) > { > - return SYSCALL_CANCEL (nanosleep, requested_time, remaining); > + int r; > + struct __timespec64 treq64, trem64; > + > + treq64 = valid_timespec_to_timespec64 (*requested_time); > + r = __nanosleep_time64 (&treq64, &trem64); > + > + if (r == 0 || errno != ENOSYS) > + { > + if (! in_time_t_range (trem64.tv_sec)) > + { > + __set_errno (EOVERFLOW); > + return -1; > + } > + > + if (remaining) > + *remaining = valid_timespec64_to_timespec (trem64); > + } > + > + return r; > } > +#endif > + > hidden_def (__nanosleep) > weak_alias (__nanosleep, nanosleep) > diff --git a/sysdeps/unix/sysv/linux/nanosleep_nocancel.c b/sysdeps/unix/sysv/linux/nanosleep_nocancel.c > index d6442bf4f7f..1a6c6c0a48a 100644 > --- a/sysdeps/unix/sysv/linux/nanosleep_nocancel.c > +++ b/sysdeps/unix/sysv/linux/nanosleep_nocancel.c > @@ -17,13 +17,74 @@ > <https://www.gnu.org/licenses/>. */ > > #include <time.h> > +#include <kernel-features.h> > #include <sysdep-cancel.h> > #include <not-cancel.h> > > +int > +__nanosleep_nocancel_time64 (const struct __timespec64 *requested_time, > + struct __timespec64 *remaining) > +{ > +#ifdef __ASSUME_TIME64_SYSCALLS > +# ifndef __NR_clock_nanosleep_time64 > +# define __NR_clock_nanosleep_time64 __NR_clock_nanosleep > +# endif > + return INLINE_SYSCALL_CALL (clock_nanosleep_time64, CLOCK_REALTIME, 0, > + requested_time, remaining); > +#else > +# ifdef __NR_clock_nanosleep_time64 > + long int ret_64; > + > + ret_64 = INLINE_SYSCALL_CALL (clock_nanosleep_time64, CLOCK_REALTIME, 0, > + requested_time, remaining); > + > + if (ret_64 == 0 || errno != ENOSYS) > + return ret_64; > +# endif /* __NR_clock_nanosleep_time64 */ > + int ret; > + struct timespec ts32, tr32; > + > + if (! in_time_t_range (requested_time->tv_sec)) > + { > + __set_errno (EOVERFLOW); > + return -1; > + } > + > + ts32 = valid_timespec64_to_timespec (*requested_time); > + ret = INLINE_SYSCALL_CALL (nanosleep, &ts32, &tr32); > + > + if (ret == 0 || errno != ENOSYS) > + *remaining = valid_timespec_to_timespec64 (tr32); > + > + return ret; > +#endif /* __ASSUME_TIME64_SYSCALLS */ > +} > + > +#if __TIMESIZE != 64 > int > __nanosleep_nocancel (const struct timespec *requested_time, > - struct timespec *remaining) > + struct timespec *remaining) > { > - return INLINE_SYSCALL_CALL (nanosleep, requested_time, remaining); > + int ret; > + struct __timespec64 treq64, trem64; > + > + treq64 = valid_timespec_to_timespec64 (*requested_time); > + ret = __nanosleep_nocancel_time64 (&treq64, &trem64); > + > + if (ret == 0 || errno != ENOSYS) > + { > + if (! in_time_t_range (trem64.tv_sec)) > + { > + __set_errno (EOVERFLOW); > + return -1; > + } > + > + if (remaining) > + *remaining = valid_timespec64_to_timespec (trem64); > + } > + > + return ret; > } > +#endif > + > hidden_def (__nanosleep_nocancel) >
On Fri, Oct 25, 2019 at 11:44 PM Joseph Myers <joseph@codesourcery.com> wrote: > > On Thu, 24 Oct 2019, Alistair Francis wrote: > > > +# ifdef __NR_clock_nanosleep_time64 > > + long int ret_64; > > + > > + ret_64 = INTERNAL_SYSCALL_CANCEL (clock_nanosleep_time64, err, CLOCK_REALTIME, > > + 0, time_point, remaining); > > + > > + if (ret_64 == 0 || errno != ENOSYS) > > + ret = ret_64; > > +# endif /* __NR_clock_nanosleep_time64 */ > > + if (ret < 0) > > + { > > + struct timespec tp32, tr32; > > + > > + if (! in_time_t_range (time_point->tv_sec)) > > + { > > + __set_errno (EOVERFLOW); > > + return -1; > > + } > > + > > + tp32 = valid_timespec64_to_timespec (*time_point); > > + ret = INTERNAL_SYSCALL_CANCEL (nanosleep, err, &tp32, &tr32); > > This looks like it would call the nanosleep syscall if > clock_nanosleep_time64 failed for some non-ENOSYS reason. That doesn't > seem right. Yes, you are right. I have fixed this in clock_nanosleep and the conversion in thrd_sleep() pointed out by Adhemerval fixes it there. > > > + if ((ret == 0 || errno != ENOSYS) && remaining) > > + *remaining = valid_timespec_to_timespec64 (tr32); > > And if nanosleep fails for a non-EINTR reason, it won't have filled in > tr32, so this could be copying uninitialized data. Yes, fixed. > > > +#if __TIMESIZE != 64 > > +int > > +thrd_sleep (const struct timespec* time_point, struct timespec* remaining) > > +{ > > + int ret; > > + struct __timespec64 tp64, tr64; > > + > > + tp64 = valid_timespec_to_timespec64 (*time_point); > > + ret = __thrd_sleep_time64 (&tp64, &tr64); > > + > > + if (ret == 0 || errno != ENOSYS) > > + { > > + if (! in_time_t_range (tr64.tv_sec)) > > + { > > + __set_errno (EOVERFLOW); > > + return -1; > > This could not only be copying uninitialized data for such a failure (e.g. > a failure for an invalid nanoseconds value), it could actually produce a > spurious EOVERFLOW error if that uninitialized data is out of range. > > As far as I know the remaining amount can't exceed the amount passed in, > so when it's copied it shouldn't actually be necessary to check for > overflow. I have removed the check when converting the remaining time from 64-bit to 32-bit for all files. > > I think similar issues apply for other functions in the patch. > > Do we have test coverage in the testsuite for invalid nanoseconds > arguments to all these functions, making sure they produce proper EINVAL > errors? That might not cover many of these bugs (it wouldn't detect > simply reading uninitialized memory, or calling more syscalls than > necessary, and detecting the spurious EOVERFLOW would depend on what > values were in that memory). But it would seem a good idea to make sure > we do have such test coverage for functions using time arguments. I did see test failures (they were running as I sent this patch) so I think this is covered, although I haven't investigated the failures. Alistair > > -- > Joseph S. Myers > joseph@codesourcery.com
On Mon, Oct 28, 2019 at 8:51 PM Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote: > > > > On 24/10/2019 20:41, Alistair Francis wrote: > > The nanosleep syscall is not supported on newer 32-bit platforms (such > > as RV32). To fix this issue let's use clock_nanosleep_time64 if it is > > avaliable. > > > > Let's use CLOCK_REALTIME when calling clock_nanosleep_time64 as the > > Linux specification says: > > "POSIX.1 specifies that nanosleep() should measure time against the > > CLOCK_REALTIME clock. However, Linux measures the time using the > > CLOCK_MONOTONIC clock. This probably does not matter, since the POSIX.1 > > specification for clock_settime(2) says that discontinuous changes in > > CLOCK_REALTIME should not affect nanosleep()" > > --- > > This was patch was runtime tested with RV32 and RV64 > > It was build tested using the ./scripts/build-many-glibcs.py script. > > > > I am currently running: > > $ make ; make install ; make check > > tests on native ARM (32-bit) with the following three confiugrations: > > - 4.19 Kernel and 4.19 Headers > > - 5.2 Kernel and 4.19 Headers > > - 5.2 Kernel and 5.2 Headers > > I should have the results tomorrow. > > > > v2: > > - Explicitly include `#include <kernel-features.h>` > > > > include/time.h | 20 ++++++ > > nptl/thrd_sleep.c | 70 +++++++++++++++++-- > > sysdeps/unix/sysv/linux/clock_nanosleep.c | 73 ++++++++++++++++++-- > > sysdeps/unix/sysv/linux/nanosleep.c | 65 ++++++++++++++++- > > sysdeps/unix/sysv/linux/nanosleep_nocancel.c | 65 ++++++++++++++++- > > 5 files changed, 280 insertions(+), 13 deletions(-) > > > > diff --git a/include/time.h b/include/time.h > > index d93b16a7810..0e703f87cef 100644 > > --- a/include/time.h > > +++ b/include/time.h > > @@ -174,6 +174,26 @@ libc_hidden_proto (__difftime64) > > > > extern double __difftime (time_t time1, time_t time0); > > > > +#if __TIMESIZE == 64 > > +# define __thrd_sleep_time64 thrd_sleep > > +# define __clock_nanosleep_time64 __clock_nanosleep > > +# define __nanosleep_time64 __nanosleep > > +# define __nanosleep_nocancel_time64 __nanosleep_nocancel > > +#else > > +extern int __thrd_sleep_time64 (const struct __timespec64* time_point, > > + struct __timespec64* remaining); > > +libc_hidden_proto (__thrd_sleep_time64) > > +extern int __clock_nanosleep_time64 (clockid_t clock_id, > > + int flags, const struct __timespec64 *req, > > + struct __timespec64 *rem); > > +libc_hidden_proto (__clock_nanosleep_time64) > > +extern int __nanosleep_time64 (const struct __timespec64 *requested_time, > > + struct __timespec64 *remaining); > > +libc_hidden_proto (__nanosleep_time64) > > +extern int __nanosleep_nocancel_time64 (const struct __timespec64 *requested_time, > > + struct __timespec64 *remaining); > > +libc_hidden_proto (__nanosleep_nocancel_time64) > > +#endif > > > > /* Use in the clock_* functions. Size of the field representing the > > actual clock ID. */ > > diff --git a/nptl/thrd_sleep.c b/nptl/thrd_sleep.c > > index 2e185dd748e..2a6bb1f9e3f 100644 > > --- a/nptl/thrd_sleep.c > > +++ b/nptl/thrd_sleep.c > > @@ -17,23 +17,85 @@ > > <https://www.gnu.org/licenses/>. */ > > > > #include <time.h> > > +#include <kernel-features.h> > > #include <sysdep-cancel.h> > > > > #include "thrd_priv.h" > > > > int > > -thrd_sleep (const struct timespec* time_point, struct timespec* remaining) > > +__thrd_sleep_time64 (const struct __timespec64* time_point, struct __timespec64* remaining) > > { > > INTERNAL_SYSCALL_DECL (err); > > - int ret = INTERNAL_SYSCALL_CANCEL (nanosleep, err, time_point, remaining); > > + int ret = -1; > > + > > +#ifdef __ASSUME_TIME64_SYSCALLS > > +# ifndef __NR_clock_nanosleep_time64 > > +# define __NR_clock_nanosleep_time64 __NR_clock_nanosleep > > +# endif > > + ret = INTERNAL_SYSCALL_CANCEL (clock_nanosleep_time64, err, CLOCK_REALTIME, > > + 0, time_point, remaining); > > +#else > > +# ifdef __NR_clock_nanosleep_time64 > > + long int ret_64; > > + > > + ret_64 = INTERNAL_SYSCALL_CANCEL (clock_nanosleep_time64, err, CLOCK_REALTIME, > > + 0, time_point, remaining); > > + > > + if (ret_64 == 0 || errno != ENOSYS) > > + ret = ret_64; > > +# endif /* __NR_clock_nanosleep_time64 */ > > + if (ret < 0) > > + { > > + struct timespec tp32, tr32; > > + > > + if (! in_time_t_range (time_point->tv_sec)) > > + { > > + __set_errno (EOVERFLOW); > > + return -1; > > + } > > + > > + tp32 = valid_timespec64_to_timespec (*time_point); > > + ret = INTERNAL_SYSCALL_CANCEL (nanosleep, err, &tp32, &tr32); > > + > > + if ((ret == 0 || errno != ENOSYS) && remaining) > > + *remaining = valid_timespec_to_timespec64 (tr32); > > + } > > +#endif /* __ASSUME_TIME64_SYSCALLS */ > > + > > if (INTERNAL_SYSCALL_ERROR_P (ret, err)) > > { > > /* C11 states thrd_sleep function returns -1 if it has been interrupted > > - by a signal, or a negative value if it fails. */ > > + by a signal, or a negative value if it fails. */ > > ret = INTERNAL_SYSCALL_ERRNO (ret, err); > > if (ret == EINTR) > > - return -1; > > + return -1; > > return -2; > > } > > return 0; > > } > > Wouldn't be simple to just call __clock_nanosleep_time64 instead of duplicate > the syscall handling and the required error code for c11 thread? Something > like: > > int > __thrd_sleep_time64 (const struct __timespec64* time_point, > struct __timespec64* remaining) > { > int ret = __clock_nanosleep (CLOCK_REALTIME, 0, time_point, remaining); > /* C11 states thrd_sleep function returns -1 if it has been interrupted > by a signal, or a negative value if it fails. */ > switch (ret) > { > case 0: return 0; > case EINTR: return -1; > default: return -2; > } > } This looks good to me, I'll try that. Alistair > > > + > > +#if __TIMESIZE != 64 > > +int > > +thrd_sleep (const struct timespec* time_point, struct timespec* remaining) > > +{ > > + int ret; > > + struct __timespec64 tp64, tr64; > > + > > + tp64 = valid_timespec_to_timespec64 (*time_point); > > + ret = __thrd_sleep_time64 (&tp64, &tr64); > > + > > + if (ret == 0 || errno != ENOSYS) > > + { > > + if (! in_time_t_range (tr64.tv_sec)) > > + { > > + __set_errno (EOVERFLOW); > > + return -1; > > + } > > + > > + if (remaining) > > + *remaining = valid_timespec64_to_timespec (tr64); > > + } > > + > > + return ret; > > +} > > +#endif > > diff --git a/sysdeps/unix/sysv/linux/clock_nanosleep.c b/sysdeps/unix/sysv/linux/clock_nanosleep.c > > index 1f240b8720a..d257ea57fb0 100644 > > --- a/sysdeps/unix/sysv/linux/clock_nanosleep.c > > +++ b/sysdeps/unix/sysv/linux/clock_nanosleep.c > > @@ -16,6 +16,7 @@ > > <https://www.gnu.org/licenses/>. */ > > > > #include <time.h> > > +#include <kernel-features.h> > > #include <errno.h> > > > > #include <sysdep-cancel.h> > > @@ -26,9 +27,11 @@ > > /* We can simply use the syscall. The CPU clocks are not supported > > with this function. */ > > int > > -__clock_nanosleep (clockid_t clock_id, int flags, const struct timespec *req, > > - struct timespec *rem) > > +__clock_nanosleep_time64 (clockid_t clock_id, int flags, const struct __timespec64 *req, > > + struct __timespec64 *rem) > > { > > + int r = -1; > > + > > if (clock_id == CLOCK_THREAD_CPUTIME_ID) > > return EINVAL; > > if (clock_id == CLOCK_PROCESS_CPUTIME_ID) > > @@ -37,12 +40,72 @@ __clock_nanosleep (clockid_t clock_id, int flags, const struct timespec *req, > > /* If the call is interrupted by a signal handler or encounters an error, > > it returns a positive value similar to errno. */ > > INTERNAL_SYSCALL_DECL (err); > > - int r = INTERNAL_SYSCALL_CANCEL (clock_nanosleep, err, clock_id, flags, > > - req, rem); > > + > > +#ifdef __ASSUME_TIME64_SYSCALLS > > +# ifndef __NR_clock_nanosleep_time64 > > +# define __NR_clock_nanosleep_time64 __NR_clock_nanosleep > > +# endif > > + r = INTERNAL_SYSCALL_CANCEL (clock_nanosleep_time64, err, clock_id, > > + flags, req, rem); > > +#else > > +# ifdef __NR_clock_nanosleep_time64 > > + long int ret_64; > > + > > + ret_64 = INTERNAL_SYSCALL_CANCEL (clock_nanosleep_time64, err, clock_id, > > + flags, req, rem); > > + > > + if (ret_64 == 0 || errno != ENOSYS) > > + r = ret_64; > > +# endif /* __NR_clock_nanosleep_time64 */ > > + if (r < 0) > > + { > > + struct timespec ts32, tr32; > > + > > + if (! in_time_t_range (req->tv_sec)) > > + { > > + __set_errno (EOVERFLOW); > > + return -1; > > + } > > + > > + ts32 = valid_timespec64_to_timespec (*req); > > + r = INTERNAL_SYSCALL_CANCEL (clock_nanosleep, err, &ts32, &tr32); > > + > > + if ((r == 0 || errno != ENOSYS) && rem) > > + *rem = valid_timespec_to_timespec64 (tr32); > > + } > > +#endif /* __ASSUME_TIME64_SYSCALLS */ > > + > > return (INTERNAL_SYSCALL_ERROR_P (r, err) > > - ? INTERNAL_SYSCALL_ERRNO (r, err) : 0); > > + ? INTERNAL_SYSCALL_ERRNO (r, err) : 0); > > } > > > > +#if __TIMESIZE != 64 > > +int > > +__clock_nanosleep (clockid_t clock_id, int flags, const struct timespec *req, > > + struct timespec *rem) > > +{ > > + int r; > > + struct __timespec64 treq64, trem64; > > + > > + treq64 = valid_timespec_to_timespec64 (*req); > > + r = __clock_nanosleep_time64 (clock_id, flags, &treq64, &trem64); > > + > > + if (r == 0 || errno != ENOSYS) > > + { > > + if (! in_time_t_range (trem64.tv_sec)) > > + { > > + __set_errno (EOVERFLOW); > > + return -1; > > + } > > + > > + if (rem) > > + *rem = valid_timespec64_to_timespec (trem64); > > + } > > + > > + return r; > > +} > > +#endif > > + > > versioned_symbol (libc, __clock_nanosleep, clock_nanosleep, GLIBC_2_17); > > /* clock_nanosleep moved to libc in version 2.17; > > old binaries may expect the symbol version it had in librt. */ > > diff --git a/sysdeps/unix/sysv/linux/nanosleep.c b/sysdeps/unix/sysv/linux/nanosleep.c > > index 6787909248f..10a6d57a1de 100644 > > --- a/sysdeps/unix/sysv/linux/nanosleep.c > > +++ b/sysdeps/unix/sysv/linux/nanosleep.c > > @@ -17,15 +17,76 @@ > > <https://www.gnu.org/licenses/>. */ > > > > #include <time.h> > > +#include <kernel-features.h> > > #include <sysdep-cancel.h> > > #include <not-cancel.h> > > > > /* Pause execution for a number of nanoseconds. */ > > int > > +__nanosleep_time64 (const struct __timespec64 *requested_time, > > + struct __timespec64 *remaining) > > +{ > > +#if defined(__ASSUME_TIME64_SYSCALLS) > > +# ifndef __NR_clock_nanosleep_time64 > > +# define __NR_clock_nanosleep_time64 __NR_clock_nanosleep > > +# endif > > + return SYSCALL_CANCEL (clock_nanosleep_time64, CLOCK_REALTIME, 0, > > + requested_time, remaining); > > +#else > > +# ifdef __NR_clock_nanosleep_time64 > > + long int ret_64; > > + > > + ret_64 = SYSCALL_CANCEL (clock_nanosleep_time64, CLOCK_REALTIME, 0, > > + requested_time, remaining); > > + > > + if (ret_64 == 0 || errno != ENOSYS) > > + return ret_64; > > +# endif /* __NR_clock_nanosleep_time64 */ > > + int ret; > > + struct timespec ts32, tr32; > > + > > + if (! in_time_t_range (requested_time->tv_sec)) > > + { > > + __set_errno (EOVERFLOW); > > + return -1; > > + } > > + > > + ts32 = valid_timespec64_to_timespec (*requested_time); > > + ret = SYSCALL_CANCEL (nanosleep, &ts32, &tr32); > > + > > + if ((ret == 0 || errno != ENOSYS) && remaining) > > + *remaining = valid_timespec_to_timespec64 (tr32); > > + > > + return ret; > > +#endif /* __ASSUME_TIME64_SYSCALLS */ > > +} > > + > > +#if __TIMESIZE != 64 > > +int > > __nanosleep (const struct timespec *requested_time, > > - struct timespec *remaining) > > + struct timespec *remaining) > > { > > - return SYSCALL_CANCEL (nanosleep, requested_time, remaining); > > + int r; > > + struct __timespec64 treq64, trem64; > > + > > + treq64 = valid_timespec_to_timespec64 (*requested_time); > > + r = __nanosleep_time64 (&treq64, &trem64); > > + > > + if (r == 0 || errno != ENOSYS) > > + { > > + if (! in_time_t_range (trem64.tv_sec)) > > + { > > + __set_errno (EOVERFLOW); > > + return -1; > > + } > > + > > + if (remaining) > > + *remaining = valid_timespec64_to_timespec (trem64); > > + } > > + > > + return r; > > } > > +#endif > > + > > hidden_def (__nanosleep) > > weak_alias (__nanosleep, nanosleep) > > diff --git a/sysdeps/unix/sysv/linux/nanosleep_nocancel.c b/sysdeps/unix/sysv/linux/nanosleep_nocancel.c > > index d6442bf4f7f..1a6c6c0a48a 100644 > > --- a/sysdeps/unix/sysv/linux/nanosleep_nocancel.c > > +++ b/sysdeps/unix/sysv/linux/nanosleep_nocancel.c > > @@ -17,13 +17,74 @@ > > <https://www.gnu.org/licenses/>. */ > > > > #include <time.h> > > +#include <kernel-features.h> > > #include <sysdep-cancel.h> > > #include <not-cancel.h> > > > > +int > > +__nanosleep_nocancel_time64 (const struct __timespec64 *requested_time, > > + struct __timespec64 *remaining) > > +{ > > +#ifdef __ASSUME_TIME64_SYSCALLS > > +# ifndef __NR_clock_nanosleep_time64 > > +# define __NR_clock_nanosleep_time64 __NR_clock_nanosleep > > +# endif > > + return INLINE_SYSCALL_CALL (clock_nanosleep_time64, CLOCK_REALTIME, 0, > > + requested_time, remaining); > > +#else > > +# ifdef __NR_clock_nanosleep_time64 > > + long int ret_64; > > + > > + ret_64 = INLINE_SYSCALL_CALL (clock_nanosleep_time64, CLOCK_REALTIME, 0, > > + requested_time, remaining); > > + > > + if (ret_64 == 0 || errno != ENOSYS) > > + return ret_64; > > +# endif /* __NR_clock_nanosleep_time64 */ > > + int ret; > > + struct timespec ts32, tr32; > > + > > + if (! in_time_t_range (requested_time->tv_sec)) > > + { > > + __set_errno (EOVERFLOW); > > + return -1; > > + } > > + > > + ts32 = valid_timespec64_to_timespec (*requested_time); > > + ret = INLINE_SYSCALL_CALL (nanosleep, &ts32, &tr32); > > + > > + if (ret == 0 || errno != ENOSYS) > > + *remaining = valid_timespec_to_timespec64 (tr32); > > + > > + return ret; > > +#endif /* __ASSUME_TIME64_SYSCALLS */ > > +} > > + > > +#if __TIMESIZE != 64 > > int > > __nanosleep_nocancel (const struct timespec *requested_time, > > - struct timespec *remaining) > > + struct timespec *remaining) > > { > > - return INLINE_SYSCALL_CALL (nanosleep, requested_time, remaining); > > + int ret; > > + struct __timespec64 treq64, trem64; > > + > > + treq64 = valid_timespec_to_timespec64 (*requested_time); > > + ret = __nanosleep_nocancel_time64 (&treq64, &trem64); > > + > > + if (ret == 0 || errno != ENOSYS) > > + { > > + if (! in_time_t_range (trem64.tv_sec)) > > + { > > + __set_errno (EOVERFLOW); > > + return -1; > > + } > > + > > + if (remaining) > > + *remaining = valid_timespec64_to_timespec (trem64); > > + } > > + > > + return ret; > > } > > +#endif > > + > > hidden_def (__nanosleep_nocancel) > >
On 24/10/2019 20:41, Alistair Francis wrote: > diff --git a/sysdeps/unix/sysv/linux/nanosleep.c b/sysdeps/unix/sysv/linux/nanosleep.c > index 6787909248f..10a6d57a1de 100644 > --- a/sysdeps/unix/sysv/linux/nanosleep.c > +++ b/sysdeps/unix/sysv/linux/nanosleep.c > @@ -17,15 +17,76 @@ > <https://www.gnu.org/licenses/>. */ > > #include <time.h> > +#include <kernel-features.h> > #include <sysdep-cancel.h> > #include <not-cancel.h> > > /* Pause execution for a number of nanoseconds. */ > int > +__nanosleep_time64 (const struct __timespec64 *requested_time, > + struct __timespec64 *remaining) > +{ > +#if defined(__ASSUME_TIME64_SYSCALLS) > +# ifndef __NR_clock_nanosleep_time64 > +# define __NR_clock_nanosleep_time64 __NR_clock_nanosleep > +# endif > + return SYSCALL_CANCEL (clock_nanosleep_time64, CLOCK_REALTIME, 0, > + requested_time, remaining); > +#else > +# ifdef __NR_clock_nanosleep_time64 > + long int ret_64; > + > + ret_64 = SYSCALL_CANCEL (clock_nanosleep_time64, CLOCK_REALTIME, 0, > + requested_time, remaining); > + > + if (ret_64 == 0 || errno != ENOSYS) > + return ret_64; > +# endif /* __NR_clock_nanosleep_time64 */ > + int ret; > + struct timespec ts32, tr32; > + > + if (! in_time_t_range (requested_time->tv_sec)) > + { > + __set_errno (EOVERFLOW); > + return -1; > + } > + > + ts32 = valid_timespec64_to_timespec (*requested_time); > + ret = SYSCALL_CANCEL (nanosleep, &ts32, &tr32); > + > + if ((ret == 0 || errno != ENOSYS) && remaining) > + *remaining = valid_timespec_to_timespec64 (tr32); > + > + return ret; > +#endif /* __ASSUME_TIME64_SYSCALLS */ > +} > + > +#if __TIMESIZE != 64 > +int > __nanosleep (const struct timespec *requested_time, > - struct timespec *remaining) > + struct timespec *remaining) > { > - return SYSCALL_CANCEL (nanosleep, requested_time, remaining); > + int r; > + struct __timespec64 treq64, trem64; > + > + treq64 = valid_timespec_to_timespec64 (*requested_time); > + r = __nanosleep_time64 (&treq64, &trem64); > + > + if (r == 0 || errno != ENOSYS) > + { > + if (! in_time_t_range (trem64.tv_sec)) > + { > + __set_errno (EOVERFLOW); > + return -1; > + } > + > + if (remaining) > + *remaining = valid_timespec64_to_timespec (trem64); > + } > + > + return r; > } > +#endif > + > hidden_def (__nanosleep) > weak_alias (__nanosleep, nanosleep) There is no need to replicate all the syscall logic, nanosleep can be implemented with __clock_nanosleep. You can do: int __nanosleep (const struct timespec *requested_time, struct timespec *remaining) { int ret = __clock_nanosleep (CLOCK_REALTIME, 0, requested_time, remaining); if (ret != 0) { __set_errno (-ret); return -1; } return ret; } I think we can even make it the default version, thus removing the linux specific file. > diff --git a/sysdeps/unix/sysv/linux/nanosleep_nocancel.c b/sysdeps/unix/sysv/linux/nanosleep_nocancel.c > index d6442bf4f7f..1a6c6c0a48a 100644 > --- a/sysdeps/unix/sysv/linux/nanosleep_nocancel.c > +++ b/sysdeps/unix/sysv/linux/nanosleep_nocancel.c > @@ -17,13 +17,74 @@ > <https://www.gnu.org/licenses/>. */ > > #include <time.h> > +#include <kernel-features.h> > #include <sysdep-cancel.h> > #include <not-cancel.h> > > +int > +__nanosleep_nocancel_time64 (const struct __timespec64 *requested_time, > + struct __timespec64 *remaining) > +{ > +#ifdef __ASSUME_TIME64_SYSCALLS > +# ifndef __NR_clock_nanosleep_time64 > +# define __NR_clock_nanosleep_time64 __NR_clock_nanosleep > +# endif > + return INLINE_SYSCALL_CALL (clock_nanosleep_time64, CLOCK_REALTIME, 0, > + requested_time, remaining); > +#else > +# ifdef __NR_clock_nanosleep_time64 > + long int ret_64; > + > + ret_64 = INLINE_SYSCALL_CALL (clock_nanosleep_time64, CLOCK_REALTIME, 0, > + requested_time, remaining); > + > + if (ret_64 == 0 || errno != ENOSYS) > + return ret_64; > +# endif /* __NR_clock_nanosleep_time64 */ > + int ret; > + struct timespec ts32, tr32; > + > + if (! in_time_t_range (requested_time->tv_sec)) > + { > + __set_errno (EOVERFLOW); > + return -1; > + } > + > + ts32 = valid_timespec64_to_timespec (*requested_time); > + ret = INLINE_SYSCALL_CALL (nanosleep, &ts32, &tr32); > + > + if (ret == 0 || errno != ENOSYS) > + *remaining = valid_timespec_to_timespec64 (tr32); > + > + return ret; > +#endif /* __ASSUME_TIME64_SYSCALLS */ > +} > + > +#if __TIMESIZE != 64 > int > __nanosleep_nocancel (const struct timespec *requested_time, > - struct timespec *remaining) > + struct timespec *remaining) > { > - return INLINE_SYSCALL_CALL (nanosleep, requested_time, remaining); > + int ret; > + struct __timespec64 treq64, trem64; > + > + treq64 = valid_timespec_to_timespec64 (*requested_time); > + ret = __nanosleep_nocancel_time64 (&treq64, &trem64); > + > + if (ret == 0 || errno != ENOSYS) > + { > + if (! in_time_t_range (trem64.tv_sec)) > + { > + __set_errno (EOVERFLOW); > + return -1; > + } > + > + if (remaining) > + *remaining = valid_timespec64_to_timespec (trem64); > + } > + > + return ret; > } > +#endif > + > hidden_def (__nanosleep_nocancel) > The __nanosleep_nocancel is just used priority mutex for pthread_mutex_timedlock, and I am almost sure that we can replace the code path with __lll_clocklock_wait. Something like the below. If you like I can sent it as a cleanup patch, since it would require some more work to remove the __nanosleep_nocancel internal definitions as well. -- diff --git a/nptl/pthread_mutex_timedlock.c b/nptl/pthread_mutex_timedlock.c index d6f0d9e31a..b9536ddb19 100644 --- a/nptl/pthread_mutex_timedlock.c +++ b/nptl/pthread_mutex_timedlock.c @@ -25,6 +25,7 @@ #include <atomic.h> #include <lowlevellock.h> #include <not-cancel.h> +#include <futex-internal.h> #include <stap-probe.h> @@ -377,50 +378,29 @@ __pthread_mutex_clocklock_common (pthread_mutex_t *mutex, int private = (robust ? PTHREAD_ROBUST_MUTEX_PSHARED (mutex) : PTHREAD_MUTEX_PSHARED (mutex)); - INTERNAL_SYSCALL_DECL (__err); - - int e = INTERNAL_SYSCALL (futex, __err, 4, &mutex->__data.__lock, - __lll_private_flag (FUTEX_LOCK_PI, - private), 1, - abstime); - if (INTERNAL_SYSCALL_ERROR_P (e, __err)) + int e = futex_lock_pi ((unsigned int *) &mutex->__data.__lock, 1, + abstime, private); + if (e == ETIMEDOUT) { - if (INTERNAL_SYSCALL_ERRNO (e, __err) == ETIMEDOUT) - return ETIMEDOUT; - - if (INTERNAL_SYSCALL_ERRNO (e, __err) == ESRCH - || INTERNAL_SYSCALL_ERRNO (e, __err) == EDEADLK) + return ETIMEDOUT; + } + else if (e == ESRCH || e == EDEADLK) + { + assert (e != EDEADLK + || (kind != PTHREAD_MUTEX_ERRORCHECK_NP + && kind != PTHREAD_MUTEX_RECURSIVE_NP)); + /* ESRCH can happen only for non-robust PI mutexes where + the owner of the lock died. */ + assert (e != ESRCH || !robust); + + /* Delay the thread until the timeout is reached. Then return + ETIMEDOUT. */ + do { - assert (INTERNAL_SYSCALL_ERRNO (e, __err) != EDEADLK - || (kind != PTHREAD_MUTEX_ERRORCHECK_NP - && kind != PTHREAD_MUTEX_RECURSIVE_NP)); - /* ESRCH can happen only for non-robust PI mutexes where - the owner of the lock died. */ - assert (INTERNAL_SYSCALL_ERRNO (e, __err) != ESRCH - || !robust); - - /* Delay the thread until the timeout is reached. - Then return ETIMEDOUT. */ - struct timespec reltime; - struct timespec now; - - INTERNAL_SYSCALL (clock_gettime, __err, 2, clockid, - &now); - reltime.tv_sec = abstime->tv_sec - now.tv_sec; - reltime.tv_nsec = abstime->tv_nsec - now.tv_nsec; - if (reltime.tv_nsec < 0) - { - reltime.tv_nsec += 1000000000; - --reltime.tv_sec; - } - if (reltime.tv_sec >= 0) - while (__nanosleep_nocancel (&reltime, &reltime) != 0) - continue; - - return ETIMEDOUT; - } - - return INTERNAL_SYSCALL_ERRNO (e, __err); + e = __lll_clocklock_wait (&(int){0}, CLOCK_REALTIME, + abstime, private); + } while (e != ETIMEDOUT); + return e; } oldval = mutex->__data.__lock; diff --git a/sysdeps/unix/sysv/linux/futex-internal.h b/sysdeps/unix/sysv/linux/futex-internal.h index 5a4f4ff818..1442df63a1 100644 --- a/sysdeps/unix/sysv/linux/futex-internal.h +++ b/sysdeps/unix/sysv/linux/futex-internal.h @@ -252,4 +252,30 @@ futex_wake (unsigned int *futex_word, int processes_to_wake, int private) } } +static __always_inline int +futex_lock_pi (unsigned int *futex_word, unsigned int expected, + const struct timespec *abstime, int private) +{ + int err = lll_futex_timed_lock_pi (futex_word, expected, abstime, private); + switch (err) + { + case 0: + case -EAGAIN: + case -EINTR: + case -ETIMEDOUT: + case -ESRCH: + case -EDEADLK: + 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 (); + } +} + #endif /* futex-internal.h */ diff --git a/sysdeps/unix/sysv/linux/lowlevellock-futex.h b/sysdeps/unix/sysv/linux/lowlevellock-futex.h index b423673ed4..5730eb2ba2 100644 --- a/sysdeps/unix/sysv/linux/lowlevellock-futex.h +++ b/sysdeps/unix/sysv/linux/lowlevellock-futex.h @@ -127,6 +127,11 @@ FUTEX_OP_CLEAR_WAKE_IF_GT_ONE) /* Priority Inheritance support. */ +#define lll_futex_timed_lock_pi(futexp, val, reltime, private) \ + lll_futex_syscall (4, futexp, \ + __lll_private_flag (FUTEX_LOCK_PI, private), \ + val, reltime) + #define lll_futex_wait_requeue_pi(futexp, val, mutex, private) \ lll_futex_timed_wait_requeue_pi (futexp, val, NULL, 0, mutex, private)
* Adhemerval Zanella: > There is no need to replicate all the syscall logic, nanosleep can be implemented > with __clock_nanosleep. You can do: > > int > __nanosleep (const struct timespec *requested_time, > struct timespec *remaining) > { > int ret = __clock_nanosleep (CLOCK_REALTIME, 0, requested_time, remaining); > if (ret != 0) > { > __set_errno (-ret); > return -1; > } > return ret; > } Why the -ret? I think it should be a plain ret. Thanks, Florian
On 30/10/2019 11:06, Florian Weimer wrote: > * Adhemerval Zanella: > >> There is no need to replicate all the syscall logic, nanosleep can be implemented >> with __clock_nanosleep. You can do: >> >> int >> __nanosleep (const struct timespec *requested_time, >> struct timespec *remaining) >> { >> int ret = __clock_nanosleep (CLOCK_REALTIME, 0, requested_time, remaining); >> if (ret != 0) >> { >> __set_errno (-ret); >> return -1; >> } >> return ret; >> } > > Why the -ret? I think it should be a plain ret. Indeed, my mistake here.
On Tue, Oct 29, 2019 at 9:16 PM Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote: > > > > On 24/10/2019 20:41, Alistair Francis wrote: > > diff --git a/sysdeps/unix/sysv/linux/nanosleep.c b/sysdeps/unix/sysv/linux/nanosleep.c > > index 6787909248f..10a6d57a1de 100644 > > --- a/sysdeps/unix/sysv/linux/nanosleep.c > > +++ b/sysdeps/unix/sysv/linux/nanosleep.c > > @@ -17,15 +17,76 @@ > > <https://www.gnu.org/licenses/>. */ > > > > #include <time.h> > > +#include <kernel-features.h> > > #include <sysdep-cancel.h> > > #include <not-cancel.h> > > > > /* Pause execution for a number of nanoseconds. */ > > int > > +__nanosleep_time64 (const struct __timespec64 *requested_time, > > + struct __timespec64 *remaining) > > +{ > > +#if defined(__ASSUME_TIME64_SYSCALLS) > > +# ifndef __NR_clock_nanosleep_time64 > > +# define __NR_clock_nanosleep_time64 __NR_clock_nanosleep > > +# endif > > + return SYSCALL_CANCEL (clock_nanosleep_time64, CLOCK_REALTIME, 0, > > + requested_time, remaining); > > +#else > > +# ifdef __NR_clock_nanosleep_time64 > > + long int ret_64; > > + > > + ret_64 = SYSCALL_CANCEL (clock_nanosleep_time64, CLOCK_REALTIME, 0, > > + requested_time, remaining); > > + > > + if (ret_64 == 0 || errno != ENOSYS) > > + return ret_64; > > +# endif /* __NR_clock_nanosleep_time64 */ > > + int ret; > > + struct timespec ts32, tr32; > > + > > + if (! in_time_t_range (requested_time->tv_sec)) > > + { > > + __set_errno (EOVERFLOW); > > + return -1; > > + } > > + > > + ts32 = valid_timespec64_to_timespec (*requested_time); > > + ret = SYSCALL_CANCEL (nanosleep, &ts32, &tr32); > > + > > + if ((ret == 0 || errno != ENOSYS) && remaining) > > + *remaining = valid_timespec_to_timespec64 (tr32); > > + > > + return ret; > > +#endif /* __ASSUME_TIME64_SYSCALLS */ > > +} > > + > > +#if __TIMESIZE != 64 > > +int > > __nanosleep (const struct timespec *requested_time, > > - struct timespec *remaining) > > + struct timespec *remaining) > > { > > - return SYSCALL_CANCEL (nanosleep, requested_time, remaining); > > + int r; > > + struct __timespec64 treq64, trem64; > > + > > + treq64 = valid_timespec_to_timespec64 (*requested_time); > > + r = __nanosleep_time64 (&treq64, &trem64); > > + > > + if (r == 0 || errno != ENOSYS) > > + { > > + if (! in_time_t_range (trem64.tv_sec)) > > + { > > + __set_errno (EOVERFLOW); > > + return -1; > > + } > > + > > + if (remaining) > > + *remaining = valid_timespec64_to_timespec (trem64); > > + } > > + > > + return r; > > } > > +#endif > > + > > hidden_def (__nanosleep) > > weak_alias (__nanosleep, nanosleep) > > There is no need to replicate all the syscall logic, nanosleep can be implemented > with __clock_nanosleep. You can do: > > int > __nanosleep (const struct timespec *requested_time, > struct timespec *remaining) > { > int ret = __clock_nanosleep (CLOCK_REALTIME, 0, requested_time, remaining); > if (ret != 0) > { > __set_errno (-ret); > return -1; > } > return ret; > } > > I think we can even make it the default version, thus removing the linux > specific file. Ok, testing now then I'll send the patch. > > > > diff --git a/sysdeps/unix/sysv/linux/nanosleep_nocancel.c b/sysdeps/unix/sysv/linux/nanosleep_nocancel.c > > index d6442bf4f7f..1a6c6c0a48a 100644 > > --- a/sysdeps/unix/sysv/linux/nanosleep_nocancel.c > > +++ b/sysdeps/unix/sysv/linux/nanosleep_nocancel.c > > @@ -17,13 +17,74 @@ > > <https://www.gnu.org/licenses/>. */ > > > > #include <time.h> > > +#include <kernel-features.h> > > #include <sysdep-cancel.h> > > #include <not-cancel.h> > > > > +int > > +__nanosleep_nocancel_time64 (const struct __timespec64 *requested_time, > > + struct __timespec64 *remaining) > > +{ > > +#ifdef __ASSUME_TIME64_SYSCALLS > > +# ifndef __NR_clock_nanosleep_time64 > > +# define __NR_clock_nanosleep_time64 __NR_clock_nanosleep > > +# endif > > + return INLINE_SYSCALL_CALL (clock_nanosleep_time64, CLOCK_REALTIME, 0, > > + requested_time, remaining); > > +#else > > +# ifdef __NR_clock_nanosleep_time64 > > + long int ret_64; > > + > > + ret_64 = INLINE_SYSCALL_CALL (clock_nanosleep_time64, CLOCK_REALTIME, 0, > > + requested_time, remaining); > > + > > + if (ret_64 == 0 || errno != ENOSYS) > > + return ret_64; > > +# endif /* __NR_clock_nanosleep_time64 */ > > + int ret; > > + struct timespec ts32, tr32; > > + > > + if (! in_time_t_range (requested_time->tv_sec)) > > + { > > + __set_errno (EOVERFLOW); > > + return -1; > > + } > > + > > + ts32 = valid_timespec64_to_timespec (*requested_time); > > + ret = INLINE_SYSCALL_CALL (nanosleep, &ts32, &tr32); > > + > > + if (ret == 0 || errno != ENOSYS) > > + *remaining = valid_timespec_to_timespec64 (tr32); > > + > > + return ret; > > +#endif /* __ASSUME_TIME64_SYSCALLS */ > > +} > > + > > +#if __TIMESIZE != 64 > > int > > __nanosleep_nocancel (const struct timespec *requested_time, > > - struct timespec *remaining) > > + struct timespec *remaining) > > { > > - return INLINE_SYSCALL_CALL (nanosleep, requested_time, remaining); > > + int ret; > > + struct __timespec64 treq64, trem64; > > + > > + treq64 = valid_timespec_to_timespec64 (*requested_time); > > + ret = __nanosleep_nocancel_time64 (&treq64, &trem64); > > + > > + if (ret == 0 || errno != ENOSYS) > > + { > > + if (! in_time_t_range (trem64.tv_sec)) > > + { > > + __set_errno (EOVERFLOW); > > + return -1; > > + } > > + > > + if (remaining) > > + *remaining = valid_timespec64_to_timespec (trem64); > > + } > > + > > + return ret; > > } > > +#endif > > + > > hidden_def (__nanosleep_nocancel) > > > > The __nanosleep_nocancel is just used priority mutex for pthread_mutex_timedlock, > and I am almost sure that we can replace the code path with __lll_clocklock_wait. > Something like the below. > > If you like I can sent it as a cleanup patch, since it would require some more > work to remove the __nanosleep_nocancel internal definitions as well. I would prefer that going in seperatley via a cleanup patch. Alistair > > -- > > diff --git a/nptl/pthread_mutex_timedlock.c b/nptl/pthread_mutex_timedlock.c > index d6f0d9e31a..b9536ddb19 100644 > --- a/nptl/pthread_mutex_timedlock.c > +++ b/nptl/pthread_mutex_timedlock.c > @@ -25,6 +25,7 @@ > #include <atomic.h> > #include <lowlevellock.h> > #include <not-cancel.h> > +#include <futex-internal.h> > > #include <stap-probe.h> > > @@ -377,50 +378,29 @@ __pthread_mutex_clocklock_common (pthread_mutex_t *mutex, > int private = (robust > ? PTHREAD_ROBUST_MUTEX_PSHARED (mutex) > : PTHREAD_MUTEX_PSHARED (mutex)); > - INTERNAL_SYSCALL_DECL (__err); > - > - int e = INTERNAL_SYSCALL (futex, __err, 4, &mutex->__data.__lock, > - __lll_private_flag (FUTEX_LOCK_PI, > - private), 1, > - abstime); > - if (INTERNAL_SYSCALL_ERROR_P (e, __err)) > + int e = futex_lock_pi ((unsigned int *) &mutex->__data.__lock, 1, > + abstime, private); > + if (e == ETIMEDOUT) > { > - if (INTERNAL_SYSCALL_ERRNO (e, __err) == ETIMEDOUT) > - return ETIMEDOUT; > - > - if (INTERNAL_SYSCALL_ERRNO (e, __err) == ESRCH > - || INTERNAL_SYSCALL_ERRNO (e, __err) == EDEADLK) > + return ETIMEDOUT; > + } > + else if (e == ESRCH || e == EDEADLK) > + { > + assert (e != EDEADLK > + || (kind != PTHREAD_MUTEX_ERRORCHECK_NP > + && kind != PTHREAD_MUTEX_RECURSIVE_NP)); > + /* ESRCH can happen only for non-robust PI mutexes where > + the owner of the lock died. */ > + assert (e != ESRCH || !robust); > + > + /* Delay the thread until the timeout is reached. Then return > + ETIMEDOUT. */ > + do > { > - assert (INTERNAL_SYSCALL_ERRNO (e, __err) != EDEADLK > - || (kind != PTHREAD_MUTEX_ERRORCHECK_NP > - && kind != PTHREAD_MUTEX_RECURSIVE_NP)); > - /* ESRCH can happen only for non-robust PI mutexes where > - the owner of the lock died. */ > - assert (INTERNAL_SYSCALL_ERRNO (e, __err) != ESRCH > - || !robust); > - > - /* Delay the thread until the timeout is reached. > - Then return ETIMEDOUT. */ > - struct timespec reltime; > - struct timespec now; > - > - INTERNAL_SYSCALL (clock_gettime, __err, 2, clockid, > - &now); > - reltime.tv_sec = abstime->tv_sec - now.tv_sec; > - reltime.tv_nsec = abstime->tv_nsec - now.tv_nsec; > - if (reltime.tv_nsec < 0) > - { > - reltime.tv_nsec += 1000000000; > - --reltime.tv_sec; > - } > - if (reltime.tv_sec >= 0) > - while (__nanosleep_nocancel (&reltime, &reltime) != 0) > - continue; > - > - return ETIMEDOUT; > - } > - > - return INTERNAL_SYSCALL_ERRNO (e, __err); > + e = __lll_clocklock_wait (&(int){0}, CLOCK_REALTIME, > + abstime, private); > + } while (e != ETIMEDOUT); > + return e; > } > > oldval = mutex->__data.__lock; > diff --git a/sysdeps/unix/sysv/linux/futex-internal.h b/sysdeps/unix/sysv/linux/futex-internal.h > index 5a4f4ff818..1442df63a1 100644 > --- a/sysdeps/unix/sysv/linux/futex-internal.h > +++ b/sysdeps/unix/sysv/linux/futex-internal.h > @@ -252,4 +252,30 @@ futex_wake (unsigned int *futex_word, int processes_to_wake, int private) > } > } > > +static __always_inline int > +futex_lock_pi (unsigned int *futex_word, unsigned int expected, > + const struct timespec *abstime, int private) > +{ > + int err = lll_futex_timed_lock_pi (futex_word, expected, abstime, private); > + switch (err) > + { > + case 0: > + case -EAGAIN: > + case -EINTR: > + case -ETIMEDOUT: > + case -ESRCH: > + case -EDEADLK: > + 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 (); > + } > +} > + > #endif /* futex-internal.h */ > diff --git a/sysdeps/unix/sysv/linux/lowlevellock-futex.h b/sysdeps/unix/sysv/linux/lowlevellock-futex.h > index b423673ed4..5730eb2ba2 100644 > --- a/sysdeps/unix/sysv/linux/lowlevellock-futex.h > +++ b/sysdeps/unix/sysv/linux/lowlevellock-futex.h > @@ -127,6 +127,11 @@ > FUTEX_OP_CLEAR_WAKE_IF_GT_ONE) > > /* Priority Inheritance support. */ > +#define lll_futex_timed_lock_pi(futexp, val, reltime, private) \ > + lll_futex_syscall (4, futexp, \ > + __lll_private_flag (FUTEX_LOCK_PI, private), \ > + val, reltime) > + > #define lll_futex_wait_requeue_pi(futexp, val, mutex, private) \ > lll_futex_timed_wait_requeue_pi (futexp, val, NULL, 0, mutex, private) >
On Tue, Oct 29, 2019 at 1:16 PM Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote: > > > > On 24/10/2019 20:41, Alistair Francis wrote: > > diff --git a/sysdeps/unix/sysv/linux/nanosleep.c b/sysdeps/unix/sysv/linux/nanosleep.c > > index 6787909248f..10a6d57a1de 100644 > > --- a/sysdeps/unix/sysv/linux/nanosleep.c > > +++ b/sysdeps/unix/sysv/linux/nanosleep.c > > @@ -17,15 +17,76 @@ > > <https://www.gnu.org/licenses/>. */ > > > > #include <time.h> > > +#include <kernel-features.h> > > #include <sysdep-cancel.h> > > #include <not-cancel.h> > > > > /* Pause execution for a number of nanoseconds. */ > > int > > +__nanosleep_time64 (const struct __timespec64 *requested_time, > > + struct __timespec64 *remaining) > > +{ > > +#if defined(__ASSUME_TIME64_SYSCALLS) > > +# ifndef __NR_clock_nanosleep_time64 > > +# define __NR_clock_nanosleep_time64 __NR_clock_nanosleep > > +# endif > > + return SYSCALL_CANCEL (clock_nanosleep_time64, CLOCK_REALTIME, 0, > > + requested_time, remaining); > > +#else > > +# ifdef __NR_clock_nanosleep_time64 > > + long int ret_64; > > + > > + ret_64 = SYSCALL_CANCEL (clock_nanosleep_time64, CLOCK_REALTIME, 0, > > + requested_time, remaining); > > + > > + if (ret_64 == 0 || errno != ENOSYS) > > + return ret_64; > > +# endif /* __NR_clock_nanosleep_time64 */ > > + int ret; > > + struct timespec ts32, tr32; > > + > > + if (! in_time_t_range (requested_time->tv_sec)) > > + { > > + __set_errno (EOVERFLOW); > > + return -1; > > + } > > + > > + ts32 = valid_timespec64_to_timespec (*requested_time); > > + ret = SYSCALL_CANCEL (nanosleep, &ts32, &tr32); > > + > > + if ((ret == 0 || errno != ENOSYS) && remaining) > > + *remaining = valid_timespec_to_timespec64 (tr32); > > + > > + return ret; > > +#endif /* __ASSUME_TIME64_SYSCALLS */ > > +} > > + > > +#if __TIMESIZE != 64 > > +int > > __nanosleep (const struct timespec *requested_time, > > - struct timespec *remaining) > > + struct timespec *remaining) > > { > > - return SYSCALL_CANCEL (nanosleep, requested_time, remaining); > > + int r; > > + struct __timespec64 treq64, trem64; > > + > > + treq64 = valid_timespec_to_timespec64 (*requested_time); > > + r = __nanosleep_time64 (&treq64, &trem64); > > + > > + if (r == 0 || errno != ENOSYS) > > + { > > + if (! in_time_t_range (trem64.tv_sec)) > > + { > > + __set_errno (EOVERFLOW); > > + return -1; > > + } > > + > > + if (remaining) > > + *remaining = valid_timespec64_to_timespec (trem64); > > + } > > + > > + return r; > > } > > +#endif > > + > > hidden_def (__nanosleep) > > weak_alias (__nanosleep, nanosleep) > > There is no need to replicate all the syscall logic, nanosleep can be implemented > with __clock_nanosleep. You can do: > > int > __nanosleep (const struct timespec *requested_time, > struct timespec *remaining) > { > int ret = __clock_nanosleep (CLOCK_REALTIME, 0, requested_time, remaining); > if (ret != 0) > { > __set_errno (-ret); > return -1; > } > return ret; > } This doesn't work as __clock_nanosleep() isn't avaliable in nptl so it fails to compile. My v3 patch attempted to fix this, but that also doesn't work and ends up squishing some patches together. Alistair > > I think we can even make it the default version, thus removing the linux > specific file. > > > > diff --git a/sysdeps/unix/sysv/linux/nanosleep_nocancel.c b/sysdeps/unix/sysv/linux/nanosleep_nocancel.c > > index d6442bf4f7f..1a6c6c0a48a 100644 > > --- a/sysdeps/unix/sysv/linux/nanosleep_nocancel.c > > +++ b/sysdeps/unix/sysv/linux/nanosleep_nocancel.c > > @@ -17,13 +17,74 @@ > > <https://www.gnu.org/licenses/>. */ > > > > #include <time.h> > > +#include <kernel-features.h> > > #include <sysdep-cancel.h> > > #include <not-cancel.h> > > > > +int > > +__nanosleep_nocancel_time64 (const struct __timespec64 *requested_time, > > + struct __timespec64 *remaining) > > +{ > > +#ifdef __ASSUME_TIME64_SYSCALLS > > +# ifndef __NR_clock_nanosleep_time64 > > +# define __NR_clock_nanosleep_time64 __NR_clock_nanosleep > > +# endif > > + return INLINE_SYSCALL_CALL (clock_nanosleep_time64, CLOCK_REALTIME, 0, > > + requested_time, remaining); > > +#else > > +# ifdef __NR_clock_nanosleep_time64 > > + long int ret_64; > > + > > + ret_64 = INLINE_SYSCALL_CALL (clock_nanosleep_time64, CLOCK_REALTIME, 0, > > + requested_time, remaining); > > + > > + if (ret_64 == 0 || errno != ENOSYS) > > + return ret_64; > > +# endif /* __NR_clock_nanosleep_time64 */ > > + int ret; > > + struct timespec ts32, tr32; > > + > > + if (! in_time_t_range (requested_time->tv_sec)) > > + { > > + __set_errno (EOVERFLOW); > > + return -1; > > + } > > + > > + ts32 = valid_timespec64_to_timespec (*requested_time); > > + ret = INLINE_SYSCALL_CALL (nanosleep, &ts32, &tr32); > > + > > + if (ret == 0 || errno != ENOSYS) > > + *remaining = valid_timespec_to_timespec64 (tr32); > > + > > + return ret; > > +#endif /* __ASSUME_TIME64_SYSCALLS */ > > +} > > + > > +#if __TIMESIZE != 64 > > int > > __nanosleep_nocancel (const struct timespec *requested_time, > > - struct timespec *remaining) > > + struct timespec *remaining) > > { > > - return INLINE_SYSCALL_CALL (nanosleep, requested_time, remaining); > > + int ret; > > + struct __timespec64 treq64, trem64; > > + > > + treq64 = valid_timespec_to_timespec64 (*requested_time); > > + ret = __nanosleep_nocancel_time64 (&treq64, &trem64); > > + > > + if (ret == 0 || errno != ENOSYS) > > + { > > + if (! in_time_t_range (trem64.tv_sec)) > > + { > > + __set_errno (EOVERFLOW); > > + return -1; > > + } > > + > > + if (remaining) > > + *remaining = valid_timespec64_to_timespec (trem64); > > + } > > + > > + return ret; > > } > > +#endif > > + > > hidden_def (__nanosleep_nocancel) > > > > The __nanosleep_nocancel is just used priority mutex for pthread_mutex_timedlock, > and I am almost sure that we can replace the code path with __lll_clocklock_wait. > Something like the below. > > If you like I can sent it as a cleanup patch, since it would require some more > work to remove the __nanosleep_nocancel internal definitions as well. > > -- > > diff --git a/nptl/pthread_mutex_timedlock.c b/nptl/pthread_mutex_timedlock.c > index d6f0d9e31a..b9536ddb19 100644 > --- a/nptl/pthread_mutex_timedlock.c > +++ b/nptl/pthread_mutex_timedlock.c > @@ -25,6 +25,7 @@ > #include <atomic.h> > #include <lowlevellock.h> > #include <not-cancel.h> > +#include <futex-internal.h> > > #include <stap-probe.h> > > @@ -377,50 +378,29 @@ __pthread_mutex_clocklock_common (pthread_mutex_t *mutex, > int private = (robust > ? PTHREAD_ROBUST_MUTEX_PSHARED (mutex) > : PTHREAD_MUTEX_PSHARED (mutex)); > - INTERNAL_SYSCALL_DECL (__err); > - > - int e = INTERNAL_SYSCALL (futex, __err, 4, &mutex->__data.__lock, > - __lll_private_flag (FUTEX_LOCK_PI, > - private), 1, > - abstime); > - if (INTERNAL_SYSCALL_ERROR_P (e, __err)) > + int e = futex_lock_pi ((unsigned int *) &mutex->__data.__lock, 1, > + abstime, private); > + if (e == ETIMEDOUT) > { > - if (INTERNAL_SYSCALL_ERRNO (e, __err) == ETIMEDOUT) > - return ETIMEDOUT; > - > - if (INTERNAL_SYSCALL_ERRNO (e, __err) == ESRCH > - || INTERNAL_SYSCALL_ERRNO (e, __err) == EDEADLK) > + return ETIMEDOUT; > + } > + else if (e == ESRCH || e == EDEADLK) > + { > + assert (e != EDEADLK > + || (kind != PTHREAD_MUTEX_ERRORCHECK_NP > + && kind != PTHREAD_MUTEX_RECURSIVE_NP)); > + /* ESRCH can happen only for non-robust PI mutexes where > + the owner of the lock died. */ > + assert (e != ESRCH || !robust); > + > + /* Delay the thread until the timeout is reached. Then return > + ETIMEDOUT. */ > + do > { > - assert (INTERNAL_SYSCALL_ERRNO (e, __err) != EDEADLK > - || (kind != PTHREAD_MUTEX_ERRORCHECK_NP > - && kind != PTHREAD_MUTEX_RECURSIVE_NP)); > - /* ESRCH can happen only for non-robust PI mutexes where > - the owner of the lock died. */ > - assert (INTERNAL_SYSCALL_ERRNO (e, __err) != ESRCH > - || !robust); > - > - /* Delay the thread until the timeout is reached. > - Then return ETIMEDOUT. */ > - struct timespec reltime; > - struct timespec now; > - > - INTERNAL_SYSCALL (clock_gettime, __err, 2, clockid, > - &now); > - reltime.tv_sec = abstime->tv_sec - now.tv_sec; > - reltime.tv_nsec = abstime->tv_nsec - now.tv_nsec; > - if (reltime.tv_nsec < 0) > - { > - reltime.tv_nsec += 1000000000; > - --reltime.tv_sec; > - } > - if (reltime.tv_sec >= 0) > - while (__nanosleep_nocancel (&reltime, &reltime) != 0) > - continue; > - > - return ETIMEDOUT; > - } > - > - return INTERNAL_SYSCALL_ERRNO (e, __err); > + e = __lll_clocklock_wait (&(int){0}, CLOCK_REALTIME, > + abstime, private); > + } while (e != ETIMEDOUT); > + return e; > } > > oldval = mutex->__data.__lock; > diff --git a/sysdeps/unix/sysv/linux/futex-internal.h b/sysdeps/unix/sysv/linux/futex-internal.h > index 5a4f4ff818..1442df63a1 100644 > --- a/sysdeps/unix/sysv/linux/futex-internal.h > +++ b/sysdeps/unix/sysv/linux/futex-internal.h > @@ -252,4 +252,30 @@ futex_wake (unsigned int *futex_word, int processes_to_wake, int private) > } > } > > +static __always_inline int > +futex_lock_pi (unsigned int *futex_word, unsigned int expected, > + const struct timespec *abstime, int private) > +{ > + int err = lll_futex_timed_lock_pi (futex_word, expected, abstime, private); > + switch (err) > + { > + case 0: > + case -EAGAIN: > + case -EINTR: > + case -ETIMEDOUT: > + case -ESRCH: > + case -EDEADLK: > + 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 (); > + } > +} > + > #endif /* futex-internal.h */ > diff --git a/sysdeps/unix/sysv/linux/lowlevellock-futex.h b/sysdeps/unix/sysv/linux/lowlevellock-futex.h > index b423673ed4..5730eb2ba2 100644 > --- a/sysdeps/unix/sysv/linux/lowlevellock-futex.h > +++ b/sysdeps/unix/sysv/linux/lowlevellock-futex.h > @@ -127,6 +127,11 @@ > FUTEX_OP_CLEAR_WAKE_IF_GT_ONE) > > /* Priority Inheritance support. */ > +#define lll_futex_timed_lock_pi(futexp, val, reltime, private) \ > + lll_futex_syscall (4, futexp, \ > + __lll_private_flag (FUTEX_LOCK_PI, private), \ > + val, reltime) > + > #define lll_futex_wait_requeue_pi(futexp, val, mutex, private) \ > lll_futex_timed_wait_requeue_pi (futexp, val, NULL, 0, mutex, private) >
On 04/11/2019 15:16, Alistair Francis wrote: > On Tue, Oct 29, 2019 at 1:16 PM Adhemerval Zanella > <adhemerval.zanella@linaro.org> wrote: >> >> >> >> On 24/10/2019 20:41, Alistair Francis wrote: >> >> There is no need to replicate all the syscall logic, nanosleep can be implemented >> with __clock_nanosleep. You can do: >> >> int >> __nanosleep (const struct timespec *requested_time, >> struct timespec *remaining) >> { >> int ret = __clock_nanosleep (CLOCK_REALTIME, 0, requested_time, remaining); >> if (ret != 0) >> { >> __set_errno (-ret); >> return -1; >> } >> return ret; >> } > > This doesn't work as __clock_nanosleep() isn't avaliable in nptl so it > fails to compile. My v3 patch attempted to fix this, but that also > doesn't work and ends up squishing some patches together. For this case you will need to add __clock_nanosleep as a GLIBC_PRIVATE exported symbol from libc and add a hidden_proto/hidden_def for libc internal plt avoidance (as below). However, I agree with Joseph and Florian that for this case it would be better to move the symbol from libpthread to libc. I will send a patch to refactor it. diff --git a/sysdeps/unix/sysv/linux/Versions b/sysdeps/unix/sysv/linux/Versions index d385085c61..475abb5004 100644 --- a/sysdeps/unix/sysv/linux/Versions +++ b/sysdeps/unix/sysv/linux/Versions @@ -187,5 +187,6 @@ libc { __sigtimedwait; # functions used by nscd __netlink_assert_response; + __clock_nanosleep; } } diff --git a/sysdeps/unix/sysv/linux/clock_nanosleep.c b/sysdeps/unix/sysv/linux/clock_nanosleep.c index 1f240b8720..f3c6fd2d5f 100644 --- a/sysdeps/unix/sysv/linux/clock_nanosleep.c +++ b/sysdeps/unix/sysv/linux/clock_nanosleep.c @@ -42,7 +42,7 @@ __clock_nanosleep (clockid_t clock_id, int flags, const struct timespec *req, return (INTERNAL_SYSCALL_ERROR_P (r, err) ? INTERNAL_SYSCALL_ERRNO (r, err) : 0); } - +libc_hidden_def (__clock_nanosleep) versioned_symbol (libc, __clock_nanosleep, clock_nanosleep, GLIBC_2_17); /* clock_nanosleep moved to libc in version 2.17; old binaries may expect the symbol version it had in librt. */ diff --git a/sysdeps/unix/sysv/linux/nanosleep.c b/sysdeps/unix/sysv/linux/nanosleep.c index 6787909248..1e2481847b 100644 --- a/sysdeps/unix/sysv/linux/nanosleep.c +++ b/sysdeps/unix/sysv/linux/nanosleep.c @@ -25,7 +25,14 @@ int __nanosleep (const struct timespec *requested_time, struct timespec *remaining) { - return SYSCALL_CANCEL (nanosleep, requested_time, remaining); + //return SYSCALL_CANCEL (nanosleep, requested_time, remaining); + int ret = __clock_nanosleep (CLOCK_REALTIME, 0, requested_time, remaining); + if (ret != 0) + { + __set_errno (-ret); + return -1; + } + return ret; } hidden_def (__nanosleep) weak_alias (__nanosleep, nanosleep)
On Mon, Nov 4, 2019 at 11:30 AM Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote: > > > > On 04/11/2019 15:16, Alistair Francis wrote: > > On Tue, Oct 29, 2019 at 1:16 PM Adhemerval Zanella > > <adhemerval.zanella@linaro.org> wrote: > >> > >> > >> > >> On 24/10/2019 20:41, Alistair Francis wrote: > > >> > >> There is no need to replicate all the syscall logic, nanosleep can be implemented > >> with __clock_nanosleep. You can do: > >> > >> int > >> __nanosleep (const struct timespec *requested_time, > >> struct timespec *remaining) > >> { > >> int ret = __clock_nanosleep (CLOCK_REALTIME, 0, requested_time, remaining); > >> if (ret != 0) > >> { > >> __set_errno (-ret); > >> return -1; > >> } > >> return ret; > >> } > > > > This doesn't work as __clock_nanosleep() isn't avaliable in nptl so it > > fails to compile. My v3 patch attempted to fix this, but that also > > doesn't work and ends up squishing some patches together. > > For this case you will need to add __clock_nanosleep as a GLIBC_PRIVATE > exported symbol from libc and add a hidden_proto/hidden_def for libc > internal plt avoidance (as below). Thanks for explaining this. > > However, I agree with Joseph and Florian that for this case it would be > better to move the symbol from libpthread to libc. I will send a patch > to refactor it. I am going to include the diff below and convert __nanosleep to call __clock_nanosleep(), but I won't change the file structure or move anything else around. I'll leave that to you in a follow up patch. Alistair > > diff --git a/sysdeps/unix/sysv/linux/Versions b/sysdeps/unix/sysv/linux/Versions > index d385085c61..475abb5004 100644 > --- a/sysdeps/unix/sysv/linux/Versions > +++ b/sysdeps/unix/sysv/linux/Versions > @@ -187,5 +187,6 @@ libc { > __sigtimedwait; > # functions used by nscd > __netlink_assert_response; > + __clock_nanosleep; > } > } > diff --git a/sysdeps/unix/sysv/linux/clock_nanosleep.c b/sysdeps/unix/sysv/linux/clock_nanosleep.c > index 1f240b8720..f3c6fd2d5f 100644 > --- a/sysdeps/unix/sysv/linux/clock_nanosleep.c > +++ b/sysdeps/unix/sysv/linux/clock_nanosleep.c > @@ -42,7 +42,7 @@ __clock_nanosleep (clockid_t clock_id, int flags, const struct timespec *req, > return (INTERNAL_SYSCALL_ERROR_P (r, err) > ? INTERNAL_SYSCALL_ERRNO (r, err) : 0); > } > - > +libc_hidden_def (__clock_nanosleep) > versioned_symbol (libc, __clock_nanosleep, clock_nanosleep, GLIBC_2_17); > /* clock_nanosleep moved to libc in version 2.17; > old binaries may expect the symbol version it had in librt. */ > diff --git a/sysdeps/unix/sysv/linux/nanosleep.c b/sysdeps/unix/sysv/linux/nanosleep.c > index 6787909248..1e2481847b 100644 > --- a/sysdeps/unix/sysv/linux/nanosleep.c > +++ b/sysdeps/unix/sysv/linux/nanosleep.c > @@ -25,7 +25,14 @@ int > __nanosleep (const struct timespec *requested_time, > struct timespec *remaining) > { > - return SYSCALL_CANCEL (nanosleep, requested_time, remaining); > + //return SYSCALL_CANCEL (nanosleep, requested_time, remaining); > + int ret = __clock_nanosleep (CLOCK_REALTIME, 0, requested_time, remaining); > + if (ret != 0) > + { > + __set_errno (-ret); > + return -1; > + } > + return ret; > } > hidden_def (__nanosleep) > weak_alias (__nanosleep, nanosleep)
diff --git a/include/time.h b/include/time.h index d93b16a7810..0e703f87cef 100644 --- a/include/time.h +++ b/include/time.h @@ -174,6 +174,26 @@ libc_hidden_proto (__difftime64) extern double __difftime (time_t time1, time_t time0); +#if __TIMESIZE == 64 +# define __thrd_sleep_time64 thrd_sleep +# define __clock_nanosleep_time64 __clock_nanosleep +# define __nanosleep_time64 __nanosleep +# define __nanosleep_nocancel_time64 __nanosleep_nocancel +#else +extern int __thrd_sleep_time64 (const struct __timespec64* time_point, + struct __timespec64* remaining); +libc_hidden_proto (__thrd_sleep_time64) +extern int __clock_nanosleep_time64 (clockid_t clock_id, + int flags, const struct __timespec64 *req, + struct __timespec64 *rem); +libc_hidden_proto (__clock_nanosleep_time64) +extern int __nanosleep_time64 (const struct __timespec64 *requested_time, + struct __timespec64 *remaining); +libc_hidden_proto (__nanosleep_time64) +extern int __nanosleep_nocancel_time64 (const struct __timespec64 *requested_time, + struct __timespec64 *remaining); +libc_hidden_proto (__nanosleep_nocancel_time64) +#endif /* Use in the clock_* functions. Size of the field representing the actual clock ID. */ diff --git a/nptl/thrd_sleep.c b/nptl/thrd_sleep.c index 2e185dd748e..2a6bb1f9e3f 100644 --- a/nptl/thrd_sleep.c +++ b/nptl/thrd_sleep.c @@ -17,23 +17,85 @@ <https://www.gnu.org/licenses/>. */ #include <time.h> +#include <kernel-features.h> #include <sysdep-cancel.h> #include "thrd_priv.h" int -thrd_sleep (const struct timespec* time_point, struct timespec* remaining) +__thrd_sleep_time64 (const struct __timespec64* time_point, struct __timespec64* remaining) { INTERNAL_SYSCALL_DECL (err); - int ret = INTERNAL_SYSCALL_CANCEL (nanosleep, err, time_point, remaining); + int ret = -1; + +#ifdef __ASSUME_TIME64_SYSCALLS +# ifndef __NR_clock_nanosleep_time64 +# define __NR_clock_nanosleep_time64 __NR_clock_nanosleep +# endif + ret = INTERNAL_SYSCALL_CANCEL (clock_nanosleep_time64, err, CLOCK_REALTIME, + 0, time_point, remaining); +#else +# ifdef __NR_clock_nanosleep_time64 + long int ret_64; + + ret_64 = INTERNAL_SYSCALL_CANCEL (clock_nanosleep_time64, err, CLOCK_REALTIME, + 0, time_point, remaining); + + if (ret_64 == 0 || errno != ENOSYS) + ret = ret_64; +# endif /* __NR_clock_nanosleep_time64 */ + if (ret < 0) + { + struct timespec tp32, tr32; + + if (! in_time_t_range (time_point->tv_sec)) + { + __set_errno (EOVERFLOW); + return -1; + } + + tp32 = valid_timespec64_to_timespec (*time_point); + ret = INTERNAL_SYSCALL_CANCEL (nanosleep, err, &tp32, &tr32); + + if ((ret == 0 || errno != ENOSYS) && remaining) + *remaining = valid_timespec_to_timespec64 (tr32); + } +#endif /* __ASSUME_TIME64_SYSCALLS */ + if (INTERNAL_SYSCALL_ERROR_P (ret, err)) { /* C11 states thrd_sleep function returns -1 if it has been interrupted - by a signal, or a negative value if it fails. */ + by a signal, or a negative value if it fails. */ ret = INTERNAL_SYSCALL_ERRNO (ret, err); if (ret == EINTR) - return -1; + return -1; return -2; } return 0; } + +#if __TIMESIZE != 64 +int +thrd_sleep (const struct timespec* time_point, struct timespec* remaining) +{ + int ret; + struct __timespec64 tp64, tr64; + + tp64 = valid_timespec_to_timespec64 (*time_point); + ret = __thrd_sleep_time64 (&tp64, &tr64); + + if (ret == 0 || errno != ENOSYS) + { + if (! in_time_t_range (tr64.tv_sec)) + { + __set_errno (EOVERFLOW); + return -1; + } + + if (remaining) + *remaining = valid_timespec64_to_timespec (tr64); + } + + return ret; +} +#endif diff --git a/sysdeps/unix/sysv/linux/clock_nanosleep.c b/sysdeps/unix/sysv/linux/clock_nanosleep.c index 1f240b8720a..d257ea57fb0 100644 --- a/sysdeps/unix/sysv/linux/clock_nanosleep.c +++ b/sysdeps/unix/sysv/linux/clock_nanosleep.c @@ -16,6 +16,7 @@ <https://www.gnu.org/licenses/>. */ #include <time.h> +#include <kernel-features.h> #include <errno.h> #include <sysdep-cancel.h> @@ -26,9 +27,11 @@ /* We can simply use the syscall. The CPU clocks are not supported with this function. */ int -__clock_nanosleep (clockid_t clock_id, int flags, const struct timespec *req, - struct timespec *rem) +__clock_nanosleep_time64 (clockid_t clock_id, int flags, const struct __timespec64 *req, + struct __timespec64 *rem) { + int r = -1; + if (clock_id == CLOCK_THREAD_CPUTIME_ID) return EINVAL; if (clock_id == CLOCK_PROCESS_CPUTIME_ID) @@ -37,12 +40,72 @@ __clock_nanosleep (clockid_t clock_id, int flags, const struct timespec *req, /* If the call is interrupted by a signal handler or encounters an error, it returns a positive value similar to errno. */ INTERNAL_SYSCALL_DECL (err); - int r = INTERNAL_SYSCALL_CANCEL (clock_nanosleep, err, clock_id, flags, - req, rem); + +#ifdef __ASSUME_TIME64_SYSCALLS +# ifndef __NR_clock_nanosleep_time64 +# define __NR_clock_nanosleep_time64 __NR_clock_nanosleep +# endif + r = INTERNAL_SYSCALL_CANCEL (clock_nanosleep_time64, err, clock_id, + flags, req, rem); +#else +# ifdef __NR_clock_nanosleep_time64 + long int ret_64; + + ret_64 = INTERNAL_SYSCALL_CANCEL (clock_nanosleep_time64, err, clock_id, + flags, req, rem); + + if (ret_64 == 0 || errno != ENOSYS) + r = ret_64; +# endif /* __NR_clock_nanosleep_time64 */ + if (r < 0) + { + struct timespec ts32, tr32; + + if (! in_time_t_range (req->tv_sec)) + { + __set_errno (EOVERFLOW); + return -1; + } + + ts32 = valid_timespec64_to_timespec (*req); + r = INTERNAL_SYSCALL_CANCEL (clock_nanosleep, err, &ts32, &tr32); + + if ((r == 0 || errno != ENOSYS) && rem) + *rem = valid_timespec_to_timespec64 (tr32); + } +#endif /* __ASSUME_TIME64_SYSCALLS */ + return (INTERNAL_SYSCALL_ERROR_P (r, err) - ? INTERNAL_SYSCALL_ERRNO (r, err) : 0); + ? INTERNAL_SYSCALL_ERRNO (r, err) : 0); } +#if __TIMESIZE != 64 +int +__clock_nanosleep (clockid_t clock_id, int flags, const struct timespec *req, + struct timespec *rem) +{ + int r; + struct __timespec64 treq64, trem64; + + treq64 = valid_timespec_to_timespec64 (*req); + r = __clock_nanosleep_time64 (clock_id, flags, &treq64, &trem64); + + if (r == 0 || errno != ENOSYS) + { + if (! in_time_t_range (trem64.tv_sec)) + { + __set_errno (EOVERFLOW); + return -1; + } + + if (rem) + *rem = valid_timespec64_to_timespec (trem64); + } + + return r; +} +#endif + versioned_symbol (libc, __clock_nanosleep, clock_nanosleep, GLIBC_2_17); /* clock_nanosleep moved to libc in version 2.17; old binaries may expect the symbol version it had in librt. */ diff --git a/sysdeps/unix/sysv/linux/nanosleep.c b/sysdeps/unix/sysv/linux/nanosleep.c index 6787909248f..10a6d57a1de 100644 --- a/sysdeps/unix/sysv/linux/nanosleep.c +++ b/sysdeps/unix/sysv/linux/nanosleep.c @@ -17,15 +17,76 @@ <https://www.gnu.org/licenses/>. */ #include <time.h> +#include <kernel-features.h> #include <sysdep-cancel.h> #include <not-cancel.h> /* Pause execution for a number of nanoseconds. */ int +__nanosleep_time64 (const struct __timespec64 *requested_time, + struct __timespec64 *remaining) +{ +#if defined(__ASSUME_TIME64_SYSCALLS) +# ifndef __NR_clock_nanosleep_time64 +# define __NR_clock_nanosleep_time64 __NR_clock_nanosleep +# endif + return SYSCALL_CANCEL (clock_nanosleep_time64, CLOCK_REALTIME, 0, + requested_time, remaining); +#else +# ifdef __NR_clock_nanosleep_time64 + long int ret_64; + + ret_64 = SYSCALL_CANCEL (clock_nanosleep_time64, CLOCK_REALTIME, 0, + requested_time, remaining); + + if (ret_64 == 0 || errno != ENOSYS) + return ret_64; +# endif /* __NR_clock_nanosleep_time64 */ + int ret; + struct timespec ts32, tr32; + + if (! in_time_t_range (requested_time->tv_sec)) + { + __set_errno (EOVERFLOW); + return -1; + } + + ts32 = valid_timespec64_to_timespec (*requested_time); + ret = SYSCALL_CANCEL (nanosleep, &ts32, &tr32); + + if ((ret == 0 || errno != ENOSYS) && remaining) + *remaining = valid_timespec_to_timespec64 (tr32); + + return ret; +#endif /* __ASSUME_TIME64_SYSCALLS */ +} + +#if __TIMESIZE != 64 +int __nanosleep (const struct timespec *requested_time, - struct timespec *remaining) + struct timespec *remaining) { - return SYSCALL_CANCEL (nanosleep, requested_time, remaining); + int r; + struct __timespec64 treq64, trem64; + + treq64 = valid_timespec_to_timespec64 (*requested_time); + r = __nanosleep_time64 (&treq64, &trem64); + + if (r == 0 || errno != ENOSYS) + { + if (! in_time_t_range (trem64.tv_sec)) + { + __set_errno (EOVERFLOW); + return -1; + } + + if (remaining) + *remaining = valid_timespec64_to_timespec (trem64); + } + + return r; } +#endif + hidden_def (__nanosleep) weak_alias (__nanosleep, nanosleep) diff --git a/sysdeps/unix/sysv/linux/nanosleep_nocancel.c b/sysdeps/unix/sysv/linux/nanosleep_nocancel.c index d6442bf4f7f..1a6c6c0a48a 100644 --- a/sysdeps/unix/sysv/linux/nanosleep_nocancel.c +++ b/sysdeps/unix/sysv/linux/nanosleep_nocancel.c @@ -17,13 +17,74 @@ <https://www.gnu.org/licenses/>. */ #include <time.h> +#include <kernel-features.h> #include <sysdep-cancel.h> #include <not-cancel.h> +int +__nanosleep_nocancel_time64 (const struct __timespec64 *requested_time, + struct __timespec64 *remaining) +{ +#ifdef __ASSUME_TIME64_SYSCALLS +# ifndef __NR_clock_nanosleep_time64 +# define __NR_clock_nanosleep_time64 __NR_clock_nanosleep +# endif + return INLINE_SYSCALL_CALL (clock_nanosleep_time64, CLOCK_REALTIME, 0, + requested_time, remaining); +#else +# ifdef __NR_clock_nanosleep_time64 + long int ret_64; + + ret_64 = INLINE_SYSCALL_CALL (clock_nanosleep_time64, CLOCK_REALTIME, 0, + requested_time, remaining); + + if (ret_64 == 0 || errno != ENOSYS) + return ret_64; +# endif /* __NR_clock_nanosleep_time64 */ + int ret; + struct timespec ts32, tr32; + + if (! in_time_t_range (requested_time->tv_sec)) + { + __set_errno (EOVERFLOW); + return -1; + } + + ts32 = valid_timespec64_to_timespec (*requested_time); + ret = INLINE_SYSCALL_CALL (nanosleep, &ts32, &tr32); + + if (ret == 0 || errno != ENOSYS) + *remaining = valid_timespec_to_timespec64 (tr32); + + return ret; +#endif /* __ASSUME_TIME64_SYSCALLS */ +} + +#if __TIMESIZE != 64 int __nanosleep_nocancel (const struct timespec *requested_time, - struct timespec *remaining) + struct timespec *remaining) { - return INLINE_SYSCALL_CALL (nanosleep, requested_time, remaining); + int ret; + struct __timespec64 treq64, trem64; + + treq64 = valid_timespec_to_timespec64 (*requested_time); + ret = __nanosleep_nocancel_time64 (&treq64, &trem64); + + if (ret == 0 || errno != ENOSYS) + { + if (! in_time_t_range (trem64.tv_sec)) + { + __set_errno (EOVERFLOW); + return -1; + } + + if (remaining) + *remaining = valid_timespec64_to_timespec (trem64); + } + + return ret; } +#endif + hidden_def (__nanosleep_nocancel)