[02/12] Change most internal uses of __gettimeofday to __clock_gettime.

  Since gettimeofday will shortly be implemented in terms of
clock_gettime on all platforms, internal code should use clock_gettime
directly; in addition to removing a layer of indirection, this will
allow us to remove the PLT-bypass gunk for gettimeofday.  In many
cases, the changed code does fewer conversions.

A few Hurd-specific files were changed to use __host_get_time instead
of __clock_gettime, as this seemed tidier.

With the exception of support/support_test_main.c, test cases are not
modified, mainly because I didn’t want to have to figure out which
test cases were testing gettimeofday specifically.

The definition of GETTIME in sysdeps/generic/memusage.h had a typo and
was not reading tv_sec at all.  I fixed this.  It appears nobody has been
generating malloc traces on a machine that doesn’t have a superseding
definition.

	* inet/deadline.c (__deadline_current_time)
	* login/logout.c (logout)
	* login/logwtmp.c (logwtmp)
	* nis/nis_call.c (__nisfind_server)
	* nptl/pthread_join_common.c (timedwait_tid)
	* nptl/pthread_mutex_timedlock.c (__pthread_mutex_clocklock_common)
	* nscd/nscd_helper.c (wait_on_socket, open_socket)
	* resolv/gai_misc.c (handle_requests)
	* resolv/gai_suspend.c (gai_suspend)
	* resolv/res_send.c (evNowTime)
	* sunrpc/auth_des.c (authdes_marshal, authdes_destroy)
	* sunrpc/auth_unix.c (authunix_create, authunix_refresh)
	* sunrpc/create_xid.c (_create_xid)
	* sunrpc/svcauth_des.c (_svcauth_des)
	* sysdeps/generic/memusage.h (GETTIME)
	* sysdeps/mach/nanosleep.c (__libc_nanosleep)
	* sysdeps/posix/tempname.c (RANDOM_BITS)
	* sysdeps/pthread/aio_misc.c (handle_fildes_io)
	* sysdeps/pthread/aio_suspend.c (aio_suspend):
	Use __clock_gettime(CLOCK_REALTIME) instead of __gettimeofday.
	Include time.h if necessary.

	* sysdeps/mach/hurd/getitimer.c (__getitimer)
	* sysdeps/mach/hurd/setitimer.c (setitimer_locked)
	* sysdeps/mach/hurd/times.c (__times):
	Use __host_get_time instead of __gettimeofday.
	Include mach.h if necessary.

	* sysdeps/mach/usleep.c (usleep): Remove unnecessary calls to
	__gettimeofday.

	* support/support_test_main.c (print_timestamp): Take a struct
	timespec argument, not a struct timeval.
	(signal_handler): Update to match.
	Use clock_gettime(CLOCK_REALTIME) instead of gettimeofday.

	* sysdeps/generic/memusage.h (GETTIME): Correct typo causing
	the seconds field of each timestamp to be ignored.
---
 inet/deadline.c                |  9 ++-------
 login/logout.c                 |  9 +++++----
 login/logwtmp.c                |  7 +++----
 nis/nis_call.c                 |  4 +++-
 nptl/pthread_join_common.c     |  7 +++----
 nptl/pthread_mutex_timedlock.c |  7 +++----
 nscd/nscd_helper.c             | 24 ++++++++++++------------
 resolv/gai_misc.c              |  6 +++---
 resolv/gai_suspend.c           |  6 +++---
 resolv/res_send.c              |  6 +-----
 sunrpc/auth_des.c              | 19 +++++++++++--------
 sunrpc/auth_unix.c             |  9 +++++----
 sunrpc/create_xid.c            |  6 +++---
 sunrpc/svcauth_des.c           |  7 ++++++-
 support/support_test_main.c    | 14 ++++++--------
 sysdeps/generic/memusage.h     | 16 ++++++++--------
 sysdeps/mach/hurd/getitimer.c  |  3 ++-
 sysdeps/mach/hurd/setitimer.c  |  3 ++-
 sysdeps/mach/hurd/times.c      |  6 +++---
 sysdeps/mach/nanosleep.c       | 33 +++++++++++++++++++++------------
 sysdeps/mach/usleep.c          |  5 -----
 sysdeps/posix/tempname.c       |  9 ++++-----
 sysdeps/pthread/aio_misc.c     |  6 +++---
 sysdeps/pthread/aio_suspend.c  |  6 +++---
 24 files changed, 115 insertions(+), 112 deletions(-)
  

Zack Weinberg wrote:
> +      long int end = (now.tv_sec * 1000 + usectmo +
> +                      (now.tv_nsec + 500000) / 1000000);

The usual GNU style is to put that trailing "+" at the start of the next line 
instead, here and elsewhere.


> --- a/support/support_test_main.c
> +++ b/support/support_test_main.c
> @@ -88,16 +88,16 @@ static pid_t test_pid;
>  static void (*cleanup_function) (void);
>  
>  static void
> -print_timestamp (const char *what, struct timeval tv)
> +print_timestamp (const char *what, struct timespec tv)
>  {
>    struct tm tm;
>    if (gmtime_r (&tv.tv_sec, &tm) == NULL)
>      printf ("%s: %lld.%06d\n",
> -            what, (long long int) tv.tv_sec, (int) tv.tv_usec);
> +            what, (long long int) tv.tv_sec, (int) tv.tv_nsec / 1000);
>    else
>      printf ("%s: %04d-%02d-%02dT%02d:%02d:%02d.%06d\n",
>              what, 1900 + tm.tm_year, tm.tm_mon + 1, tm.tm_mday,
> -            tm.tm_hour, tm.tm_min, tm.tm_sec, (int) tv.tv_usec);
> +            tm.tm_hour, tm.tm_min, tm.tm_sec, (int) tv.tv_nsec / 1000);
>  }

Since the goal of this test code is to print the timestamp, the code should 
simply use %09d in the format and not divide by 1000.
  

