[PATCH 1/2] condition_variable: Report early wakeup of wait_until as no_timeout

[PATCH 1/2] condition_variable: Report early wakeup of wait_until as no_timeout

[Mike Crowe]

As currently implemented, condition_variable always ultimately waits
against std::chrono::system_clock. This clock can be changed in arbitrary
ways by the user which may result in us waking up too early or too late
when measured against the caller-supplied clock.

We can't (yet) do much about waking up too late[1], but
if we wake up too early we must return cv_status::no_timeout to indicate a
spurious wakeup rather than incorrectly returning cv_status::timeout.

[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=41861
---
 libstdc++-v3/ChangeLog                      | 5 +++++
 libstdc++-v3/include/std/condition_variable | 8 +++++++-
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/libstdc++-v3/ChangeLog b/libstdc++-v3/ChangeLog
index cceef0271ae..ea7875ace9f 100644
--- a/libstdc++-v3/ChangeLog
+++ b/libstdc++-v3/ChangeLog
@@ -1,3 +1,8 @@
+2018-07-09  Mike Crowe <mac@mcrowe.com>
+       * include/std/condition_variable (wait_until): Only report timeout
+       if we really have timed out when measured against the
+       caller-supplied clock.
+
 2018-07-06  François Dumont  <fdumont@gcc.gnu.org>

        * include/debug/functions.h (__gnu_debug::__check_string): Move...
diff --git a/libstdc++-v3/include/std/condition_variable b/libstdc++-v3/include/std/condition_variable
index 84863a162d6..a2d146a9b09 100644
--- a/libstdc++-v3/include/std/condition_variable
+++ b/libstdc++-v3/include/std/condition_variable
@@ -116,7 +116,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
        const auto __delta = __atime - __c_entry;
        const auto __s_atime = __s_entry + __delta;

-       return __wait_until_impl(__lock, __s_atime);
+       // We might get a timeout when measured against __clock_t but
+       // we need to check against the caller-supplied clock to tell
+       // whether we should return a timeout.
+       if (__wait_until_impl(__lock, __s_atime) == cv_status::timeout)
+         return _Clock::now() < __atime ? cv_status::no_timeout : cv_status::timeout;
+       else
+         return cv_status::no_timeout;
       }

     template<typename _Clock, typename _Duration, typename _Predicate>
--
2.11.0

BrightSign considers your privacy to be very important. The emails you send to us will be protected and secured. Furthermore, we will only use your email and contact information for the reasons you sent them to us and for tracking how effectively we respond to your requests.

[PATCH 2/2] condition_variable: Use steady_clock to implement wait_for

[Mike Crowe]

I believe[1][2] that the C++ standard says that
std::condition_variable::wait_for should be implemented to be equivalent
to:

 return wait_until(lock, chrono::steady_clock::now() + rel_time);

But the existing implementation uses chrono::system_clock. Now that
wait_until has potentially-different behaviour for chrono::steady_clock,
let's at least try to wait using the correct clock.

[1] https://en.cppreference.com/w/cpp/thread/condition_variable/wait_for
[2] https://github.com/cplusplus/draft/blob/master/source/threads.tex
---
 libstdc++-v3/ChangeLog                      | 3 +++
 libstdc++-v3/include/std/condition_variable | 5 +++--
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/libstdc++-v3/ChangeLog b/libstdc++-v3/ChangeLog
index ea7875ace9f..4500273ace7 100644
--- a/libstdc++-v3/ChangeLog
+++ b/libstdc++-v3/ChangeLog
@@ -1,4 +1,7 @@
 2018-07-09  Mike Crowe <mac@mcrowe.com>
+       * include/std/condition_variable (wait_for): Use steady_clock.
+
+2018-07-09  Mike Crowe <mac@mcrowe.com>
        * include/std/condition_variable (wait_until): Only report timeout
        if we really have timed out when measured against the
        caller-supplied clock.
