[v4] sysdeps/nanosleep: Use clock_nanosleep_time64 if avaliable

  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()"

Let's also expose __clock_nanosleep internally to avoid code
duplication.
---
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.

v4:
 - Rebase on master
 - Use __clock_nanosleep to avoid duplicate implementations
 - Fix the error handling when a syscall fails
v2:
 - Explicitly include `#include <kernel-features.h>`

 include/time.h                            | 23 ++++++++
 nptl/thrd_sleep.c                         | 25 ++++-----
 sysdeps/unix/sysv/linux/Versions          |  1 +
 sysdeps/unix/sysv/linux/clock_nanosleep.c | 68 +++++++++++++++++++++--
 sysdeps/unix/sysv/linux/nanosleep.c       | 13 +++--
 5 files changed, 106 insertions(+), 24 deletions(-)
  

Hi Alistair, Joseph,

> 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()"
> 
> Let's also expose __clock_nanosleep internally to avoid code
> duplication.
> ---
> 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.
> 
> v4:
>  - Rebase on master
>  - Use __clock_nanosleep to avoid duplicate implementations
>  - Fix the error handling when a syscall fails
> v2:
>  - Explicitly include `#include <kernel-features.h>`
> 
>  include/time.h                            | 23 ++++++++
>  nptl/thrd_sleep.c                         | 25 ++++-----
>  sysdeps/unix/sysv/linux/Versions          |  1 +
>  sysdeps/unix/sysv/linux/clock_nanosleep.c | 68
> +++++++++++++++++++++-- sysdeps/unix/sysv/linux/nanosleep.c       |
> 13 +++-- 5 files changed, 106 insertions(+), 24 deletions(-)
> 
> diff --git a/include/time.h b/include/time.h
> index 8ac58e891b5..e0974309726 100644
> --- a/include/time.h
> +++ b/include/time.h
> @@ -184,6 +184,9 @@ extern struct tm *__tz_convert (__time64_t timer,
> int use_localtime, extern int __nanosleep (const struct timespec
> *__requested_time, struct timespec *__remaining);
>  hidden_proto (__nanosleep)
> +extern int __clock_nanosleep (clockid_t clock_id, int flags,
> +                              const struct timespec *req, struct
> timespec *rem); +libc_hidden_proto (__clock_nanosleep)
>  extern int __getdate_r (const char *__string, struct tm *__resbufp)
>    attribute_hidden;
>  
> @@ -206,6 +209,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..c865f815eea 100644
> --- a/nptl/thrd_sleep.c
> +++ b/nptl/thrd_sleep.c
> @@ -17,23 +17,18 @@
>     <https://www.gnu.org/licenses/>.  */
>  
>  #include <time.h>
> -#include <sysdep-cancel.h>
> -
> -#include "thrd_priv.h"
> +#include <errno.h>
>  
>  int
>  thrd_sleep (const struct timespec* time_point, struct timespec*
> remaining) {
> -  INTERNAL_SYSCALL_DECL (err);
> -  int ret = INTERNAL_SYSCALL_CANCEL (nanosleep, err, time_point,
> remaining);
> -  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.  */
> -      ret = INTERNAL_SYSCALL_ERRNO (ret, err);
> -      if (ret == EINTR)
> -	return -1;
> -      return -2;
> -    }
> -  return 0;
> +  int ret = __clock_nanosleep (CLOCK_REALTIME, 0, time_point,
> remaining);

Maybe this is a bit off topic (generic question to 64 bit time support)
- but I'm wondering if we shall not use __clock_nanosleep_time64 here
just from the outset? 

With using __clock_nanosleep() as the internal function, the
architectures with __TIMESIZE != 64 (e.g. ARM32 bit) will not benefit
from having internal glibc support for 64 bit time representation (as
their __clock_nanosleep version uses 32 bit to represent time).

Of course we can stick to __clock_nanosleep, but then those functions
would need to be aliased or (better) renamed in the code before Y2038.

The same situation is with e.g. internal usage of __clock_settime.

> +  /* 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;
> +  }
>  }
> diff --git a/sysdeps/unix/sysv/linux/Versions
> b/sysdeps/unix/sysv/linux/Versions index d385085c611..475abb50040
> 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
> 1f240b8720a..13ea9c6f802 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
   ^^^^^^^^^^^^^^^^^ - just an open question - shall we have the naming
   convention (for this function) following the new syscalls names or
   use e.g. __clock_nanosleep64() ?

> 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,67 @@ __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;

I think that 'int ret;' would be a bit better.

the clock_nanosleep syscall [1] returns int.

> +
> +  ret_64 = INTERNAL_SYSCALL_CANCEL (clock_nanosleep_time64, err,
> clock_id,
> +                                    flags, req, rem);
> +
> +  if (ret_64 == 0 || errno != ENOSYS)
> +    {
> +      return (INTERNAL_SYSCALL_ERROR_P (ret_64, err)
> +              ? INTERNAL_SYSCALL_ERRNO (ret_64, err) : 0);
> +    }
> +# endif /* __NR_clock_nanosleep_time64 */
> +  struct timespec ts32, tr32;
> +
> +  if (! in_time_t_range (req->tv_sec))
> +    {
> +      __set_errno (EOVERFLOW);
> +      return -1;
> +    }
> +
> +  ts32 = valid_timespec64_to_timespec (*req);

The man pages for clock_nanosleep (e.g. [2], [3]) do not mention about
the possibility to have *req equal to NULL, hence I do assume that we
don't need to do it here as well...

> +  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 (rem)
> +        *rem = valid_timespec64_to_timespec (trem64);
> +    }
> +
> +  return r;
> +}
> +#endif
> +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 6787909248f..30cc26909a7
> 100644 --- a/sysdeps/unix/sysv/linux/nanosleep.c
> +++ b/sysdeps/unix/sysv/linux/nanosleep.c
> @@ -17,15 +17,20 @@
>     <https://www.gnu.org/licenses/>.  */
>  
>  #include <time.h>
> -#include <sysdep-cancel.h>
> -#include <not-cancel.h>
> +#include <errno.h>
>  
>  /* Pause execution for a number of nanoseconds.  */
>  int
>  __nanosleep (const struct timespec *requested_time,
> -	     struct timespec *remaining)
> +             struct timespec *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)


