Re: [PATCH v6] y2038: linux: Provide __gettimeofday64 implementation

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

On 12/02/2020 05:45, 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 refactored and
> moved to ./sysdeps/unix/sysv/linux/gettimeofday.c to be Linux specific.

In fact the function implementation is replicated on Linux version. Once this
patch is upstream I plan to make a generic implementation and refactor alpha,
Linux, and generic implementation to avoid code duplication.

> 
> 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.

You can remove this part about alpha from commit message, this
patch does not change anything about alpha behaviour.

> 
> The USE_IFUNC_GETTIMEOFDAY for powerpc and i686 has been undefined on
> purpose. Due to that the support for gettimeofday vDSO on them has been
> traded for Y2038 safeness (as this syscall is going to be obsolete).

I think it would be useful to extend a bit why this optimization was
removed from i686 and powerpc32.  Maybe add:

  The iFUNC vDSO direct call optimization has been removed from both
  i686 and powerpc32 (USE_IFUNC_GETTIMEOFDAY is not defined for the
  architectures anymore).  The Linux kernel does not provide a y2038
  safe implementation of gettimeofday neither it plans to provide it
  in the future, clock_gettime64 should be use instead.  It meant to
  keep support for this optimization it would require to handle another
  build permutation (!__ASSUME_TIME64_SYSCALLS && USE_IFUNC_GETTIMEOFDAY)
  with adds more complexity and has limited use (since the idea is to
  eventually have a y2038 safe glibc build).

> 
> 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.

LGTM with the suggested commit message changes.

Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>

> 
> ---
> Changes for v6:
> - Do not check if tv pointer is NULL as in generic implementation of
>   ./time/gettimeofday.c. Moreover, prototype for this function has
>   __nonnull GCC attribute for this parameter.
> 
> Changes for v5:
> - Replace '___gettimeofday64' with '__gettimeofday64' in the subject of the
>   patch
> 
> Changes for v4:
> - Rename ___gettimeofday{64} to __gettimeofday{64} as '___' prefix is not
>   needed for our implementation
> - Correctly handle the case when tv is NULL (also in __gettimeofday).
> - Do not define USE_IFUNC_GETTIMEOFDAY for 32 bit archs - namely powerpc and
>   i686.
> 
> Changes for v3:
> - New patch
> ---
>  include/time.h                                |  4 ++
>  sysdeps/unix/sysv/linux/gettimeofday.c        | 40 ++++++++++++++++++-
>  .../unix/sysv/linux/powerpc/gettimeofday.c    |  4 +-
>  sysdeps/unix/sysv/linux/x86/gettimeofday.c    |  4 +-
>  4 files changed, 49 insertions(+), 3 deletions(-)
> 
> diff --git a/include/time.h b/include/time.h
> index 73f66277ac..61806658e7 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,

Ok.

> diff --git a/sysdeps/unix/sysv/linux/gettimeofday.c b/sysdeps/unix/sysv/linux/gettimeofday.c
> index d5cdb22495..cb57bc9cf2 100644
> --- a/sysdeps/unix/sysv/linux/gettimeofday.c
> +++ b/sysdeps/unix/sysv/linux/gettimeofday.c
> @@ -54,5 +54,43 @@ __gettimeofday (struct timeval *restrict tv, void *restrict tz)
>  # endif
>  weak_alias (__gettimeofday, gettimeofday)
>  #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;
> +  if (__clock_gettime64 (CLOCK_REALTIME, &ts64))
> +	  return -1;
> +
> +  *tv = timespec64_to_timeval64 (ts64);
> +  return 0;
> +}
> +
> +# if __TIMESIZE != 64
> +libc_hidden_def (__gettimeofday64)
> +
> +int
> +__gettimeofday (struct timeval *restrict tv, void *restrict tz)
> +{
> +  struct __timeval64 tv64;
> +  if (__gettimeofday64 (&tv64, tz))
> +	  return -1;
> +
> +  if (! in_time_t_range (tv64.tv_sec))
> +    {
> +      __set_errno (EOVERFLOW);
> +      return -1;
> +    }
> +
> +  *tv = valid_timeval64_to_timeval (tv64);
> +  return 0;
> +}
> +# endif
> +weak_alias (__gettimeofday, gettimeofday)
>  #endif

Ok.

> diff --git a/sysdeps/unix/sysv/linux/powerpc/gettimeofday.c b/sysdeps/unix/sysv/linux/powerpc/gettimeofday.c
> index 183fb0ac70..2d6978f333 100644
> --- a/sysdeps/unix/sysv/linux/powerpc/gettimeofday.c
> +++ b/sysdeps/unix/sysv/linux/powerpc/gettimeofday.c
> @@ -15,5 +15,7 @@
>     License along with the GNU C Library; if not, see
>     <https://www.gnu.org/licenses/>.  */
>  
> -#define USE_IFUNC_GETTIMEOFDAY
> +#ifdef __powerpc64__
> +# define USE_IFUNC_GETTIMEOFDAY
> +#endif
>  #include <sysdeps/unix/sysv/linux/gettimeofday.c>

Ok.

> diff --git a/sysdeps/unix/sysv/linux/x86/gettimeofday.c b/sysdeps/unix/sysv/linux/x86/gettimeofday.c
> index 1b7aa880a2..0c1779dc83 100644
> --- a/sysdeps/unix/sysv/linux/x86/gettimeofday.c
> +++ b/sysdeps/unix/sysv/linux/x86/gettimeofday.c
> @@ -16,5 +16,7 @@
>     License along with the GNU C Library; if not, see
>     <https://www.gnu.org/licenses/>.  */
>  
> -#define USE_IFUNC_GETTIMEOFDAY
> +#ifdef __x86_64__
> +# define USE_IFUNC_GETTIMEOFDAY
> +#endif
>  #include <sysdeps/unix/sysv/linux/gettimeofday.c>
> 

Ok.