diff --git a/libstdc++-v3/include/std/condition_variable b/libstdc++-v3/include/std/condition_variable
index a2d146a9b09..ce583990b9d 100644
--- a/libstdc++-v3/include/std/condition_variable
+++ b/libstdc++-v3/include/std/condition_variable
@@ -65,6 +65,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
   class condition_variable
   {
     typedef chrono::system_clock       __clock_t;
+    typedef chrono::steady_clock       __steady_clock_t;
     typedef __gthread_cond_t           __native_type;

 #ifdef __GTHREAD_COND_INIT
@@ -142,11 +143,11 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       wait_for(unique_lock<mutex>& __lock,
               const chrono::duration<_Rep, _Period>& __rtime)
       {
-       using __dur = typename __clock_t::duration;
+       using __dur = typename __steady_clock_t::duration;
        auto __reltime = chrono::duration_cast<__dur>(__rtime);
        if (__reltime < __rtime)
          ++__reltime;
-       return wait_until(__lock, __clock_t::now() + __reltime);
+       return wait_until(__lock, __steady_clock_t::now() + __reltime);
       }

     template<typename _Rep, typename _Period, typename _Predicate>
--
2.11.0

BrightSign considers your privacy to be very important. The emails you send to us will be protected and secured. Furthermore, we will only use your email and contact information for the reasons you sent them to us and for tracking how effectively we respond to your requests.

Re: [PATCH 1/2] condition_variable: Report early wakeup of wait_until as no_timeout

[Jonathan Wakely]

On 10/07/18 11:09 +0100, Mike Crowe wrote:
>As currently implemented, condition_variable always ultimately waits
>against std::chrono::system_clock. This clock can be changed in arbitrary
>ways by the user which may result in us waking up too early or too late
>when measured against the caller-supplied clock.
>
>We can't (yet) do much about waking up too late[1], but
>if we wake up too early we must return cv_status::no_timeout to indicate a
>spurious wakeup rather than incorrectly returning cv_status::timeout.

The patch looks good, thanks.

Can we come up with a test for this, using a user-defined clock that
jumps back?


>[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=41861
>---
> libstdc++-v3/ChangeLog                      | 5 +++++
> libstdc++-v3/include/std/condition_variable | 8 +++++++-
> 2 files changed, 12 insertions(+), 1 deletion(-)
>
>diff --git a/libstdc++-v3/ChangeLog b/libstdc++-v3/ChangeLog
>index cceef0271ae..ea7875ace9f 100644
>--- a/libstdc++-v3/ChangeLog
>+++ b/libstdc++-v3/ChangeLog
>@@ -1,3 +1,8 @@
>+2018-07-09  Mike Crowe <mac@mcrowe.com>
>+       * include/std/condition_variable (wait_until): Only report timeout
>+       if we really have timed out when measured against the
>+       caller-supplied clock.
>+
> 2018-07-06  François Dumont  <fdumont@gcc.gnu.org>
>
>        * include/debug/functions.h (__gnu_debug::__check_string): Move...
>diff --git a/libstdc++-v3/include/std/condition_variable b/libstdc++-v3/include/std/condition_variable
>index 84863a162d6..a2d146a9b09 100644
>--- a/libstdc++-v3/include/std/condition_variable
>+++ b/libstdc++-v3/include/std/condition_variable
>@@ -116,7 +116,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>        const auto __delta = __atime - __c_entry;
>        const auto __s_atime = __s_entry + __delta;
>
>-       return __wait_until_impl(__lock, __s_atime);
>+       // We might get a timeout when measured against __clock_t but
>+       // we need to check against the caller-supplied clock to tell
>+       // whether we should return a timeout.
>+       if (__wait_until_impl(__lock, __s_atime) == cv_status::timeout)
>+         return _Clock::now() < __atime ? cv_status::no_timeout : cv_status::timeout;
>+       else
>+         return cv_status::no_timeout;
>       }
>
>     template<typename _Clock, typename _Duration, typename _Predicate>
>--
>2.11.0
>
>BrightSign considers your privacy to be very important. The emails you send to us will be protected and secured. Furthermore, we will only use your email and contact information for the reasons you sent them to us and for tracking how effectively we respond to your requests.

Re: [PATCH 1/2] condition_variable: Report early wakeup of wait_until as no_timeout

[Mike Crowe]

On Friday 20 July 2018 at 11:53:38 +0100, Jonathan Wakely wrote:
> On 10/07/18 11:09 +0100, Mike Crowe wrote:
> > As currently implemented, condition_variable always ultimately waits
> > against std::chrono::system_clock. This clock can be changed in arbitrary
> > ways by the user which may result in us waking up too early or too late
> > when measured against the caller-supplied clock.
> > 
> > We can't (yet) do much about waking up too late[1], but
> > if we wake up too early we must return cv_status::no_timeout to indicate a
> > spurious wakeup rather than incorrectly returning cv_status::timeout.
> 
> The patch looks good, thanks.
> 
> Can we come up with a test for this, using a user-defined clock that
> jumps back?

That's a good idea. I'll do that.

Thanks.

Mike.

Re: [PATCH 1/2] condition_variable: Report early wakeup of wait_until as no_timeout

[Jonathan Wakely]

On 20/07/18 12:30 +0100, Mike Crowe wrote:
>On Friday 20 July 2018 at 11:53:38 +0100, Jonathan Wakely wrote:
>> On 10/07/18 11:09 +0100, Mike Crowe wrote:
>> > As currently implemented, condition_variable always ultimately waits
>> > against std::chrono::system_clock. This clock can be changed in arbitrary
>> > ways by the user which may result in us waking up too early or too late
>> > when measured against the caller-supplied clock.
>> >
>> > We can't (yet) do much about waking up too late[1], but
>> > if we wake up too early we must return cv_status::no_timeout to indicate a
>> > spurious wakeup rather than incorrectly returning cv_status::timeout.
>>
>> The patch looks good, thanks.
>>
>> Can we come up with a test for this, using a user-defined clock that
>> jumps back?
>
>That's a good idea. I'll do that.

Thanks.

There are "broken" clocks in testsuite/30_threads/this_thread/60421.cc
and testsuite/30_threads/timed_mutex/try_lock_until/57641.cc but I'm
not sure if either of them is reusable. A custom one for this test
should be reasonably easy.

Re: [PATCH 1/2] condition_variable: Report early wakeup of wait_until as no_timeout

[Jonathan Wakely]

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

On 20/07/18 17:49 +0100, Mike Crowe wrote:
>As currently implemented, condition_variable always ultimately waits
>against std::chrono::system_clock. This clock can be changed in arbitrary
>ways by the user which may result in us waking up too early or too late
>when measured against the caller-supplied clock.
>
>We can't (yet) do much about waking up too late[1], but
>if we wake up too early we must return cv_status::no_timeout to indicate a
>spurious wakeup rather than incorrectly returning cv_status::timeout.
>
>[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=41861

Committed to trunk, thanks very much.

I reformatted it slightly, to keep the line below 80 columns. The
version I committed is attached.


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

commit 79a8b4c1d70287ac0e668c4f2335d70d97c3002e
Author: redi <redi@138bc75d-0d04-0410-961f-82ee72b054a4>
Date:   Wed Aug 1 15:39:45 2018 +0000

    Report early wakeup of condition_variable::wait_until as no_timeout
    
    As currently implemented, condition_variable always ultimately waits
    against std::chrono::system_clock. This clock can be changed in arbitrary
    ways by the user which may result in us waking up too early or too late
    when measured against the caller-supplied clock.
    
    We can't (yet) do much about waking up too late (PR 41861), but
    if we wake up too early we must return cv_status::no_timeout to indicate a
    spurious wakeup rather than incorrectly returning cv_status::timeout.
    
    2018-08-01  Mike Crowe  <mac@mcrowe.com>
    
            * include/std/condition_variable (wait_until): Only report timeout
            if we really have timed out when measured against the
            caller-supplied clock.
            * testsuite/30_threads/condition_variable/members/2.cc: Add test
            case to confirm above behaviour.
    
    git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@263224 138bc75d-0d04-0410-961f-82ee72b054a4

diff --git a/libstdc++-v3/include/std/condition_variable b/libstdc++-v3/include/std/condition_variable
index 3f690c81799..c00afa2b7ae 100644
--- a/libstdc++-v3/include/std/condition_variable
+++ b/libstdc++-v3/include/std/condition_variable
@@ -117,7 +117,14 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	const auto __delta = __atime - __c_entry;
 	const auto __s_atime = __s_entry + __delta;
 
-	return __wait_until_impl(__lock, __s_atime);
+	if (__wait_until_impl(__lock, __s_atime) == cv_status::no_timeout)
+	  return cv_status::no_timeout;
+	// We got a timeout when measured against __clock_t but
+	// we need to check against the caller-supplied clock
+	// to tell whether we should return a timeout.
+	if (_Clock::now() < __atime)
+	  return cv_status::no_timeout;
+	return cv_status::timeout;
       }
 
     template<typename _Clock, typename _Duration, typename _Predicate>
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 0a44c4fa7cf..09a717801e1 100644
--- a/libstdc++-v3/testsuite/30_threads/condition_variable/members/2.cc
+++ b/libstdc++-v3/testsuite/30_threads/condition_variable/members/2.cc
@@ -51,8 +51,60 @@ void test01()
     }
 }
 
+struct slow_clock
+{
+  using rep = std::chrono::system_clock::rep;
+  using period = std::chrono::system_clock::period;
+  using duration = std::chrono::system_clock::duration;
+  using time_point = std::chrono::time_point<slow_clock, duration>;
+  static constexpr bool is_steady = false;
+
+  static time_point now()
+  {
+    auto real = std::chrono::system_clock::now();
+    return time_point{real.time_since_epoch() / 3};
+  }
+};
+
+
+void test01_alternate_clock()
+{
+  try
+    {
+      std::condition_variable c1;
+      std::mutex m;
+      std::unique_lock<std::mutex> l(m);
+      auto const expire = slow_clock::now() + std::chrono::seconds(1);
+
+      while (slow_clock::now() < expire)
+       {
+         auto const result = c1.wait_until(l, expire);
+
+         // If wait_until returns before the timeout has expired when
+         // measured against the supplied clock, then wait_until must
+         // return no_timeout.
+         if (slow_clock::now() < expire)
+           VERIFY(result == std::cv_status::no_timeout);
+
+         // If wait_until returns timeout then the timeout must have
+         // expired.
+         if (result == std::cv_status::timeout)
+           VERIFY(slow_clock::now() >= expire);
+       }
+    }
+  catch (const std::system_error& e)
+    {
+      VERIFY( false );
+    }
+  catch (...)
+    {
+      VERIFY( false );
+    }
+}
+
 int main()
 {
   test01();
+  test01_alternate_clock();
   return 0;
 }

[PATCH 1/2] condition_variable: Report early wakeup of wait_until as no_timeout

[Mike Crowe]

As currently implemented, condition_variable always ultimately waits
against std::chrono::system_clock. This clock can be changed in arbitrary
ways by the user which may result in us waking up too early or too late
when measured against the caller-supplied clock.

We can't (yet) do much about waking up too late[1], but
if we wake up too early we must return cv_status::no_timeout to indicate a
spurious wakeup rather than incorrectly returning cv_status::timeout.

[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=41861
---
 libstdc++-v3/ChangeLog                                            |  7 +++++++
 libstdc++-v3/include/std/condition_variable                       |  8 +++++++-
 libstdc++-v3/testsuite/30_threads/condition_variable/members/2.cc | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 66 insertions(+), 1 deletion(-)

diff --git a/libstdc++-v3/ChangeLog b/libstdc++-v3/ChangeLog
index c9cd62a..4657af7 100644
--- a/libstdc++-v3/ChangeLog
+++ b/libstdc++-v3/ChangeLog
@@ -1,3 +1,10 @@
+2018-07-20  Mike Crowe <mac@mcrowe.com>
+       * include/std/condition_variable (wait_until): Only report timeout
+       if we really have timed out when measured against the
+       caller-supplied clock.
+       * testsuite/30_threads/condition_variable/members/2.cc: Add test
+       case to confirm above behaviour.
+
 2018-07-20  Jonathan Wakely  <jwakely@redhat.com>

        PR libstdc++/86595
diff --git a/libstdc++-v3/include/std/condition_variable b/libstdc++-v3/include/std/condition_variable
index 84863a1..a2d146a 100644
--- a/libstdc++-v3/include/std/condition_variable
+++ b/libstdc++-v3/include/std/condition_variable
@@ -116,7 +116,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
        const auto __delta = __atime - __c_entry;
        const auto __s_atime = __s_entry + __delta;

-       return __wait_until_impl(__lock, __s_atime);
+       // We might get a timeout when measured against __clock_t but
+       // we need to check against the caller-supplied clock to tell
+       // whether we should return a timeout.
+       if (__wait_until_impl(__lock, __s_atime) == cv_status::timeout)
+         return _Clock::now() < __atime ? cv_status::no_timeout : cv_status::timeout;
+       else
+         return cv_status::no_timeout;
       }

     template<typename _Clock, typename _Duration, typename _Predicate>
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 6f9724b..16850a4 100644
--- a/libstdc++-v3/testsuite/30_threads/condition_variable/members/2.cc
+++ b/libstdc++-v3/testsuite/30_threads/condition_variable/members/2.cc
@@ -52,8 +52,60 @@ void test01()
     }
 }

