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

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

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?

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

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.

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.

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

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

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
  

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
>
  

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?

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

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

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

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
>
  

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
  

On Wed, 5 Feb 2020, Adhemerval Zanella wrote:

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

I don't see that being appropriate for a very long time.

Some symbols marked obsolescent by POSIX are genuinely out of use for a 
very long time, or genuinely have serious deficiencies, and may be 
candidates for obsoleting in various ways in glibc.  Some, such as 
gettimeofday are in very widespread use and pose no particular problems - 
so when it's not in current POSIX, I expect we should still declare it for 
__USE_MISC, without a deprecation attribute.  (And some symbols are marked 
obsolescent by POSIX because of issues specifying them portably, but are 
very relevant for less-portable code - vfork and getcontext, for example.)
  

On 06/02/2020 18:41, Joseph Myers wrote:
> On Wed, 5 Feb 2020, Adhemerval Zanella wrote:
> 
>> 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__.
> 
> I don't see that being appropriate for a very long time.
> 
> Some symbols marked obsolescent by POSIX are genuinely out of use for a 
> very long time, or genuinely have serious deficiencies, and may be 
> candidates for obsoleting in various ways in glibc.  Some, such as 
> gettimeofday are in very widespread use and pose no particular problems - 
> so when it's not in current POSIX, I expect we should still declare it for 
> __USE_MISC, without a deprecation attribute.  (And some symbols are marked 
> obsolescent by POSIX because of issues specifying them portably, but are 
> very relevant for less-portable code - vfork and getcontext, for example.)
> 

I see this as a possible option, I agree that for 'gettimeofday' due its
extensive usage I don't see much gain in setting it as deprecated in near 
future.
  

On 06/02/2020 07:08, Lukasz Majewski wrote:
>>>> 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

Only if the ABI have legacy 32 bits time support. More specifically,
for x86 implementation:

#ifdef __x86_64__
# define USE_IFUNC_GETTIMEOFDAY
#endif

And for powerpc:

#ifdef __powerpc64__
# define USE_IFUNC_GETTIMEOFDAY
#endif

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

My point is the vDSO ifunc optimization on a y2038 safe kernel does not
really matter, since there won't be a 64-bit time gettimeofday implementation
and all architectures will need to call clock_gettime64 (either by syscall or
vDSO) anyway.
  

Hi Adhemerval,

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

I've double check this and in the documentation/manual [1] for
gettimeofday there is a sentence:

----8<--------
If either tv or tz is NULL, the corresponding structure is not set or
returned. (However, compilation warnings will result if tv is NULL.) 
---->8--------

That was the rationale to add the check
if (ret == 0 && tv)


I also think that the code as is now is correct - it returns the result
of getting the time from Linux, but it is not updating the tv structure.


Links:

[1] - https://linux.die.net/man/2/gettimeofday


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
  

On 08/02/2020 19:15, Lukasz Majewski wrote:
> Hi Adhemerval,
> 
>>>>  #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.
> 
> I've double check this and in the documentation/manual [1] for
> gettimeofday there is a sentence:
> 
> ----8<--------
> If either tv or tz is NULL, the corresponding structure is not set or
> returned. (However, compilation warnings will result if tv is NULL.) 
> ---->8--------
> 
> That was the rationale to add the check
> if (ret == 0 && tv)
> 
> 
> I also think that the code as is now is correct - it returns the result
> of getting the time from Linux, but it is not updating the tv structure.

The man-pages is not really the glibc manual, but rather documents de facto
glibc/kernel behaviour.  The glibc 'gettimeofday' entry in manual 
(manual/time.texi) is also not explicit about this, and the generic 
implementation (time/gettimeofday.c) also does not add this test.

But again, I am not against of this change as QoI and it is what kernel vDSO
symbol does anyway (lib/vdso/gettimeofday.c:270).  However, I think we should
be done in a separated patch and for 'all' implementation to get a concise
behaviour.


> 
> 
> Links:
> 
> [1] - https://linux.die.net/man/2/gettimeofday
> 
> 
> Best regards,
> 
> Lukasz Majewski
>
  

Hi Adhemerval,

