Re: [PATCH 2/2] PR libstdc++/41861 Add full steady_clock support to condition_variable

[Jonathan Wakely <jwakely@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3053 bytes --]

On 04/09/19 18:21 +0100, Mike Crowe wrote:
>On Wednesday 04 September 2019 at 17:57:45 +0100, Mike Crowe wrote:
>> On Wednesday 04 September 2019 at 17:14:30 +0100, Jonathan Wakely wrote:
>> > On 04/09/19 15:49 +0100, Mike Crowe wrote:
>> > > On Wednesday 04 September 2019 at 14:39:35 +0100, Jonathan Wakely wrote:
>> > > > I noticed that the new tests you added in [PATCH 1/2] pass on current
>> > > > trunk, even without [PATCH 2/2], is that expected?
>> > >
>> > > Unfortunately, yes. CLOCK_MONOTONIC and CLOCK_REALTIME will tick at the
>> > > same rate, so unless someone warps CLOCK_REALTIME during the test we can't
>> > > tell the difference between the old implementation of translating
>> > > steady_clock to system_clock and the new implementation that uses
>> > > steady_clock directly. :( I added the tests in the hope of finding other
>> > > mistakes in the new implementation rather than to reproduce the original
>> > > problem.
>> >
>> > OK, that was what I figured was the case. Thanks for confirming.
>> >
>> > > Maybe I should add comments to the test to make that clear along the lines
>> > > of those found in testsuite/27_io/objects/char/3_xin.cc ?
>> >
>> > More comments are usually useful, otherwise I'll just ask the same
>> > question again and again :-)
>>
>> How about something like:
>>
>> --8<--
>> It's not possible for this test to automatically ensure that the
>> system_clock test cases result in a wait on CLOCK_REALTIME and steady_clock
>> test cases result in a wait on CLOCK_MONOTONIC. It's recommended to run the
>> test under strace(1) and check whether the expected futex calls are made by
>> glibc.
>> -->8--
>>
>> Unfortunately I'm unable to determine how I actually managed to run the
>> test under strace. Perhaps I just compiled a similar test myself rather
>> than using dejagnu. :(
>
>Ah, I did it. Here's a new comment:
>
>--8<--
>It's not possible for this test to automatically ensure that the
>system_clock test cases result in a wait on CLOCK_REALTIME and steady_clock
>test cases result in a wait on CLOCK_MONOTONIC. It's recommended to run the
>test under strace(1) and check whether the expected futex calls are made by
>glibc. The easiest way to do this is to copy and paste the line used to
>build the test from the output of:
>
> make -j8 check-target-libstdc++-v3 RUNTESTFLAGS="conformance.exp=30_threads/condition_variable/members/* -v -v"
>
>to compile the test with only the tests for one clock enabled and then run it as:
>
> strace ./2.exe
>
>You should see calls to:
>
> futex(..., FUTEX_WAIT_BITSET_PRIVATE|FUTEX_CLOCK_REALTIME, ...)
>
>with large values of tv_sec when using system_clock and calls to:
>
> futex(..., FUTEX_WAIT_BITSET_PRIVATE, ...)
>
>with values of tv_sec based on the system uptime when using steady_clock.
>-->8--

Great. I've committed the patch I sent earlier (with the renamed
typedefs) and the attached one for the tests (using an abridged form
of the comment above, linking to the copy of your mail in the list
archives for the full instructions).

Thanks!


[-- Attachment #2: patch.txt --]
[-- Type: text/x-patch, Size: 4733 bytes --]

commit 8f84bffbd3c4173d684fc825f0847d857a71f54b
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Wed Sep 4 12:41:34 2019 +0100

    Add user-defined clock to libstdc++ condition_variable tests
    
    2019-09-04  Mike Crowe  <mac@mcrowe.com>
    
            * testsuite/30_threads/condition_variable/members/2.cc (test01):
            Parameterise so that test can be run against an arbitrary clock.
            (main): Test using std::chrono::steady_clock and a user-defined
            clock in addition to the previous std::chrono::system_clock.
            * testsuite/30_threads/condition_variable_any/members/2.cc: Likewise.

diff --git a/libstdc++-v3/testsuite/30_threads/condition_variable/members/2.cc b/libstdc++-v3/testsuite/30_threads/condition_variable/members/2.cc
index 0ecb09d80ed..cbac3fa1932 100644
--- a/libstdc++-v3/testsuite/30_threads/condition_variable/members/2.cc
+++ b/libstdc++-v3/testsuite/30_threads/condition_variable/members/2.cc
@@ -26,6 +26,7 @@
 #include <system_error>
 #include <testsuite_hooks.h>
 
+template <typename ClockType>
 void test01()
 {
   try 
@@ -35,10 +36,10 @@ void test01()
       std::mutex m;
       std::unique_lock<std::mutex> l(m);
 
-      auto then = std::chrono::steady_clock::now();
+      auto then = ClockType::now();
       std::cv_status result = c1.wait_until(l, then + ms);
       VERIFY( result == std::cv_status::timeout );
-      VERIFY( (std::chrono::steady_clock::now() - then) >= ms );
+      VERIFY( (ClockType::now() - then) >= ms );
       VERIFY( l.owns_lock() );
     }
   catch (const std::system_error& e)
@@ -102,9 +103,39 @@ void test01_alternate_clock()
     }
 }
 
+/* User defined clock that ticks in two-thousandths of a second
+   forty-two minutes ahead of steady_clock. */
+struct user_defined_clock
+{
+  typedef std::chrono::steady_clock::rep rep;
+  typedef std::ratio<1, 2000> period;
+  typedef std::chrono::duration<rep, period> duration;
+  typedef std::chrono::time_point<user_defined_clock> time_point;
+
+  static constexpr bool is_steady = true;
+
+  static time_point now() noexcept
+  {
+    using namespace std::chrono;
+    const auto steady_since_epoch = steady_clock::now().time_since_epoch();
+    const auto user_since_epoch = duration_cast<duration>(steady_since_epoch);
+    return time_point(user_since_epoch + minutes(42));
+  }
+};
+
+/*
+It's not possible for this test to automatically ensure that the
+system_clock test cases result in a wait on CLOCK_REALTIME and steady_clock
+test cases result in a wait on CLOCK_MONOTONIC. It's recommended to run the
+test under strace(1) and check whether the expected futex calls are made by
+glibc. See https://gcc.gnu.org/ml/libstdc++/2019-09/msg00022.html for
+instructions.
+*/
+
 int main()
 {
-  test01();
+  test01<std::chrono::steady_clock>();
+  test01<std::chrono::system_clock>();
+  test01<user_defined_clock>();
   test01_alternate_clock();
-  return 0;
 }
diff --git a/libstdc++-v3/testsuite/30_threads/condition_variable_any/members/2.cc b/libstdc++-v3/testsuite/30_threads/condition_variable_any/members/2.cc
index e6fbc44f6f9..897fa86f514 100644
--- a/libstdc++-v3/testsuite/30_threads/condition_variable_any/members/2.cc
+++ b/libstdc++-v3/testsuite/30_threads/condition_variable_any/members/2.cc
@@ -51,6 +51,7 @@ struct Mutex
 };
 
 
+template <typename ClockType>
 void test01()
 {
   try 
@@ -60,10 +61,10 @@ void test01()
       Mutex m;
       m.lock();
 
-      auto then = std::chrono::steady_clock::now();
+      auto then = ClockType::now();
       std::cv_status result = c1.wait_until(m, then + ms);
       VERIFY( result == std::cv_status::timeout );
-      VERIFY( (std::chrono::steady_clock::now() - then) >= ms );
+      VERIFY( (ClockType::now() - then) >= ms );
       VERIFY( m.locked );
     }
   catch (const std::system_error& e)
@@ -76,8 +77,29 @@ void test01()
     }
 }
 
+/* User defined clock that ticks in two-thousandths of a second
+   forty-two minutes ahead of steady_clock. */
+struct user_defined_clock
+{
+  typedef std::chrono::steady_clock::rep rep;
+  typedef std::ratio<1, 2000> period;
+  typedef std::chrono::duration<rep, period> duration;
+  typedef std::chrono::time_point<user_defined_clock> time_point;
+
+  static constexpr bool is_steady = true;
+
+  static time_point now() noexcept
+  {
+    using namespace std::chrono;
+    const auto steady_since_epoch = steady_clock::now().time_since_epoch();
+    const auto user_since_epoch = duration_cast<duration>(steady_since_epoch);
+    return time_point(user_since_epoch + minutes(42));
+  }
+};
+
 int main()
 {
-  test01();
-  return 0;
+  test01<std::chrono::steady_clock>();
+  test01<std::chrono::system_clock>();
+  test01<user_defined_clock>();
 }