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

[Adhemerval Zanella]

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.

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.

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

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

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