Re: [committed] libstdc++: Use custom timespec in system calls [PR 93421]

[Jonathan Wakely <jwakely.gcc@gmail.com>]

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).