Links:

[1] -
https://elixir.bootlin.com/linux/v5.4-rc5/source/kernel/time/posix-timers.c#L1230

[2] - http://man7.org/linux/man-pages/man2/nanosleep.2.html

[3] - https://linux.die.net/man/3/clock_nanosleep

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
  

On 06/11/2019 07:11, Lukasz Majewski wrote:
>> diff --git a/nptl/thrd_sleep.c b/nptl/thrd_sleep.c
>> index 2e185dd748e..c865f815eea 100644
>> --- a/nptl/thrd_sleep.c
>> +++ b/nptl/thrd_sleep.c
>> @@ -17,23 +17,18 @@
>>     <https://www.gnu.org/licenses/>.  */
>>  
>>  #include <time.h>
>> -#include <sysdep-cancel.h>
>> -
>> -#include "thrd_priv.h"
>> +#include <errno.h>
>>  
>>  int
>>  thrd_sleep (const struct timespec* time_point, struct timespec*
>> remaining) {
>> -  INTERNAL_SYSCALL_DECL (err);
>> -  int ret = INTERNAL_SYSCALL_CANCEL (nanosleep, err, time_point,
>> remaining);
>> -  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.  */
>> -      ret = INTERNAL_SYSCALL_ERRNO (ret, err);
>> -      if (ret == EINTR)
>> -	return -1;
>> -      return -2;
>> -    }
>> -  return 0;
>> +  int ret = __clock_nanosleep (CLOCK_REALTIME, 0, time_point,
>> remaining);
> 
> Maybe this is a bit off topic (generic question to 64 bit time support)
> - but I'm wondering if we shall not use __clock_nanosleep_time64 here
> just from the outset? 
> 
> With using __clock_nanosleep() as the internal function, the
> architectures with __TIMESIZE != 64 (e.g. ARM32 bit) will not benefit
> from having internal glibc support for 64 bit time representation (as
> their __clock_nanosleep version uses 32 bit to represent time).
> 
> Of course we can stick to __clock_nanosleep, but then those functions
> would need to be aliased or (better) renamed in the code before Y2038.
> 
> The same situation is with e.g. internal usage of __clock_settime.
> 
The C11 thrd_sleep would require a time64 support using the same
strategy as the other symbols, i.e, adding a thrd_sleep_time64 for
time64 enabled architectures and refactor thrd_sleep in terms of
thrd_sleep_time64.
  

On 05/11/2019 20:40, 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()"
> 
> Let's also expose __clock_nanosleep internally to avoid code
> duplication.
> ---
> 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.
> 
> v4:
>  - Rebase on master
>  - Use __clock_nanosleep to avoid duplicate implementations
>  - Fix the error handling when a syscall fails

I sent a nanosleep/thrd_sleep refactor that should simplify this code
with advantage it consolidates more symbols on libc.so (instead of
exporting more through GLIBC_PRIVATE).
  

