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

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

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 only ABI that requires versioning (alpha) includes
the generic implementation with a arch-specific implementation.

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

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

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