+struct slow_clock
+{
+  using rep = std::chrono::system_clock::rep;
+  using period = std::chrono::system_clock::period;
+  using duration = std::chrono::system_clock::duration;
+  using time_point = std::chrono::time_point<slow_clock, duration>;
+  static constexpr bool is_steady = false;
+
+  static time_point now()
+  {
+    auto real = std::chrono::system_clock::now();
+    return time_point{real.time_since_epoch() / 3};
+  }
+};
+
+
+void test01_alternate_clock()
+{
+  try
+    {
+      std::condition_variable c1;
+      std::mutex m;
+      std::unique_lock<std::mutex> l(m);
+      auto const expire = slow_clock::now() + std::chrono::seconds(1);
+
+      while (slow_clock::now() < expire)
+       {
+         auto const result = c1.wait_until(l, expire);
+
+         // If wait_until returns before the timeout has expired when
+         // measured against the supplied clock, then wait_until must
+         // return no_timeout.
+         if (slow_clock::now() < expire)
+           VERIFY(result == std::cv_status::no_timeout);
+
+         // If wait_until returns timeout then the timeout must have
+         // expired.
+         if (result == std::cv_status::timeout)
+           VERIFY(slow_clock::now() >= expire);
+       }
+    }
+  catch (const std::system_error& e)
+    {
+      VERIFY( false );
+    }
+  catch (...)
+    {
+      VERIFY( false );
+    }
+}
+
 int main()
 {
   test01();
+  test01_alternate_clock();
   return 0;
 }

base-commit: 3052e4ec519e4f5456ab63f4954ae098524316ce
--
git-series 0.9.1
BrightSign considers your privacy to be very important. The emails you send to us will be protected and secured. Furthermore, we will only use your email and contact information for the reasons you sent them to us and for tracking how effectively we respond to your requests.