On Wed, Nov 6, 2019 at 2:11 AM Lukasz Majewski <lukma@denx.de> wrote:
>
> Hi Alistair, Joseph,
>
> > 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()"
> >
> > Let's also expose __clock_nanosleep internally to avoid code
> > duplication.
> > ---
> > 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.
> >
> > v4:
> >  - Rebase on master
> >  - Use __clock_nanosleep to avoid duplicate implementations
> >  - Fix the error handling when a syscall fails
> > v2:
> >  - Explicitly include `#include <kernel-features.h>`
> >
> >  include/time.h                            | 23 ++++++++
> >  nptl/thrd_sleep.c                         | 25 ++++-----
> >  sysdeps/unix/sysv/linux/Versions          |  1 +
> >  sysdeps/unix/sysv/linux/clock_nanosleep.c | 68
> > +++++++++++++++++++++-- sysdeps/unix/sysv/linux/nanosleep.c       |
> > 13 +++-- 5 files changed, 106 insertions(+), 24 deletions(-)
> >
> > diff --git a/include/time.h b/include/time.h
> > index 8ac58e891b5..e0974309726 100644
> > --- a/include/time.h
> > +++ b/include/time.h
> > @@ -184,6 +184,9 @@ extern struct tm *__tz_convert (__time64_t timer,
> > int use_localtime, extern int __nanosleep (const struct timespec
> > *__requested_time, struct timespec *__remaining);
> >  hidden_proto (__nanosleep)
> > +extern int __clock_nanosleep (clockid_t clock_id, int flags,
> > +                              const struct timespec *req, struct
> > timespec *rem); +libc_hidden_proto (__clock_nanosleep)
> >  extern int __getdate_r (const char *__string, struct tm *__resbufp)
> >    attribute_hidden;
> >
> > @@ -206,6 +209,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..c865f815eea 100644
> > --- a/nptl/thrd_sleep.c
> > +++ b/nptl/thrd_sleep.c
> > @@ -17,23 +17,18 @@
> >     <https://www.gnu.org/licenses/>.  */
> >
> >  #include <time.h>
> > -#include <sysdep-cancel.h>
> > -
> > -#include "thrd_priv.h"
> > +#include <errno.h>
> >
> >  int
> >  thrd_sleep (const struct timespec* time_point, struct timespec*
> > remaining) {
> > -  INTERNAL_SYSCALL_DECL (err);
> > -  int ret = INTERNAL_SYSCALL_CANCEL (nanosleep, err, time_point,
> > remaining);
> > -  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.  */
> > -      ret = INTERNAL_SYSCALL_ERRNO (ret, err);
> > -      if (ret == EINTR)
> > -     return -1;
> > -      return -2;
> > -    }
> > -  return 0;
> > +  int ret = __clock_nanosleep (CLOCK_REALTIME, 0, time_point,
> > remaining);
>
> Maybe this is a bit off topic (generic question to 64 bit time support)
> - but I'm wondering if we shall not use __clock_nanosleep_time64 here
> just from the outset?
>
> With using __clock_nanosleep() as the internal function, the
> architectures with __TIMESIZE != 64 (e.g. ARM32 bit) will not benefit
> from having internal glibc support for 64 bit time representation (as
> their __clock_nanosleep version uses 32 bit to represent time).
>
> Of course we can stick to __clock_nanosleep, but then those functions
> would need to be aliased or (better) renamed in the code before Y2038.
>
> The same situation is with e.g. internal usage of __clock_settime.
>
> > +  /* 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;
> > +  }
> >  }
> > diff --git a/sysdeps/unix/sysv/linux/Versions
> > b/sysdeps/unix/sysv/linux/Versions index d385085c611..475abb50040
> > 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
> > 1f240b8720a..13ea9c6f802 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
>    ^^^^^^^^^^^^^^^^^ - just an open question - shall we have the naming
>    convention (for this function) following the new syscalls names or
>    use e.g. __clock_nanosleep64() ?
>
> > 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,67 @@ __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;
>
> I think that 'int ret;' would be a bit better.
>
> the clock_nanosleep syscall [1] returns int.

I have changed this to int.

>
> > +
> > +  ret_64 = INTERNAL_SYSCALL_CANCEL (clock_nanosleep_time64, err,
> > clock_id,
> > +                                    flags, req, rem);
> > +
> > +  if (ret_64 == 0 || errno != ENOSYS)
> > +    {
> > +      return (INTERNAL_SYSCALL_ERROR_P (ret_64, err)
> > +              ? INTERNAL_SYSCALL_ERRNO (ret_64, err) : 0);
> > +    }
> > +# endif /* __NR_clock_nanosleep_time64 */
> > +  struct timespec ts32, tr32;
> > +
> > +  if (! in_time_t_range (req->tv_sec))
> > +    {
> > +      __set_errno (EOVERFLOW);
> > +      return -1;
> > +    }
> > +
> > +  ts32 = valid_timespec64_to_timespec (*req);
>
> The man pages for clock_nanosleep (e.g. [2], [3]) do not mention about
> the possibility to have *req equal to NULL, hence I do assume that we
> don't need to do it here as well...

I really don't like accessing a pointer without first checking it.
__clock_nanosleep is going to be called from inside glibc so I think
it makes sense to keep the check in.

If people prefer that I remove it I can.

>
> > +  r =  INTERNAL_SYSCALL_CANCEL (clock_nanosleep, err, &ts32, &tr32);

^

This line is broken. I am not passing in flags or clock_id and it
causes test failures. I will re-test and send a new version.

Alistair

> > +
> > +  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 (rem)
> > +        *rem = valid_timespec64_to_timespec (trem64);
> > +    }
> > +
> > +  return r;
> > +}
> > +#endif
> > +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 6787909248f..30cc26909a7
> > 100644 --- a/sysdeps/unix/sysv/linux/nanosleep.c
> > +++ b/sysdeps/unix/sysv/linux/nanosleep.c
> > @@ -17,15 +17,20 @@
> >     <https://www.gnu.org/licenses/>.  */
> >
> >  #include <time.h>
> > -#include <sysdep-cancel.h>
> > -#include <not-cancel.h>
> > +#include <errno.h>
> >
> >  /* Pause execution for a number of nanoseconds.  */
> >  int
> >  __nanosleep (const struct timespec *requested_time,
> > -          struct timespec *remaining)
> > +             struct timespec *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)
>
>
> Links:
>
> [1] -
> https://elixir.bootlin.com/linux/v5.4-rc5/source/kernel/time/posix-timers.c#L1230
>
> [2] - http://man7.org/linux/man-pages/man2/nanosleep.2.html
>
> [3] - https://linux.die.net/man/3/clock_nanosleep
>
> 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
  

