Hi Adhemerval, Thank you for the review. > On 12/02/2020 05:45, 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 > > refactored and moved to ./sysdeps/unix/sysv/linux/gettimeofday.c to > > be Linux specific. > > In fact the function implementation is replicated on Linux version. > Once this patch is upstream I plan to make a generic implementation > and refactor alpha, Linux, and generic implementation to avoid code > duplication. Ok. I shall apply this patch till the end of the day. > > > > > 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. > > You can remove this part about alpha from commit message, this > patch does not change anything about alpha behaviour. Ok. > > > > > The USE_IFUNC_GETTIMEOFDAY for powerpc and i686 has been undefined > > on purpose. Due to that the support for gettimeofday vDSO on them > > has been traded for Y2038 safeness (as this syscall is going to be > > obsolete). > > I think it would be useful to extend a bit why this optimization was > removed from i686 and powerpc32. Maybe add: > > The iFUNC vDSO direct call optimization has been removed from both > i686 and powerpc32 (USE_IFUNC_GETTIMEOFDAY is not defined for the > architectures anymore). The Linux kernel does not provide a y2038 > safe implementation of gettimeofday neither it plans to provide it > in the future, clock_gettime64 should be use instead. It meant to > keep support for this optimization it would require to handle > another build permutation (!__ASSUME_TIME64_SYSCALLS && > USE_IFUNC_GETTIMEOFDAY) with adds more complexity and has limited use > (since the idea is to eventually have a y2038 safe glibc build). > Ok. I will add this description. > > > > 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. > > LGTM with the suggested commit message changes. > > Reviewed-by: Adhemerval Zanella Thanks. Could you also review following patch set: https://patchwork.ozlabs.org/cover/1234918/ (It seems to be straightforward and has been posted to the mailing list some time ago). Thanks in advance. > > > > > --- > > Changes for v6: > > - Do not check if tv pointer is NULL as in generic implementation of > > ./time/gettimeofday.c. Moreover, prototype for this function has > > __nonnull GCC attribute for this parameter. > > > > Changes for v5: > > - Replace '___gettimeofday64' with '__gettimeofday64' in the > > subject of the patch > > > > Changes for v4: > > - Rename ___gettimeofday{64} to __gettimeofday{64} as '___' prefix > > is not needed for our implementation > > - Correctly handle the case when tv is NULL (also in > > __gettimeofday). > > - Do not define USE_IFUNC_GETTIMEOFDAY for 32 bit archs - namely > > powerpc and i686. > > > > Changes for v3: > > - New patch > > --- > > include/time.h | 4 ++ > > sysdeps/unix/sysv/linux/gettimeofday.c | 40 > > ++++++++++++++++++- .../unix/sysv/linux/powerpc/gettimeofday.c | > > 4 +- sysdeps/unix/sysv/linux/x86/gettimeofday.c | 4 +- > > 4 files changed, 49 insertions(+), 3 deletions(-) > > > > diff --git a/include/time.h b/include/time.h > > index 73f66277ac..61806658e7 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, > > Ok. > > > diff --git a/sysdeps/unix/sysv/linux/gettimeofday.c > > b/sysdeps/unix/sysv/linux/gettimeofday.c index > > d5cdb22495..cb57bc9cf2 100644 --- > > a/sysdeps/unix/sysv/linux/gettimeofday.c +++ > > b/sysdeps/unix/sysv/linux/gettimeofday.c @@ -54,5 +54,43 @@ > > __gettimeofday (struct timeval *restrict tv, void *restrict tz) # > > endif weak_alias (__gettimeofday, gettimeofday) > > #else /* USE_IFUNC_GETTIMEOFDAY */ > > -# include