* [PATCH] time: Skip overflow itimer tests on 32-bit systems @ 2021-06-07 13:18 Stafford Horne 2021-06-09 13:50 ` Adhemerval Zanella 0 siblings, 1 reply; 8+ messages in thread From: Stafford Horne @ 2021-06-07 13:18 UTC (permalink / raw) To: GLIBC patches; +Cc: Stafford Horne, Adhemerval Zanella, Alistair Francis On the port of OpenRISC I am working on and it appears the rv32 port we have sets __TIMESIZE == 64 && __WORDSIZE == 32. This causes the size of time_t to be 8 bytes, but the tv_sec in the kernel is still 32-bit causing truncation. The truncations are unavoidable on these systems so skip the testing/failures by guarding with __KERNEL_OLD_TIMEVAL_MATCHES_TIMEVAL64. --- I am open to other suggestions, this seemed the most correct to me. Cc: Adhemerval Zanella <adhemerval.zanella@linaro.org> Cc: Alistair Francis <alistair.francis@wdc.com> time/tst-itimer.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/time/tst-itimer.c b/time/tst-itimer.c index 929c2b74c7..0c99d46d7e 100644 --- a/time/tst-itimer.c +++ b/time/tst-itimer.c @@ -89,6 +89,7 @@ do_test (void) TEST_COMPARE (it.it_interval.tv_sec, it_old.it_interval.tv_sec); TEST_COMPARE (it.it_interval.tv_usec, it_old.it_interval.tv_usec); +#if __KERNEL_OLD_TIMEVAL_MATCHES_TIMEVAL64 if (sizeof (time_t) == 4) continue; @@ -146,6 +147,7 @@ do_test (void) TEST_COMPARE (setitimer (timers[i], &it, NULL), -1); TEST_COMPARE (errno, EOVERFLOW); } +#endif } { -- 2.31.1 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] time: Skip overflow itimer tests on 32-bit systems 2021-06-07 13:18 [PATCH] time: Skip overflow itimer tests on 32-bit systems Stafford Horne @ 2021-06-09 13:50 ` Adhemerval Zanella 2021-06-09 21:38 ` Stafford Horne 0 siblings, 1 reply; 8+ messages in thread From: Adhemerval Zanella @ 2021-06-09 13:50 UTC (permalink / raw) To: Stafford Horne, GLIBC patches; +Cc: Alistair Francis On 07/06/2021 10:18, Stafford Horne wrote: > On the port of OpenRISC I am working on and it appears the rv32 port > we have sets __TIMESIZE == 64 && __WORDSIZE == 32. This causes the > size of time_t to be 8 bytes, but the tv_sec in the kernel is still 32-bit > causing truncation. > > The truncations are unavoidable on these systems so skip the > testing/failures by guarding with __KERNEL_OLD_TIMEVAL_MATCHES_TIMEVAL64. Sigh, I was hoping that we won't need to handle this situation (glibc support only 64-bit time_t, but kernel still providing some 32-bit syscall). > --- > > I am open to other suggestions, this seemed the most correct to me. > > Cc: Adhemerval Zanella <adhemerval.zanella@linaro.org> > Cc: Alistair Francis <alistair.francis@wdc.com> > > time/tst-itimer.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/time/tst-itimer.c b/time/tst-itimer.c > index 929c2b74c7..0c99d46d7e 100644 > --- a/time/tst-itimer.c > +++ b/time/tst-itimer.c > @@ -89,6 +89,7 @@ do_test (void) > TEST_COMPARE (it.it_interval.tv_sec, it_old.it_interval.tv_sec); > TEST_COMPARE (it.it_interval.tv_usec, it_old.it_interval.tv_usec); > > +#if __KERNEL_OLD_TIMEVAL_MATCHES_TIMEVAL64 > if (sizeof (time_t) == 4) > continue; > > @@ -146,6 +147,7 @@ do_test (void) > TEST_COMPARE (setitimer (timers[i], &it, NULL), -1); > TEST_COMPARE (errno, EOVERFLOW); > } > +#endif > } > > { > Instead of disabling, I think it would be better to use __KERNEL_OLD_TIMEVAL_MATCHES_TIMEVAL64 instead of __time_t sizeof (so we can still tests the EOVERFLOW): diff --git a/time/tst-itimer.c b/time/tst-itimer.c index 929c2b74c7..bd7d7afe83 100644 --- a/time/tst-itimer.c +++ b/time/tst-itimer.c @@ -100,7 +100,7 @@ do_test (void) /* Linux does not provide 64 bit time_t support for getitimer and setitimer on architectures with 32 bit time_t support. */ - if (sizeof (__time_t) == 8) + if (__KERNEL_OLD_TIMEVAL_MATCHES_TIMEVAL64) { TEST_COMPARE (setitimer (timers[i], &it, NULL), 0); TEST_COMPARE (setitimer (timers[i], &(struct itimerval) { 0 }, @@ -131,7 +131,7 @@ do_test (void) it.it_interval.tv_usec = 20; it.it_value.tv_sec = 30; it.it_value.tv_usec = 40; - if (sizeof (__time_t) == 8) + if (__KERNEL_OLD_TIMEVAL_MATCHES_TIMEVAL64) { TEST_COMPARE (setitimer (timers[i], &it, NULL), 0); ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] time: Skip overflow itimer tests on 32-bit systems 2021-06-09 13:50 ` Adhemerval Zanella @ 2021-06-09 21:38 ` Stafford Horne 2021-06-12 9:19 ` Stafford Horne 0 siblings, 1 reply; 8+ messages in thread From: Stafford Horne @ 2021-06-09 21:38 UTC (permalink / raw) To: Adhemerval Zanella; +Cc: GLIBC patches, Alistair Francis On Wed, Jun 09, 2021 at 10:50:23AM -0300, Adhemerval Zanella wrote: > > > On 07/06/2021 10:18, Stafford Horne wrote: > > On the port of OpenRISC I am working on and it appears the rv32 port > > we have sets __TIMESIZE == 64 && __WORDSIZE == 32. This causes the > > size of time_t to be 8 bytes, but the tv_sec in the kernel is still 32-bit > > causing truncation. > > > > The truncations are unavoidable on these systems so skip the > > testing/failures by guarding with __KERNEL_OLD_TIMEVAL_MATCHES_TIMEVAL64. > > Sigh, I was hoping that we won't need to handle this situation (glibc > support only 64-bit time_t, but kernel still providing some 32-bit > syscall). > > > --- > > > > I am open to other suggestions, this seemed the most correct to me. > > > > Cc: Adhemerval Zanella <adhemerval.zanella@linaro.org> > > Cc: Alistair Francis <alistair.francis@wdc.com> > > > > time/tst-itimer.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/time/tst-itimer.c b/time/tst-itimer.c > > index 929c2b74c7..0c99d46d7e 100644 > > --- a/time/tst-itimer.c > > +++ b/time/tst-itimer.c > > @@ -89,6 +89,7 @@ do_test (void) > > TEST_COMPARE (it.it_interval.tv_sec, it_old.it_interval.tv_sec); > > TEST_COMPARE (it.it_interval.tv_usec, it_old.it_interval.tv_usec); > > > > +#if __KERNEL_OLD_TIMEVAL_MATCHES_TIMEVAL64 > > if (sizeof (time_t) == 4) > > continue; > > > > @@ -146,6 +147,7 @@ do_test (void) > > TEST_COMPARE (setitimer (timers[i], &it, NULL), -1); > > TEST_COMPARE (errno, EOVERFLOW); > > } > > +#endif > > } > > > > { > > > > Instead of disabling, I think it would be better to use > __KERNEL_OLD_TIMEVAL_MATCHES_TIMEVAL64 instead of __time_t sizeof > (so we can still tests the EOVERFLOW): > > diff --git a/time/tst-itimer.c b/time/tst-itimer.c > index 929c2b74c7..bd7d7afe83 100644 > --- a/time/tst-itimer.c > +++ b/time/tst-itimer.c > @@ -100,7 +100,7 @@ do_test (void) > > /* Linux does not provide 64 bit time_t support for getitimer and > setitimer on architectures with 32 bit time_t support. */ > - if (sizeof (__time_t) == 8) > + if (__KERNEL_OLD_TIMEVAL_MATCHES_TIMEVAL64) > { > TEST_COMPARE (setitimer (timers[i], &it, NULL), 0); > TEST_COMPARE (setitimer (timers[i], &(struct itimerval) { 0 }, > @@ -131,7 +131,7 @@ do_test (void) > it.it_interval.tv_usec = 20; > it.it_value.tv_sec = 30; > it.it_value.tv_usec = 40; > - if (sizeof (__time_t) == 8) > + if (__KERNEL_OLD_TIMEVAL_MATCHES_TIMEVAL64) > { > TEST_COMPARE (setitimer (timers[i], &it, NULL), 0); This looks good to me, I can update to this, test and resend the patch when I get some time. Probably later tonight. -Strafford ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] time: Skip overflow itimer tests on 32-bit systems 2021-06-09 21:38 ` Stafford Horne @ 2021-06-12 9:19 ` Stafford Horne 2021-07-06 19:58 ` Adhemerval Zanella 0 siblings, 1 reply; 8+ messages in thread From: Stafford Horne @ 2021-06-12 9:19 UTC (permalink / raw) To: Adhemerval Zanella; +Cc: GLIBC patches, Alistair Francis On Thu, Jun 10, 2021 at 06:38:04AM +0900, Stafford Horne wrote: > On Wed, Jun 09, 2021 at 10:50:23AM -0300, Adhemerval Zanella wrote: > > > > > > On 07/06/2021 10:18, Stafford Horne wrote: > > > On the port of OpenRISC I am working on and it appears the rv32 port > > > we have sets __TIMESIZE == 64 && __WORDSIZE == 32. This causes the > > > size of time_t to be 8 bytes, but the tv_sec in the kernel is still 32-bit > > > causing truncation. > > > > > > The truncations are unavoidable on these systems so skip the > > > testing/failures by guarding with __KERNEL_OLD_TIMEVAL_MATCHES_TIMEVAL64. > > > > Sigh, I was hoping that we won't need to handle this situation (glibc > > support only 64-bit time_t, but kernel still providing some 32-bit > > syscall). > > > > > --- > > > > > > I am open to other suggestions, this seemed the most correct to me. > > > > > > Cc: Adhemerval Zanella <adhemerval.zanella@linaro.org> > > > Cc: Alistair Francis <alistair.francis@wdc.com> > > > > > > time/tst-itimer.c | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/time/tst-itimer.c b/time/tst-itimer.c > > > index 929c2b74c7..0c99d46d7e 100644 > > > --- a/time/tst-itimer.c > > > +++ b/time/tst-itimer.c > > > @@ -89,6 +89,7 @@ do_test (void) > > > TEST_COMPARE (it.it_interval.tv_sec, it_old.it_interval.tv_sec); > > > TEST_COMPARE (it.it_interval.tv_usec, it_old.it_interval.tv_usec); > > > > > > +#if __KERNEL_OLD_TIMEVAL_MATCHES_TIMEVAL64 > > > if (sizeof (time_t) == 4) > > > continue; > > > > > > @@ -146,6 +147,7 @@ do_test (void) > > > TEST_COMPARE (setitimer (timers[i], &it, NULL), -1); > > > TEST_COMPARE (errno, EOVERFLOW); > > > } > > > +#endif > > > } > > > > > > { > > > > > > > Instead of disabling, I think it would be better to use > > __KERNEL_OLD_TIMEVAL_MATCHES_TIMEVAL64 instead of __time_t sizeof > > (so we can still tests the EOVERFLOW): > > > > diff --git a/time/tst-itimer.c b/time/tst-itimer.c > > index 929c2b74c7..bd7d7afe83 100644 > > --- a/time/tst-itimer.c > > +++ b/time/tst-itimer.c > > @@ -100,7 +100,7 @@ do_test (void) > > > > /* Linux does not provide 64 bit time_t support for getitimer and > > setitimer on architectures with 32 bit time_t support. */ > > - if (sizeof (__time_t) == 8) > > + if (__KERNEL_OLD_TIMEVAL_MATCHES_TIMEVAL64) > > { > > TEST_COMPARE (setitimer (timers[i], &it, NULL), 0); > > TEST_COMPARE (setitimer (timers[i], &(struct itimerval) { 0 }, > > @@ -131,7 +131,7 @@ do_test (void) > > it.it_interval.tv_usec = 20; > > it.it_value.tv_sec = 30; > > it.it_value.tv_usec = 40; > > - if (sizeof (__time_t) == 8) > > + if (__KERNEL_OLD_TIMEVAL_MATCHES_TIMEVAL64) > > { > > TEST_COMPARE (setitimer (timers[i], &it, NULL), 0); > > This looks good to me, I can update to this, test and resend the patch when I > get some time. Probably later tonight. I tested this and it exposes an issue in the linux setitimer wrapper. On my platform I get EINVAL instead of EOVERFLOW. FAIL: time/tst-itimer original exit status 1 tst-itimer.c:125: numeric comparison failure left: 22 (0x16); from: errno right: 75 (0x4b); from: EOVERFLOW tst-itimer.c:147: numeric comparison failure left: 22 (0x16); from: errno right: 75 (0x4b); from: EOVERFLOW It seems this is because sysdeps/unix/sysv/linux/setitimer.c, checks that the incoming value is in the range of time_t. The problem is that that we need to fit the value in __int32_t not time_t. When testing the time_t range check does not detect the overflow and setitimer ends up passing a -1 to the kernel causing EINVAL. I can fix that, as per the patch below, but It will take me some time to audit other places this might be an issue. if (! in_time_t_range (new_value->it_interval.tv_sec) || ! in_time_t_range (new_value->it_value.tv_sec)) { __set_errno (EOVERFLOW); return -1; } new_value_32.it_interval = valid_timeval64_to_timeval32 (new_value->it_interval); new_value_32.it_value = valid_timeval64_to_timeval32 (new_value->it_value); The below patch works for me, but there is probably a better thing to do then create a new functrion. diff --git a/include/time.h b/include/time.h index 4372bfbd96..377a4a45ea 100644 --- a/include/time.h +++ b/include/time.h @@ -342,6 +342,14 @@ in_time_t_range (__time64_t t) return s == t; } +/* Check whether T fits in a timeval32 (__int32_t). */ +static inline bool +in_timeval32_range (__time64_t t) +{ + __int32_t s = t; + return s == t; +} + /* Convert a known valid struct timeval into a struct __timespec64. */ static inline struct __timespec64 valid_timeval_to_timespec64 (const struct timeval tv) diff --git a/sysdeps/unix/sysv/linux/setitimer.c b/sysdeps/unix/sysv/linux/setitimer.c index 083a25cf35..bada30ba02 100644 --- a/sysdeps/unix/sysv/linux/setitimer.c +++ b/sysdeps/unix/sysv/linux/setitimer.c @@ -32,8 +32,8 @@ __setitimer64 (__itimer_which_t which, #else struct __itimerval32 new_value_32; - if (! in_time_t_range (new_value->it_interval.tv_sec) - || ! in_time_t_range (new_value->it_value.tv_sec)) + if (! in_timeval32_range (new_value->it_interval.tv_sec) + || ! in_timeval32_range (new_value->it_value.tv_sec)) { -Stafford ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] time: Skip overflow itimer tests on 32-bit systems 2021-06-12 9:19 ` Stafford Horne @ 2021-07-06 19:58 ` Adhemerval Zanella 2021-07-07 21:11 ` Stafford Horne 2022-10-28 19:47 ` Aurelien Jarno 0 siblings, 2 replies; 8+ messages in thread From: Adhemerval Zanella @ 2021-07-06 19:58 UTC (permalink / raw) To: Stafford Horne; +Cc: GLIBC patches, Alistair Francis On 12/06/2021 06:19, Stafford Horne wrote: > On Thu, Jun 10, 2021 at 06:38:04AM +0900, Stafford Horne wrote: >> On Wed, Jun 09, 2021 at 10:50:23AM -0300, Adhemerval Zanella wrote: >>> >>> >>> On 07/06/2021 10:18, Stafford Horne wrote: >>>> On the port of OpenRISC I am working on and it appears the rv32 port >>>> we have sets __TIMESIZE == 64 && __WORDSIZE == 32. This causes the >>>> size of time_t to be 8 bytes, but the tv_sec in the kernel is still 32-bit >>>> causing truncation. >>>> >>>> The truncations are unavoidable on these systems so skip the >>>> testing/failures by guarding with __KERNEL_OLD_TIMEVAL_MATCHES_TIMEVAL64. >>> >>> Sigh, I was hoping that we won't need to handle this situation (glibc >>> support only 64-bit time_t, but kernel still providing some 32-bit >>> syscall). >>> >>>> --- >>>> >>>> I am open to other suggestions, this seemed the most correct to me. >>>> >>>> Cc: Adhemerval Zanella <adhemerval.zanella@linaro.org> >>>> Cc: Alistair Francis <alistair.francis@wdc.com> >>>> >>>> time/tst-itimer.c | 2 ++ >>>> 1 file changed, 2 insertions(+) >>>> >>>> diff --git a/time/tst-itimer.c b/time/tst-itimer.c >>>> index 929c2b74c7..0c99d46d7e 100644 >>>> --- a/time/tst-itimer.c >>>> +++ b/time/tst-itimer.c >>>> @@ -89,6 +89,7 @@ do_test (void) >>>> TEST_COMPARE (it.it_interval.tv_sec, it_old.it_interval.tv_sec); >>>> TEST_COMPARE (it.it_interval.tv_usec, it_old.it_interval.tv_usec); >>>> >>>> +#if __KERNEL_OLD_TIMEVAL_MATCHES_TIMEVAL64 >>>> if (sizeof (time_t) == 4) >>>> continue; >>>> >>>> @@ -146,6 +147,7 @@ do_test (void) >>>> TEST_COMPARE (setitimer (timers[i], &it, NULL), -1); >>>> TEST_COMPARE (errno, EOVERFLOW); >>>> } >>>> +#endif >>>> } >>>> >>>> { >>>> >>> >>> Instead of disabling, I think it would be better to use >>> __KERNEL_OLD_TIMEVAL_MATCHES_TIMEVAL64 instead of __time_t sizeof >>> (so we can still tests the EOVERFLOW): >>> >>> diff --git a/time/tst-itimer.c b/time/tst-itimer.c >>> index 929c2b74c7..bd7d7afe83 100644 >>> --- a/time/tst-itimer.c >>> +++ b/time/tst-itimer.c >>> @@ -100,7 +100,7 @@ do_test (void) >>> >>> /* Linux does not provide 64 bit time_t support for getitimer and >>> setitimer on architectures with 32 bit time_t support. */ >>> - if (sizeof (__time_t) == 8) >>> + if (__KERNEL_OLD_TIMEVAL_MATCHES_TIMEVAL64) >>> { >>> TEST_COMPARE (setitimer (timers[i], &it, NULL), 0); >>> TEST_COMPARE (setitimer (timers[i], &(struct itimerval) { 0 }, >>> @@ -131,7 +131,7 @@ do_test (void) >>> it.it_interval.tv_usec = 20; >>> it.it_value.tv_sec = 30; >>> it.it_value.tv_usec = 40; >>> - if (sizeof (__time_t) == 8) >>> + if (__KERNEL_OLD_TIMEVAL_MATCHES_TIMEVAL64) >>> { >>> TEST_COMPARE (setitimer (timers[i], &it, NULL), 0); >> >> This looks good to me, I can update to this, test and resend the patch when I >> get some time. Probably later tonight. > > I tested this and it exposes an issue in the linux setitimer wrapper. On my > platform I get EINVAL instead of EOVERFLOW. > > FAIL: time/tst-itimer > original exit status 1 > tst-itimer.c:125: numeric comparison failure > left: 22 (0x16); from: errno > right: 75 (0x4b); from: EOVERFLOW > tst-itimer.c:147: numeric comparison failure > left: 22 (0x16); from: errno > right: 75 (0x4b); from: EOVERFLOW > > > It seems this is because sysdeps/unix/sysv/linux/setitimer.c, checks that > the incoming value is in the range of time_t. The problem is that > that we need to fit the value in __int32_t not time_t. When testing the time_t > range check does not detect the overflow and setitimer ends up passing a -1 to > the kernel causing EINVAL. > > I can fix that, as per the patch below, but It will take me some time to audit > other places this might be an issue. > > > > if (! in_time_t_range (new_value->it_interval.tv_sec) > || ! in_time_t_range (new_value->it_value.tv_sec)) > { > __set_errno (EOVERFLOW); > return -1; > } > new_value_32.it_interval > = valid_timeval64_to_timeval32 (new_value->it_interval); > new_value_32.it_value > = valid_timeval64_to_timeval32 (new_value->it_value); > > > Sigh... it seems that openrisc will the only *one* architecture with 64-bit time_t in userland which uses legacy 32-bit kernel ABI. > > The below patch works for me, but there is probably a better thing to do then > create a new functrion. > > > > diff --git a/include/time.h b/include/time.h > index 4372bfbd96..377a4a45ea 100644 > --- a/include/time.h > +++ b/include/time.h > @@ -342,6 +342,14 @@ in_time_t_range (__time64_t t) > return s == t; > } > > +/* Check whether T fits in a timeval32 (__int32_t). */ > +static inline bool > +in_timeval32_range (__time64_t t) > +{ > + __int32_t s = t; > + return s == t; > +} > + The name is confusing, it is mixing timeval from 'struct timeval' and time_t. And there is no need to use __int32_t, we need to use it only on installed headers to avoid namespace pollution. I fact I think it would be better to just change 'in_time_t_range' to use int32_t internally instead of time_t; I am pretty sure that all usages assume that sizeof(time_t) == 32. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] time: Skip overflow itimer tests on 32-bit systems 2021-07-06 19:58 ` Adhemerval Zanella @ 2021-07-07 21:11 ` Stafford Horne 2022-10-28 19:47 ` Aurelien Jarno 1 sibling, 0 replies; 8+ messages in thread From: Stafford Horne @ 2021-07-07 21:11 UTC (permalink / raw) To: Adhemerval Zanella; +Cc: GLIBC patches, Alistair Francis On Tue, Jul 06, 2021 at 04:58:58PM -0300, Adhemerval Zanella wrote: > ... > > Sigh... it seems that openrisc will the only *one* architecture with > 64-bit time_t in userland which uses legacy 32-bit kernel ABI. In order to adhere to the "don't break userspace" rule I don't think I can change that, I would like to. Is there any documented migration path you know of? > > > > The below patch works for me, but there is probably a better thing to do then > > create a new functrion. > > > > > > > > diff --git a/include/time.h b/include/time.h > > index 4372bfbd96..377a4a45ea 100644 > > --- a/include/time.h > > +++ b/include/time.h > > @@ -342,6 +342,14 @@ in_time_t_range (__time64_t t) > > return s == t; > > } > > > > +/* Check whether T fits in a timeval32 (__int32_t). */ > > +static inline bool > > +in_timeval32_range (__time64_t t) > > +{ > > + __int32_t s = t; > > + return s == t; > > +} > > + > > The name is confusing, it is mixing timeval from 'struct timeval' and > time_t. And there is no need to use __int32_t, we need to use it only > on installed headers to avoid namespace pollution. Right, I did think about that, but just wanted to get something to show the change, I couldn't think of a better name. > I fact I think it would be better to just change 'in_time_t_range' to > use int32_t internally instead of time_t; I am pretty sure that all > usages assume that sizeof(time_t) == 32. Yeah, I like that better too. I will respin the patch and post again. -Stafford ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] time: Skip overflow itimer tests on 32-bit systems 2021-07-06 19:58 ` Adhemerval Zanella 2021-07-07 21:11 ` Stafford Horne @ 2022-10-28 19:47 ` Aurelien Jarno 2022-10-31 14:02 ` Adhemerval Zanella Netto 1 sibling, 1 reply; 8+ messages in thread From: Aurelien Jarno @ 2022-10-28 19:47 UTC (permalink / raw) To: Adhemerval Zanella; +Cc: Stafford Horne, Alistair Francis, GLIBC patches On 2021-07-06 16:58, Adhemerval Zanella via Libc-alpha wrote: > > diff --git a/include/time.h b/include/time.h > > index 4372bfbd96..377a4a45ea 100644 > > --- a/include/time.h > > +++ b/include/time.h > > @@ -342,6 +342,14 @@ in_time_t_range (__time64_t t) > > return s == t; > > } > > > > +/* Check whether T fits in a timeval32 (__int32_t). */ > > +static inline bool > > +in_timeval32_range (__time64_t t) > > +{ > > + __int32_t s = t; > > + return s == t; > > +} > > + > > The name is confusing, it is mixing timeval from 'struct timeval' and > time_t. And there is no need to use __int32_t, we need to use it only > on installed headers to avoid namespace pollution. > > I fact I think it would be better to just change 'in_time_t_range' to > use int32_t internally instead of time_t; I am pretty sure that all > usages assume that sizeof(time_t) == 32. Unfortunately this is not correct. MIPS N64 uses a 64-bit time_t, however it does not define XSTAT_IS_XSTAT64. Therefore fstatat is different than fstatat64, and uses the default sysdeps/unix/sysv/linux/fstatat.c implementation, which uses in_time_t_range() with a 64-bit time_t. Therefore this change broke y2038 support in fstat/fstatat/lstat on MIPS N64, which now returns EOVERFLOW for files which need such support. I have opened BZ #29730. -- Aurelien Jarno GPG: 4096R/1DDD8C9B aurelien@aurel32.net http://www.aurel32.net ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] time: Skip overflow itimer tests on 32-bit systems 2022-10-28 19:47 ` Aurelien Jarno @ 2022-10-31 14:02 ` Adhemerval Zanella Netto 0 siblings, 0 replies; 8+ messages in thread From: Adhemerval Zanella Netto @ 2022-10-31 14:02 UTC (permalink / raw) To: Stafford Horne, Alistair Francis, GLIBC patches On 28/10/22 16:47, Aurelien Jarno wrote: > On 2021-07-06 16:58, Adhemerval Zanella via Libc-alpha wrote: >>> diff --git a/include/time.h b/include/time.h >>> index 4372bfbd96..377a4a45ea 100644 >>> --- a/include/time.h >>> +++ b/include/time.h >>> @@ -342,6 +342,14 @@ in_time_t_range (__time64_t t) >>> return s == t; >>> } >>> >>> +/* Check whether T fits in a timeval32 (__int32_t). */ >>> +static inline bool >>> +in_timeval32_range (__time64_t t) >>> +{ >>> + __int32_t s = t; >>> + return s == t; >>> +} >>> + >> >> The name is confusing, it is mixing timeval from 'struct timeval' and >> time_t. And there is no need to use __int32_t, we need to use it only >> on installed headers to avoid namespace pollution. >> >> I fact I think it would be better to just change 'in_time_t_range' to >> use int32_t internally instead of time_t; I am pretty sure that all >> usages assume that sizeof(time_t) == 32. > > Unfortunately this is not correct. MIPS N64 uses a 64-bit time_t, > however it does not define XSTAT_IS_XSTAT64. Therefore fstatat is > different than fstatat64, and uses the default > sysdeps/unix/sysv/linux/fstatat.c implementation, which uses > in_time_t_range() with a 64-bit time_t. > > Therefore this change broke y2038 support in fstat/fstatat/lstat on MIPS > N64, which now returns EOVERFLOW for files which need such support. I > have opened BZ #29730. I think the issue in fact is XSTAT_IS_XSTAT64 is not a correct macro to define whether non-LFS and LFS symbols aliases. It is used on both old compat code (__xstat*) and on kernel syscall wrapper to define whether the kernel struct needs to be adjusted to glibc exported layout. In fact a better solution would be to add a new define, STAT_IS_STAT64, and define it 1 to both sparc64 and mips64. It would both mips64 and sparc64 alias non-LFS to LFS, making it both avoid memory copies between struct stat and struct stat64 (since for both ABI there essentially the same layout) and avoid the time_t range check (which should be used solely for compatibility code. ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-10-31 14:02 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-06-07 13:18 [PATCH] time: Skip overflow itimer tests on 32-bit systems Stafford Horne 2021-06-09 13:50 ` Adhemerval Zanella 2021-06-09 21:38 ` Stafford Horne 2021-06-12 9:19 ` Stafford Horne 2021-07-06 19:58 ` Adhemerval Zanella 2021-07-07 21:11 ` Stafford Horne 2022-10-28 19:47 ` Aurelien Jarno 2022-10-31 14:02 ` Adhemerval Zanella Netto
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).