On 11/6/19 10:01 AM, Alistair Francis wrote:
>>> +  ts32 = valid_timespec64_to_timespec (*req);
>> The man pages for clock_nanosleep (e.g. [2], [3]) do not mention about
>> the possibility to have *req equal to NULL, hence I do assume that we
>> don't need to do it here as well...
> I really don't like accessing a pointer without first checking it.
> __clock_nanosleep is going to be called from inside glibc so I think
> it makes sense to keep the check in.

Sorry, I'm not understanding this part of the discussion. req is a 
pointer to a structure, so I assumed that "*req equal to NULL" was 
intended to read "req equal to NULL" and Lukasz was commenting that he 
was assuming that the code was correct as-is and does not need a "req != 
NULL" test. But your reply makes it sound like there's already such a 
test, which is not something that I see in the proposed patch.

As I understand it, the glibc tradition generally is as Lukasz 
suggested, in that if there's no requirement to check for null pointers 
we generally don't add one. (Of course there are exceptions for 
historical reasons etc.)
  

On Wed, Nov 6, 2019 at 10:57 AM Paul Eggert <eggert@cs.ucla.edu> wrote:
>
> On 11/6/19 10:01 AM, Alistair Francis wrote:
> >>> +  ts32 = valid_timespec64_to_timespec (*req);
> >> The man pages for clock_nanosleep (e.g. [2], [3]) do not mention about
> >> the possibility to have *req equal to NULL, hence I do assume that we
> >> don't need to do it here as well...
> > I really don't like accessing a pointer without first checking it.
> > __clock_nanosleep is going to be called from inside glibc so I think
> > it makes sense to keep the check in.
>
> Sorry, I'm not understanding this part of the discussion. req is a
> pointer to a structure, so I assumed that "*req equal to NULL" was
> intended to read "req equal to NULL" and Lukasz was commenting that he

Yes, I think we both mean "req equal to NULL"

> was assuming that the code was correct as-is and does not need a "req !=
> NULL" test. But your reply makes it sound like there's already such a
> test, which is not something that I see in the proposed patch.

You are correct, I got confused between req and rem.

I am happy leaving no checks for req as it should always be set, my
worry was removing the check for rem, which Lukasz wasn't talking
about. I am happy with the checking as is and it sounds like others
are as well. Sorry for the confusion here.

>
> As I understand it, the glibc tradition generally is as Lukasz
> suggested, in that if there's no requirement to check for null pointers
> we generally don't add one. (Of course there are exceptions for
> historical reasons etc.)

Understood!

Alistair