> On 08/02/2020 19:15, Lukasz Majewski wrote:
> > Hi Adhemerval,
> >   
> >>>>  #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.  
> > 
> > I've double check this and in the documentation/manual [1] for
> > gettimeofday there is a sentence:
> > 
> > ----8<--------
> > If either tv or tz is NULL, the corresponding structure is not set
> > or returned. (However, compilation warnings will result if tv is
> > NULL.) ---->8--------  
> > 
> > That was the rationale to add the check
> > if (ret == 0 && tv)
> > 
> > 
> > I also think that the code as is now is correct - it returns the
> > result of getting the time from Linux, but it is not updating the
> > tv structure.  
> 
> The man-pages is not really the glibc manual, but rather documents de
> facto glibc/kernel behaviour.  The glibc 'gettimeofday' entry in
> manual (manual/time.texi) is also not explicit about this, and the
> generic implementation (time/gettimeofday.c) also does not add this
> test.

Ok.

> 
> But again, I am not against of this change as QoI and it is what
> kernel vDSO symbol does anyway (lib/vdso/gettimeofday.c:270).

I do guess that you refer to:
https://elixir.bootlin.com/linux/latest/source/lib/vdso/gettimeofday.c#L144

We could rely on the kernel if there weren't conversions (64 <->
32 bit time ) needed. 

> However, I think we should be done in a separated patch and for 'all'
> implementation to get a concise behaviour.

Shall I:

1. Extend this patch to add this extra check to all eligible
gettimeofday places.

2. Do not check if tv is NULL at all

3. Do not check if tv is NULL in this particular patch and prepare next
patch which would add check for tv != NULL to all gettimeofday
implementations in glibc ?

> 
> 
> > 
> > 
> > Links:
> > 
> > [1] - https://linux.die.net/man/2/gettimeofday
> > 
> > 
> > Best regards,
> > 
> > Lukasz Majewski
> >   




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
  

On 10/02/2020 13:57, Lukasz Majewski wrote:
>>>> 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.  
>>>
>>> I've double check this and in the documentation/manual [1] for
>>> gettimeofday there is a sentence:
>>>
>>> ----8<--------
>>> If either tv or tz is NULL, the corresponding structure is not set
>>> or returned. (However, compilation warnings will result if tv is
>>> NULL.) ---->8--------  
>>>
>>> That was the rationale to add the check
>>> if (ret == 0 && tv)
>>>
>>>
>>> I also think that the code as is now is correct - it returns the
>>> result of getting the time from Linux, but it is not updating the
>>> tv structure.  
>>
>> The man-pages is not really the glibc manual, but rather documents de
>> facto glibc/kernel behaviour.  The glibc 'gettimeofday' entry in
>> manual (manual/time.texi) is also not explicit about this, and the
>> generic implementation (time/gettimeofday.c) also does not add this
>> test.
> 
> Ok.
> 
>>
>> But again, I am not against of this change as QoI and it is what
>> kernel vDSO symbol does anyway (lib/vdso/gettimeofday.c:270).
> 
> I do guess that you refer to:
> https://elixir.bootlin.com/linux/latest/source/lib/vdso/gettimeofday.c#L144

Yes (I was using current master branch instead of a tag release).

> 
> We could rely on the kernel if there weren't conversions (64 <->
> 32 bit time ) needed. 

My point was that kernel vDSO gettimeofday implementation already does 
the tv check before setting its value, so it should be an QoI to do this
on glibc as well.

> 
>> However, I think we should be done in a separated patch and for 'all'
>> implementation to get a concise behaviour.
> 
> Shall I:
> 
> 1. Extend this patch to add this extra check to all eligible
> gettimeofday places.
> 
> 2. Do not check if tv is NULL at all
> 
> 3. Do not check if tv is NULL in this particular patch and prepare next
> patch which would add check for tv != NULL to all gettimeofday
> implementations in glibc ?

I would prefer to remove the check on this patch and send an extra
patch to actually adding the check on all implementations.

> 
>>
>>
>>>
>>>
>>> Links:
>>>
>>> [1] - https://linux.die.net/man/2/gettimeofday
>>>
>>>
>>> Best regards,
>>>
>>> Lukasz Majewski
>>>   
> 
> 
> 
> 
> 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
>