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

Message ID 3ee6c1e52cbefe6f6dbd7aef423f13607ff50402.1565398513.git.alistair.francis@wdc.com
State New
Headers show
Series
  • RISC-V glibc port for the 32-bit
Related show

Commit Message

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

-- 
2.22.0

Comments

Joseph Myers Aug. 12, 2019, 7:46 p.m. | #1
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.

-- 
Joseph S. Myers
joseph@codesourcery.com
Alistair Francis Aug. 15, 2019, 5:59 p.m. | #2
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. | #3
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.

-- 
Joseph S. Myers
joseph@codesourcery.com
Alistair Francis Aug. 15, 2019, 8:48 p.m. | #4
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. | #5
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.

-- 
Joseph S. Myers
joseph@codesourcery.com
Alistair Francis Aug. 15, 2019, 9:12 p.m. | #6
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. | #7
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.).

-- 
Joseph S. Myers
joseph@codesourcery.com
Alistair Francis Aug. 15, 2019, 9:19 p.m. | #8
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. | #9
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).

-- 
Joseph S. Myers
joseph@codesourcery.com
Alistair Francis Aug. 15, 2019, 9:31 p.m. | #10
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: