* [committed] libstdc++: Use custom timespec in system calls [PR 93421]
@ 2020-11-14 0:17 Jonathan Wakely
2020-11-14 13:29 ` Mike Crowe
0 siblings, 1 reply; 7+ messages in thread
From: Jonathan Wakely @ 2020-11-14 0:17 UTC (permalink / raw)
To: libstdc++, gcc-patches
[-- Attachment #1: Type: text/plain, Size: 1745 bytes --]
On 32-bit targets where userspace has switched to 64-bit time_t, we
cannot pass struct timespec to SYS_futex or SYS_clock_gettime, because
the userspace definition of struct timespec will not match what the
kernel expects.
We use the existence of the SYS_futex_time64 or SYS_clock_gettime_time64
macros to imply that userspace *might* have switched to the new timespec
definition. This is a conservative assumption. It's possible that the
new syscall numbers are defined in the libc headers but that timespec
hasn't been updated yet (as is the case for glibc currently). But using
the alternative struct with two longs is still OK, it's just redundant
if userspace timespec still uses a 32-bit time_t.
We also check that SYS_futex_time64 != SYS_futex so that we don't try
to use a 32-bit tv_sec on modern targets that only support the 64-bit
system calls and define the old macro to the same value as the new one.
We could possibly check #ifdef __USE_TIME_BITS64 to see whether
userspace has actually been updated, but it's not clear if user code
is meant to inspect that or if it's only for libc internal use.
libstdc++-v3/ChangeLog:
PR libstdc++/93421
* src/c++11/chrono.cc [_GLIBCXX_USE_CLOCK_GETTIME_SYSCALL]
(syscall_timespec): Define a type suitable for SYS_clock_gettime
calls.
(system_clock::now(), steady_clock::now()): Use syscall_timespec
instead of timespec.
* src/c++11/futex.cc (syscall_timespec): Define a type suitable
for SYS_futex and SYS_clock_gettime calls.
(relative_timespec): Use syscall_timespec instead of timespec.
(__atomic_futex_unsigned_base::_M_futex_wait_until): Likewise.
(__atomic_futex_unsigned_base::_M_futex_wait_until_steady):
Likewise.
Tested x86_64-linux, -m32 too. Committed to trunk.
[-- Attachment #2: patch.txt --]
[-- Type: text/plain, Size: 5978 bytes --]
commit 4d039cb9a1d0ffc6910fe09b726c3561b64527dc
Author: Jonathan Wakely <jwakely@redhat.com>
Date: Thu Sep 24 17:35:52 2020
libstdc++: Use custom timespec in system calls [PR 93421]
On 32-bit targets where userspace has switched to 64-bit time_t, we
cannot pass struct timespec to SYS_futex or SYS_clock_gettime, because
the userspace definition of struct timespec will not match what the
kernel expects.
We use the existence of the SYS_futex_time64 or SYS_clock_gettime_time64
macros to imply that userspace *might* have switched to the new timespec
definition. This is a conservative assumption. It's possible that the
new syscall numbers are defined in the libc headers but that timespec
hasn't been updated yet (as is the case for glibc currently). But using
the alternative struct with two longs is still OK, it's just redundant
if userspace timespec still uses a 32-bit time_t.
We also check that SYS_futex_time64 != SYS_futex so that we don't try
to use a 32-bit tv_sec on modern targets that only support the 64-bit
system calls and define the old macro to the same value as the new one.
We could possibly check #ifdef __USE_TIME_BITS64 to see whether
userspace has actually been updated, but it's not clear if user code
is meant to inspect that or if it's only for libc internal use.
libstdc++-v3/ChangeLog:
PR libstdc++/93421
* src/c++11/chrono.cc [_GLIBCXX_USE_CLOCK_GETTIME_SYSCALL]
(syscall_timespec): Define a type suitable for SYS_clock_gettime
calls.
(system_clock::now(), steady_clock::now()): Use syscall_timespec
instead of timespec.
* src/c++11/futex.cc (syscall_timespec): Define a type suitable
for SYS_futex and SYS_clock_gettime calls.
(relative_timespec): Use syscall_timespec instead of timespec.
(__atomic_futex_unsigned_base::_M_futex_wait_until): Likewise.
(__atomic_futex_unsigned_base::_M_futex_wait_until_steady):
Likewise.
diff --git a/libstdc++-v3/src/c++11/chrono.cc b/libstdc++-v3/src/c++11/chrono.cc
index 723f3002d11a..f10be7d8c073 100644
--- a/libstdc++-v3/src/c++11/chrono.cc
+++ b/libstdc++-v3/src/c++11/chrono.cc
@@ -35,6 +35,17 @@
#ifdef _GLIBCXX_USE_CLOCK_GETTIME_SYSCALL
#include <unistd.h>
#include <sys/syscall.h>
+
+# if defined(SYS_clock_gettime_time64) \
+ && SYS_clock_gettime_time64 != SYS_clock_gettime
+ // Userspace knows about the new time64 syscalls, so it's possible that
+ // userspace has also updated timespec to use a 64-bit tv_sec.
+ // The SYS_clock_gettime syscall still uses the old definition
+ // of timespec where tv_sec is 32 bits, so define a type that matches that.
+ struct syscall_timespec { long tv_sec; long tv_nsec; };
+# else
+ using syscall_timespec = ::timespec;
+# endif
#endif
namespace std _GLIBCXX_VISIBILITY(default)
@@ -52,11 +63,12 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
system_clock::now() noexcept
{
#ifdef _GLIBCXX_USE_CLOCK_REALTIME
- timespec tp;
// -EINVAL, -EFAULT
#ifdef _GLIBCXX_USE_CLOCK_GETTIME_SYSCALL
+ syscall_timespec tp;
syscall(SYS_clock_gettime, CLOCK_REALTIME, &tp);
#else
+ timespec tp;
clock_gettime(CLOCK_REALTIME, &tp);
#endif
return time_point(duration(chrono::seconds(tp.tv_sec)
@@ -80,11 +92,12 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
steady_clock::now() noexcept
{
#ifdef _GLIBCXX_USE_CLOCK_MONOTONIC
- timespec tp;
// -EINVAL, -EFAULT
#ifdef _GLIBCXX_USE_CLOCK_GETTIME_SYSCALL
+ syscall_timespec tp;
syscall(SYS_clock_gettime, CLOCK_MONOTONIC, &tp);
#else
+ timespec tp;
clock_gettime(CLOCK_MONOTONIC, &tp);
#endif
return time_point(duration(chrono::seconds(tp.tv_sec)
diff --git a/libstdc++-v3/src/c++11/futex.cc b/libstdc++-v3/src/c++11/futex.cc
index c008798318c9..9adfb898b4f1 100644
--- a/libstdc++-v3/src/c++11/futex.cc
+++ b/libstdc++-v3/src/c++11/futex.cc
@@ -58,13 +58,23 @@ namespace
std::atomic<bool> futex_clock_realtime_unavailable;
std::atomic<bool> futex_clock_monotonic_unavailable;
+#if defined(SYS_futex_time64) && SYS_futex_time64 != SYS_futex
+ // Userspace knows about the new time64 syscalls, so it's possible that
+ // userspace has also updated timespec to use a 64-bit tv_sec.
+ // The SYS_futex and SYS_clock_gettime syscalls still use the old definition
+ // of timespec where tv_sec is 32 bits, so define a type that matches that.
+ struct syscall_timespec { long tv_sec; long tv_nsec; };
+#else
+ using syscall_timespec = ::timespec;
+#endif
+
// Return the relative duration from (now_s + now_ns) to (abs_s + abs_ns)
- // as a timespec.
- struct timespec
+ // as a timespec suitable for syscalls.
+ syscall_timespec
relative_timespec(chrono::seconds abs_s, chrono::nanoseconds abs_ns,
time_t now_s, long now_ns)
{
- struct timespec rt;
+ syscall_timespec rt;
// Did we already time out?
if (now_s > abs_s.count())
@@ -119,7 +129,7 @@ namespace
if (__s.count() < 0)
return false;
- struct timespec rt;
+ syscall_timespec rt;
if (__s.count() > __int_traits<time_t>::__max) [[unlikely]]
rt.tv_sec = __int_traits<time_t>::__max;
else
@@ -195,7 +205,7 @@ namespace
if (__s.count() < 0) [[unlikely]]
return false;
- struct timespec rt;
+ syscall_timespec rt;
if (__s.count() > __int_traits<time_t>::__max) [[unlikely]]
rt.tv_sec = __int_traits<time_t>::__max;
else
@@ -224,10 +234,11 @@ namespace
// We only get to here if futex_clock_monotonic_unavailable was
// true or has just been set to true.
- struct timespec ts;
#ifdef _GLIBCXX_USE_CLOCK_GETTIME_SYSCALL
+ syscall_timespec ts;
syscall(SYS_clock_gettime, CLOCK_MONOTONIC, &ts);
#else
+ struct timespec ts;
clock_gettime(CLOCK_MONOTONIC, &ts);
#endif
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [committed] libstdc++: Use custom timespec in system calls [PR 93421]
2020-11-14 0:17 [committed] libstdc++: Use custom timespec in system calls [PR 93421] Jonathan Wakely
@ 2020-11-14 13:29 ` Mike Crowe
2020-11-14 14:23 ` Jonathan Wakely
0 siblings, 1 reply; 7+ messages in thread
From: Mike Crowe @ 2020-11-14 13:29 UTC (permalink / raw)
To: Jonathan Wakely; +Cc: libstdc++, gcc-patches
On Saturday 14 November 2020 at 00:17:59 +0000, Jonathan Wakely via Libstdc++ wrote:
> On 32-bit targets where userspace has switched to 64-bit time_t, we
> cannot pass struct timespec to SYS_futex or SYS_clock_gettime, because
> the userspace definition of struct timespec will not match what the
> kernel expects.
>
> We use the existence of the SYS_futex_time64 or SYS_clock_gettime_time64
> macros to imply that userspace *might* have switched to the new timespec
> definition. This is a conservative assumption. It's possible that the
> new syscall numbers are defined in the libc headers but that timespec
> hasn't been updated yet (as is the case for glibc currently). But using
> the alternative struct with two longs is still OK, it's just redundant
> if userspace timespec still uses a 32-bit time_t.
>
> We also check that SYS_futex_time64 != SYS_futex so that we don't try
> to use a 32-bit tv_sec on modern targets that only support the 64-bit
> system calls and define the old macro to the same value as the new one.
>
> We could possibly check #ifdef __USE_TIME_BITS64 to see whether
> userspace has actually been updated, but it's not clear if user code
> is meant to inspect that or if it's only for libc internal use.
>
> libstdc++-v3/ChangeLog:
>
> PR libstdc++/93421
> * src/c++11/chrono.cc [_GLIBCXX_USE_CLOCK_GETTIME_SYSCALL]
> (syscall_timespec): Define a type suitable for SYS_clock_gettime
> calls.
> (system_clock::now(), steady_clock::now()): Use syscall_timespec
> instead of timespec.
> * src/c++11/futex.cc (syscall_timespec): Define a type suitable
> for SYS_futex and SYS_clock_gettime calls.
> (relative_timespec): Use syscall_timespec instead of timespec.
> (__atomic_futex_unsigned_base::_M_futex_wait_until): Likewise.
> (__atomic_futex_unsigned_base::_M_futex_wait_until_steady):
> Likewise.
>
> Tested x86_64-linux, -m32 too. Committed to trunk.
>
> commit 4d039cb9a1d0ffc6910fe09b726c3561b64527dc
> Author: Jonathan Wakely <jwakely@redhat.com>
> Date: Thu Sep 24 17:35:52 2020
>
> libstdc++: Use custom timespec in system calls [PR 93421]
>
> On 32-bit targets where userspace has switched to 64-bit time_t, we
> cannot pass struct timespec to SYS_futex or SYS_clock_gettime, because
> the userspace definition of struct timespec will not match what the
> kernel expects.
>
> We use the existence of the SYS_futex_time64 or SYS_clock_gettime_time64
> macros to imply that userspace *might* have switched to the new timespec
> definition. This is a conservative assumption. It's possible that the
> new syscall numbers are defined in the libc headers but that timespec
> hasn't been updated yet (as is the case for glibc currently). But using
> the alternative struct with two longs is still OK, it's just redundant
> if userspace timespec still uses a 32-bit time_t.
>
> We also check that SYS_futex_time64 != SYS_futex so that we don't try
> to use a 32-bit tv_sec on modern targets that only support the 64-bit
> system calls and define the old macro to the same value as the new one.
>
> We could possibly check #ifdef __USE_TIME_BITS64 to see whether
> userspace has actually been updated, but it's not clear if user code
> is meant to inspect that or if it's only for libc internal use.
Presumably this is change is only good for the short term? We really want
to be calling the 64-bit time_t versions of SYS_futex and SYS_clock_gettime
passing 64-bit struct timespec so that this code continues to work
correctly after 2038 (for CLOCK_REALTIME) or if someone is unlucky enough
to have a system uptime of over 68 years (for CLOCK_MONOTONIC.) Perhaps
that's part of the post-GCC11 work that you plan to do.
(There's another comment on the patch itself below.)
> libstdc++-v3/ChangeLog:
>
> PR libstdc++/93421
> * src/c++11/chrono.cc [_GLIBCXX_USE_CLOCK_GETTIME_SYSCALL]
> (syscall_timespec): Define a type suitable for SYS_clock_gettime
> calls.
> (system_clock::now(), steady_clock::now()): Use syscall_timespec
> instead of timespec.
> * src/c++11/futex.cc (syscall_timespec): Define a type suitable
> for SYS_futex and SYS_clock_gettime calls.
> (relative_timespec): Use syscall_timespec instead of timespec.
> (__atomic_futex_unsigned_base::_M_futex_wait_until): Likewise.
> (__atomic_futex_unsigned_base::_M_futex_wait_until_steady):
> Likewise.
>
> diff --git a/libstdc++-v3/src/c++11/chrono.cc b/libstdc++-v3/src/c++11/chrono.cc
> index 723f3002d11a..f10be7d8c073 100644
> --- a/libstdc++-v3/src/c++11/chrono.cc
> +++ b/libstdc++-v3/src/c++11/chrono.cc
> @@ -35,6 +35,17 @@
> #ifdef _GLIBCXX_USE_CLOCK_GETTIME_SYSCALL
> #include <unistd.h>
> #include <sys/syscall.h>
> +
> +# if defined(SYS_clock_gettime_time64) \
> + && SYS_clock_gettime_time64 != SYS_clock_gettime
> + // Userspace knows about the new time64 syscalls, so it's possible that
> + // userspace has also updated timespec to use a 64-bit tv_sec.
> + // The SYS_clock_gettime syscall still uses the old definition
> + // of timespec where tv_sec is 32 bits, so define a type that matches that.
> + struct syscall_timespec { long tv_sec; long tv_nsec; };
> +# else
> + using syscall_timespec = ::timespec;
> +# endif
> #endif
>
> namespace std _GLIBCXX_VISIBILITY(default)
> @@ -52,11 +63,12 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> system_clock::now() noexcept
> {
> #ifdef _GLIBCXX_USE_CLOCK_REALTIME
> - timespec tp;
> // -EINVAL, -EFAULT
> #ifdef _GLIBCXX_USE_CLOCK_GETTIME_SYSCALL
> + syscall_timespec tp;
> syscall(SYS_clock_gettime, CLOCK_REALTIME, &tp);
> #else
> + timespec tp;
> clock_gettime(CLOCK_REALTIME, &tp);
> #endif
> return time_point(duration(chrono::seconds(tp.tv_sec)
> @@ -80,11 +92,12 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> steady_clock::now() noexcept
> {
> #ifdef _GLIBCXX_USE_CLOCK_MONOTONIC
> - timespec tp;
> // -EINVAL, -EFAULT
> #ifdef _GLIBCXX_USE_CLOCK_GETTIME_SYSCALL
> + syscall_timespec tp;
> syscall(SYS_clock_gettime, CLOCK_MONOTONIC, &tp);
> #else
> + timespec tp;
> clock_gettime(CLOCK_MONOTONIC, &tp);
> #endif
> return time_point(duration(chrono::seconds(tp.tv_sec)
> diff --git a/libstdc++-v3/src/c++11/futex.cc b/libstdc++-v3/src/c++11/futex.cc
> index c008798318c9..9adfb898b4f1 100644
> --- a/libstdc++-v3/src/c++11/futex.cc
> +++ b/libstdc++-v3/src/c++11/futex.cc
> @@ -58,13 +58,23 @@ namespace
> std::atomic<bool> futex_clock_realtime_unavailable;
> std::atomic<bool> futex_clock_monotonic_unavailable;
>
> +#if defined(SYS_futex_time64) && SYS_futex_time64 != SYS_futex
> + // Userspace knows about the new time64 syscalls, so it's possible that
> + // userspace has also updated timespec to use a 64-bit tv_sec.
> + // The SYS_futex and SYS_clock_gettime syscalls still use the old definition
> + // of timespec where tv_sec is 32 bits, so define a type that matches that.
> + struct syscall_timespec { long tv_sec; long tv_nsec; };
> +#else
> + using syscall_timespec = ::timespec;
> +#endif
> +
> // Return the relative duration from (now_s + now_ns) to (abs_s + abs_ns)
> - // as a timespec.
> - struct timespec
> + // as a timespec suitable for syscalls.
> + syscall_timespec
> relative_timespec(chrono::seconds abs_s, chrono::nanoseconds abs_ns,
> time_t now_s, long now_ns)
> {
> - struct timespec rt;
> + syscall_timespec rt;
>
> // Did we already time out?
> if (now_s > abs_s.count())
> @@ -119,7 +129,7 @@ namespace
> if (__s.count() < 0)
> return false;
>
> - struct timespec rt;
> + syscall_timespec rt;
> if (__s.count() > __int_traits<time_t>::__max) [[unlikely]]
> rt.tv_sec = __int_traits<time_t>::__max;
> else
> @@ -195,7 +205,7 @@ namespace
> if (__s.count() < 0) [[unlikely]]
> return false;
>
> - struct timespec rt;
> + syscall_timespec rt;
> if (__s.count() > __int_traits<time_t>::__max) [[unlikely]]
> rt.tv_sec = __int_traits<time_t>::__max;
Do these now need to be __int_traits<long>::__max in case time_t is 64-bit
yet syscall_timespec is using 32-bit long?
> else
> @@ -224,10 +234,11 @@ namespace
>
> // We only get to here if futex_clock_monotonic_unavailable was
> // true or has just been set to true.
> - struct timespec ts;
> #ifdef _GLIBCXX_USE_CLOCK_GETTIME_SYSCALL
> + syscall_timespec ts;
> syscall(SYS_clock_gettime, CLOCK_MONOTONIC, &ts);
> #else
> + struct timespec ts;
> clock_gettime(CLOCK_MONOTONIC, &ts);
> #endif
>
Thanks.
Mike.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [committed] libstdc++: Use custom timespec in system calls [PR 93421]
2020-11-14 13:29 ` Mike Crowe
@ 2020-11-14 14:23 ` Jonathan Wakely
2020-11-18 0:01 ` Jonathan Wakely
0 siblings, 1 reply; 7+ messages in thread
From: Jonathan Wakely @ 2020-11-14 14:23 UTC (permalink / raw)
To: Mike Crowe; +Cc: Jonathan Wakely, libstdc++, gcc-patches
On Sat, 14 Nov 2020, 13:30 Mike Crowe via Libstdc++, <libstdc++@gcc.gnu.org>
wrote:
> On Saturday 14 November 2020 at 00:17:59 +0000, Jonathan Wakely via
> Libstdc++ wrote:
> > On 32-bit targets where userspace has switched to 64-bit time_t, we
> > cannot pass struct timespec to SYS_futex or SYS_clock_gettime, because
> > the userspace definition of struct timespec will not match what the
> > kernel expects.
> >
> > We use the existence of the SYS_futex_time64 or SYS_clock_gettime_time64
> > macros to imply that userspace *might* have switched to the new timespec
> > definition. This is a conservative assumption. It's possible that the
> > new syscall numbers are defined in the libc headers but that timespec
> > hasn't been updated yet (as is the case for glibc currently). But using
> > the alternative struct with two longs is still OK, it's just redundant
> > if userspace timespec still uses a 32-bit time_t.
> >
> > We also check that SYS_futex_time64 != SYS_futex so that we don't try
> > to use a 32-bit tv_sec on modern targets that only support the 64-bit
> > system calls and define the old macro to the same value as the new one.
> >
> > We could possibly check #ifdef __USE_TIME_BITS64 to see whether
> > userspace has actually been updated, but it's not clear if user code
> > is meant to inspect that or if it's only for libc internal use.
> >
> > libstdc++-v3/ChangeLog:
> >
> > PR libstdc++/93421
> > * src/c++11/chrono.cc [_GLIBCXX_USE_CLOCK_GETTIME_SYSCALL]
> > (syscall_timespec): Define a type suitable for SYS_clock_gettime
> > calls.
> > (system_clock::now(), steady_clock::now()): Use syscall_timespec
> > instead of timespec.
> > * src/c++11/futex.cc (syscall_timespec): Define a type suitable
> > for SYS_futex and SYS_clock_gettime calls.
> > (relative_timespec): Use syscall_timespec instead of timespec.
> > (__atomic_futex_unsigned_base::_M_futex_wait_until): Likewise.
> > (__atomic_futex_unsigned_base::_M_futex_wait_until_steady):
> > Likewise.
> >
> > Tested x86_64-linux, -m32 too. Committed to trunk.
> >
>
> > commit 4d039cb9a1d0ffc6910fe09b726c3561b64527dc
> > Author: Jonathan Wakely <jwakely@redhat.com>
> > Date: Thu Sep 24 17:35:52 2020
> >
> > libstdc++: Use custom timespec in system calls [PR 93421]
> >
> > On 32-bit targets where userspace has switched to 64-bit time_t, we
> > cannot pass struct timespec to SYS_futex or SYS_clock_gettime,
> because
> > the userspace definition of struct timespec will not match what the
> > kernel expects.
> >
> > We use the existence of the SYS_futex_time64 or
> SYS_clock_gettime_time64
> > macros to imply that userspace *might* have switched to the new
> timespec
> > definition. This is a conservative assumption. It's possible that
> the
> > new syscall numbers are defined in the libc headers but that timespec
> > hasn't been updated yet (as is the case for glibc currently). But
> using
> > the alternative struct with two longs is still OK, it's just
> redundant
> > if userspace timespec still uses a 32-bit time_t.
> >
> > We also check that SYS_futex_time64 != SYS_futex so that we don't try
> > to use a 32-bit tv_sec on modern targets that only support the 64-bit
> > system calls and define the old macro to the same value as the new
> one.
> >
> > We could possibly check #ifdef __USE_TIME_BITS64 to see whether
> > userspace has actually been updated, but it's not clear if user code
> > is meant to inspect that or if it's only for libc internal use.
>
> Presumably this is change is only good for the short term? We really want
> to be calling the 64-bit time_t versions of SYS_futex and SYS_clock_gettime
> passing 64-bit struct timespec so that this code continues to work
> correctly after 2038 (for CLOCK_REALTIME) or if someone is unlucky enough
> to have a system uptime of over 68 years (for CLOCK_MONOTONIC.) Perhaps
> that's part of the post-GCC11 work that you plan to do.
>
Right. This is definitely a short term solution, but I ran out of time to
do something better for GCC 11.
> (There's another comment on the patch itself below.)
>
> > libstdc++-v3/ChangeLog:
> >
> > PR libstdc++/93421
> > * src/c++11/chrono.cc [_GLIBCXX_USE_CLOCK_GETTIME_SYSCALL]
> > (syscall_timespec): Define a type suitable for
> SYS_clock_gettime
> > calls.
> > (system_clock::now(), steady_clock::now()): Use
> syscall_timespec
> > instead of timespec.
> > * src/c++11/futex.cc (syscall_timespec): Define a type
> suitable
> > for SYS_futex and SYS_clock_gettime calls.
> > (relative_timespec): Use syscall_timespec instead of
> timespec.
> > (__atomic_futex_unsigned_base::_M_futex_wait_until):
> Likewise.
> > (__atomic_futex_unsigned_base::_M_futex_wait_until_steady):
> > Likewise.
> >
> > diff --git a/libstdc++-v3/src/c++11/chrono.cc
> b/libstdc++-v3/src/c++11/chrono.cc
> > index 723f3002d11a..f10be7d8c073 100644
> > --- a/libstdc++-v3/src/c++11/chrono.cc
> > +++ b/libstdc++-v3/src/c++11/chrono.cc
> > @@ -35,6 +35,17 @@
> > #ifdef _GLIBCXX_USE_CLOCK_GETTIME_SYSCALL
> > #include <unistd.h>
> > #include <sys/syscall.h>
> > +
> > +# if defined(SYS_clock_gettime_time64) \
> > + && SYS_clock_gettime_time64 != SYS_clock_gettime
> > + // Userspace knows about the new time64 syscalls, so it's possible
> that
> > + // userspace has also updated timespec to use a 64-bit tv_sec.
> > + // The SYS_clock_gettime syscall still uses the old definition
> > + // of timespec where tv_sec is 32 bits, so define a type that matches
> that.
> > + struct syscall_timespec { long tv_sec; long tv_nsec; };
> > +# else
> > + using syscall_timespec = ::timespec;
> > +# endif
> > #endif
> >
> > namespace std _GLIBCXX_VISIBILITY(default)
> > @@ -52,11 +63,12 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> > system_clock::now() noexcept
> > {
> > #ifdef _GLIBCXX_USE_CLOCK_REALTIME
> > - timespec tp;
> > // -EINVAL, -EFAULT
> > #ifdef _GLIBCXX_USE_CLOCK_GETTIME_SYSCALL
> > + syscall_timespec tp;
> > syscall(SYS_clock_gettime, CLOCK_REALTIME, &tp);
> > #else
> > + timespec tp;
> > clock_gettime(CLOCK_REALTIME, &tp);
> > #endif
> > return time_point(duration(chrono::seconds(tp.tv_sec)
> > @@ -80,11 +92,12 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> > steady_clock::now() noexcept
> > {
> > #ifdef _GLIBCXX_USE_CLOCK_MONOTONIC
> > - timespec tp;
> > // -EINVAL, -EFAULT
> > #ifdef _GLIBCXX_USE_CLOCK_GETTIME_SYSCALL
> > + syscall_timespec tp;
> > syscall(SYS_clock_gettime, CLOCK_MONOTONIC, &tp);
> > #else
> > + timespec tp;
> > clock_gettime(CLOCK_MONOTONIC, &tp);
> > #endif
> > return time_point(duration(chrono::seconds(tp.tv_sec)
> > diff --git a/libstdc++-v3/src/c++11/futex.cc
> b/libstdc++-v3/src/c++11/futex.cc
> > index c008798318c9..9adfb898b4f1 100644
> > --- a/libstdc++-v3/src/c++11/futex.cc
> > +++ b/libstdc++-v3/src/c++11/futex.cc
> > @@ -58,13 +58,23 @@ namespace
> > std::atomic<bool> futex_clock_realtime_unavailable;
> > std::atomic<bool> futex_clock_monotonic_unavailable;
> >
> > +#if defined(SYS_futex_time64) && SYS_futex_time64 != SYS_futex
> > + // Userspace knows about the new time64 syscalls, so it's possible
> that
> > + // userspace has also updated timespec to use a 64-bit tv_sec.
> > + // The SYS_futex and SYS_clock_gettime syscalls still use the old
> definition
> > + // of timespec where tv_sec is 32 bits, so define a type that matches
> that.
> > + struct syscall_timespec { long tv_sec; long tv_nsec; };
> > +#else
> > + using syscall_timespec = ::timespec;
> > +#endif
> > +
> > // Return the relative duration from (now_s + now_ns) to (abs_s +
> abs_ns)
> > - // as a timespec.
> > - struct timespec
> > + // as a timespec suitable for syscalls.
> > + syscall_timespec
> > relative_timespec(chrono::seconds abs_s, chrono::nanoseconds abs_ns,
> > time_t now_s, long now_ns)
> > {
> > - struct timespec rt;
> > + syscall_timespec rt;
> >
> > // Did we already time out?
> > if (now_s > abs_s.count())
> > @@ -119,7 +129,7 @@ namespace
> > if (__s.count() < 0)
> > return false;
> >
> > - struct timespec rt;
> > + syscall_timespec rt;
> > if (__s.count() > __int_traits<time_t>::__max) [[unlikely]]
> > rt.tv_sec = __int_traits<time_t>::__max;
> > else
> > @@ -195,7 +205,7 @@ namespace
> > if (__s.count() < 0) [[unlikely]]
> > return false;
> >
> > - struct timespec rt;
> > + syscall_timespec rt;
> > if (__s.count() > __int_traits<time_t>::__max) [[unlikely]]
> > rt.tv_sec = __int_traits<time_t>::__max;
>
> Do these now need to be __int_traits<long>::__max in case time_t is 64-bit
> yet syscall_timespec is using 32-bit long?
>
Ah yes. Maybe decltype(rt.tv_sec).
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [committed] libstdc++: Use custom timespec in system calls [PR 93421]
2020-11-14 14:23 ` Jonathan Wakely
@ 2020-11-18 0:01 ` Jonathan Wakely
2020-11-18 20:22 ` Jonathan Wakely
0 siblings, 1 reply; 7+ messages in thread
From: Jonathan Wakely @ 2020-11-18 0:01 UTC (permalink / raw)
To: Jonathan Wakely via Libstdc++; +Cc: Mike Crowe, gcc-patches
[-- Attachment #1: Type: text/plain, Size: 5203 bytes --]
On 14/11/20 14:23 +0000, Jonathan Wakely via Libstdc++ wrote:
>On Sat, 14 Nov 2020, 13:30 Mike Crowe via Libstdc++, <libstdc++@gcc.gnu.org>
>wrote:
>
>> On Saturday 14 November 2020 at 00:17:59 +0000, Jonathan Wakely via
>> Libstdc++ wrote:
>> > On 32-bit targets where userspace has switched to 64-bit time_t, we
>> > cannot pass struct timespec to SYS_futex or SYS_clock_gettime, because
>> > the userspace definition of struct timespec will not match what the
>> > kernel expects.
>> >
>> > We use the existence of the SYS_futex_time64 or SYS_clock_gettime_time64
>> > macros to imply that userspace *might* have switched to the new timespec
>> > definition. This is a conservative assumption. It's possible that the
>> > new syscall numbers are defined in the libc headers but that timespec
>> > hasn't been updated yet (as is the case for glibc currently). But using
>> > the alternative struct with two longs is still OK, it's just redundant
>> > if userspace timespec still uses a 32-bit time_t.
>> >
>> > We also check that SYS_futex_time64 != SYS_futex so that we don't try
>> > to use a 32-bit tv_sec on modern targets that only support the 64-bit
>> > system calls and define the old macro to the same value as the new one.
>> >
>> > We could possibly check #ifdef __USE_TIME_BITS64 to see whether
>> > userspace has actually been updated, but it's not clear if user code
>> > is meant to inspect that or if it's only for libc internal use.
>> >
>> > libstdc++-v3/ChangeLog:
>> >
>> > PR libstdc++/93421
>> > * src/c++11/chrono.cc [_GLIBCXX_USE_CLOCK_GETTIME_SYSCALL]
>> > (syscall_timespec): Define a type suitable for SYS_clock_gettime
>> > calls.
>> > (system_clock::now(), steady_clock::now()): Use syscall_timespec
>> > instead of timespec.
>> > * src/c++11/futex.cc (syscall_timespec): Define a type suitable
>> > for SYS_futex and SYS_clock_gettime calls.
>> > (relative_timespec): Use syscall_timespec instead of timespec.
>> > (__atomic_futex_unsigned_base::_M_futex_wait_until): Likewise.
>> > (__atomic_futex_unsigned_base::_M_futex_wait_until_steady):
>> > Likewise.
>> >
>> > Tested x86_64-linux, -m32 too. Committed to trunk.
>> >
>>
>> > commit 4d039cb9a1d0ffc6910fe09b726c3561b64527dc
>> > Author: Jonathan Wakely <jwakely@redhat.com>
>> > Date: Thu Sep 24 17:35:52 2020
>> >
>> > libstdc++: Use custom timespec in system calls [PR 93421]
>> >
>> > On 32-bit targets where userspace has switched to 64-bit time_t, we
>> > cannot pass struct timespec to SYS_futex or SYS_clock_gettime,
>> because
>> > the userspace definition of struct timespec will not match what the
>> > kernel expects.
>> >
>> > We use the existence of the SYS_futex_time64 or
>> SYS_clock_gettime_time64
>> > macros to imply that userspace *might* have switched to the new
>> timespec
>> > definition. This is a conservative assumption. It's possible that
>> the
>> > new syscall numbers are defined in the libc headers but that timespec
>> > hasn't been updated yet (as is the case for glibc currently). But
>> using
>> > the alternative struct with two longs is still OK, it's just
>> redundant
>> > if userspace timespec still uses a 32-bit time_t.
>> >
>> > We also check that SYS_futex_time64 != SYS_futex so that we don't try
>> > to use a 32-bit tv_sec on modern targets that only support the 64-bit
>> > system calls and define the old macro to the same value as the new
>> one.
>> >
>> > We could possibly check #ifdef __USE_TIME_BITS64 to see whether
>> > userspace has actually been updated, but it's not clear if user code
>> > is meant to inspect that or if it's only for libc internal use.
>>
>> Presumably this is change is only good for the short term? We really want
>> to be calling the 64-bit time_t versions of SYS_futex and SYS_clock_gettime
>> passing 64-bit struct timespec so that this code continues to work
>> correctly after 2038 (for CLOCK_REALTIME) or if someone is unlucky enough
>> to have a system uptime of over 68 years (for CLOCK_MONOTONIC.) Perhaps
>> that's part of the post-GCC11 work that you plan to do.
>>
>
>Right. This is definitely a short term solution, but I ran out of time to
>do something better for GCC 11.
We can still do a bit better than that patch though, which was just
broken (it's SYS_clock_gettime64 not SYS_clock_gettime_time64 for a
start).
This partially reverts it, to remove the conditional compilation
related to SYS_clock_gettime64, because that's almost certainly never
going to be needed.
Tested x86_64-linux, with glibc 2.12 and 2.31, committed to trunk.
>> > @@ -195,7 +205,7 @@ namespace
>> > if (__s.count() < 0) [[unlikely]]
>> > return false;
>> >
>> > - struct timespec rt;
>> > + syscall_timespec rt;
>> > if (__s.count() > __int_traits<time_t>::__max) [[unlikely]]
>> > rt.tv_sec = __int_traits<time_t>::__max;
>>
>> Do these now need to be __int_traits<long>::__max in case time_t is 64-bit
>> yet syscall_timespec is using 32-bit long?
>>
>
>Ah yes. Maybe decltype(rt.tv_sec).
I'll fix that in the next patch.
[-- Attachment #2: patch.txt --]
[-- Type: text/x-patch, Size: 6806 bytes --]
commit 1e3e6c700f04fe6992b9077541e434172c1cbdae
Author: Jonathan Wakely <jwakely@redhat.com>
Date: Tue Nov 17 16:13:02 2020
libstdc++: Revert changes for SYS_clock_gettime64 [PR 93421]
As discussed in the PR, it's incredibly unlikely that a system that
needs to use the SYS_clock_gettime syscall (e.g. glibc 2.16 or older) is
going to define the SYS_clock_gettime64 macro. Ancient systems that need
to use the syscall aren't going to have time64 support.
This reverts the recent changes to try and make clock_gettime syscalls
be compatible with systems that have been updated for time64 (those
changes were wrong anyway as they misspelled the SYS_clock_gettime64
macro). The changes for futex syscalls are retained, because we still
use them on modern systems that might be using time64.
To ensure that the clock_gettime syscalls are safe, configure will fail
if SYS_clock_gettime is needed, and SYS_clock_gettime64 is also defined
(but to a distinct value from SYS_clock_gettime), and the tv_sec member
of timespec is larger than long. This means we will be unable to build
on a hypothetical system where we need the time32 version of
SYS_clock_gettime but where userspace is using a time64 struct timespec.
In the unlikely event that this failure is triggered on any real
systems, we can fix it later. But we probably won't need to.
libstdc++-v3/ChangeLog:
PR libstdc++/93421
* acinclude.m4 (GLIBCXX_ENABLE_LIBSTDCXX_TIME): Fail if struct
timespec isn't compatible with SYS_clock_gettime.
* configure: Regenerate.
* src/c++11/chrono.cc: Revert changes for time64 compatibility.
Add static_assert instead.
* src/c++11/futex.cc (_M_futex_wait_until_steady): Assume
SYS_clock_gettime can use struct timespec.
diff --git a/libstdc++-v3/acinclude.m4 b/libstdc++-v3/acinclude.m4
index 650d63ab3d75..486347b34d94 100644
--- a/libstdc++-v3/acinclude.m4
+++ b/libstdc++-v3/acinclude.m4
@@ -1561,13 +1561,34 @@ AC_DEFUN([GLIBCXX_ENABLE_LIBSTDCXX_TIME], [
#endif
syscall(SYS_clock_gettime, CLOCK_MONOTONIC, &tp);
syscall(SYS_clock_gettime, CLOCK_REALTIME, &tp);
- ], [ac_has_clock_monotonic_syscall=yes], [ac_has_clock_monotonic_syscall=no])
- AC_MSG_RESULT($ac_has_clock_monotonic_syscall)
- if test x"$ac_has_clock_monotonic_syscall" = x"yes"; then
+ ], [ac_has_clock_gettime_syscall=yes], [ac_has_clock_gettime_syscall=no])
+ AC_MSG_RESULT($ac_has_clock_gettime_syscall)
+ if test x"$ac_has_clock_gettime_syscall" = x"yes"; then
AC_DEFINE(_GLIBCXX_USE_CLOCK_GETTIME_SYSCALL, 1,
- [ Defined if clock_gettime syscall has monotonic and realtime clock support. ])
+ [Defined if clock_gettime syscall has monotonic and realtime clock support. ])
ac_has_clock_monotonic=yes
ac_has_clock_realtime=yes
+ AC_MSG_CHECKING([for struct timespec that matches syscall])
+ AC_TRY_COMPILE(
+ [#include <time.h>
+ #include <sys/syscall.h>
+ ],
+ [#ifdef SYS_clock_gettime64
+ #if SYS_clock_gettime64 != SYS_clock_gettime
+ // We need to use SYS_clock_gettime and libc appears to
+ // also know about the SYS_clock_gettime64 syscall.
+ // Check that userspace doesn't use time64 version of timespec.
+ static_assert(sizeof(timespec::tv_sec) == sizeof(long),
+ "struct timespec must be compatible with SYS_clock_gettime");
+ #endif
+ #endif
+ ],
+ [ac_timespec_matches_syscall=yes],
+ [ac_timespec_matches_syscall=no])
+ AC_MSG_RESULT($ac_timespec_matches_syscall)
+ if test x"$ac_timespec_matches_syscall" = no; then
+ AC_MSG_ERROR([struct timespec is not compatible with SYS_clock_gettime, please report a bug to http://gcc.gnu.org/bugzilla])
+ fi
fi;;
esac
fi
diff --git a/libstdc++-v3/src/c++11/chrono.cc b/libstdc++-v3/src/c++11/chrono.cc
index f10be7d8c073..723f3002d11a 100644
--- a/libstdc++-v3/src/c++11/chrono.cc
+++ b/libstdc++-v3/src/c++11/chrono.cc
@@ -35,17 +35,6 @@
#ifdef _GLIBCXX_USE_CLOCK_GETTIME_SYSCALL
#include <unistd.h>
#include <sys/syscall.h>
-
-# if defined(SYS_clock_gettime_time64) \
- && SYS_clock_gettime_time64 != SYS_clock_gettime
- // Userspace knows about the new time64 syscalls, so it's possible that
- // userspace has also updated timespec to use a 64-bit tv_sec.
- // The SYS_clock_gettime syscall still uses the old definition
- // of timespec where tv_sec is 32 bits, so define a type that matches that.
- struct syscall_timespec { long tv_sec; long tv_nsec; };
-# else
- using syscall_timespec = ::timespec;
-# endif
#endif
namespace std _GLIBCXX_VISIBILITY(default)
@@ -63,12 +52,11 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
system_clock::now() noexcept
{
#ifdef _GLIBCXX_USE_CLOCK_REALTIME
+ timespec tp;
// -EINVAL, -EFAULT
#ifdef _GLIBCXX_USE_CLOCK_GETTIME_SYSCALL
- syscall_timespec tp;
syscall(SYS_clock_gettime, CLOCK_REALTIME, &tp);
#else
- timespec tp;
clock_gettime(CLOCK_REALTIME, &tp);
#endif
return time_point(duration(chrono::seconds(tp.tv_sec)
@@ -92,12 +80,11 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
steady_clock::now() noexcept
{
#ifdef _GLIBCXX_USE_CLOCK_MONOTONIC
+ timespec tp;
// -EINVAL, -EFAULT
#ifdef _GLIBCXX_USE_CLOCK_GETTIME_SYSCALL
- syscall_timespec tp;
syscall(SYS_clock_gettime, CLOCK_MONOTONIC, &tp);
#else
- timespec tp;
clock_gettime(CLOCK_MONOTONIC, &tp);
#endif
return time_point(duration(chrono::seconds(tp.tv_sec)
diff --git a/libstdc++-v3/src/c++11/futex.cc b/libstdc++-v3/src/c++11/futex.cc
index 9adfb898b4f1..33e2097e19cf 100644
--- a/libstdc++-v3/src/c++11/futex.cc
+++ b/libstdc++-v3/src/c++11/futex.cc
@@ -61,8 +61,8 @@ namespace
#if defined(SYS_futex_time64) && SYS_futex_time64 != SYS_futex
// Userspace knows about the new time64 syscalls, so it's possible that
// userspace has also updated timespec to use a 64-bit tv_sec.
- // The SYS_futex and SYS_clock_gettime syscalls still use the old definition
- // of timespec where tv_sec is 32 bits, so define a type that matches that.
+ // The SYS_futex syscall still uses the old definition of timespec
+ // where tv_sec is 32 bits, so define a type that matches that.
struct syscall_timespec { long tv_sec; long tv_nsec; };
#else
using syscall_timespec = ::timespec;
@@ -234,11 +234,10 @@ namespace
// We only get to here if futex_clock_monotonic_unavailable was
// true or has just been set to true.
+ struct timespec ts;
#ifdef _GLIBCXX_USE_CLOCK_GETTIME_SYSCALL
- syscall_timespec ts;
syscall(SYS_clock_gettime, CLOCK_MONOTONIC, &ts);
#else
- struct timespec ts;
clock_gettime(CLOCK_MONOTONIC, &ts);
#endif
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [committed] libstdc++: Use custom timespec in system calls [PR 93421]
2020-11-18 0:01 ` Jonathan Wakely
@ 2020-11-18 20:22 ` Jonathan Wakely
2020-11-18 20:49 ` Mike Crowe
2020-11-19 13:33 ` Jonathan Wakely
0 siblings, 2 replies; 7+ messages in thread
From: Jonathan Wakely @ 2020-11-18 20:22 UTC (permalink / raw)
To: Jonathan Wakely via Libstdc++; +Cc: Mike Crowe, gcc-patches
[-- Attachment #1: Type: text/plain, Size: 758 bytes --]
On 18/11/20 00:01 +0000, Jonathan Wakely wrote:
>On 14/11/20 14:23 +0000, Jonathan Wakely wrote:
>>On Sat, 14 Nov 2020, 13:30 Mike Crowe wrote:
>>>> @@ -195,7 +205,7 @@ namespace
>>>> if (__s.count() < 0) [[unlikely]]
>>>> return false;
>>>>
>>>> - struct timespec rt;
>>>> + syscall_timespec rt;
>>>> if (__s.count() > __int_traits<time_t>::__max) [[unlikely]]
>>>> rt.tv_sec = __int_traits<time_t>::__max;
>>>
>>>Do these now need to be __int_traits<long>::__max in case time_t is 64-bit
>>>yet syscall_timespec is using 32-bit long?
>>>
>>
>>Ah yes. Maybe decltype(rt.tv_sec).
>
>I'll fix that in the next patch.
And here's that next patch. I'm testing this and will commit if all
goes well.
[-- Attachment #2: patch.txt --]
[-- Type: text/x-patch, Size: 2897 bytes --]
commit 11dfb2a0cca90b277f6bfff9306339f4424bbbdb
Author: Jonathan Wakely <jwakely@redhat.com>
Date: Wed Nov 18 15:05:25 2020
libstdc++: Fix overflow checks to use the correct "time_t" [PR 93456]
I recently added overflow checks to src/c++11/futex.cc for PR 93456, but
then changed the type of the timespec for PR 93421. This meant the
overflow checks were no longer using the right range, because the
variable being written to might be smaller than time_t.
This introduces new typedef that corresponds to the tv_sec member of the
struct being passed to the syscall, and uses that typedef in the range
checks.
libstdc++-v3/ChangeLog:
PR libstdc++/93421
PR libstdc++/93456
* src/c++11/futex.cc (syscall_time_t): New typedef for
the type of the syscall_timespec::tv_sec member.
(relative_timespec, _M_futex_wait_until)
(_M_futex_wait_until_steady): Use syscall_time_t in overflow
checks, not time_t.
diff --git a/libstdc++-v3/src/c++11/futex.cc b/libstdc++-v3/src/c++11/futex.cc
index 33e2097e19cf..290201ae2540 100644
--- a/libstdc++-v3/src/c++11/futex.cc
+++ b/libstdc++-v3/src/c++11/futex.cc
@@ -64,8 +64,10 @@ namespace
// The SYS_futex syscall still uses the old definition of timespec
// where tv_sec is 32 bits, so define a type that matches that.
struct syscall_timespec { long tv_sec; long tv_nsec; };
+ using syscall_time_t = long;
#else
using syscall_timespec = ::timespec;
+ using syscall_time_t = time_t;
#endif
// Return the relative duration from (now_s + now_ns) to (abs_s + abs_ns)
@@ -86,9 +88,9 @@ namespace
const auto rel_s = abs_s.count() - now_s;
// Convert the absolute timeout to a relative timeout, without overflow.
- if (rel_s > __int_traits<time_t>::__max) [[unlikely]]
+ if (rel_s > __int_traits<syscall_time_t>::__max) [[unlikely]]
{
- rt.tv_sec = __int_traits<time_t>::__max;
+ rt.tv_sec = __int_traits<syscall_time_t>::__max;
rt.tv_nsec = 999999999;
}
else
@@ -130,8 +132,8 @@ namespace
return false;
syscall_timespec rt;
- if (__s.count() > __int_traits<time_t>::__max) [[unlikely]]
- rt.tv_sec = __int_traits<time_t>::__max;
+ if (__s.count() > __int_traits<syscall_time_t>::__max) [[unlikely]]
+ rt.tv_sec = __int_traits<syscall_time_t>::__max;
else
rt.tv_sec = __s.count();
rt.tv_nsec = __ns.count();
@@ -206,8 +208,8 @@ namespace
return false;
syscall_timespec rt;
- if (__s.count() > __int_traits<time_t>::__max) [[unlikely]]
- rt.tv_sec = __int_traits<time_t>::__max;
+ if (__s.count() > __int_traits<syscall_time_t>::__max) [[unlikely]]
+ rt.tv_sec = __int_traits<syscall_time_t>::__max;
else
rt.tv_sec = __s.count();
rt.tv_nsec = __ns.count();
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [committed] libstdc++: Use custom timespec in system calls [PR 93421]
2020-11-18 20:22 ` Jonathan Wakely
@ 2020-11-18 20:49 ` Mike Crowe
2020-11-19 13:33 ` Jonathan Wakely
1 sibling, 0 replies; 7+ messages in thread
From: Mike Crowe @ 2020-11-18 20:49 UTC (permalink / raw)
To: Jonathan Wakely; +Cc: Jonathan Wakely via Libstdc++, gcc-patches
On Wednesday 18 November 2020 at 20:22:53 +0000, Jonathan Wakely wrote:
> On 18/11/20 00:01 +0000, Jonathan Wakely wrote:
> > On 14/11/20 14:23 +0000, Jonathan Wakely wrote:
> > > On Sat, 14 Nov 2020, 13:30 Mike Crowe wrote:
> > > > > @@ -195,7 +205,7 @@ namespace
> > > > > if (__s.count() < 0) [[unlikely]]
> > > > > return false;
> > > > >
> > > > > - struct timespec rt;
> > > > > + syscall_timespec rt;
> > > > > if (__s.count() > __int_traits<time_t>::__max) [[unlikely]]
> > > > > rt.tv_sec = __int_traits<time_t>::__max;
> > > >
> > > > Do these now need to be __int_traits<long>::__max in case time_t is 64-bit
> > > > yet syscall_timespec is using 32-bit long?
> > > >
> > >
> > > Ah yes. Maybe decltype(rt.tv_sec).
> >
> > I'll fix that in the next patch.
>
> And here's that next patch. I'm testing this and will commit if all
> goes well.
>
>
> commit 11dfb2a0cca90b277f6bfff9306339f4424bbbdb
> Author: Jonathan Wakely <jwakely@redhat.com>
> Date: Wed Nov 18 15:05:25 2020
>
> libstdc++: Fix overflow checks to use the correct "time_t" [PR 93456]
>
> I recently added overflow checks to src/c++11/futex.cc for PR 93456, but
> then changed the type of the timespec for PR 93421. This meant the
> overflow checks were no longer using the right range, because the
> variable being written to might be smaller than time_t.
>
> This introduces new typedef that corresponds to the tv_sec member of the
> struct being passed to the syscall, and uses that typedef in the range
> checks.
>
> libstdc++-v3/ChangeLog:
>
> PR libstdc++/93421
> PR libstdc++/93456
> * src/c++11/futex.cc (syscall_time_t): New typedef for
> the type of the syscall_timespec::tv_sec member.
> (relative_timespec, _M_futex_wait_until)
> (_M_futex_wait_until_steady): Use syscall_time_t in overflow
> checks, not time_t.
>
> diff --git a/libstdc++-v3/src/c++11/futex.cc b/libstdc++-v3/src/c++11/futex.cc
> index 33e2097e19cf..290201ae2540 100644
> --- a/libstdc++-v3/src/c++11/futex.cc
> +++ b/libstdc++-v3/src/c++11/futex.cc
> @@ -64,8 +64,10 @@ namespace
> // The SYS_futex syscall still uses the old definition of timespec
> // where tv_sec is 32 bits, so define a type that matches that.
> struct syscall_timespec { long tv_sec; long tv_nsec; };
> + using syscall_time_t = long;
> #else
> using syscall_timespec = ::timespec;
> + using syscall_time_t = time_t;
> #endif
>
> // Return the relative duration from (now_s + now_ns) to (abs_s + abs_ns)
> @@ -86,9 +88,9 @@ namespace
> const auto rel_s = abs_s.count() - now_s;
>
> // Convert the absolute timeout to a relative timeout, without overflow.
> - if (rel_s > __int_traits<time_t>::__max) [[unlikely]]
> + if (rel_s > __int_traits<syscall_time_t>::__max) [[unlikely]]
> {
> - rt.tv_sec = __int_traits<time_t>::__max;
> + rt.tv_sec = __int_traits<syscall_time_t>::__max;
> rt.tv_nsec = 999999999;
> }
> else
> @@ -130,8 +132,8 @@ namespace
> return false;
>
> syscall_timespec rt;
> - if (__s.count() > __int_traits<time_t>::__max) [[unlikely]]
> - rt.tv_sec = __int_traits<time_t>::__max;
> + if (__s.count() > __int_traits<syscall_time_t>::__max) [[unlikely]]
> + rt.tv_sec = __int_traits<syscall_time_t>::__max;
> else
> rt.tv_sec = __s.count();
> rt.tv_nsec = __ns.count();
> @@ -206,8 +208,8 @@ namespace
> return false;
>
> syscall_timespec rt;
> - if (__s.count() > __int_traits<time_t>::__max) [[unlikely]]
> - rt.tv_sec = __int_traits<time_t>::__max;
> + if (__s.count() > __int_traits<syscall_time_t>::__max) [[unlikely]]
> + rt.tv_sec = __int_traits<syscall_time_t>::__max;
> else
> rt.tv_sec = __s.count();
> rt.tv_nsec = __ns.count();
LGTM.
Mike.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [committed] libstdc++: Use custom timespec in system calls [PR 93421]
2020-11-18 20:22 ` Jonathan Wakely
2020-11-18 20:49 ` Mike Crowe
@ 2020-11-19 13:33 ` Jonathan Wakely
1 sibling, 0 replies; 7+ messages in thread
From: Jonathan Wakely @ 2020-11-19 13:33 UTC (permalink / raw)
To: Jonathan Wakely via Libstdc++; +Cc: Mike Crowe, gcc-patches
On 18/11/20 20:22 +0000, Jonathan Wakely wrote:
>On 18/11/20 00:01 +0000, Jonathan Wakely wrote:
>>On 14/11/20 14:23 +0000, Jonathan Wakely wrote:
>>>On Sat, 14 Nov 2020, 13:30 Mike Crowe wrote:
>>>>>@@ -195,7 +205,7 @@ namespace
>>>>> if (__s.count() < 0) [[unlikely]]
>>>>> return false;
>>>>>
>>>>>- struct timespec rt;
>>>>>+ syscall_timespec rt;
>>>>> if (__s.count() > __int_traits<time_t>::__max) [[unlikely]]
>>>>> rt.tv_sec = __int_traits<time_t>::__max;
>>>>
>>>>Do these now need to be __int_traits<long>::__max in case time_t is 64-bit
>>>>yet syscall_timespec is using 32-bit long?
>>>>
>>>
>>>Ah yes. Maybe decltype(rt.tv_sec).
>>
>>I'll fix that in the next patch.
>
>And here's that next patch. I'm testing this and will commit if all
>goes well.
Committed.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-11-19 13:33 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-14 0:17 [committed] libstdc++: Use custom timespec in system calls [PR 93421] Jonathan Wakely
2020-11-14 13:29 ` Mike Crowe
2020-11-14 14:23 ` Jonathan Wakely
2020-11-18 0:01 ` Jonathan Wakely
2020-11-18 20:22 ` Jonathan Wakely
2020-11-18 20:49 ` Mike Crowe
2020-11-19 13:33 ` Jonathan Wakely
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).