On 20/08/2019 10:21, Zack Weinberg wrote:
> Since gettimeofday will shortly be implemented in terms of
> clock_gettime on all platforms, internal code should use clock_gettime
> directly; in addition to removing a layer of indirection, this will
> allow us to remove the PLT-bypass gunk for gettimeofday.  In many
> cases, the changed code does fewer conversions.
> 
> A few Hurd-specific files were changed to use __host_get_time instead
> of __clock_gettime, as this seemed tidier.
> 
> With the exception of support/support_test_main.c, test cases are not
> modified, mainly because I didn’t want to have to figure out which
> test cases were testing gettimeofday specifically.
> 
> The definition of GETTIME in sysdeps/generic/memusage.h had a typo and
> was not reading tv_sec at all.  I fixed this.  It appears nobody has been
> generating malloc traces on a machine that doesn’t have a superseding
> definition.
> 
> 	* inet/deadline.c (__deadline_current_time)
> 	* login/logout.c (logout)
> 	* login/logwtmp.c (logwtmp)
> 	* nis/nis_call.c (__nisfind_server)
> 	* nptl/pthread_join_common.c (timedwait_tid)
> 	* nptl/pthread_mutex_timedlock.c (__pthread_mutex_clocklock_common)
> 	* nscd/nscd_helper.c (wait_on_socket, open_socket)
> 	* resolv/gai_misc.c (handle_requests)
> 	* resolv/gai_suspend.c (gai_suspend)
> 	* resolv/res_send.c (evNowTime)
> 	* sunrpc/auth_des.c (authdes_marshal, authdes_destroy)
> 	* sunrpc/auth_unix.c (authunix_create, authunix_refresh)
> 	* sunrpc/create_xid.c (_create_xid)
> 	* sunrpc/svcauth_des.c (_svcauth_des)
> 	* sysdeps/generic/memusage.h (GETTIME)
> 	* sysdeps/mach/nanosleep.c (__libc_nanosleep)
> 	* sysdeps/posix/tempname.c (RANDOM_BITS)
> 	* sysdeps/pthread/aio_misc.c (handle_fildes_io)
> 	* sysdeps/pthread/aio_suspend.c (aio_suspend):
> 	Use __clock_gettime(CLOCK_REALTIME) instead of __gettimeofday.
> 	Include time.h if necessary.
> 
> 	* sysdeps/mach/hurd/getitimer.c (__getitimer)
> 	* sysdeps/mach/hurd/setitimer.c (setitimer_locked)
> 	* sysdeps/mach/hurd/times.c (__times):
> 	Use __host_get_time instead of __gettimeofday.
> 	Include mach.h if necessary.
> 
> 	* sysdeps/mach/usleep.c (usleep): Remove unnecessary calls to
> 	__gettimeofday.
> 
> 	* support/support_test_main.c (print_timestamp): Take a struct
> 	timespec argument, not a struct timeval.
> 	(signal_handler): Update to match.
> 	Use clock_gettime(CLOCK_REALTIME) instead of gettimeofday.
> 
> 	* sysdeps/generic/memusage.h (GETTIME): Correct typo causing
> 	the seconds field of each timestamp to be ignored.
> ---
>  inet/deadline.c                |  9 ++-------
>  login/logout.c                 |  9 +++++----
>  login/logwtmp.c                |  7 +++----
>  nis/nis_call.c                 |  4 +++-
>  nptl/pthread_join_common.c     |  7 +++----
>  nptl/pthread_mutex_timedlock.c |  7 +++----
>  nscd/nscd_helper.c             | 24 ++++++++++++------------
>  resolv/gai_misc.c              |  6 +++---
>  resolv/gai_suspend.c           |  6 +++---
>  resolv/res_send.c              |  6 +-----
>  sunrpc/auth_des.c              | 19 +++++++++++--------
>  sunrpc/auth_unix.c             |  9 +++++----
>  sunrpc/create_xid.c            |  6 +++---
>  sunrpc/svcauth_des.c           |  7 ++++++-
>  support/support_test_main.c    | 14 ++++++--------
>  sysdeps/generic/memusage.h     | 16 ++++++++--------
>  sysdeps/mach/hurd/getitimer.c  |  3 ++-
>  sysdeps/mach/hurd/setitimer.c  |  3 ++-
>  sysdeps/mach/hurd/times.c      |  6 +++---
>  sysdeps/mach/nanosleep.c       | 33 +++++++++++++++++++++------------
>  sysdeps/mach/usleep.c          |  5 -----
>  sysdeps/posix/tempname.c       |  9 ++++-----
>  sysdeps/pthread/aio_misc.c     |  6 +++---
>  sysdeps/pthread/aio_suspend.c  |  6 +++---
>  24 files changed, 115 insertions(+), 112 deletions(-)
> 
> diff --git a/inet/deadline.c b/inet/deadline.c
> index ab275c266d..dee4637732 100644
> --- a/inet/deadline.c
> +++ b/inet/deadline.c
> @@ -29,13 +29,8 @@ __deadline_current_time (void)
>  {
>    struct deadline_current_time result;
>    if (__clock_gettime (CLOCK_MONOTONIC, &result.current) != 0)
> -    {
> -      struct timeval current_tv;
> -      if (__gettimeofday (&current_tv, NULL) == 0)
> -        __libc_fatal ("Fatal error: gettimeofday system call failed\n");
> -      result.current.tv_sec = current_tv.tv_sec;
> -      result.current.tv_nsec = current_tv.tv_usec * 1000;
> -    }
> +    if (__clock_gettime (CLOCK_REALTIME, &result.current) != 0)
> +      __libc_fatal ("Fatal error: clock_gettime failed\n");
>    assert (result.current.tv_sec >= 0);
>    return result;
>  }

I think it is a fair assumption that CLOCK_REALTIME is always supported, as it
is also specific by POSIX.1-2017. It allows simplify it further and remove the
libc_fatal fallback code.

> diff --git a/login/logout.c b/login/logout.c
> index 5015c1af0b..f1313ded77 100644
> --- a/login/logout.c
> +++ b/login/logout.c
> @@ -19,6 +19,7 @@
>  #include <errno.h>
>  #include <string.h>
>  #include <utmp.h>
> +#include <time.h>
>  #include <sys/time.h>
>  
>  int
> @@ -45,10 +46,10 @@ logout (const char *line)
>        /* Clear information about who & from where.  */
>        memset (ut->ut_name, '\0', sizeof ut->ut_name);
>        memset (ut->ut_host, '\0', sizeof ut->ut_host);
> -      struct timeval tv;
> -      __gettimeofday (&tv, NULL);
> -      ut->ut_tv.tv_sec = tv.tv_sec;
> -      ut->ut_tv.tv_usec = tv.tv_usec;
> +
> +      struct timespec ts;
> +      __clock_gettime (CLOCK_REALTIME, &ts);
> +      TIMESPEC_TO_TIMEVAL (&ut->ut_tv, &ts);
>        ut->ut_type = DEAD_PROCESS;
>  
>        if (pututline (ut) != NULL)

Ok.

> diff --git a/login/logwtmp.c b/login/logwtmp.c
> index 50d14976c7..ec41375383 100644
> --- a/login/logwtmp.c
> +++ b/login/logwtmp.c
> @@ -36,10 +36,9 @@ logwtmp (const char *line, const char *name, const char *host)
>    strncpy (ut.ut_name, name, sizeof ut.ut_name);
>    strncpy (ut.ut_host, host, sizeof ut.ut_host);
>  
> -  struct timeval tv;
> -  __gettimeofday (&tv, NULL);
> -  ut.ut_tv.tv_sec = tv.tv_sec;
> -  ut.ut_tv.tv_usec = tv.tv_usec;
> +  struct timespec ts;
> +  __clock_gettime (CLOCK_REALTIME, &ts);
> +  TIMESPEC_TO_TIMEVAL (&ut.ut_tv, &ts);
>  
>    updwtmp (_PATH_WTMP, &ut);
>  }

Ok.

