Patchwork [RFC,v4,06/24] sysdeps/timespec_get: Use clock_gettime64 if avaliable

login
register
mail settings
Submitter Alistair Francis
Date Aug. 10, 2019, 1 a.m.
Message ID <3ee6c1e52cbefe6f6dbd7aef423f13607ff50402.1565398513.git.alistair.francis@wdc.com>
Download mbox | patch
Permalink /patch/34033/
State New
Headers show

Comments

Alistair Francis - Aug. 10, 2019, 1 a.m.
Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
 sysdeps/unix/sysv/linux/timespec_get.c | 56 +++++++++++++++++++++++++-
 1 file changed, 54 insertions(+), 2 deletions(-)
Joseph Myers - Aug. 12, 2019, 7:46 p.m.
On Fri, 9 Aug 2019, Alistair Francis wrote:

> diff --git a/sysdeps/unix/sysv/linux/timespec_get.c b/sysdeps/unix/sysv/linux/timespec_get.c
> index 52080ddf08a..97ef9c5f799 100644
> --- a/sysdeps/unix/sysv/linux/timespec_get.c
> +++ b/sysdeps/unix/sysv/linux/timespec_get.c
> @@ -24,6 +24,58 @@
>  #endif
>  #include <sysdep-vdso.h>
>  
> +int
> +__timespec_get (struct timespec *ts, int base)
> +{
> +#ifdef __ASSUME_TIME64_SYSCALLS
> +  return INTERNAL_VSYSCALL (clock_gettime64, err, 2, CLOCK_REALTIME, ts);
> +#else

This has the usual problems with missing conversions.
Alistair Francis - Aug. 15, 2019, 5:59 p.m.
On Mon, Aug 12, 2019 at 12:46 PM Joseph Myers <joseph@codesourcery.com> wrote:
>
> On Fri, 9 Aug 2019, Alistair Francis wrote:
>
> > diff --git a/sysdeps/unix/sysv/linux/timespec_get.c b/sysdeps/unix/sysv/linux/timespec_get.c
> > index 52080ddf08a..97ef9c5f799 100644
> > --- a/sysdeps/unix/sysv/linux/timespec_get.c
> > +++ b/sysdeps/unix/sysv/linux/timespec_get.c
> > @@ -24,6 +24,58 @@
> >  #endif
> >  #include <sysdep-vdso.h>
> >
> > +int
> > +__timespec_get (struct timespec *ts, int base)
> > +{
> > +#ifdef __ASSUME_TIME64_SYSCALLS
> > +  return INTERNAL_VSYSCALL (clock_gettime64, err, 2, CLOCK_REALTIME, ts);
> > +#else
>
> This has the usual problems with missing conversions.

Is this the type of layout you are looking for? (untested, just a general idea)

int
__timespec_get64 (struct __timespec64 *ts, int base)
{
#ifdef __ASSUME_TIME64_SYSCALLS
  return INTERNAL_VSYSCALL (clock_gettime64, err, 2, CLOCK_REALTIME, ts);
#else
  long int ret;
# ifdef __NR_clock_gettime64
  ret = INTERNAL_VSYSCALL (clock_gettime64, err, 2, CLOCK_REALTIME, ts);

  if (ret == 0 || errno != ENOSYS)
    {
      return ret;
    }
# endif /* __NR_clock_gettime64 */
  struct timespec ts32;

  if (! in_time_t_range (ts->tv_sec))
    {
      __set_errno (EOVERFLOW);
      return -1;
    }

  valid_timespec64_to_timespec (ts, &ts32);
  ret = INTERNAL_VSYSCALL (clock_gettime, err, 2, CLOCK_REALTIME, &ts32);

  if ((ret == 0 || errno != ENOSYS) && ts)
    {
      ts->tv_sec = ts32.tv_sec;
      ts->tv_nsec = ts32.tv_nsec;
    }
  return ret;
#endif
}

#if __TIMESIZE != 64
int
__timespec_get32 (struct timespec *ts, int base)
{
  struct __timespec64 ts64;

  ret = __timespec_get64 (ts64, base);

  if ((ret == 0 || errno != ENOSYS) && ts)
    {
      ts->tv_sec = ts64.tv_sec;
      ts->tv_nsec = ts64.tv_nsec;
    }

  return ret;
}
#endif

#if __TIMESIZE == 64
# define __timespec_get __timespec_get64
#else
# define __timespec_get __timespec_get32
#endif

/* Set TS to calendar time based in time base BASE.  */
int
timespec_get (struct timespec *ts, int base)
{
  switch (base)
    {
      int res;
      INTERNAL_SYSCALL_DECL (err);
    case TIME_UTC:
      res = __timespec_get (ts, base);
      if (INTERNAL_SYSCALL_ERROR_P (res, err))
        return 0;
      break;

    default:
      return 0;
    }

  return base;
}

Alistair

>
> --
> Joseph S. Myers
> joseph@codesourcery.com
Joseph Myers - Aug. 15, 2019, 7:46 p.m.
On Thu, 15 Aug 2019, Alistair Francis wrote:

> Is this the type of layout you are looking for? (untested, just a general idea)

I'm not convinced making timespec_get into a wrapper round another 
function is a good thing.

You need to end up, in the __TIMESIZE == 32 case, with two functions that 
have the public timespec_get semantics - not two internal functions and a 
single wrapper - in preparation for when we support _TIME_BITS == 64.  So 
I'd expect defining __timespec_get64 as a function with the public 
semantics (not as an internal function that only calls the syscall), and 
conditionally defining timespec_get as a thin wrapper in the case of 
32-bit time_t, and having a #define of __timespec_get64 to timespec_get in 
an internal header in the case of 64-bit time_t.

> int
> __timespec_get64 (struct __timespec64 *ts, int base)
> {
> #ifdef __ASSUME_TIME64_SYSCALLS
>   return INTERNAL_VSYSCALL (clock_gettime64, err, 2, CLOCK_REALTIME, ts);
> #else

You need

# ifndef __NR_clock_gettime64
#  define __NR_clock_gettime64 __NR_clock_gettime
# endif

here, because 64-bit platforms define __ASSUME_TIME64_SYSCALLS but with 
unsuffixed syscall names.

>   long int ret;
> # ifdef __NR_clock_gettime64
>   ret = INTERNAL_VSYSCALL (clock_gettime64, err, 2, CLOCK_REALTIME, ts);
> 
>   if (ret == 0 || errno != ENOSYS)
>     {
>       return ret;
>     }

You need to check INTERNAL_SYSCALL_ERRNO (...) not errno; errno isn't set 
by INTERNAL_*, only by INLINE_*.

No braces when only a single statement is inside them.

>   ret = INTERNAL_VSYSCALL (clock_gettime, err, 2, CLOCK_REALTIME, &ts32);
> 
>   if ((ret == 0 || errno != ENOSYS) && ts)

No need to consider ENOSYS here.  NULL ts isn't valid so no need to check 
for it being non-NULL.

> /* Set TS to calendar time based in time base BASE.  */
> int
> timespec_get (struct timespec *ts, int base)
> {
>   switch (base)
>     {
>       int res;
>       INTERNAL_SYSCALL_DECL (err);
>     case TIME_UTC:
>       res = __timespec_get (ts, base);
>       if (INTERNAL_SYSCALL_ERROR_P (res, err))
>         return 0;

This wrapper is using an uninitialized variable err.
Alistair Francis - Aug. 15, 2019, 8:48 p.m.
On Thu, Aug 15, 2019 at 12:46 PM Joseph Myers <joseph@codesourcery.com> wrote:
>
> On Thu, 15 Aug 2019, Alistair Francis wrote:
>
> > Is this the type of layout you are looking for? (untested, just a general idea)
>
> I'm not convinced making timespec_get into a wrapper round another
> function is a good thing.
>
> You need to end up, in the __TIMESIZE == 32 case, with two functions that
> have the public timespec_get semantics - not two internal functions and a
> single wrapper - in preparation for when we support _TIME_BITS == 64.  So
> I'd expect defining __timespec_get64 as a function with the public
> semantics (not as an internal function that only calls the syscall), and
> conditionally defining timespec_get as a thin wrapper in the case of
> 32-bit time_t, and having a #define of __timespec_get64 to timespec_get in
> an internal header in the case of 64-bit time_t.

Ok, so more like this?

#if __TIMESIZE == 64
# define timespec_get __timespec_get64
#else
# define timespec_get __timespec_get32
#endif

/* Set TS to calendar time based in time base BASE.  */
int
__timespec_get64 (struct timespec *ts, int base)
{
  switch (base)
    {
      int res;
      INTERNAL_SYSCALL_DECL (err);
    case TIME_UTC:
#ifdef __ASSUME_TIME64_SYSCALLS
  res = INTERNAL_VSYSCALL (clock_gettime64, err, 2, CLOCK_REALTIME, ts);
#else
# ifdef __NR_clock_gettime64
  res = INTERNAL_VSYSCALL (clock_gettime64, err, 2, CLOCK_REALTIME, ts);
# endif /* __NR_clock_gettime64 */
  struct timespec ts32;

  if (! in_time_t_range (ts->tv_sec))
    {
      __set_errno (EOVERFLOW);
      return -1;
    }

  valid_timespec64_to_timespec (ts, &ts32);
  res = INTERNAL_VSYSCALL (clock_gettime, err, 2, CLOCK_REALTIME, &ts32);

  if (res == 0 || !INTERNAL_SYSCALL_ERROR_P (res, err))
    {
      ts->tv_sec = ts32.tv_sec;
      ts->tv_nsec = ts32.tv_nsec;
    }
#endif

      if (INTERNAL_SYSCALL_ERROR_P (res, err))
        return 0;
      break;

    default:
      return 0;
    }

  return base;
}

#if __TIMESIZE != 64
int
__timespec_get32 (struct timespec *ts, int base)
{
  struct __timespec64 ts64;

  ret = __timespec_get64 (ts64, base);

  if (res == 0 || !INTERNAL_SYSCALL_ERROR_P (res, err))
    {
      ts->tv_sec = ts64.tv_sec;
      ts->tv_nsec = ts64.tv_nsec;
    }

  return ret;
}
#endif

>
> > int
> > __timespec_get64 (struct __timespec64 *ts, int base)
> > {
> > #ifdef __ASSUME_TIME64_SYSCALLS
> >   return INTERNAL_VSYSCALL (clock_gettime64, err, 2, CLOCK_REALTIME, ts);
> > #else
>
> You need
>
> # ifndef __NR_clock_gettime64
> #  define __NR_clock_gettime64 __NR_clock_gettime
> # endif
>
> here, because 64-bit platforms define __ASSUME_TIME64_SYSCALLS but with
> unsuffixed syscall names.

The kernel defines 64 suffixed syscalls, is this required because
older kernels don't do this?

>
> >   long int ret;
> > # ifdef __NR_clock_gettime64
> >   ret = INTERNAL_VSYSCALL (clock_gettime64, err, 2, CLOCK_REALTIME, ts);
> >
> >   if (ret == 0 || errno != ENOSYS)
> >     {
> >       return ret;
> >     }
>
> You need to check INTERNAL_SYSCALL_ERRNO (...) not errno; errno isn't set
> by INTERNAL_*, only by INLINE_*.

Fixed! Thanks

>
> No braces when only a single statement is inside them.
>
> >   ret = INTERNAL_VSYSCALL (clock_gettime, err, 2, CLOCK_REALTIME, &ts32);
> >
> >   if ((ret == 0 || errno != ENOSYS) && ts)
>
> No need to consider ENOSYS here.  NULL ts isn't valid so no need to check
> for it being non-NULL.

Fixed

>
> > /* Set TS to calendar time based in time base BASE.  */
> > int
> > timespec_get (struct timespec *ts, int base)
> > {
> >   switch (base)
> >     {
> >       int res;
> >       INTERNAL_SYSCALL_DECL (err);
> >     case TIME_UTC:
> >       res = __timespec_get (ts, base);
> >       if (INTERNAL_SYSCALL_ERROR_P (res, err))
> >         return 0;
>
> This wrapper is using an uninitialized variable err.

Good point, fixed.

Alistair

>
> --
> Joseph S. Myers
> joseph@codesourcery.com
Joseph Myers - Aug. 15, 2019, 8:59 p.m.
On Thu, 15 Aug 2019, Alistair Francis wrote:

> Ok, so more like this?
> 
> #if __TIMESIZE == 64
> # define timespec_get __timespec_get64
> #else
> # define timespec_get __timespec_get32
> #endif

No.  Please see what's done for mktime, for example (but it's simpler here 
because mktime supports being built outside of glibc, which is irrelevant 
for timespec_get).

* The function __mktime64 is defined, unconditionally.

* The function mktime is defined as a thin wrapper, conditionally (only 
when 32-bit time is supported).

* There's no __mktime32 anywhere.

* mktime-internal.h deals with defining __mktime64 back to mktime in the 
case where __TIMESIZE == 64 and so only a single function is needed with 
no wrapper.

> > # ifndef __NR_clock_gettime64
> > #  define __NR_clock_gettime64 __NR_clock_gettime
> > # endif
> >
> > here, because 64-bit platforms define __ASSUME_TIME64_SYSCALLS but with
> > unsuffixed syscall names.
> 
> The kernel defines 64 suffixed syscalls, is this required because
> older kernels don't do this?

The kernel does *not* define 64-suffixed syscalls on platforms where 
__SYSCALL_WORDSIZE == 64.  It defined unsuffixed syscalls with the same 
semantics.
Alistair Francis - Aug. 15, 2019, 9:12 p.m.
On Thu, Aug 15, 2019 at 1:59 PM Joseph Myers <joseph@codesourcery.com> wrote:
>
> On Thu, 15 Aug 2019, Alistair Francis wrote:
>
> > Ok, so more like this?
> >
> > #if __TIMESIZE == 64
> > # define timespec_get __timespec_get64
> > #else
> > # define timespec_get __timespec_get32
> > #endif
>
> No.  Please see what's done for mktime, for example (but it's simpler here
> because mktime supports being built outside of glibc, which is irrelevant
> for timespec_get).
>
> * The function __mktime64 is defined, unconditionally.
>
> * The function mktime is defined as a thin wrapper, conditionally (only
> when 32-bit time is supported).
>
> * There's no __mktime32 anywhere.
>
> * mktime-internal.h deals with defining __mktime64 back to mktime in the
> case where __TIMESIZE == 64 and so only a single function is needed with
> no wrapper.

There is no internal header for timespec_get, would you prefer me to
create one or put the define in time/time.h?

>
> > > # ifndef __NR_clock_gettime64
> > > #  define __NR_clock_gettime64 __NR_clock_gettime
> > > # endif
> > >
> > > here, because 64-bit platforms define __ASSUME_TIME64_SYSCALLS but with
> > > unsuffixed syscall names.
> >
> > The kernel defines 64 suffixed syscalls, is this required because
> > older kernels don't do this?
>
> The kernel does *not* define 64-suffixed syscalls on platforms where
> __SYSCALL_WORDSIZE == 64.  It defined unsuffixed syscalls with the same
> semantics.

Ah, you are right. I have added these defines.

Alistair

>
> --
> Joseph S. Myers
> joseph@codesourcery.com
Joseph Myers - Aug. 15, 2019, 9:19 p.m.
On Thu, 15 Aug 2019, Alistair Francis wrote:

> There is no internal header for timespec_get, would you prefer me to
> create one or put the define in time/time.h?

time/time.h is an installed header, so it mustn't go there.  
include/time.h would be fine (it has several such defines, for 
__localtime64 etc.).
Alistair Francis - Aug. 15, 2019, 9:19 p.m.
On Thu, Aug 15, 2019 at 2:19 PM Joseph Myers <joseph@codesourcery.com> wrote:
>
> On Thu, 15 Aug 2019, Alistair Francis wrote:
>
> > There is no internal header for timespec_get, would you prefer me to
> > create one or put the define in time/time.h?
>
> time/time.h is an installed header, so it mustn't go there.
> include/time.h would be fine (it has several such defines, for
> __localtime64 etc.).

Ok, I have this in include/time.h:

#if __TIMESIZE == 64
#define timespec_get __timespec_get64
#else

and this in the c file:

/* Set TS to calendar time based in time base BASE.  */
int
__timespec_get64 (struct timespec *ts, int base)
{
  switch (base)
    {
      int res;
      INTERNAL_SYSCALL_DECL (err);
    case TIME_UTC:
#ifdef __ASSUME_TIME64_SYSCALLS
# ifndef __NR_clock_gettime64
#  define __NR_clock_gettime64 __NR_clock_gettime
# endif
  res = INTERNAL_VSYSCALL (clock_gettime64, err, 2, CLOCK_REALTIME, ts);
#else
# ifdef __NR_clock_gettime64
  res = INTERNAL_VSYSCALL (clock_gettime64, err, 2, CLOCK_REALTIME, ts);
# endif /* __NR_clock_gettime64 */
  struct timespec ts32;

  if (! in_time_t_range (ts->tv_sec))
    {
      __set_errno (EOVERFLOW);
      return -1;
    }

  valid_timespec64_to_timespec (ts, &ts32);
  res = INTERNAL_VSYSCALL (clock_gettime, err, 2, CLOCK_REALTIME, &ts32);

  if (res == 0 || !INTERNAL_SYSCALL_ERROR_P (res, err))
    {
      ts->tv_sec = ts32.tv_sec;
      ts->tv_nsec = ts32.tv_nsec;
    }
#endif

      if (INTERNAL_SYSCALL_ERROR_P (res, err))
        return 0;
      break;

    default:
      return 0;
    }

  return base;
}

#if __TIMESIZE != 64
int
timespec_get (struct timespec *ts, int base)
{
  struct __timespec64 ts64;

  ret = __timespec_get64 (ts64, base);

  if (res == 0 || !INTERNAL_SYSCALL_ERROR_P (res, err))
    {
      ts->tv_sec = ts64.tv_sec;
      ts->tv_nsec = ts64.tv_nsec;
    }

  return ret;
}
#endif

Alistair

>
> --
> Joseph S. Myers
> joseph@codesourcery.com
Joseph Myers - Aug. 15, 2019, 9:28 p.m.
On Thu, 15 Aug 2019, Alistair Francis wrote:

> On Thu, Aug 15, 2019 at 2:19 PM Joseph Myers <joseph@codesourcery.com> wrote:
> >
> > On Thu, 15 Aug 2019, Alistair Francis wrote:
> >
> > > There is no internal header for timespec_get, would you prefer me to
> > > create one or put the define in time/time.h?
> >
> > time/time.h is an installed header, so it mustn't go there.
> > include/time.h would be fine (it has several such defines, for
> > __localtime64 etc.).
> 
> Ok, I have this in include/time.h:
> 
> #if __TIMESIZE == 64
> #define timespec_get __timespec_get64
> #else

That should be the other way round (defining __timespec_get64 to 
timespec_get so that timespec_get gets properly exported from libc).
Alistair Francis - Aug. 15, 2019, 9:31 p.m.
On Thu, Aug 15, 2019 at 2:28 PM Joseph Myers <joseph@codesourcery.com> wrote:
>
> On Thu, 15 Aug 2019, Alistair Francis wrote:
>
> > On Thu, Aug 15, 2019 at 2:19 PM Joseph Myers <joseph@codesourcery.com> wrote:
> > >
> > > On Thu, 15 Aug 2019, Alistair Francis wrote:
> > >
> > > > There is no internal header for timespec_get, would you prefer me to
> > > > create one or put the define in time/time.h?
> > >
> > > time/time.h is an installed header, so it mustn't go there.
> > > include/time.h would be fine (it has several such defines, for
> > > __localtime64 etc.).
> >
> > Ok, I have this in include/time.h:
> >
> > #if __TIMESIZE == 64
> > #define timespec_get __timespec_get64
> > #else
>
> That should be the other way round (defining __timespec_get64 to
> timespec_get so that timespec_get gets properly exported from libc).

Ok.

Thanks for your help with this. I'll update the other patches with a
similar style and look at sending a full patch series after running
tests.

Alistair

>
> --
> Joseph S. Myers
> joseph@codesourcery.com

Patch

diff --git a/sysdeps/unix/sysv/linux/timespec_get.c b/sysdeps/unix/sysv/linux/timespec_get.c
index 52080ddf08a..97ef9c5f799 100644
--- a/sysdeps/unix/sysv/linux/timespec_get.c
+++ b/sysdeps/unix/sysv/linux/timespec_get.c
@@ -24,6 +24,58 @@ 
 #endif
 #include <sysdep-vdso.h>
 
+int
+__timespec_get (struct timespec *ts, int base)
+{
+#ifdef __ASSUME_TIME64_SYSCALLS
+  return INTERNAL_VSYSCALL (clock_gettime64, err, 2, CLOCK_REALTIME, ts);
+#else
+  long int ret;
+# ifdef __NR_clock_gettime64
+#  if __TIMESIZE == 64
+  ret = INTERNAL_VSYSCALL (clock_gettime64, err, 2, CLOCK_REALTIME, ts);
+
+  if (ret == 0 || errno != ENOSYS)
+    {
+      return ret;
+    }
+#  else
+  struct __timespec64 ts64;
+
+  ret = INTERNAL_VSYSCALL (clock_gettime64, err, 2, CLOCK_REALTIME, &ts64);
+
+  if (ret == 0 || errno != ENOSYS)
+    {
+      ts->tv_sec = ts64.tv_sec;
+      ts->tv_nsec = ts64.tv_nsec;
+      return ret;
+    }
+#  endif /* __TIMESIZE == 64 */
+# endif /* __NR_clock_gettime64 */
+# if __TIMESIZE == 64
+  struct timespec ts32;
+
+  if (! in_time_t_range (ts->tv_sec))
+    {
+      __set_errno (EOVERFLOW);
+      return -1;
+    }
+
+  valid_timespec64_to_timespec (ts, &ts32);
+  ret = INTERNAL_VSYSCALL (clock_gettime, err, 2, CLOCK_REALTIME, &ts32);
+
+  if (ret == 0 || errno != ENOSYS)
+    {
+      ts->tv_sec = ts32.tv_sec;
+      ts->tv_nsec = ts32.tv_nsec;
+    }
+  return ret;
+# else
+  return INTERNAL_VSYSCALL (clock_gettime, err, 2, CLOCK_REALTIME, ts);
+# endif /* __TIMESIZE == 64 */
+#endif
+}
+
 /* Set TS to calendar time based in time base BASE.  */
 int
 timespec_get (struct timespec *ts, int base)
@@ -33,9 +85,9 @@  timespec_get (struct timespec *ts, int base)
       int res;
       INTERNAL_SYSCALL_DECL (err);
     case TIME_UTC:
-      res = INTERNAL_VSYSCALL (clock_gettime, err, 2, CLOCK_REALTIME, ts);
+      res = __timespec_get (ts, base);
       if (INTERNAL_SYSCALL_ERROR_P (res, err))
-	return 0;
+        return 0;
       break;
 
     default: