| Message ID | cb015d0d1d29e4b948c7118c5b12ff2bed83a6ec.1561421042.git.alistair.francis@wdc.com |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC,v2,01/20] y2038: Introduce internal for glibc struct __timespec64 | expand |
On 24/06/2019 21:09, Alistair Francis wrote: > Not all architectures support the obsolete gettimeofday so use the > newer clock_gettime64 syscall if it is avaliable. This fixes RV32 > build issues. > > Signed-off-by: Alistair Francis <alistair.francis@wdc.com> > --- > ChangeLog | 1 + > sysdeps/unix/sysv/linux/gettimeofday.c | 28 ++++++++++++++++++++++++++ > 2 files changed, 29 insertions(+) > > diff --git a/ChangeLog b/ChangeLog > index a700783ef3..f1c7acb6ab 100644 > --- a/ChangeLog > +++ b/ChangeLog > @@ -4,6 +4,7 @@ > * sysdeps/unix/sysv/linux/nanosleep.c: Likewise. > * sysdeps/unix/sysv/linux/nanosleep_nocancel.c: Likewise. > * sysdeps/unix/sysv/linux/lowlevellock-futex.h: Use __NR_futex_time64 if we don't have __NR_futex. > + * sysdeps/unix/sysv/linux/gettimeofday.c: Use clock_gettime64 syscall for gettimeofday. > > 2019-06-20 Dmitry V. Levin <ldv@altlinux.org> > Florian Weimer <fweimer@redhat.com> > diff --git a/sysdeps/unix/sysv/linux/gettimeofday.c b/sysdeps/unix/sysv/linux/gettimeofday.c > index a74f03825a..3d2b943123 100644 > --- a/sysdeps/unix/sysv/linux/gettimeofday.c > +++ b/sysdeps/unix/sysv/linux/gettimeofday.c > @@ -32,7 +32,35 @@ > int > __gettimeofday (struct timeval *tv, struct timezone *tz) > { > +#ifdef __ASSUME_TIME64_SYSCALLS > + int ret; > + struct timespec now; > + > + ret = INLINE_VSYSCALL (clock_gettime64, 2, CLOCK_REALTIME, > + &now); > + > + /* Convert from timespec to timeval */ > + tv->tv_sec = now.tv_sec; > + tv->tv_usec = now.tv_nsec / 1000; > + > + return ret; > +#else > +# ifdef __NR_clock_gettime64 > + long int ret; > + struct timespec now; > + > + ret = INLINE_VSYSCALL (clock_gettime64, 2, CLOCK_REALTIME, > + &now); > + > + /* Convert from timespec to timeval */ > + tv->tv_sec = now.tv_sec; > + tv->tv_usec = now.tv_nsec / 1000; > + > + if (ret == 0 || errno != ENOSYS) > + return ret; > +# endif > return INLINE_VSYSCALL (gettimeofday, 2, tv, tz); > +#endif > } > libc_hidden_def (__gettimeofday) > weak_alias (__gettimeofday, gettimeofday) > Wouldn't be simpler to just call __clock_gettime instead: -- __gettimeofday (struct timeval *tv, struct timezone *tz) { if (tv == NULL) return 0; struct timespec ts; __clock_gettime (CLOCK_REALTIME, &ts); tv->tv_sec = ts.tv_sec; tv->tv_usec = (__suseconds_t)ts.tv_nsec / 1000; return 0; } -- From the patch 'linux: Provide __clock_settime64 implementation' internal include/time.h will redefine __clock_gettime to __clock_gettime64 if the case.
On Thu, Jun 27, 2019 at 6:15 AM Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote: > > > > On 24/06/2019 21:09, Alistair Francis wrote: > > Not all architectures support the obsolete gettimeofday so use the > > newer clock_gettime64 syscall if it is avaliable. This fixes RV32 > > build issues. > > > > Signed-off-by: Alistair Francis <alistair.francis@wdc.com> > > --- > > ChangeLog | 1 + > > sysdeps/unix/sysv/linux/gettimeofday.c | 28 ++++++++++++++++++++++++++ > > 2 files changed, 29 insertions(+) > > > > diff --git a/ChangeLog b/ChangeLog > > index a700783ef3..f1c7acb6ab 100644 > > --- a/ChangeLog > > +++ b/ChangeLog > > @@ -4,6 +4,7 @@ > > * sysdeps/unix/sysv/linux/nanosleep.c: Likewise. > > * sysdeps/unix/sysv/linux/nanosleep_nocancel.c: Likewise. > > * sysdeps/unix/sysv/linux/lowlevellock-futex.h: Use __NR_futex_time64 if we don't have __NR_futex. > > + * sysdeps/unix/sysv/linux/gettimeofday.c: Use clock_gettime64 syscall for gettimeofday. > > > > 2019-06-20 Dmitry V. Levin <ldv@altlinux.org> > > Florian Weimer <fweimer@redhat.com> > > diff --git a/sysdeps/unix/sysv/linux/gettimeofday.c b/sysdeps/unix/sysv/linux/gettimeofday.c > > index a74f03825a..3d2b943123 100644 > > --- a/sysdeps/unix/sysv/linux/gettimeofday.c > > +++ b/sysdeps/unix/sysv/linux/gettimeofday.c > > @@ -32,7 +32,35 @@ > > int > > __gettimeofday (struct timeval *tv, struct timezone *tz) > > { > > +#ifdef __ASSUME_TIME64_SYSCALLS > > + int ret; > > + struct timespec now; > > + > > + ret = INLINE_VSYSCALL (clock_gettime64, 2, CLOCK_REALTIME, > > + &now); > > + > > + /* Convert from timespec to timeval */ > > + tv->tv_sec = now.tv_sec; > > + tv->tv_usec = now.tv_nsec / 1000; > > + > > + return ret; > > +#else > > +# ifdef __NR_clock_gettime64 > > + long int ret; > > + struct timespec now; > > + > > + ret = INLINE_VSYSCALL (clock_gettime64, 2, CLOCK_REALTIME, > > + &now); > > + > > + /* Convert from timespec to timeval */ > > + tv->tv_sec = now.tv_sec; > > + tv->tv_usec = now.tv_nsec / 1000; > > + > > + if (ret == 0 || errno != ENOSYS) > > + return ret; > > +# endif > > return INLINE_VSYSCALL (gettimeofday, 2, tv, tz); > > +#endif > > } > > libc_hidden_def (__gettimeofday) > > weak_alias (__gettimeofday, gettimeofday) > > > > Wouldn't be simpler to just call __clock_gettime instead: > > -- > __gettimeofday (struct timeval *tv, struct timezone *tz) > { > if (tv == NULL) > return 0; > > struct timespec ts; > __clock_gettime (CLOCK_REALTIME, &ts); > tv->tv_sec = ts.tv_sec; > tv->tv_usec = (__suseconds_t)ts.tv_nsec / 1000; > return 0; > } > -- > > From the patch 'linux: Provide __clock_settime64 implementation' internal > include/time.h will redefine __clock_gettime to __clock_gettime64 if the > case. The version of this patch that I have only implements __clock_settime and not __clock_gettime. Alistair
On Mon, 24 Jun 2019, Alistair Francis wrote: > Not all architectures support the obsolete gettimeofday so use the > newer clock_gettime64 syscall if it is avaliable. This fixes RV32 > build issues. A key question when using newer syscalls to implement gettimeofday or settimeofday, which the commit message needs to answer, is: what are the semantics for how the obsolete tz argument is handled, if not NULL? I'd expect the proposed commit message to include a discussion of what the semantics are for that obsolete argument in Linux kernel versions supported by glibc, and what the semantics are for that argument in glibc when some other syscall gets used by glibc. For example, I'd expect that if tz is not NULL, any implementation using other syscalls would at least fill in some dummy values in *tz, if that's what the gettimeofday syscall would do. And on architectures where the gettimeofday / settimeofday syscalls exist, if it is the case that some information gets passed from settimeofday to gettimeofday through this argument, I'd expect that information to continue to get passed through rather than being lost through the use of newer syscalls. (You could reasonably argue for not supporting anything with that argument in the _TIME_BITS=64 case, but there could still be issues of keeping ABI compatibility for this argument for existing ABIs with 32-bit time, unless the current kernel semantics don't actually support doing anything with this argument anyway. In any case, we need that explanation of current semantics in order to review any semantic changes from the patch.)
On Wed, Jul 24, 2019 at 1:22 PM Joseph Myers <joseph@codesourcery.com> wrote: > > On Mon, 24 Jun 2019, Alistair Francis wrote: > > > Not all architectures support the obsolete gettimeofday so use the > > newer clock_gettime64 syscall if it is avaliable. This fixes RV32 > > build issues. > > A key question when using newer syscalls to implement gettimeofday or > settimeofday, which the commit message needs to answer, is: what are the > semantics for how the obsolete tz argument is handled, if not NULL? > > I'd expect the proposed commit message to include a discussion of what the > semantics are for that obsolete argument in Linux kernel versions > supported by glibc, and what the semantics are for that argument in glibc > when some other syscall gets used by glibc. Yep, I missed this. I have already added it to the commit message of the next version. This has the side effect of not setting the struct timezone *tz variable if __ASSUME_TIME64_SYSCALLS or __NR_clock_gettime64 is defined. There are two things to consider here: - 32-bit systems with __ARCH_WANT_TIME32_SYSCALLS not defined have to way to get the struct timezone via a syscall. - The Linux documentation says that "The use of the timezone structure is obsolete; the tz argument should normally be specified as NULL." So let's not worry about it. Alistair > > For example, I'd expect that if tz is not NULL, any implementation using > other syscalls would at least fill in some dummy values in *tz, if that's > what the gettimeofday syscall would do. And on architectures where the > gettimeofday / settimeofday syscalls exist, if it is the case that some > information gets passed from settimeofday to gettimeofday through this > argument, I'd expect that information to continue to get passed through > rather than being lost through the use of newer syscalls. (You could > reasonably argue for not supporting anything with that argument in the > _TIME_BITS=64 case, but there could still be issues of keeping ABI > compatibility for this argument for existing ABIs with 32-bit time, unless > the current kernel semantics don't actually support doing anything with > this argument anyway. In any case, we need that explanation of current > semantics in order to review any semantic changes from the patch.) > > -- > Joseph S. Myers > joseph@codesourcery.com
On Thu, Jul 25, 2019 at 1:03 AM Alistair Francis <alistair23@gmail.com> wrote: > > On Wed, Jul 24, 2019 at 1:22 PM Joseph Myers <joseph@codesourcery.com> wrote: > > > > On Mon, 24 Jun 2019, Alistair Francis wrote: > > > > > Not all architectures support the obsolete gettimeofday so use the > > > newer clock_gettime64 syscall if it is avaliable. This fixes RV32 > > > build issues. > > > > A key question when using newer syscalls to implement gettimeofday or > > settimeofday, which the commit message needs to answer, is: what are the > > semantics for how the obsolete tz argument is handled, if not NULL? > > > > I'd expect the proposed commit message to include a discussion of what the > > semantics are for that obsolete argument in Linux kernel versions > > supported by glibc, and what the semantics are for that argument in glibc > > when some other syscall gets used by glibc. > > Yep, I missed this. I have already added it to the commit message of > the next version. > > This has the side effect of not setting the struct timezone *tz variable > if __ASSUME_TIME64_SYSCALLS or __NR_clock_gettime64 is defined. There > are two things to consider here: > - 32-bit systems with __ARCH_WANT_TIME32_SYSCALLS not defined have to > way to get the struct timezone via a syscall. > - The Linux documentation says that "The use of the timezone structure > is obsolete; the tz argument should normally be specified as NULL." > So let's not worry about it. That's fine with me, but I would like to point out that the omission of the timezone get/set interfaces was not intentional. I think I considered the POSIX man page (which says undefined behavior) and I ported musl over (which ignores the timezone argument), but I did not realize that glibc passes it on and that there are applications that make use of that. Using debian code search, I found hwclock as something that sets the timezone at boot, and there are a small number of kernel files that use the information at runtime (full list below). If we want to keep the traditional settimeofday()/gettimeofday() behavior working, a new kernel interface could be added, e.g. in one of the reserved fields of clock_adjtime(), or as a new syscall. Arnd --- drivers/media/platform/vivid/vivid-rds-gen.c: (sys_tz.tz_minuteswest >= 0 ? 0x20 : 0) | drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c: record->end_utc_bias = cpu_to_le16(sys_tz.tz_minuteswest * 60); drivers/scsi/3w-9xxx.c: local_time = (u32)(ktime_get_real_seconds() - (sys_tz.tz_minuteswest * 60)); drivers/scsi/3w-sas.c: local_time = (u32)(ktime_get_real_seconds() - (sys_tz.tz_minuteswest * 60)); drivers/scsi/aacraid/commsup.c: local_time = (now->tv_sec - (sys_tz.tz_minuteswest * 60)); drivers/scsi/arcmsr/arcmsr_hba.c: time64_to_tm(ktime_get_real_seconds(), -sys_tz.tz_minuteswest * 60, &tm); drivers/scsi/mvumi.c: local_time = (time - (sys_tz.tz_minuteswest * 60)); drivers/scsi/smartpqi/smartpqi_init.c: time64_to_tm(local_time, -sys_tz.tz_minuteswest * 60, &tm); fs/affs/amigaffs.c: secs -= sys_tz.tz_minuteswest * 60 + ((8 * 365 + 2) * 24 * 60 * 60); fs/affs/inode.c: sys_tz.tz_minuteswest * 60; fs/fat/misc.c: sys_tz.tz_minuteswest) * SECS_PER_MIN; fs/hfs/hfs_fs.h: return ut + sys_tz.tz_minuteswest * 60; fs/hfs/inode.c: HFS_I(inode)->tz_secondswest = sys_tz.tz_minuteswest * 60; fs/hfs/sysdep.c: diff = sys_tz.tz_minuteswest * 60 - HFS_I(inode)->tz_secondswest; fs/hpfs/hpfs_fn.h: return t + sys_tz.tz_minuteswest * 60 + hpfs_sb(s)->sb_timeshift; fs/udf/udftime.c: offset = -sys_tz.tz_minuteswest; kernel/debug/kdb/kdb_main.c: sys_tz.tz_minuteswest); kernel/time/ntp.c: adjust.tv_sec -= (sys_tz.tz_minuteswest * 60); kernel/time/timekeeping.c: if (sys_tz.tz_minuteswest != 0) { kernel/time/vsyscall.c: vdata[CS_HRES_COARSE].tz_minuteswest = sys_tz.tz_minuteswest; lib/vdso/gettimeofday.c: tz->tz_minuteswest = vd[CS_HRES_COARSE].tz_minuteswest; net/netfilter/xt_time.c: stamp -= 60 * sys_tz.tz_minuteswest;
Arnd Bergmann wrote: > If we want to keep > the traditional settimeofday()/gettimeofday() behavior working, a new > kernel interface could be added Let's not. That behavior was a bad idea even in the 1980s, and applications stopped using it decades ago. It has been completely obsoleted by TZ strings.
On Thu, Jul 25, 2019 at 1:03 PM Paul Eggert <eggert@cs.ucla.edu> wrote: > > Arnd Bergmann wrote: > > If we want to keep > > the traditional settimeofday()/gettimeofday() behavior working, a new > > kernel interface could be added > > Let's not. That behavior was a bad idea even in the 1980s, and applications > stopped using it decades ago. It has been completely obsoleted by TZ strings. Do we think we could get away with having both functions fail (with EINVAL) whenever the tz argument is non-null? zw
On Thu, Jul 25, 2019 at 7:21 PM Zack Weinberg <zackw@panix.com> wrote: > > On Thu, Jul 25, 2019 at 1:03 PM Paul Eggert <eggert@cs.ucla.edu> wrote: > > > > Arnd Bergmann wrote: > > > If we want to keep > > > the traditional settimeofday()/gettimeofday() behavior working, a new > > > kernel interface could be added > > > > Let's not. That behavior was a bad idea even in the 1980s, and applications > > stopped using it decades ago. It has been completely obsoleted by TZ strings. > > Do we think we could get away with having both functions fail (with > EINVAL) whenever the tz argument is non-null? From my findings at Debian code search, I found code like struct timeval my_gettime(void) { struct timezone tz_ignored; struct timeval tv; gettimeofday(&tv, &tz_ignored); return tv; } In this case, the safer choice would be to silently ignore it. Another alternative would be to hide the definition of 'struct timezone' in the libc headers and only leave a forward declaration. That would lead to a compile-time error here, and force a change in any source code that actually tries to use the timezone in a meaningful way. We also need to deal with other compile-time failures for code that is not y2038 safe (e.g. __NR_futex users), so this may be a reasonable compromise. Arnd
On Wed, 24 Jul 2019, Alistair Francis wrote: > > A key question when using newer syscalls to implement gettimeofday or > > settimeofday, which the commit message needs to answer, is: what are the > > semantics for how the obsolete tz argument is handled, if not NULL? > > > > I'd expect the proposed commit message to include a discussion of what the > > semantics are for that obsolete argument in Linux kernel versions > > supported by glibc, and what the semantics are for that argument in glibc > > when some other syscall gets used by glibc. > > Yep, I missed this. I have already added it to the commit message of > the next version. > > This has the side effect of not setting the struct timezone *tz variable > if __ASSUME_TIME64_SYSCALLS or __NR_clock_gettime64 is defined. There > are two things to consider here: > - 32-bit systems with __ARCH_WANT_TIME32_SYSCALLS not defined have to > way to get the struct timezone via a syscall. > - The Linux documentation says that "The use of the timezone structure > is obsolete; the tz argument should normally be specified as NULL." > So let's not worry about it. What I'd like to see in the commit message is: 1. A detailed discussion of what the current ABI is regarding this argument (not how some systems used to use it for timezone handling, just how the Linux kernel handles it as an argument to the gettimeofday and settimeofday syscalls). 2. A discussion of how the patch achieves maximum ABI and API compatibility with existing programs using this argument (and in turn, the patch should be implemented so as to achieve that compatibility). There, I'd suggest: (a) For existing glibc ABIs, the tz value should continue to be passed to / from the kernel exactly as at present (which might mean continuing to use the old syscalls when implementing those functions, even when newer syscalls are available - whether this value is passed to/from the kernel should not depend on __ASSUME_TIME64_SYSCALLS). (b) For new ABIs, including _TIME_BITS=64 for systems that currently have 32-bit time in glibc, you can make a reasonable case for not passing the argument to / from the kernel, at least if the special case mentioned in the settimeofday manpage for the hardware clock being in local time can be justified as being irrelevant for such ABIs. But in this case I'd still think you should at least set *tz to some sensible default value in gettimeofday rather than leaving it as uninitialized data. (Thus the tz argument might result in gettimeofday being an exception to the normal rule of 32-bit functions being thin wrappers around 64-bit ones, if the _TIME_BITS=64 version ends up handling tz differently from how the 32-bit syscall does.)
* Arnd Bergmann: > On Thu, Jul 25, 2019 at 7:21 PM Zack Weinberg <zackw@panix.com> wrote: >> >> On Thu, Jul 25, 2019 at 1:03 PM Paul Eggert <eggert@cs.ucla.edu> wrote: >> > >> > Arnd Bergmann wrote: >> > > If we want to keep >> > > the traditional settimeofday()/gettimeofday() behavior working, a new >> > > kernel interface could be added >> > >> > Let's not. That behavior was a bad idea even in the 1980s, and applications >> > stopped using it decades ago. It has been completely obsoleted by TZ strings. >> >> Do we think we could get away with having both functions fail (with >> EINVAL) whenever the tz argument is non-null? > > From my findings at Debian code search, I found code like > > struct timeval my_gettime(void) > { > struct timezone tz_ignored; > struct timeval tv; > gettimeofday(&tv, &tz_ignored); > return tv; > } > > In this case, the safer choice would be to silently ignore it. > > Another alternative would be to hide the definition of 'struct timezone' > in the libc headers and only leave a forward declaration. Renaming the struct timezone members might be sufficient. Then the code above would still compile, but something that actually depends on the struct timezone data would not. Thanks, Florian
On Fri, Jul 26, 2019 at 9:01 AM Florian Weimer <fweimer@redhat.com> wrote: > > * Arnd Bergmann: > > > On Thu, Jul 25, 2019 at 7:21 PM Zack Weinberg <zackw@panix.com> wrote: > >> > >> On Thu, Jul 25, 2019 at 1:03 PM Paul Eggert <eggert@cs.ucla.edu> wrote: > >> > > >> > Arnd Bergmann wrote: > >> > > If we want to keep > >> > > the traditional settimeofday()/gettimeofday() behavior working, a new > >> > > kernel interface could be added > >> > > >> > Let's not. That behavior was a bad idea even in the 1980s, and applications > >> > stopped using it decades ago. It has been completely obsoleted by TZ strings. > >> > >> Do we think we could get away with having both functions fail (with > >> EINVAL) whenever the tz argument is non-null? > > > > From my findings at Debian code search, I found code like > > > > struct timeval my_gettime(void) > > { > > struct timezone tz_ignored; > > struct timeval tv; > > gettimeofday(&tv, &tz_ignored); > > return tv; > > } > > > > In this case, the safer choice would be to silently ignore it. > > > > Another alternative would be to hide the definition of 'struct timezone' > > in the libc headers and only leave a forward declaration. > > Renaming the struct timezone members might be sufficient. Then the code > above would still compile, but something that actually depends on the > struct timezone data would not. I like this idea. We could escalate to hiding the definition later. zw
diff --git a/ChangeLog b/ChangeLog index a700783ef3..f1c7acb6ab 100644 --- a/ChangeLog +++ b/ChangeLog @@ -4,6 +4,7 @@ * sysdeps/unix/sysv/linux/nanosleep.c: Likewise. * sysdeps/unix/sysv/linux/nanosleep_nocancel.c: Likewise. * sysdeps/unix/sysv/linux/lowlevellock-futex.h: Use __NR_futex_time64 if we don't have __NR_futex. + * sysdeps/unix/sysv/linux/gettimeofday.c: Use clock_gettime64 syscall for gettimeofday. 2019-06-20 Dmitry V. Levin <ldv@altlinux.org> Florian Weimer <fweimer@redhat.com> diff --git a/sysdeps/unix/sysv/linux/gettimeofday.c b/sysdeps/unix/sysv/linux/gettimeofday.c index a74f03825a..3d2b943123 100644 --- a/sysdeps/unix/sysv/linux/gettimeofday.c +++ b/sysdeps/unix/sysv/linux/gettimeofday.c @@ -32,7 +32,35 @@ int __gettimeofday (struct timeval *tv, struct timezone *tz) { +#ifdef __ASSUME_TIME64_SYSCALLS + int ret; + struct timespec now; + + ret = INLINE_VSYSCALL (clock_gettime64, 2, CLOCK_REALTIME, + &now); + + /* Convert from timespec to timeval */ + tv->tv_sec = now.tv_sec; + tv->tv_usec = now.tv_nsec / 1000; + + return ret; +#else +# ifdef __NR_clock_gettime64 + long int ret; + struct timespec now; + + ret = INLINE_VSYSCALL (clock_gettime64, 2, CLOCK_REALTIME, + &now); + + /* Convert from timespec to timeval */ + tv->tv_sec = now.tv_sec; + tv->tv_usec = now.tv_nsec / 1000; + + if (ret == 0 || errno != ENOSYS) + return ret; +# endif return INLINE_VSYSCALL (gettimeofday, 2, tv, tz); +#endif } libc_hidden_def (__gettimeofday) weak_alias (__gettimeofday, gettimeofday)
Not all architectures support the obsolete gettimeofday so use the newer clock_gettime64 syscall if it is avaliable. This fixes RV32 build issues. Signed-off-by: Alistair Francis <alistair.francis@wdc.com> --- ChangeLog | 1 + sysdeps/unix/sysv/linux/gettimeofday.c | 28 ++++++++++++++++++++++++++ 2 files changed, 29 insertions(+)