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

[Lukasz Majewski <lukma at denx dot de>]

Hi Adhemerval,

> On 05/02/2020 13:58, Lukasz Majewski wrote:
> > Hi Adhemerval,
> >   
> >> On 04/02/2020 21:05, Lukasz Majewski wrote:  
> >>> 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?    
> >>
> >> Not really wrong, but now that Linux implementation is not using
> >> the generic implementation any longer the extra strong_alias is
> >> redundant.   
> > 
> > The strong alias would be needed only if we would call
> > __gettimeofday internally in glibc. I've grep'ed the source and
> > there were no references to it.
> > Is that the rationale for removing the strong alias?  
> 
> The triple underscore on 'time/gettimeofday.c' is only an
> implementation detail for this specific code (it could any other
> name).  It was done to tie a single gettimeofday implementation to
> two versioned symbols (gettimeofday and __gettimeofday) and the
> triple underscore is a trick to avoid a double symbol definition by
> the static linker.
> 

Thanks for the explanation. I will rename ___gettimeofday{64} to
__gettimeofday{64} in the next version of this patch.

> However this specific code for alpha is not really required, since
> 04da832e16a7 it implements its gettimeofday version in its own 
> syscalls.list. So this code is not used anywhere (along with alpha
> gettimeofday.c implementation) and I push a patch to remove it.

Ok.

> 
> > 
> > And why would we need the strong_alias(___gettimeofday,
> > __gettimeofday) in ./time/gettimeofday,c ?
> > In the same file the ___gettimeofday is aliased (as weak) to "final"
> > gettimeofday symbol.
> >   
> >> The only ABI that requires versioning (alpha) includes
> >> the generic implementation with a arch-specific implementation.  
> > 
> > Yes.
> >   
> >>  
> >>>     
> >>>>    
> >>>>> 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).
> >>>     
> >>
> >> Yes, on powerpc32 and i686 (which defines USE_IFUNC_GETTIMEOFDAY)
> >> won't use the y2038 safe implementation.  
> > 
> > As the gettimeofday is going to be obsolete, why should we care (and
> > implement the fallback for this vDSO syscall)?   
> 
> Although marked obsolescent by POSIX, we can't really removed since a
> program might require to build against a specific POSIX version 
> (_POSIX_C_SOURCE).
> 
> Once POSIX does remove the symbol, we can remove its definition from
> default visibility (usually by setting the latest POSIX as default
> version) and add __attribute_deprecated__.
> 
> So we still need to adapt it to be y2038 proof on current release.

Ok.

> 
> >   
> >>  
> >>>>
> >>>> 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 really don't think optimizing USE_IFUNC_GETTIMEOFDAY for
> >> architectures which still support fallback time32 is worth.  It
> >> will require much more code complexity and it would result in more
> >> build permutations (one for !__ASSUME_TIME64_SYSCALLS which calls
> >> the vDSO through iFUNC, another one that calls the vDSO fallback
> >> and finally the one that call clock_gettime).  Also, on build with
> >> time64 support the vDSO will only used as fallback.
> >>
> >> I would say to just not define USE_IFUNC_GETTIMEOFDAY for i686 and
> >> powerpc32 and make them follow the expected code path for y2038
> >> safeness.  
> > 
> > Shall this be added to this patch? Or shall I left the
> > USE_IFUNC_GETTIMEOFDAY untouched?  
> 
> Since this patch aims to add time64 gettimeofday for all supported
> Linux ABI, I think it should be along this patch.

So the proposed solution here is to remove 

#define USE_IFUNC_GETTIMEOFDAY

from 

sysdeps/unix/sysv/linux/powerpc/gettimeofday.c
sysdeps/unix/sysv/linux/x86/gettimeofday.c

?

I do guess that it would be acceptable to trade some minimal extra
overhead for gettimeofday serving on those archs for glibc simplicity
and Y2038 safeness?

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




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: pgp22PqjnQBFi.pgp
Description: OpenPGP digital signature