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

[Lukasz Majewski <lukma at denx dot de>]

Hi Adhemerval,

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

Unfortunately, the __gettimeofday was already used in some symbol
aliasing in ./time/gettimeofday.c for alpha.

To avoid any potential name clashing (when using both symbols -
__gettimeofday/__gettimeofday64 with aliasing), I've decided to
introduce new ones - namely ___gettimeofday/___gettimeofday64.

Or maybe I'm wrong here?

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

By "handle" do you mean to make them Y2038 safe? The code in this patch
target archs, which do not have vDSO support for gettimeofday (like
arm32).

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

Ok, so we do need a fallback for vDSO gettimeofday. I will poke around
and try to add this in v2 (but I'm only runtime testing Y2039 on ARM32).

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

No, this was just my mistake. There was a discussion with Paul and
Joseph earlier. We shall _only_ check for NULL when it is required by
syscalls/command documentation. This is the case for e.g. setitimer's
*old_value pointer.

In other examples we just allow segfault when dereference the NULL
pointer.

I will fix it in v2 of this patch series.

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

I've followed the idiom from ./time/gettimeofday.c

The strong alias will bind this local definition of ___gettimeofday to
glibc's internal __gettimeofday (used internally by glibc).

In the case where gettimeofday exported function is not defined, the
___gettimeofday will be a weak alias for it.


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




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

Attachment: pgpD_GYMwHCpB.pgp
Description: OpenPGP digital signature