| Message ID | 3ee6c1e52cbefe6f6dbd7aef423f13607ff50402.1565398513.git.alistair.francis@wdc.com |
|---|---|
| State | New |
| Headers | show |
| Series | RISC-V glibc port for the 32-bit | expand |
On Fri, 9 Aug 2019, Alistair Francis wrote: > diff --git a/sysdeps/unix/sysv/linux/timespec_get.c b/sysdeps/unix/sysv/linux/timespec_get.c > index 52080ddf08a..97ef9c5f799 100644 > --- a/sysdeps/unix/sysv/linux/timespec_get.c > +++ b/sysdeps/unix/sysv/linux/timespec_get.c > @@ -24,6 +24,58 @@ > #endif > #include <sysdep-vdso.h> > > +int > +__timespec_get (struct timespec *ts, int base) > +{ > +#ifdef __ASSUME_TIME64_SYSCALLS > + return INTERNAL_VSYSCALL (clock_gettime64, err, 2, CLOCK_REALTIME, ts); > +#else This has the usual problems with missing conversions.
On Mon, Aug 12, 2019 at 12:46 PM Joseph Myers <joseph@codesourcery.com> wrote: > > On Fri, 9 Aug 2019, Alistair Francis wrote: > > > diff --git a/sysdeps/unix/sysv/linux/timespec_get.c b/sysdeps/unix/sysv/linux/timespec_get.c > > index 52080ddf08a..97ef9c5f799 100644 > > --- a/sysdeps/unix/sysv/linux/timespec_get.c > > +++ b/sysdeps/unix/sysv/linux/timespec_get.c > > @@ -24,6 +24,58 @@ > > #endif > > #include <sysdep-vdso.h> > > > > +int > > +__timespec_get (struct timespec *ts, int base) > > +{ > > +#ifdef __ASSUME_TIME64_SYSCALLS > > + return INTERNAL_VSYSCALL (clock_gettime64, err, 2, CLOCK_REALTIME, ts); > > +#else > > This has the usual problems with missing conversions. Is this the type of layout you are looking for? (untested, just a general idea) int __timespec_get64 (struct __timespec64 *ts, int base) { #ifdef __ASSUME_TIME64_SYSCALLS return INTERNAL_VSYSCALL (clock_gettime64, err, 2, CLOCK_REALTIME, ts); #else long int ret; # ifdef __NR_clock_gettime64 ret = INTERNAL_VSYSCALL (clock_gettime64, err, 2, CLOCK_REALTIME, ts); if (ret == 0 || errno != ENOSYS) { return ret; } # endif /* __NR_clock_gettime64 */ struct timespec ts32; if (! in_time_t_range (ts->tv_sec)) { __set_errno (EOVERFLOW); return -1; } valid_timespec64_to_timespec (ts, &ts32); ret = INTERNAL_VSYSCALL (clock_gettime, err, 2, CLOCK_REALTIME, &ts32); if ((ret == 0 || errno != ENOSYS) && ts) { ts->tv_sec = ts32.tv_sec; ts->tv_nsec = ts32.tv_nsec; } return ret; #endif } #if __TIMESIZE != 64 int __timespec_get32 (struct timespec *ts, int base) { struct __timespec64 ts64; ret = __timespec_get64 (ts64, base); if ((ret == 0 || errno != ENOSYS) && ts) { ts->tv_sec = ts64.tv_sec; ts->tv_nsec = ts64.tv_nsec; } return ret; } #endif #if __TIMESIZE == 64 # define __timespec_get __timespec_get64 #else # define __timespec_get __timespec_get32 #endif /* Set TS to calendar time based in time base BASE. */ int timespec_get (struct timespec *ts, int base) { switch (base) { int res; INTERNAL_SYSCALL_DECL (err); case TIME_UTC: res = __timespec_get (ts, base); if (INTERNAL_SYSCALL_ERROR_P (res, err)) return 0; break; default: return 0; } return base; } Alistair > > -- > Joseph S. Myers > joseph@codesourcery.com
On Thu, 15 Aug 2019, Alistair Francis wrote: > Is this the type of layout you are looking for? (untested, just a general idea) I'm not convinced making timespec_get into a wrapper round another function is a good thing. You need to end up, in the __TIMESIZE == 32 case, with two functions that have the public timespec_get semantics - not two internal functions and a single wrapper - in preparation for when we support _TIME_BITS == 64. So I'd expect defining __timespec_get64 as a function with the public semantics (not as an internal function that only calls the syscall), and conditionally defining timespec_get as a thin wrapper in the case of 32-bit time_t, and having a #define of __timespec_get64 to timespec_get in an internal header in the case of 64-bit time_t. > int > __timespec_get64 (struct __timespec64 *ts, int base) > { > #ifdef __ASSUME_TIME64_SYSCALLS > return INTERNAL_VSYSCALL (clock_gettime64, err, 2, CLOCK_REALTIME, ts); > #else You need # ifndef __NR_clock_gettime64 # define __NR_clock_gettime64 __NR_clock_gettime # endif here, because 64-bit platforms define __ASSUME_TIME64_SYSCALLS but with unsuffixed syscall names. > long int ret; > # ifdef __NR_clock_gettime64 > ret = INTERNAL_VSYSCALL (clock_gettime64, err, 2, CLOCK_REALTIME, ts); > > if (ret == 0 || errno != ENOSYS) > { > return ret; > } You need to check INTERNAL_SYSCALL_ERRNO (...) not errno; errno isn't set by INTERNAL_*, only by INLINE_*. No braces when only a single statement is inside them. > ret = INTERNAL_VSYSCALL (clock_gettime, err, 2, CLOCK_REALTIME, &ts32); > > if ((ret == 0 || errno != ENOSYS) && ts) No need to consider ENOSYS here. NULL ts isn't valid so no need to check for it being non-NULL. > /* Set TS to calendar time based in time base BASE. */ > int > timespec_get (struct timespec *ts, int base) > { > switch (base) > { > int res; > INTERNAL_SYSCALL_DECL (err); > case TIME_UTC: > res = __timespec_get (ts, base); > if (INTERNAL_SYSCALL_ERROR_P (res, err)) > return 0; This wrapper is using an uninitialized variable err.
On Thu, Aug 15, 2019 at 12:46 PM Joseph Myers <joseph@codesourcery.com> wrote: > > On Thu, 15 Aug 2019, Alistair Francis wrote: > > > Is this the type of layout you are looking for? (untested, just a general idea) > > I'm not convinced making timespec_get into a wrapper round another > function is a good thing. > > You need to end up, in the __TIMESIZE == 32 case, with two functions that > have the public timespec_get semantics - not two internal functions and a > single wrapper - in preparation for when we support _TIME_BITS == 64. So > I'd expect defining __timespec_get64 as a function with the public > semantics (not as an internal function that only calls the syscall), and > conditionally defining timespec_get as a thin wrapper in the case of > 32-bit time_t, and having a #define of __timespec_get64 to timespec_get in > an internal header in the case of 64-bit time_t. Ok, so more like this? #if __TIMESIZE == 64 # define timespec_get __timespec_get64 #else # define timespec_get __timespec_get32 #endif /* Set TS to calendar time based in time base BASE. */ int __timespec_get64 (struct timespec *ts, int base) { switch (base) { int res; INTERNAL_SYSCALL_DECL (err); case TIME_UTC: #ifdef __ASSUME_TIME64_SYSCALLS res = INTERNAL_VSYSCALL (clock_gettime64, err, 2, CLOCK_REALTIME, ts); #else # ifdef __NR_clock_gettime64 res = INTERNAL_VSYSCALL (clock_gettime64, err, 2, CLOCK_REALTIME, ts); # endif /* __NR_clock_gettime64 */ struct timespec ts32; if (! in_time_t_range (ts->tv_sec)) { __set_errno (EOVERFLOW); return -1; } valid_timespec64_to_timespec (ts, &ts32); res = INTERNAL_VSYSCALL (clock_gettime, err, 2, CLOCK_REALTIME, &ts32); if (res == 0 || !INTERNAL_SYSCALL_ERROR_P (res, err)) { ts->tv_sec = ts32.tv_sec; ts->tv_nsec = ts32.tv_nsec; } #endif if (INTERNAL_SYSCALL_ERROR_P (res, err)) return 0; break; default: return 0; } return base; } #if __TIMESIZE != 64 int __timespec_get32 (struct timespec *ts, int base) { struct __timespec64 ts64; ret = __timespec_get64 (ts64, base); if (res == 0 || !INTERNAL_SYSCALL_ERROR_P (res, err)) { ts->tv_sec = ts64.tv_sec; ts->tv_nsec = ts64.tv_nsec; } return ret; } #endif > > > int > > __timespec_get64 (struct __timespec64 *ts, int base) > > { > > #ifdef __ASSUME_TIME64_SYSCALLS > > return INTERNAL_VSYSCALL (clock_gettime64, err, 2, CLOCK_REALTIME, ts); > > #else > > You need > > # ifndef __NR_clock_gettime64 > # define __NR_clock_gettime64 __NR_clock_gettime > # endif > > here, because 64-bit platforms define __ASSUME_TIME64_SYSCALLS but with > unsuffixed syscall names. The kernel defines 64 suffixed syscalls, is this required because older kernels don't do this? > > > long int ret; > > # ifdef __NR_clock_gettime64 > > ret = INTERNAL_VSYSCALL (clock_gettime64, err, 2, CLOCK_REALTIME, ts); > > > > if (ret == 0 || errno != ENOSYS) > > { > > return ret; > > } > > You need to check INTERNAL_SYSCALL_ERRNO (...) not errno; errno isn't set > by INTERNAL_*, only by INLINE_*. Fixed! Thanks > > No braces when only a single statement is inside them. > > > ret = INTERNAL_VSYSCALL (clock_gettime, err, 2, CLOCK_REALTIME, &ts32); > > > > if ((ret == 0 || errno != ENOSYS) && ts) > > No need to consider ENOSYS here. NULL ts isn't valid so no need to check > for it being non-NULL. Fixed > > > /* Set TS to calendar time based in time base BASE. */ > > int > > timespec_get (struct timespec *ts, int base) > > { > > switch (base) > > { > > int res; > > INTERNAL_SYSCALL_DECL (err); > > case TIME_UTC: > > res = __timespec_get (ts, base); > > if (INTERNAL_SYSCALL_ERROR_P (res, err)) > > return 0; > > This wrapper is using an uninitialized variable err. Good point, fixed. Alistair > > -- > Joseph S. Myers > joseph@codesourcery.com
On Thu, 15 Aug 2019, Alistair Francis wrote: > Ok, so more like this? > > #if __TIMESIZE == 64 > # define timespec_get __timespec_get64 > #else > # define timespec_get __timespec_get32 > #endif No. Please see what's done for mktime, for example (but it's simpler here because mktime supports being built outside of glibc, which is irrelevant for timespec_get). * The function __mktime64 is defined, unconditionally. * The function mktime is defined as a thin wrapper, conditionally (only when 32-bit time is supported). * There's no __mktime32 anywhere. * mktime-internal.h deals with defining __mktime64 back to mktime in the case where __TIMESIZE == 64 and so only a single function is needed with no wrapper. > > # ifndef __NR_clock_gettime64 > > # define __NR_clock_gettime64 __NR_clock_gettime > > # endif > > > > here, because 64-bit platforms define __ASSUME_TIME64_SYSCALLS but with > > unsuffixed syscall names. > > The kernel defines 64 suffixed syscalls, is this required because > older kernels don't do this? The kernel does *not* define 64-suffixed syscalls on platforms where __SYSCALL_WORDSIZE == 64. It defined unsuffixed syscalls with the same semantics.
On Thu, Aug 15, 2019 at 1:59 PM Joseph Myers <joseph@codesourcery.com> wrote: > > On Thu, 15 Aug 2019, Alistair Francis wrote: > > > Ok, so more like this? > > > > #if __TIMESIZE == 64 > > # define timespec_get __timespec_get64 > > #else > > # define timespec_get __timespec_get32 > > #endif > > No. Please see what's done for mktime, for example (but it's simpler here > because mktime supports being built outside of glibc, which is irrelevant > for timespec_get). > > * The function __mktime64 is defined, unconditionally. > > * The function mktime is defined as a thin wrapper, conditionally (only > when 32-bit time is supported). > > * There's no __mktime32 anywhere. > > * mktime-internal.h deals with defining __mktime64 back to mktime in the > case where __TIMESIZE == 64 and so only a single function is needed with > no wrapper. There is no internal header for timespec_get, would you prefer me to create one or put the define in time/time.h? > > > > # ifndef __NR_clock_gettime64 > > > # define __NR_clock_gettime64 __NR_clock_gettime > > > # endif > > > > > > here, because 64-bit platforms define __ASSUME_TIME64_SYSCALLS but with > > > unsuffixed syscall names. > > > > The kernel defines 64 suffixed syscalls, is this required because > > older kernels don't do this? > > The kernel does *not* define 64-suffixed syscalls on platforms where > __SYSCALL_WORDSIZE == 64. It defined unsuffixed syscalls with the same > semantics. Ah, you are right. I have added these defines. Alistair > > -- > Joseph S. Myers > joseph@codesourcery.com
On Thu, 15 Aug 2019, Alistair Francis wrote: > There is no internal header for timespec_get, would you prefer me to > create one or put the define in time/time.h? time/time.h is an installed header, so it mustn't go there. include/time.h would be fine (it has several such defines, for __localtime64 etc.).
On Thu, Aug 15, 2019 at 2:19 PM Joseph Myers <joseph@codesourcery.com> wrote: > > On Thu, 15 Aug 2019, Alistair Francis wrote: > > > There is no internal header for timespec_get, would you prefer me to > > create one or put the define in time/time.h? > > time/time.h is an installed header, so it mustn't go there. > include/time.h would be fine (it has several such defines, for > __localtime64 etc.). Ok, I have this in include/time.h: #if __TIMESIZE == 64 #define timespec_get __timespec_get64 #else and this in the c file: /* Set TS to calendar time based in time base BASE. */ int __timespec_get64 (struct timespec *ts, int base) { switch (base) { int res; INTERNAL_SYSCALL_DECL (err); case TIME_UTC: #ifdef __ASSUME_TIME64_SYSCALLS # ifndef __NR_clock_gettime64 # define __NR_clock_gettime64 __NR_clock_gettime # endif res = INTERNAL_VSYSCALL (clock_gettime64, err, 2, CLOCK_REALTIME, ts); #else # ifdef __NR_clock_gettime64 res = INTERNAL_VSYSCALL (clock_gettime64, err, 2, CLOCK_REALTIME, ts); # endif /* __NR_clock_gettime64 */ struct timespec ts32; if (! in_time_t_range (ts->tv_sec)) { __set_errno (EOVERFLOW); return -1; } valid_timespec64_to_timespec (ts, &ts32); res = INTERNAL_VSYSCALL (clock_gettime, err, 2, CLOCK_REALTIME, &ts32); if (res == 0 || !INTERNAL_SYSCALL_ERROR_P (res, err)) { ts->tv_sec = ts32.tv_sec; ts->tv_nsec = ts32.tv_nsec; } #endif if (INTERNAL_SYSCALL_ERROR_P (res, err)) return 0; break; default: return 0; } return base; } #if __TIMESIZE != 64 int timespec_get (struct timespec *ts, int base) { struct __timespec64 ts64; ret = __timespec_get64 (ts64, base); if (res == 0 || !INTERNAL_SYSCALL_ERROR_P (res, err)) { ts->tv_sec = ts64.tv_sec; ts->tv_nsec = ts64.tv_nsec; } return ret; } #endif Alistair > > -- > Joseph S. Myers > joseph@codesourcery.com
On Thu, 15 Aug 2019, Alistair Francis wrote: > On Thu, Aug 15, 2019 at 2:19 PM Joseph Myers <joseph@codesourcery.com> wrote: > > > > On Thu, 15 Aug 2019, Alistair Francis wrote: > > > > > There is no internal header for timespec_get, would you prefer me to > > > create one or put the define in time/time.h? > > > > time/time.h is an installed header, so it mustn't go there. > > include/time.h would be fine (it has several such defines, for > > __localtime64 etc.). > > Ok, I have this in include/time.h: > > #if __TIMESIZE == 64 > #define timespec_get __timespec_get64 > #else That should be the other way round (defining __timespec_get64 to timespec_get so that timespec_get gets properly exported from libc).
On Thu, Aug 15, 2019 at 2:28 PM Joseph Myers <joseph@codesourcery.com> wrote: > > On Thu, 15 Aug 2019, Alistair Francis wrote: > > > On Thu, Aug 15, 2019 at 2:19 PM Joseph Myers <joseph@codesourcery.com> wrote: > > > > > > On Thu, 15 Aug 2019, Alistair Francis wrote: > > > > > > > There is no internal header for timespec_get, would you prefer me to > > > > create one or put the define in time/time.h? > > > > > > time/time.h is an installed header, so it mustn't go there. > > > include/time.h would be fine (it has several such defines, for > > > __localtime64 etc.). > > > > Ok, I have this in include/time.h: > > > > #if __TIMESIZE == 64 > > #define timespec_get __timespec_get64 > > #else > > That should be the other way round (defining __timespec_get64 to > timespec_get so that timespec_get gets properly exported from libc). Ok. Thanks for your help with this. I'll update the other patches with a similar style and look at sending a full patch series after running tests. Alistair > > -- > Joseph S. Myers > joseph@codesourcery.com
diff --git a/sysdeps/unix/sysv/linux/timespec_get.c b/sysdeps/unix/sysv/linux/timespec_get.c index 52080ddf08a..97ef9c5f799 100644 --- a/sysdeps/unix/sysv/linux/timespec_get.c +++ b/sysdeps/unix/sysv/linux/timespec_get.c @@ -24,6 +24,58 @@ #endif #include <sysdep-vdso.h> +int +__timespec_get (struct timespec *ts, int base) +{ +#ifdef __ASSUME_TIME64_SYSCALLS + return INTERNAL_VSYSCALL (clock_gettime64, err, 2, CLOCK_REALTIME, ts); +#else + long int ret; +# ifdef __NR_clock_gettime64 +# if __TIMESIZE == 64 + ret = INTERNAL_VSYSCALL (clock_gettime64, err, 2, CLOCK_REALTIME, ts); + + if (ret == 0 || errno != ENOSYS) + { + return ret; + } +# else + struct __timespec64 ts64; + + ret = INTERNAL_VSYSCALL (clock_gettime64, err, 2, CLOCK_REALTIME, &ts64); + + if (ret == 0 || errno != ENOSYS) + { + ts->tv_sec = ts64.tv_sec; + ts->tv_nsec = ts64.tv_nsec; + return ret; + } +# endif /* __TIMESIZE == 64 */ +# endif /* __NR_clock_gettime64 */ +# if __TIMESIZE == 64 + struct timespec ts32; + + if (! in_time_t_range (ts->tv_sec)) + { + __set_errno (EOVERFLOW); + return -1; + } + + valid_timespec64_to_timespec (ts, &ts32); + ret = INTERNAL_VSYSCALL (clock_gettime, err, 2, CLOCK_REALTIME, &ts32); + + if (ret == 0 || errno != ENOSYS) + { + ts->tv_sec = ts32.tv_sec; + ts->tv_nsec = ts32.tv_nsec; + } + return ret; +# else + return INTERNAL_VSYSCALL (clock_gettime, err, 2, CLOCK_REALTIME, ts); +# endif /* __TIMESIZE == 64 */ +#endif +} + /* Set TS to calendar time based in time base BASE. */ int timespec_get (struct timespec *ts, int base) @@ -33,9 +85,9 @@ timespec_get (struct timespec *ts, int base) int res; INTERNAL_SYSCALL_DECL (err); case TIME_UTC: - res = INTERNAL_VSYSCALL (clock_gettime, err, 2, CLOCK_REALTIME, ts); + res = __timespec_get (ts, base); if (INTERNAL_SYSCALL_ERROR_P (res, err)) - return 0; + return 0; break; default:
Signed-off-by: Alistair Francis <alistair.francis@wdc.com> --- sysdeps/unix/sysv/linux/timespec_get.c | 56 +++++++++++++++++++++++++- 1 file changed, 54 insertions(+), 2 deletions(-)