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

[Jonathan Wakely <jwakely@redhat.com>]

[-- 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