On Sat, 14 Nov 2020, 13:30 Mike Crowe via Libstdc++, <libstd...@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 <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.
>
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).
