Re: [PATCH] libstdc++: Fix semaphore to work with system_clock timeouts

2021-04-23 Thread Richard Biener via Gcc-patches
On Thu, Apr 22, 2021 at 8:48 PM Jonathan Wakely via Gcc-patches
 wrote:
>
> The __cond_wait_until_impl function takes a steady_clock timeout, but
> then sometimes tries to compare it to a time from the system_clock,
> which won't compile.  Additionally, that function gets called with
> system_clock timeouts, which also won't compile. This makes the function
> accept timeouts for either clock, and compare to the time from the right
> clock.
>
> This fixes the compilation error that was causing two tests to fail on
> non-futex targets, so we can revert the r12-11 change to disable them.
>
> libstdc++-v3/ChangeLog:
>
> * include/bits/atomic_timed_wait.h (__cond_wait_until_impl):
> Handle system_clock as well as steady_clock.
> * testsuite/30_threads/semaphore/try_acquire_for.cc: Re-enable.
> * testsuite/30_threads/semaphore/try_acquire_until.cc:
> Re-enable.
>
> I'm testing this now on x86_64-linux, powerpc64le-linux, sparc-linux,
> power-aix and sparc-solaris. It looks good so far, so I'll push to
> trunk when the tests finish.
>
> This should also go to the gcc-11 branch, or the timed waits for
> semaphores can't be used with system_clock times on non-futed targets.

Fine with me.

>
>


[PATCH] libstdc++: Fix semaphore to work with system_clock timeouts

2021-04-22 Thread Jonathan Wakely via Gcc-patches
The __cond_wait_until_impl function takes a steady_clock timeout, but
then sometimes tries to compare it to a time from the system_clock,
which won't compile.  Additionally, that function gets called with
system_clock timeouts, which also won't compile. This makes the function
accept timeouts for either clock, and compare to the time from the right
clock.

This fixes the compilation error that was causing two tests to fail on
non-futex targets, so we can revert the r12-11 change to disable them.

libstdc++-v3/ChangeLog:

* include/bits/atomic_timed_wait.h (__cond_wait_until_impl):
Handle system_clock as well as steady_clock.
* testsuite/30_threads/semaphore/try_acquire_for.cc: Re-enable.
* testsuite/30_threads/semaphore/try_acquire_until.cc:
Re-enable.

I'm testing this now on x86_64-linux, powerpc64le-linux, sparc-linux,
power-aix and sparc-solaris. It looks good so far, so I'll push to
trunk when the tests finish.

This should also go to the gcc-11 branch, or the timed waits for
semaphores can't be used with system_clock times on non-futed targets.



commit 6924588774a02dec63fb4b0ad19aed303accc2d2
Author: Jonathan Wakely 
Date:   Thu Apr 22 17:26:50 2021

libstdc++: Fix semaphore to work with system_clock timeouts

The __cond_wait_until_impl function takes a steady_clock timeout, but
then sometimes tries to compare it to a time from the system_clock,
which won't compile.  Additionally, that function gets called with
system_clock timeouts, which also won't compile. This makes the function
accept timeouts for either clock, and compare to the time from the right
clock.

This fixes the compilation error that was causing two tests to fail on
non-futex targets, so we can revert the r12-11 change to disable them.

libstdc++-v3/ChangeLog:

* include/bits/atomic_timed_wait.h (__cond_wait_until_impl):
Handle system_clock as well as steady_clock.
* testsuite/30_threads/semaphore/try_acquire_for.cc: Re-enable.
* testsuite/30_threads/semaphore/try_acquire_until.cc:
Re-enable.

diff --git a/libstdc++-v3/include/bits/atomic_timed_wait.h 
b/libstdc++-v3/include/bits/atomic_timed_wait.h
index 70e5335cfd7..ec7ff51cdbc 100644
--- a/libstdc++-v3/include/bits/atomic_timed_wait.h
+++ b/libstdc++-v3/include/bits/atomic_timed_wait.h
@@ -139,12 +139,16 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 // (e.g. __ulock_wait())which is better than pthread_cond_clockwait
 #endif // ! PLATFORM_TIMED_WAIT
 
-// returns true if wait ended before timeout
-template
+// Returns true if wait ended before timeout.
+// _Clock must be either steady_clock or system_clock.
+template
   bool
   __cond_wait_until_impl(__condvar& __cv, mutex& __mx,
- const chrono::time_point& __atime)
+const chrono::time_point<_Clock, _Dur>& __atime)
   {
+   static_assert(std::__is_one_of<_Clock, chrono::steady_clock,
+  chrono::system_clock>::value);
+
auto __s = chrono::time_point_cast(__atime);
auto __ns = chrono::duration_cast(__atime - __s);
 
@@ -155,12 +159,12 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
  };
 
 #ifdef _GLIBCXX_USE_PTHREAD_COND_CLOCKWAIT
-   __cv.wait_until(__mx, CLOCK_MONOTONIC, __ts);
-   return chrono::steady_clock::now() < __atime;
-#else
-   __cv.wait_until(__mx, __ts);
-   return chrono::system_clock::now() < __atime;
-#endif // ! _GLIBCXX_USE_PTHREAD_COND_CLOCKWAIT
+   if constexpr (is_same_v)
+ __cv.wait_until(__mx, CLOCK_MONOTONIC, __ts);
+   else
+#endif
+ __cv.wait_until(__mx, __ts);
+   return _Clock::now() < __atime;
   }
 
 // returns true if wait ended before timeout
diff --git a/libstdc++-v3/testsuite/30_threads/semaphore/try_acquire_for.cc 
b/libstdc++-v3/testsuite/30_threads/semaphore/try_acquire_for.cc
index 248ecb07e56..e7edc9eeef1 100644
--- a/libstdc++-v3/testsuite/30_threads/semaphore/try_acquire_for.cc
+++ b/libstdc++-v3/testsuite/30_threads/semaphore/try_acquire_for.cc
@@ -21,8 +21,6 @@
 // { dg-require-gthreads "" }
 // { dg-add-options libatomic }
 
-// { dg-skip-if "FIXME: fails" { ! futex } }
-
 #include 
 #include 
 #include 
diff --git a/libstdc++-v3/testsuite/30_threads/semaphore/try_acquire_until.cc 
b/libstdc++-v3/testsuite/30_threads/semaphore/try_acquire_until.cc
index eb1351cd2bf..49ba33b4999 100644
--- a/libstdc++-v3/testsuite/30_threads/semaphore/try_acquire_until.cc
+++ b/libstdc++-v3/testsuite/30_threads/semaphore/try_acquire_until.cc
@@ -21,8 +21,6 @@
 // { dg-additional-options "-pthread" { target pthread } }
 // { dg-add-options libatomic }
 
-// { dg-skip-if "FIXME: fails" { ! futex } }
-
 #include 
 #include 
 #include