> diff --git a/nis/nis_call.c b/nis/nis_call.c
> index a48ecc39a8..db81fa3568 100644
> --- a/nis/nis_call.c
> +++ b/nis/nis_call.c
> @@ -709,6 +709,7 @@ __nisfind_server (const_nis_name name, int search_parent,
>    nis_error status;
>    directory_obj *obj;
>    struct timeval now;
> +  struct timespec ts;
>    unsigned int server_used = ~0;
>    unsigned int current_ep = ~0;
>  
> @@ -718,7 +719,8 @@ __nisfind_server (const_nis_name name, int search_parent,
>    if (*dir != NULL)
>      return NIS_SUCCESS;
>  
> -  (void) gettimeofday (&now, NULL);
> +  __clock_gettime (CLOCK_REALTIME, &ts);
> +  TIMESPEC_TO_TIMEVAL (&now, &ts);
>  
>    if ((flags & NO_CACHE) == 0)
>      *dir = nis_server_cache_search (name, search_parent, &server_used,

Ok.

> diff --git a/nptl/pthread_join_common.c b/nptl/pthread_join_common.c
> index 5224ee2110..f1b14266de 100644
> --- a/nptl/pthread_join_common.c
> +++ b/nptl/pthread_join_common.c
> @@ -46,15 +46,14 @@ timedwait_tid (pid_t *tidp, const struct timespec *abstime)
>    /* Repeat until thread terminated.  */
>    while ((tid = *tidp) != 0)
>      {
> -      struct timeval tv;
>        struct timespec rt;
>  
>        /* Get the current time.  */
> -      __gettimeofday (&tv, NULL);
> +      __clock_gettime (CLOCK_REALTIME, &rt);
>  
>        /* Compute relative timeout.  */
> -      rt.tv_sec = abstime->tv_sec - tv.tv_sec;
> -      rt.tv_nsec = abstime->tv_nsec - tv.tv_usec * 1000;
> +      rt.tv_sec = abstime->tv_sec - rt.tv_sec;
> +      rt.tv_nsec = abstime->tv_nsec - rt.tv_nsec;
>        if (rt.tv_nsec < 0)
>          {
>            rt.tv_nsec += 1000000000;

I think we can simplify it even more by adding a cancellable lll_futex_clock_wait_bitset
variant and change to just:

  /* Repeat until thread terminated.  */
  while ((tid = *tidp) != 0)
    {
      if (lll_futex_clock_wait_bitset_cancel (tidp, tid, CLOCK_REALTIME,
                                              abstime, LLL_SHARED)
          == -ETIMEDOUT)
        return ETIMEDOUT;
    }

  

> diff --git a/nptl/pthread_mutex_timedlock.c b/nptl/pthread_mutex_timedlock.c
> index 52c258e33d..2b194e5bee 100644
> --- a/nptl/pthread_mutex_timedlock.c
> +++ b/nptl/pthread_mutex_timedlock.c
> @@ -567,15 +567,14 @@ __pthread_mutex_clocklock_common (pthread_mutex_t *mutex,
>  			goto failpp;
>  		      }
>  
> -		    struct timeval tv;
>  		    struct timespec rt;
>  
>  		    /* Get the current time.  */
> -		    (void) __gettimeofday (&tv, NULL);
> +                    __clock_gettime (CLOCK_REALTIME, &rt);
>  
>  		    /* Compute relative timeout.  */
> -		    rt.tv_sec = abstime->tv_sec - tv.tv_sec;
> -		    rt.tv_nsec = abstime->tv_nsec - tv.tv_usec * 1000;
> +		    rt.tv_sec = abstime->tv_sec - rt.tv_sec;
> +		    rt.tv_nsec = abstime->tv_nsec - rt.tv_nsec;
>  		    if (rt.tv_nsec < 0)
>  		      {
>  			rt.tv_nsec += 1000000000;

With cancelable lll_futex_clock_wait_bitset we can simplify it:

                if (oldval != ceilval)
                  {
                    /* Reject invalid timeouts.  */
                    if (abstime->tv_nsec < 0 || abstime->tv_nsec >= 1000000000)
                      {
                        result = EINVAL;
                        goto failpp;
                      }

                    lll_futex_clock_wait_bitset_cancel (&mutex->__data.__lock,
                                                        ceilval | 2,
                                                        CLOCK_REALTIME, &rt,
                                                        PTHREAD_MUTEX_PSHARED (mutex));
                  }
 

> diff --git a/nscd/nscd_helper.c b/nscd/nscd_helper.c
> index 733c2a60cd..e12769ba03 100644
> --- a/nscd/nscd_helper.c
> +++ b/nscd/nscd_helper.c
> @@ -59,9 +59,10 @@ wait_on_socket (int sock, long int usectmo)
>        /* Handle the case where the poll() call is interrupted by a
>  	 signal.  We cannot just use TEMP_FAILURE_RETRY since it might
>  	 lead to infinite loops.  */
> -      struct timeval now;
> -      (void) __gettimeofday (&now, NULL);
> -      long int end = now.tv_sec * 1000 + usectmo + (now.tv_usec + 500) / 1000;
> +      struct timespec now;
> +      __clock_gettime (CLOCK_REALTIME, &now);
> +      long int end = (now.tv_sec * 1000 + usectmo +
> +                      (now.tv_nsec + 500000) / 1000000);
>        long int timeout = usectmo;
>        while (1)
>  	{
> @@ -70,8 +71,9 @@ wait_on_socket (int sock, long int usectmo)
>  	    break;
>  
>  	  /* Recompute the timeout time.  */
> -	  (void) __gettimeofday (&now, NULL);
> -	  timeout = end - (now.tv_sec * 1000 + (now.tv_usec + 500) / 1000);
> +          __clock_gettime (CLOCK_REALTIME, &now);
> +	  timeout = end - ((now.tv_sec * 1000 +
> +                            (now.tv_nsec + 500000) / 1000000));
>  	}
>      }
>  

Couldn't we use the support/timespec-* to operate with struct timespec
along with a helper function to get get current time? It has add and sub
operation that already handle overflow, so the above code could be 
simplified to:

---
static inline struct timespec
timespec_now (void)
{
  struct timespec r;
  __clock_gettime (CLOCK_REALTIME, &r);
  return r;
}

[...]

static int
wait_on_socket (int sock, long int usectmo)
{ 
  struct pollfd fds[1];
  fds[0].fd = sock;
  fds[0].events = POLLIN | POLLERR | POLLHUP;
  int n = __poll (fds, 1, usectmo);
  if (n == -1 && __glibc_unlikely (errno == EINTR))
    { 
      /* Handle the case where the poll() call is interrupted by a
         signal.  We cannot just use TEMP_FAILURE_RETRY since it might
         lead to infinite loops.  */

      struct timespec timeout = make_timespec (usectmo, 0);
      struct timespec end = timespec_add (timespec_now (), timeout);
      
      while (1)
        { 
          n = __ppoll (fds, 1, &timeout, NULL);
          if (n != -1 || errno != EINTR)
            break;

          /* Recompute the timeout time.  */
	  timeout = timespec_sub (end, timespec_now());
        }
    }

  return n;
}


> @@ -191,9 +193,7 @@ open_socket (request_type type, const char *key, size_t keylen)
>    memcpy (reqdata->key, key, keylen);
>  
>    bool first_try = true;
> -  struct timeval tvend;
> -  /* Fake initializing tvend.  */
> -  asm ("" : "=m" (tvend));
> +  struct timespec tvend = { 0, 0 };
>    while (1)
>      {
>  #ifndef MSG_NOSIGNAL
> @@ -212,18 +212,18 @@ open_socket (request_type type, const char *key, size_t keylen)
>  
>        /* The daemon is busy wait for it.  */
>        int to;
> -      struct timeval now;
> -      (void) __gettimeofday (&now, NULL);
> +      struct timespec now;
> +      __clock_gettime (CLOCK_REALTIME, &now);
>        if (first_try)
>  	{
> -	  tvend.tv_usec = now.tv_usec;
> +	  tvend.tv_nsec = now.tv_nsec;
>  	  tvend.tv_sec = now.tv_sec + 5;
>  	  to = 5 * 1000;
>  	  first_try = false;
>  	}
>        else
>  	to = ((tvend.tv_sec - now.tv_sec) * 1000
> -	      + (tvend.tv_usec - now.tv_usec) / 1000);
> +	      + (tvend.tv_nsec - now.tv_nsec) / 1000000);
>  
>        struct pollfd fds[1];
>        fds[0].fd = sock;

Same as before, with helper timespec function we can simplify to:

  struct timeval tvend;
  while (1)
    {
#ifndef MSG_NOSIGNAL
# define MSG_NOSIGNAL 0
#endif
      ssize_t wres = TEMP_FAILURE_RETRY (__send (sock, reqdata,
                                                 real_sizeof_reqdata,
                                                 MSG_NOSIGNAL));
      if (__glibc_likely (wres == (ssize_t) real_sizeof_reqdata))
        /* We managed to send the request.  */
        return sock;

      if (wres != -1 || errno != EAGAIN)
        /* Something is really wrong, no chance to continue.  */
        break;

      /* The daemon is busy wait for it.  */
      struct timespec to;
      if (first_try)
	{
	  tvend = timespec_add (timespec_now (), make_timespec (5, 0));
	  to = tvend;
	}
      else
	to = timespec_sub (tvend, timespec_now ());

      struct pollfd fds[1];
      fds[0].fd = sock;
      fds[0].events = POLLOUT | POLLERR | POLLHUP;
      if (__ppoll (fds, 1, &to, NULL) <= 0)
        /* The connection timed out or broke down.  */
        break;

      /* We try to write again.  */
    }




> diff --git a/resolv/gai_misc.c b/resolv/gai_misc.c
> index 69d7086ae6..5d1e310147 100644
> --- a/resolv/gai_misc.c
> +++ b/resolv/gai_misc.c
> @@ -357,13 +357,13 @@ handle_requests (void *arg)
>  	 something to arrive in it. */
>        if (runp == NULL && optim.gai_idle_time >= 0)
>  	{
> -	  struct timeval now;
> +	  struct timespec now;
>  	  struct timespec wakeup_time;
>  
>  	  ++idle_thread_count;
> -	  gettimeofday (&now, NULL);
> +          __clock_gettime (CLOCK_REALTIME, &now);
>  	  wakeup_time.tv_sec = now.tv_sec + optim.gai_idle_time;
> -	  wakeup_time.tv_nsec = now.tv_usec * 1000;
> +	  wakeup_time.tv_nsec = now.tv_nsec;
>  	  if (wakeup_time.tv_nsec >= 1000000000)
>  	    {
>  	      wakeup_time.tv_nsec -= 1000000000;

With timespec helper function it can be simplified with:

      /* If the runlist is empty, then we sleep for a while, waiting for
         something to arrive in it. */
      if (runp == NULL && optim.gai_idle_time >= 0)
        { 
          struct timespec wakeup_time;
          ++idle_thread_count;

	  wakeup_time = timespec_add (timespec_now (), 
				      make_timespec (optim.gai_idle_time, 0));
          pthread_cond_timedwait (&__gai_new_request_notification,
                                  &__gai_requests_mutex, &wakeup_time);
          --idle_thread_count;
          runp = requests;
          while (runp != NULL && runp->running != 0)
            runp = runp->next;
        }


> diff --git a/resolv/gai_suspend.c b/resolv/gai_suspend.c
> index eee3bcebe9..8f81e5a3dd 100644
> --- a/resolv/gai_suspend.c
> +++ b/resolv/gai_suspend.c
> @@ -91,11 +91,11 @@ gai_suspend (const struct gaicb *const list[], int ent,
>  	{
>  	  /* We have to convert the relative timeout value into an
>  	     absolute time value with pthread_cond_timedwait expects.  */
> -	  struct timeval now;
> +	  struct timespec now;
>  	  struct timespec abstime;
>  
> -	  __gettimeofday (&now, NULL);
> -	  abstime.tv_nsec = timeout->tv_nsec + now.tv_usec * 1000;
> +          __clock_gettime (CLOCK_REALTIME, &now);
> +	  abstime.tv_nsec = timeout->tv_nsec + now.tv_nsec;
>  	  abstime.tv_sec = timeout->tv_sec + now.tv_sec;
>  	  if (abstime.tv_nsec >= 1000000000)
>  	    {

As before.

> diff --git a/resolv/res_send.c b/resolv/res_send.c
> index ed27f3abf8..0cd35f99d7 100644
> --- a/resolv/res_send.c
> +++ b/resolv/res_send.c
> @@ -172,12 +172,8 @@ evCmpTime(struct timespec a, struct timespec b) {
>  
>  static void
>  evNowTime(struct timespec *res) {
> -	struct timeval now;
> -
> -	if (gettimeofday(&now, NULL) < 0)
> +	if (__clock_gettime(CLOCK_REALTIME, res) < 0)
>  		evConsTime(res, 0, 0);
> -	else
> -		TIMEVAL_TO_TIMESPEC (&now, res);
>  }
>  
>  

I think we can assume CLOCK_REALTIME always succeed.

> diff --git a/sunrpc/auth_des.c b/sunrpc/auth_des.c
> index 5b6f985bc2..9079b30397 100644
> --- a/sunrpc/auth_des.c
> +++ b/sunrpc/auth_des.c
> @@ -41,6 +41,7 @@
>  #include <rpc/xdr.h>
>  #include <netinet/in.h>		/* XXX: just to get htonl() and ntohl() */
>  #include <sys/socket.h>
> +#include <time.h>
>  #include <shlib-compat.h>
>  
>  #define MILLION		1000000L
> @@ -246,15 +247,15 @@ authdes_marshal (AUTH *auth, XDR *xdrs)
>    int status;
>    int len;
>    register int32_t *ixdr;
> -  struct timeval tval;
> +  struct timespec now;
>  
>    /*
>     * Figure out the "time", accounting for any time difference
>     * with the server if necessary.
>     */
> -  __gettimeofday (&tval, (struct timezone *) NULL);
> -  ad->ad_timestamp.tv_sec = tval.tv_sec + ad->ad_timediff.tv_sec;
> -  ad->ad_timestamp.tv_usec = tval.tv_usec + ad->ad_timediff.tv_usec;
> +  __clock_gettime (CLOCK_REALTIME, &now);
> +  ad->ad_timestamp.tv_sec = now.tv_sec + ad->ad_timediff.tv_sec;
> +  ad->ad_timestamp.tv_usec = (now.tv_nsec / 1000) + ad->ad_timediff.tv_usec;
>    if (ad->ad_timestamp.tv_usec >= MILLION)
>      {
>        ad->ad_timestamp.tv_usec -= MILLION;

Ok (I think we probably should handle overflow here).

> @@ -445,21 +446,23 @@ authdes_destroy (AUTH *auth)
>  static bool_t
>  synchronize (struct sockaddr *syncaddr, struct rpc_timeval *timep)
>  {
> -  struct timeval mytime;
> +  struct timespec mytime;
>    struct rpc_timeval timeout;
> +  long myusec;
>  
>    timeout.tv_sec = RTIME_TIMEOUT;
>    timeout.tv_usec = 0;
>    if (rtime ((struct sockaddr_in *) syncaddr, timep, &timeout) < 0)
>      return FALSE;
>  
> -  __gettimeofday (&mytime, (struct timezone *) NULL);
> +  __clock_gettime (CLOCK_REALTIME, &mytime);
>    timep->tv_sec -= mytime.tv_sec;
> -  if (mytime.tv_usec > timep->tv_usec)
> +  myusec = mytime.tv_nsec / 1000;
> +  if (myusec > timep->tv_usec)
>      {
>        timep->tv_sec -= 1;
>        timep->tv_usec += MILLION;
>      }
> -  timep->tv_usec -= mytime.tv_usec;
> +  timep->tv_usec -= myusec;
>    return TRUE;
>  }

Ok (I think we probably should handle overflow here).

> diff --git a/sunrpc/auth_unix.c b/sunrpc/auth_unix.c
> index b035fdd870..ff0d2eb933 100644
> --- a/sunrpc/auth_unix.c
> +++ b/sunrpc/auth_unix.c
> @@ -43,6 +43,7 @@
>  #include <stdio.h>
>  #include <string.h>
>  #include <unistd.h>
> +#include <time.h>
>  #include <libintl.h>
>  #include <sys/param.h>
>  #include <wchar.h>
> @@ -96,7 +97,7 @@ authunix_create (char *machname, uid_t uid, gid_t gid, int len,
>  {
>    struct authunix_parms aup;
>    char mymem[MAX_AUTH_BYTES];
> -  struct timeval now;
> +  struct timespec now;
>    XDR xdrs;
>    AUTH *auth;
>    struct audata *au;
> @@ -122,7 +123,7 @@ no_memory:
>    /*
>     * fill in param struct from the given params
>     */
> -  (void) __gettimeofday (&now, (struct timezone *) 0);
> +  __clock_gettime (CLOCK_REALTIME, &now);
>    aup.aup_time = now.tv_sec;
>    aup.aup_machname = machname;
>    aup.aup_uid = uid;

With a helper we can just simplify to:

     aup.aup_time = timespec_now ().tv_sec;

> @@ -276,7 +277,7 @@ authunix_refresh (AUTH *auth)
>  {
>    struct audata *au = AUTH_PRIVATE (auth);
>    struct authunix_parms aup;
> -  struct timeval now;
> +  struct timespec now;
>    XDR xdrs;
>    int stat;
>  
> @@ -297,7 +298,7 @@ authunix_refresh (AUTH *auth)
>      goto done;
>  
>    /* update the time and serialize in place */
> -  (void) __gettimeofday (&now, (struct timezone *) 0);
> +  __clock_gettime (CLOCK_REALTIME, &now);
>    aup.aup_time = now.tv_sec;
>    xdrs.x_op = XDR_ENCODE;
>    XDR_SETPOS (&xdrs, 0);

As before.

> diff --git a/sunrpc/create_xid.c b/sunrpc/create_xid.c
> index a44187f07c..8d1e722dad 100644
> --- a/sunrpc/create_xid.c
> +++ b/sunrpc/create_xid.c
> @@ -39,10 +39,10 @@ _create_xid (void)
>    pid_t pid = getpid ();
>    if (is_initialized != pid)
>      {
> -      struct timeval now;
> +      struct timespec now;
>  
> -      __gettimeofday (&now, (struct timezone *) 0);
> -      __srand48_r (now.tv_sec ^ now.tv_usec ^ pid,
> +      __clock_gettime (CLOCK_REALTIME, &now);
> +      __srand48_r (now.tv_sec ^ now.tv_nsec ^ pid,
>  		   &__rpc_lrand48_data);
>        is_initialized = pid;
>      }

Ok.

> diff --git a/sunrpc/svcauth_des.c b/sunrpc/svcauth_des.c
> index c5a512d6f8..7607abc818 100644
> --- a/sunrpc/svcauth_des.c
> +++ b/sunrpc/svcauth_des.c
> @@ -44,6 +44,7 @@
>  #include <limits.h>
>  #include <string.h>
>  #include <stdint.h>
> +#include <time.h>
>  #include <sys/param.h>
>  #include <netinet/in.h>
>  #include <rpc/rpc.h>
> @@ -295,7 +296,11 @@ _svcauth_des (register struct svc_req *rqst, register struct rpc_msg *msg)
>  	debug ("timestamp before last seen");
>  	return AUTH_REJECTEDVERF;	/* replay */
>        }
> -    __gettimeofday (&current, (struct timezone *) NULL);
> +    {
> +      struct timespec now;
> +      __clock_gettime (CLOCK_REALTIME, &now);
> +      TIMESPEC_TO_TIMEVAL (&current, &now);
> +    }
>      current.tv_sec -= window;	/* allow for expiration */
>      if (!BEFORE (&current, &timestamp))
>        {

Ok.

> diff --git a/support/support_test_main.c b/support/support_test_main.c
> index 7e7b9edbb0..bc4502c030 100644
> --- a/support/support_test_main.c
> +++ b/support/support_test_main.c
> @@ -88,16 +88,16 @@ static pid_t test_pid;
>  static void (*cleanup_function) (void);
>  
>  static void
> -print_timestamp (const char *what, struct timeval tv)
> +print_timestamp (const char *what, struct timespec tv)
>  {
>    struct tm tm;
>    if (gmtime_r (&tv.tv_sec, &tm) == NULL)
>      printf ("%s: %lld.%06d\n",
> -            what, (long long int) tv.tv_sec, (int) tv.tv_usec);
> +            what, (long long int) tv.tv_sec, (int) tv.tv_nsec / 1000);
>    else
>      printf ("%s: %04d-%02d-%02dT%02d:%02d:%02d.%06d\n",
>              what, 1900 + tm.tm_year, tm.tm_mon + 1, tm.tm_mday,
> -            tm.tm_hour, tm.tm_min, tm.tm_sec, (int) tv.tv_usec);
> +            tm.tm_hour, tm.tm_min, tm.tm_sec, (int) tv.tv_nsec / 1000);
>  }
>  
>  /* Timeout handler.  We kill the child and exit with an error.  */
> @@ -110,8 +110,8 @@ signal_handler (int sig)
>  
>    /* Do this first to avoid further interference from the
>       subprocess.  */
> -  struct timeval now;
> -  bool now_available = gettimeofday (&now, NULL) == 0;
> +  struct timespec now;
> +  bool now_available = clock_gettime (CLOCK_REALTIME, &now) == 0;
>    struct stat64 st;
>    bool st_available = fstat64 (STDOUT_FILENO, &st) == 0 && st.st_mtime != 0;
>  
> @@ -168,9 +168,7 @@ signal_handler (int sig)
>    if (now_available)
>      print_timestamp ("Termination time", now);
>    if (st_available)
> -    print_timestamp ("Last write to standard output",
> -                     (struct timeval) { st.st_mtim.tv_sec,
> -                         st.st_mtim.tv_nsec / 1000 });
> +    print_timestamp ("Last write to standard output", st.st_mtim);
>  
>    /* Exit with an error.  */
>    exit (1);

Same as Paul has said about printing timestamps.

> diff --git a/sysdeps/generic/memusage.h b/sysdeps/generic/memusage.h
> index 480bdf79ee..c29feb8edd 100644
> --- a/sysdeps/generic/memusage.h
> +++ b/sysdeps/generic/memusage.h
> @@ -26,14 +26,14 @@
>  #endif
>  
>  #ifndef GETTIME
> -# define GETTIME(low,high) \
> -  {									      \
> -    struct timeval tval;						      \
> -    uint64_t usecs;							      \
> -    gettimeofday (&tval, NULL);						      \
> -    usecs = (uint64_t) tval.tv_usec + (uint64_t) tval.tv_usec * 1000000;      \
> -    low = usecs & 0xffffffff;						      \
> -    high = usecs >> 32;							      \
> +# define GETTIME(low,high)						   \
> +  {									   \
> +    struct timespec now;						   \
> +    uint64_t usecs;							   \
> +    __clock_gettime (CLOCK_REALTIME, &now);				   \
> +    usecs = (uint64_t)now.tv_nsec / 1000 + (uint64_t)now.tv_sec * 1000000; \
> +    low = usecs & 0xffffffff;						   \
> +    high = usecs >> 32;							   \
>    }
>  #endif
>  

I think it should have a space after the cast type. 

> diff --git a/sysdeps/mach/hurd/getitimer.c b/sysdeps/mach/hurd/getitimer.c
> index 69a0751ead..6a0fc3cb0d 100644
> --- a/sysdeps/mach/hurd/getitimer.c
> +++ b/sysdeps/mach/hurd/getitimer.c
> @@ -19,6 +19,7 @@
>  #include <errno.h>
>  #include <sys/time.h>
>  #include <hurd.h>
> +#include <mach.h>
>  
>  /* XXX Temporary cheezoid implementation; see __setitmr.c.  */
>  
> @@ -61,7 +62,7 @@ __getitimer (enum __itimer_which which, struct itimerval *value)
>      }
>  
>    /* Get the time now.  */
> -  if (__gettimeofday (&elapsed, NULL) < 0)
> +  if (__host_get_time (__mach_host_self (), (time_value_t *) &elapsed) < 0)
>      return -1;
>  
>    /* Extract the current timer setting; and the time it was set, so we can

Looks ok (I can't really check if Hurd change is fully correct though).

> diff --git a/sysdeps/mach/hurd/setitimer.c b/sysdeps/mach/hurd/setitimer.c
> index 61e37c5f5d..c829a5869b 100644
> --- a/sysdeps/mach/hurd/setitimer.c
> +++ b/sysdeps/mach/hurd/setitimer.c
> @@ -23,6 +23,7 @@
>  #include <hurd/signal.h>
>  #include <hurd/sigpreempt.h>
>  #include <hurd/msg_request.h>
> +#include <mach.h>
>  #include <mach/message.h>
>  
>  /* XXX Temporary cheezoid implementation of ITIMER_REAL/SIGALRM.  */
> @@ -239,7 +240,7 @@ setitimer_locked (const struct itimerval *new, struct itimerval *old,
>    if ((newval.it_value.tv_sec | newval.it_value.tv_usec) != 0 || old != NULL)
>      {
>        /* Calculate how much time is remaining for the pending alarm.  */
> -      if (__gettimeofday (&now, NULL) < 0)
> +      if (__host_get_time (__mach_host_self (), (time_value_t *) &now) < 0)
>  	{
>  	  __spin_unlock (&_hurd_itimer_lock);
>  	  _hurd_critical_section_unlock (crit);

Looks ok.

> diff --git a/sysdeps/mach/hurd/times.c b/sysdeps/mach/hurd/times.c
> index 7758311d83..aafa7b15d9 100644
> --- a/sysdeps/mach/hurd/times.c
> +++ b/sysdeps/mach/hurd/times.c
> @@ -42,7 +42,7 @@ __times (struct tms *tms)
>    struct task_basic_info bi;
>    struct task_thread_times_info tti;
>    mach_msg_type_number_t count;
> -  union { time_value_t tvt; struct timeval tv; } now;
> +  time_value_t now;
>    error_t err;
>  
>    count = TASK_BASIC_INFO_COUNT;
> @@ -65,10 +65,10 @@ __times (struct tms *tms)
>    /* XXX This can't be implemented until getrusage(RUSAGE_CHILDREN) can be.  */
>    tms->tms_cutime = tms->tms_cstime = 0;
>  
> -  if (__gettimeofday (&now.tv, NULL) < 0)
> +  if (__host_get_time (__mach_host_self (), &now) < 0)
>      return -1;
>  
> -  return (clock_from_time_value (&now.tvt)
> +  return (clock_from_time_value (&now)
>  	  - clock_from_time_value (&bi.creation_time));
>  }
>  weak_alias (__times, times)

Looks ok.

> diff --git a/sysdeps/mach/nanosleep.c b/sysdeps/mach/nanosleep.c
> index b4790aaf31..0be48db0d9 100644
> --- a/sysdeps/mach/nanosleep.c
> +++ b/sysdeps/mach/nanosleep.c
> @@ -18,16 +18,26 @@
>  
>  #include <errno.h>
>  #include <mach.h>
> -#include <sys/time.h>
>  #include <time.h>
>  #include <unistd.h>
>  
> +# define timespec_sub(a, b, result)					      \
> +  do {									      \
> +    (result)->tv_sec = (a)->tv_sec - (b)->tv_sec;			      \
> +    (result)->tv_nsec = (a)->tv_nsec - (b)->tv_nsec;			      \
> +    if ((result)->tv_nsec < 0) {					      \
> +      --(result)->tv_sec;						      \
> +      (result)->tv_nsec += 1000000000;					      \
> +    }									      \
> +  } while (0)
> +

As before, I would prefer to use the support/timerspec_* routines intead of
reimplement them.

>  int
>  __libc_nanosleep (const struct timespec *requested_time,
> -	     struct timespec *remaining)
> +                  struct timespec *remaining)
>  {
>    mach_port_t recv;
> -  struct timeval before, after;
> +  struct timespec before, after;
> +  error_t err;
>  
>    if (requested_time->tv_sec < 0
>        || requested_time->tv_nsec < 0
> @@ -43,20 +53,19 @@ __libc_nanosleep (const struct timespec *requested_time,
>  
>    recv = __mach_reply_port ();
>  
> -  if (remaining && __gettimeofday (&before, NULL) < 0)
> +  if (remaining && __clock_gettime (CLOCK_REALTIME, &before) < 0)
>      return -1;
> -  error_t err = __mach_msg (NULL, MACH_RCV_MSG|MACH_RCV_TIMEOUT|MACH_RCV_INTERRUPT,
> -			    0, 0, recv, ms, MACH_PORT_NULL);
> +
> +  err = __mach_msg (NULL, MACH_RCV_MSG|MACH_RCV_TIMEOUT|MACH_RCV_INTERRUPT,
> +                    0, 0, recv, ms, MACH_PORT_NULL);
>    __mach_port_destroy (mach_task_self (), recv);
>    if (err == EMACH_RCV_INTERRUPTED)
>      {
> -      if (remaining && __gettimeofday (&after, NULL) >= 0)
> +      if (remaining && __clock_gettime (CLOCK_REALTIME, &after) >= 0)
>  	{
> -	  struct timeval req_time, elapsed, rem;
> -	  TIMESPEC_TO_TIMEVAL (&req_time, requested_time);
> -	  timersub (&after, &before, &elapsed);
> -	  timersub (&req_time, &elapsed, &rem);
> -	  TIMEVAL_TO_TIMESPEC (&rem, remaining);
> +	  struct timespec elapsed;
> +	  timespec_sub (&after, &before, &elapsed);
> +	  timespec_sub (requested_time, &elapsed, remaining);
>  	}
>  
>        errno = EINTR;
> diff --git a/sysdeps/mach/usleep.c b/sysdeps/mach/usleep.c
> index 5d4bd205e1..8428ace6ef 100644
> --- a/sysdeps/mach/usleep.c
> +++ b/sysdeps/mach/usleep.c
> @@ -25,17 +25,12 @@ int
>  usleep (useconds_t useconds)
>  {
>    mach_port_t recv;
> -  struct timeval before, after;
>  
>    recv = __mach_reply_port ();
>  
> -  if (__gettimeofday (&before, NULL) < 0)
> -    return -1;
>    (void) __mach_msg (NULL, MACH_RCV_MSG|MACH_RCV_TIMEOUT|MACH_RCV_INTERRUPT,
>  		     0, 0, recv, (useconds + 999) / 1000, MACH_PORT_NULL);
>    __mach_port_destroy (mach_task_self (), recv);
> -  if (__gettimeofday (&after, NULL) < 0)
> -    return -1;
>  
>    return 0;
>  }

Looks ok.

> diff --git a/sysdeps/posix/tempname.c b/sysdeps/posix/tempname.c
> index 310df3c4ca..c3956498ce 100644
> --- a/sysdeps/posix/tempname.c
> +++ b/sysdeps/posix/tempname.c
> @@ -50,7 +50,7 @@
>  #include <string.h>
>  
>  #include <fcntl.h>
> -#include <sys/time.h>
> +#include <time.h>
>  #include <stdint.h>
>  #include <unistd.h>
>  
> @@ -63,7 +63,6 @@
>  # define struct_stat64 struct stat
>  # define __gen_tempname gen_tempname
>  # define __getpid getpid
> -# define __gettimeofday gettimeofday
>  # define __mkdir mkdir
>  # define __open open
>  # define __lxstat64(version, file, buf) lstat (file, buf)
> @@ -76,9 +75,9 @@
>  # else
>  # define RANDOM_BITS(Var) \
>      {                                                                         \
> -      struct timeval tv;                                                      \
> -      __gettimeofday (&tv, NULL);                                             \
> -      (Var) = ((uint64_t) tv.tv_usec << 16) ^ tv.tv_sec;                      \
> +      struct timespec ts;                                                     \
> +      clock_gettime (CLOCK_REALTIME, &ts);                                    \
> +      (Var) = ((uint64_t) tv.tv_nsec << 16) ^ tv.tv_sec;                      \
>      }
>  #endif
>  

It should be __clock_gettime.

> diff --git a/sysdeps/pthread/aio_misc.c b/sysdeps/pthread/aio_misc.c
> index 0180ddb3c3..49ab08de3a 100644
> --- a/sysdeps/pthread/aio_misc.c
> +++ b/sysdeps/pthread/aio_misc.c
> @@ -614,13 +614,13 @@ handle_fildes_io (void *arg)
>  	 something to arrive in it. */
>        if (runp == NULL && optim.aio_idle_time >= 0)
>  	{
> -	  struct timeval now;
> +	  struct timespec now;
>  	  struct timespec wakeup_time;
>  
>  	  ++idle_thread_count;
> -	  __gettimeofday (&now, NULL);
> +          __clock_gettime (CLOCK_REALTIME, &now);
>  	  wakeup_time.tv_sec = now.tv_sec + optim.aio_idle_time;
> -	  wakeup_time.tv_nsec = now.tv_usec * 1000;
> +	  wakeup_time.tv_nsec = now.tv_nsec;
>  	  if (wakeup_time.tv_nsec >= 1000000000)
>  	    {
>  	      wakeup_time.tv_nsec -= 1000000000;

As before, I think we can simplify to:

	  wakeup_time = timespec_add (timespec_now (), 
				      make_timespec (optim.aio_idle_time, 0));
          pthread_cond_timedwait (&__aio_new_request_notification,
                                  &__aio_requests_mutex,
                                  &wakeup_time);


> diff --git a/sysdeps/pthread/aio_suspend.c b/sysdeps/pthread/aio_suspend.c
> index 06bd914672..fdd4087abb 100644
> --- a/sysdeps/pthread/aio_suspend.c
> +++ b/sysdeps/pthread/aio_suspend.c
> @@ -183,11 +183,11 @@ aio_suspend (const struct aiocb *const list[], int nent,
>  	{
>  	  /* We have to convert the relative timeout value into an
>  	     absolute time value with pthread_cond_timedwait expects.  */
> -	  struct timeval now;
> +	  struct timespec now;
>  	  struct timespec abstime;
>  
> -	  __gettimeofday (&now, NULL);
> -	  abstime.tv_nsec = timeout->tv_nsec + now.tv_usec * 1000;
> +	  __clock_gettime (CLOCK_REALTIME, &now);
> +	  abstime.tv_nsec = timeout->tv_nsec + now.tv_nsec;
>  	  abstime.tv_sec = timeout->tv_sec + now.tv_sec;
>  	  if (abstime.tv_nsec >= 1000000000)
>  	    {
> 

As before, I think we can simplify to:

	  abstime = timespec_add (timespec_now (), timeout); 
          result = pthread_cond_timedwait (&cond, &__aio_requests_mutex,
                                           &abstime);
  

On Tue, Aug 20, 2019 at 4:56 PM Adhemerval Zanella
<adhemerval.zanella@linaro.org> wrote:
> On 20/08/2019 10:21, Zack Weinberg wrote:
> > Since gettimeofday will shortly be implemented in terms of
> > clock_gettime on all platforms, internal code should use clock_gettime
> > directly; in addition to removing a layer of indirection, this will
> > allow us to remove the PLT-bypass gunk for gettimeofday.  In many
> > cases, the changed code does fewer conversions.
...
> >    if (__clock_gettime (CLOCK_MONOTONIC, &result.current) != 0)
> > -    {
> > -      struct timeval current_tv;
> > -      if (__gettimeofday (&current_tv, NULL) == 0)
> > -        __libc_fatal ("Fatal error: gettimeofday system call failed\n");
> > -      result.current.tv_sec = current_tv.tv_sec;
> > -      result.current.tv_nsec = current_tv.tv_usec * 1000;
> > -    }
> > +    if (__clock_gettime (CLOCK_REALTIME, &result.current) != 0)
> > +      __libc_fatal ("Fatal error: clock_gettime failed\n");
> >    assert (result.current.tv_sec >= 0);
> >    return result;
> >  }
>
> I think it is a fair assumption that CLOCK_REALTIME is always supported, as it
> is also specific by POSIX.1-2017. It allows simplify it further and remove the
> libc_fatal fallback code.

OK, I will make that change (here and elsewhere).

...
> > -      __gettimeofday (&tv, NULL);
> > +      __clock_gettime (CLOCK_REALTIME, &rt);
> >
> >        /* Compute relative timeout.  */
> > -      rt.tv_sec = abstime->tv_sec - tv.tv_sec;
> > -      rt.tv_nsec = abstime->tv_nsec - tv.tv_usec * 1000;
> > +      rt.tv_sec = abstime->tv_sec - rt.tv_sec;
> > +      rt.tv_nsec = abstime->tv_nsec - rt.tv_nsec;
> >        if (rt.tv_nsec < 0)
> >          {
> >            rt.tv_nsec += 1000000000;
>
> I think we can simplify it even more by adding a cancellable
> lll_futex_clock_wait_bitset variant

I would rather not do that in this patch set.

This patch in particular should be as mechanical a replacement of
(__)gettimeofday with __clock_gettime as possible, but also, this
patch *set* is supposed to be about groundwork for 64-bit time_t.
Adding a new lowlevellock function in order to tighten up code is too
much scope creep for my taste.

...
> >         /* Recompute the timeout time.  */
> > -       (void) __gettimeofday (&now, NULL);
> > -       timeout = end - (now.tv_sec * 1000 + (now.tv_usec + 500) / 1000);
> > +          __clock_gettime (CLOCK_REALTIME, &now);
> > +       timeout = end - ((now.tv_sec * 1000 +
> > +                            (now.tv_nsec + 500000) / 1000000));
> >       }
> >      }
> >
>
> Couldn't we use the support/timespec-* to operate with struct timespec
> along with a helper function to get get current time?

Similarly, although I think we probably _should_ add timespec_add,
timespec_sub, timespec_cmp functions to the public <time.h> API, as
well as using them internally, I do not want to do that in this patch set.

> static inline struct timespec
> timespec_now (void)
> {
>   struct timespec r;
>   __clock_gettime (CLOCK_REALTIME, &r);
>   return r;
> }

It is not obvious whether this function should use CLOCK_REALTIME or
CLOCK_MONOTONIC, so I do not think we should have it at all.

> >    /*
> >     * Figure out the "time", accounting for any time difference
> >     * with the server if necessary.
> >     */
> > -  __gettimeofday (&tval, (struct timezone *) NULL);
> > -  ad->ad_timestamp.tv_sec = tval.tv_sec + ad->ad_timediff.tv_sec;
> > -  ad->ad_timestamp.tv_usec = tval.tv_usec + ad->ad_timediff.tv_usec;
> > +  __clock_gettime (CLOCK_REALTIME, &now);
> > +  ad->ad_timestamp.tv_sec = now.tv_sec + ad->ad_timediff.tv_sec;
> > +  ad->ad_timestamp.tv_usec = (now.tv_nsec / 1000) + ad->ad_timediff.tv_usec;
> >    if (ad->ad_timestamp.tv_usec >= MILLION)
> >      {
> >        ad->ad_timestamp.tv_usec -= MILLION;
>
> Ok (I think we probably should handle overflow here).

I don't understand the parenthetical.  Are you saying that we already
do handle overflow and that's good, or that overflow handling needs to
be added?

> >      printf ("%s: %lld.%06d\n",
> > -            what, (long long int) tv.tv_sec, (int) tv.tv_usec);
> > +            what, (long long int) tv.tv_sec, (int) tv.tv_nsec / 1000);
> >    else
> >      printf ("%s: %04d-%02d-%02dT%02d:%02d:%02d.%06d\n",
> >              what, 1900 + tm.tm_year, tm.tm_mon + 1, tm.tm_mday,
> > -            tm.tm_hour, tm.tm_min, tm.tm_sec, (int) tv.tv_usec);
> > +            tm.tm_hour, tm.tm_min, tm.tm_sec, (int) tv.tv_nsec / 1000);
...
> Same as Paul has said about printing timestamps.

OK, will change.

> > +# define GETTIME(low,high)                                              \
> > +  {                                                                     \
> > +    struct timespec now;                                                \
> > +    uint64_t usecs;                                                     \
> > +    __clock_gettime (CLOCK_REALTIME, &now);                             \
> > +    usecs = (uint64_t)now.tv_nsec / 1000 + (uint64_t)now.tv_sec * 1000000; \
> > +    low = usecs & 0xffffffff;                                                   \
> > +    high = usecs >> 32;                                                         \
> >    }
> >  #endif
> >
>
> I think it should have a space after the cast type.

Then it wouldn't fit into 80 columns.

> > -  if (__gettimeofday (&elapsed, NULL) < 0)
> > +  if (__host_get_time (__mach_host_self (), (time_value_t *) &elapsed) < 0)
> >      return -1;
>
> Looks ok (I can't really check if Hurd change is fully correct though).

Oh, this is actually wrong, I need to change it to use an actual
time_value_t rather than type punning.  GCC 9 complains about it in
other places, dunno why not this one.

The construct

    __host_get_time (__mach_host_self (), &tvt)

seems to be Mach's equivalent of gettimeofday(), but I do not actually
know this any better than you do, I copied it out of the former
sysdeps/mach/gettimeofday.c.

> > -      if (__gettimeofday (&now, NULL) < 0)
> > +      if (__host_get_time (__mach_host_self (), (time_value_t *) &now) < 0)
> >       {
> >         __spin_unlock (&_hurd_itimer_lock);
> >         _hurd_critical_section_unlock (crit);
>
> Looks ok.

Similar issue with time_value_t vs struct timeval, will fix.

> > +# define timespec_sub(a, b, result)                                        \
> > +  do {                                                                             \
> > +    (result)->tv_sec = (a)->tv_sec - (b)->tv_sec;                          \
> > +    (result)->tv_nsec = (a)->tv_nsec - (b)->tv_nsec;                       \
> > +    if ((result)->tv_nsec < 0) {                                           \
> > +      --(result)->tv_sec;                                                  \
> > +      (result)->tv_nsec += 1000000000;                                             \
> > +    }                                                                              \
> > +  } while (0)
> > +
>
> As before, I would prefer to use the support/timerspec_* routines intead of
> reimplement them.

As before: would really rather not do that in this patch series.

> >  # define RANDOM_BITS(Var) \
> >      {                                                                         \
> > -      struct timeval tv;                                                      \
> > -      __gettimeofday (&tv, NULL);                                             \
> > -      (Var) = ((uint64_t) tv.tv_usec << 16) ^ tv.tv_sec;                      \
> > +      struct timespec ts;                                                     \
> > +      clock_gettime (CLOCK_REALTIME, &ts);                                    \
> > +      (Var) = ((uint64_t) tv.tv_nsec << 16) ^ tv.tv_sec;                      \
> >      }
> >  #endif
>
> It should be __clock_gettime.

Not here.  If you look at the whole file, this definition of
RANDOM_BITS is used only when _LIBC is *not* defined.

zw
  

On 8/20/19 2:53 PM, Paul Eggert wrote:
> Zack Weinberg wrote:
>> +      long int end = (now.tv_sec * 1000 + usectmo +
>> +                      (now.tv_nsec + 500000) / 1000000);
> 
> The usual GNU style is to put that trailing "+" at the start of the next
> line instead, here and elsewhere.

Thanks, will fix.  (I've been working a lot on code that uses the other
convention recently.)

>> -            tm.tm_hour, tm.tm_min, tm.tm_sec, (int) tv.tv_usec);
>> +            tm.tm_hour, tm.tm_min, tm.tm_sec, (int) tv.tv_nsec / 1000);
>>  }
> 
> Since the goal of this test code is to print the timestamp, the code
> should simply use %09d in the format and not divide by 1000.

Yeah, OK.

zw
  

On 21/08/2019 18:09, Zack Weinberg wrote:
> On Tue, Aug 20, 2019 at 4:56 PM Adhemerval Zanella
> <adhemerval.zanella@linaro.org> wrote:
>> On 20/08/2019 10:21, Zack Weinberg wrote:
>>> Since gettimeofday will shortly be implemented in terms of
>>> clock_gettime on all platforms, internal code should use clock_gettime
>>> directly; in addition to removing a layer of indirection, this will
>>> allow us to remove the PLT-bypass gunk for gettimeofday.  In many
>>> cases, the changed code does fewer conversions.
> ...
>>>    if (__clock_gettime (CLOCK_MONOTONIC, &result.current) != 0)
>>> -    {
>>> -      struct timeval current_tv;
>>> -      if (__gettimeofday (&current_tv, NULL) == 0)
>>> -        __libc_fatal ("Fatal error: gettimeofday system call failed\n");
>>> -      result.current.tv_sec = current_tv.tv_sec;
>>> -      result.current.tv_nsec = current_tv.tv_usec * 1000;
>>> -    }
>>> +    if (__clock_gettime (CLOCK_REALTIME, &result.current) != 0)
>>> +      __libc_fatal ("Fatal error: clock_gettime failed\n");
>>>    assert (result.current.tv_sec >= 0);
>>>    return result;
>>>  }
>>
>> I think it is a fair assumption that CLOCK_REALTIME is always supported, as it
>> is also specific by POSIX.1-2017. It allows simplify it further and remove the
>> libc_fatal fallback code.
> 
> OK, I will make that change (here and elsewhere).
> 
> ...
>>> -      __gettimeofday (&tv, NULL);
>>> +      __clock_gettime (CLOCK_REALTIME, &rt);
>>>
>>>        /* Compute relative timeout.  */
>>> -      rt.tv_sec = abstime->tv_sec - tv.tv_sec;
>>> -      rt.tv_nsec = abstime->tv_nsec - tv.tv_usec * 1000;
>>> +      rt.tv_sec = abstime->tv_sec - rt.tv_sec;
>>> +      rt.tv_nsec = abstime->tv_nsec - rt.tv_nsec;
>>>        if (rt.tv_nsec < 0)
>>>          {
>>>            rt.tv_nsec += 1000000000;
>>
>> I think we can simplify it even more by adding a cancellable
>> lll_futex_clock_wait_bitset variant
> 
> I would rather not do that in this patch set.
> 
> This patch in particular should be as mechanical a replacement of
> (__)gettimeofday with __clock_gettime as possible, but also, this
> patch *set* is supposed to be about groundwork for 64-bit time_t.
> Adding a new lowlevellock function in order to tighten up code is too
> much scope creep for my taste.

Alright, I think I will send it as a subsequent cleanup after this patch
has landed.


> 
> ...
>>>         /* Recompute the timeout time.  */
>>> -       (void) __gettimeofday (&now, NULL);
>>> -       timeout = end - (now.tv_sec * 1000 + (now.tv_usec + 500) / 1000);
>>> +          __clock_gettime (CLOCK_REALTIME, &now);
>>> +       timeout = end - ((now.tv_sec * 1000 +
>>> +                            (now.tv_nsec + 500000) / 1000000));
>>>       }
>>>      }
>>>
>>
>> Couldn't we use the support/timespec-* to operate with struct timespec
>> along with a helper function to get get current time?
> 
> Similarly, although I think we probably _should_ add timespec_add,
> timespec_sub, timespec_cmp functions to the public <time.h> API, as
> well as using them internally, I do not want to do that in this patch set.

In fact we are using them just for libsupport, my suggestion would be to
include them on libc internally to help on the clock_gettime usage. The
public inclusion on <time.h> should be in another discussion/patch.

> 
>> static inline struct timespec
>> timespec_now (void)
>> {
>>   struct timespec r;
>>   __clock_gettime (CLOCK_REALTIME, &r);
>>   return r;
>> }
> 
> It is not obvious whether this function should use CLOCK_REALTIME or
> CLOCK_MONOTONIC, so I do not think we should have it at all.

Alright, although I think we can still add the helper functions with a
different prototype:

static inline struct timespec
timespec_now_clockid (clockid_t clk_id)
{
  _Static_assert (clk_id == CLOCK_REALTIME);
  struct timespec r;
  __clock_gettime (clk_id, &r);
  return r;
}

static inline struct timespec
timespec_now_realtime (void)
{
  /* CLOCK_REALTIME is always support.  */
  return timespec_now_clockid (CLOCK_REALTIME);
}

This allows rule out usage with invalid clocks and simplify its usage
on most common cases.

> 
>>>    /*
>>>     * Figure out the "time", accounting for any time difference
>>>     * with the server if necessary.
>>>     */
>>> -  __gettimeofday (&tval, (struct timezone *) NULL);
>>> -  ad->ad_timestamp.tv_sec = tval.tv_sec + ad->ad_timediff.tv_sec;
>>> -  ad->ad_timestamp.tv_usec = tval.tv_usec + ad->ad_timediff.tv_usec;
>>> +  __clock_gettime (CLOCK_REALTIME, &now);
>>> +  ad->ad_timestamp.tv_sec = now.tv_sec + ad->ad_timediff.tv_sec;
>>> +  ad->ad_timestamp.tv_usec = (now.tv_nsec / 1000) + ad->ad_timediff.tv_usec;
>>>    if (ad->ad_timestamp.tv_usec >= MILLION)
>>>      {
>>>        ad->ad_timestamp.tv_usec -= MILLION;
>>
>> Ok (I think we probably should handle overflow here).
> 
> I don't understand the parenthetical.  Are you saying that we already
> do handle overflow and that's good, or that overflow handling needs to
> be added?

I tend to frown upon these kind of operations that blindly adjust values
without some sort of sanitization, but it might be the case where overflow
values can't happen or are not really an issue.   The 'ad_timediff' seems 
to be adjusted only at this place (other places seems to zero it), and
below there an explicit comment that y2038 might trigger an issue.  But
for a mechanical patch to replace gettimeoday I think the patch looks ok.

> 
>>>      printf ("%s: %lld.%06d\n",
>>> -            what, (long long int) tv.tv_sec, (int) tv.tv_usec);
>>> +            what, (long long int) tv.tv_sec, (int) tv.tv_nsec / 1000);
>>>    else
>>>      printf ("%s: %04d-%02d-%02dT%02d:%02d:%02d.%06d\n",
>>>              what, 1900 + tm.tm_year, tm.tm_mon + 1, tm.tm_mday,
>>> -            tm.tm_hour, tm.tm_min, tm.tm_sec, (int) tv.tv_usec);
>>> +            tm.tm_hour, tm.tm_min, tm.tm_sec, (int) tv.tv_nsec / 1000);
> ...
>> Same as Paul has said about printing timestamps.
> 
> OK, will change.
> 
>>> +# define GETTIME(low,high)                                              \
>>> +  {                                                                     \
>>> +    struct timespec now;                                                \
>>> +    uint64_t usecs;                                                     \
>>> +    __clock_gettime (CLOCK_REALTIME, &now);                             \
>>> +    usecs = (uint64_t)now.tv_nsec / 1000 + (uint64_t)now.tv_sec * 1000000; \
>>> +    low = usecs & 0xffffffff;                                                   \
>>> +    high = usecs >> 32;                                                         \
>>>    }
>>>  #endif
>>>
>>
>> I think it should have a space after the cast type.
> 
> Then it wouldn't fit into 80 columns.
> 
>>> -  if (__gettimeofday (&elapsed, NULL) < 0)
>>> +  if (__host_get_time (__mach_host_self (), (time_value_t *) &elapsed) < 0)
>>>      return -1;
>>
>> Looks ok (I can't really check if Hurd change is fully correct though).
> 
> Oh, this is actually wrong, I need to change it to use an actual
> time_value_t rather than type punning.  GCC 9 complains about it in
> other places, dunno why not this one.
> 
> The construct
> 
>     __host_get_time (__mach_host_self (), &tvt)
> 
> seems to be Mach's equivalent of gettimeofday(), but I do not actually
> know this any better than you do, I copied it out of the former
> sysdeps/mach/gettimeofday.c.

The information I could get is __host_get_time accessed an mmap area (similar
to vDSO) that contains the system time. And yeah, even though Hurd time_value_t
is a struct with similar layout of struct timeval, it is not really valid to
cast is on __host_get_time (struct timeval also defines as a tv_usec, which
is __suseconds_t/long int).

> 
>>> -      if (__gettimeofday (&now, NULL) < 0)
>>> +      if (__host_get_time (__mach_host_self (), (time_value_t *) &now) < 0)
>>>       {
>>>         __spin_unlock (&_hurd_itimer_lock);
>>>         _hurd_critical_section_unlock (crit);
>>
>> Looks ok.
> 
> Similar issue with time_value_t vs struct timeval, will fix.
> 
>>> +# define timespec_sub(a, b, result)                                        \
>>> +  do {                                                                             \
>>> +    (result)->tv_sec = (a)->tv_sec - (b)->tv_sec;                          \
>>> +    (result)->tv_nsec = (a)->tv_nsec - (b)->tv_nsec;                       \
>>> +    if ((result)->tv_nsec < 0) {                                           \
>>> +      --(result)->tv_sec;                                                  \
>>> +      (result)->tv_nsec += 1000000000;                                             \
>>> +    }                                                                              \
>>> +  } while (0)
>>> +
>>
>> As before, I would prefer to use the support/timerspec_* routines intead of
>> reimplement them.
> 
> As before: would really rather not do that in this patch series.

I understand you want the patch to mechanically as possible, however I also see
counterproductive to replicate a routine that we already have a better implemented
one in the same code base.  These additions tend to linger until someone actively
refactors it.

> 
>>>  # define RANDOM_BITS(Var) \
>>>      {                                                                         \
>>> -      struct timeval tv;                                                      \
>>> -      __gettimeofday (&tv, NULL);                                             \
>>> -      (Var) = ((uint64_t) tv.tv_usec << 16) ^ tv.tv_sec;                      \
>>> +      struct timespec ts;                                                     \
>>> +      clock_gettime (CLOCK_REALTIME, &ts);                                    \
>>> +      (Var) = ((uint64_t) tv.tv_nsec << 16) ^ tv.tv_sec;                      \
>>>      }
>>>  #endif
>>
>> It should be __clock_gettime.
> 
> Not here.  If you look at the whole file, this definition of
> RANDOM_BITS is used only when _LIBC is *not* defined.

Right, I think we should follow Wilco suggestion and just clean this file up
with onyl glibc definitions.