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 <jwak...@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.
