Re: [PATCH v3 7/7] y2038: linux: Provide ___gettimeofday64 implementation

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

On 29/01/2020 09:59, Lukasz Majewski wrote:
> In the glibc the gettimeofday can use vDSO (on power and x86 the
> USE_IFUNC_GETTIMEOFDAY is defined), gettimeofday syscall or 'default'
> ___gettimeofday() from ./time/gettime.c (as a fallback).
> 
> In this patch the last function (___gettimeofday) has been reimplemented and
> moved to ./sysdeps/unix/sysv/linux/gettimeofday.c to be Linux specific.
> 
> The new ___gettimeofday64 explicit 64 bit function for getting 64 bit time from
> the kernel (by internally calling __clock_gettime64) has been introduced.
> 
> Moreover, a 32 bit version - ___gettimeofday has been refactored to internally
> use __gettimeofday64.
> 
> The ___gettimeofday is now supposed to be used on systems still supporting 32
> bit time (__TIMESIZE != 64) - hence the necessary check for time_t potential
> overflow and conversion of struct __timeval64 to 32 bit struct timespec.
> 
> The alpha port is a bit problematic for this change - it supports 64 bit time
> and can safely use gettimeofday implementation from ./time/gettimeofday.c as it
> has ./sysdeps/unix/sysv/linux/alpha/gettimeofday.c, which includes
> ./time/gettimeofday.c, so the Linux specific one can be avoided.
> For that reason the code to set default gettimeofday symbol has not been moved
> to ./sysdeps/unix/sysv/linux/gettimeofday.c as only alpha defines
> VERSION_gettimeofday.
> 
> 
> Build tests:
> ./src/scripts/build-many-glibcs.py glibcs
> 
> Run-time tests:
> - Run specific tests on ARM/x86 32bit systems (qemu):
>   https://github.com/lmajewski/meta-y2038 and run tests:
>   https://github.com/lmajewski/y2038-tests/commits/master
> 
> Above tests were performed with Y2038 redirection applied as well as without
> to test proper usage of both ___gettimeofday64 and __gettimeofday.
> 
> ---
> Changes for v3:
> - New patch
> ---
>  include/time.h                         |  4 +++
>  sysdeps/unix/sysv/linux/gettimeofday.c | 46 +++++++++++++++++++++++++-
>  2 files changed, 49 insertions(+), 1 deletion(-)
> 
> diff --git a/include/time.h b/include/time.h
> index 0345803db2..931c9a3bd7 100644
> --- a/include/time.h
> +++ b/include/time.h
> @@ -227,10 +227,14 @@ libc_hidden_proto (__sched_rr_get_interval64);
>  
>  #if __TIMESIZE == 64
>  # define __settimeofday64 __settimeofday
> +# define ___gettimeofday64 ___gettimeofday
>  #else
>  extern int __settimeofday64 (const struct __timeval64 *tv,
>                               const struct timezone *tz);
>  libc_hidden_proto (__settimeofday64)
> +extern int ___gettimeofday64 (struct __timeval64 *restrict tv,
> +                              void *restrict tz);
> +libc_hidden_proto (___gettimeofday64)
>  #endif
>  
>  /* Compute the `struct tm' representation of T,

Why ___gettimeofday64 and not __gettimeofday?

> diff --git a/sysdeps/unix/sysv/linux/gettimeofday.c b/sysdeps/unix/sysv/linux/gettimeofday.c
> index d5cdb22495..728ef45eed 100644
> --- a/sysdeps/unix/sysv/linux/gettimeofday.c
> +++ b/sysdeps/unix/sysv/linux/gettimeofday.c
> @@ -54,5 +54,49 @@ __gettimeofday (struct timeval *restrict tv, void *restrict tz)
>  # endif
>  weak_alias (__gettimeofday, gettimeofday)

We still need to handle 32-bit architecture that define USE_IFUNC_GETTIMEOFDAY,
currently i686 and powerpc.

We can either:

  1. Define the INLINE_VSYSCALL __gettimeofday for USE_IFUNC_GETTIMEOFDAY,
     or ___gettimeofday that calls __clock_gettime64 otherwise.  The
     __gettimeofday64 will call either of this implementations.

  2. Do not define USE_IFUNC_GETTIMEOFDAY for i686 and powerpc32, and add
     a _Static_assert (__TIMESIZE == 64) within iFUNC definition.

I am more inclined to 2., it simplifies the somewhat macro convolution and
such optimization most likely won't make much difference for the specific
platforms.  Thoughts? 

>  #else /* USE_IFUNC_GETTIMEOFDAY  */
> -# include <time/gettimeofday.c>
> +/* Conversion of gettimeofday function to support 64 bit time on archs
> +   with __WORDSIZE == 32 and __TIMESIZE == 32/64  */
> +#include <errno.h>
> +
> +int
> +___gettimeofday64 (struct __timeval64 *restrict tv, void *restrict tz)
> +{
> +  if (__glibc_unlikely (tz != 0))
> +    memset (tz, 0, sizeof (struct timezone));
> +
> +  struct __timespec64 ts64;
> +  int ret = __clock_gettime64 (CLOCK_REALTIME, &ts64);
> +
> +  if (ret == 0 && tv)
> +    *tv = timespec64_to_timeval64 (ts64);

No implicit checks.  Also, we already set 'tv' with nonull attribute, so I
am not sure if it is worth to add an extra check for 'tv' validity
(specially because users tend to expect low latency for the symbol).

In any case, if the idea is to add such check as QoI I think it would
be better to do a early bail before actually issue __clock_gettime64.

> +
> +  return ret;
> +}

Ok (no 64-bit gettimeofday on any architecture).

> +
> +# if __TIMESIZE != 64
> +libc_hidden_def (___gettimeofday64)
> +
> +int
> +___gettimeofday (struct timeval *restrict tv, void *restrict tz)
> +{
> +  struct __timeval64 tv64;
> +  int ret = ___gettimeofday64 (&tv64, tz);
> +
> +  if (ret == 0)
> +    {
> +      if (! in_time_t_range (tv64.tv_sec))
> +        {
> +          __set_errno (EOVERFLOW);
> +          return -1;
> +        }
> +
> +      if (tv)
> +        *tv = valid_timeval64_to_timeval (tv64);
> +    }
> +
> +  return ret;
> +}
> +# endif
> +strong_alias (___gettimeofday, __gettimeofday)

I am failing to see the need of this alias.

> +weak_alias (___gettimeofday, gettimeofday)
>  #endif
>