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

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

[Jonathan Wakely]

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

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

[Mike Crowe]

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.

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

[Jonathan Wakely]

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

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

[Jonathan Wakely]

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

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

[Jonathan Wakely]

[-- 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();

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

[Mike Crowe]

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.

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

[Jonathan Wakely]

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.