[v5,7/8] libstdc++ condition_variable: Avoid rounding errors on custom clocks

Message ID 55a17a4c2254cbbcccb036e898aa27ace663a908.1590732962.git-series.mac@mcrowe.com
State New
Headers show
Series
  • std::future::wait_* and std::condition_variable improvements
Related show

Commit Message

Kees Cook via Gcc-patches May 29, 2020, 6:17 a.m.
The fix for PR68519 in 83fd5e73b3c16296e0d7ba54f6c547e01c7eae7b only
applied to condition_variable::wait_for. This problem can also apply to
condition_variable::wait_until but only if the custom clock is using a
more recent epoch so that a small enough delta can be calculated. let's
use the newly-added chrono::__detail::ceil to fix this and also make use
of that function to simplify the previous wait_for fixes.

Also, simplify the existing test case for PR68519 a little and make its
variables local so we can add a new test case for the above
problem. Unfortunately, the test would have only started failing if
sufficient time has passed since the chrono::steady_clock epoch had
passed anyway, but it's better than nothing.

	* libstdc++-v3/include/std/condition_variable:
          (condition_variable::wait_until): Convert delta to
          steady_clock duration before adding to current steady_clock
          time to avoid rounding errors described in
          PR68519. (condition_variable::wait_for): Simplify calculation
          of absolute time by using chrono::__detail::ceil in both
          overloads.  *
          libstdc++-v3/testsuite/30_threads/condition_variable/members/68519.cc:
          (test_wait_for): Renamed from test01. Replace unassigned val
          variable with constant false. Reduce scope of mx and cv
          variables to just test_wait_for function. (test_wait_until):
          Add new test case.
---
 libstdc++-v3/include/std/condition_variable                           | 18 +++++++++---------
 libstdc++-v3/testsuite/30_threads/condition_variable/members/68519.cc | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-------
 2 files changed, 63 insertions(+), 16 deletions(-)

-- 
git-series 0.9.1

Patch

diff --git a/libstdc++-v3/include/std/condition_variable b/libstdc++-v3/include/std/condition_variable
index 2db9dff..0796ca9 100644
--- a/libstdc++-v3/include/std/condition_variable
+++ b/libstdc++-v3/include/std/condition_variable
@@ -133,10 +133,12 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
 #if __cplusplus > 201703L
 	static_assert(chrono::is_clock_v<_Clock>);
 #endif
+	using __s_dur = typename __clock_t::duration;
 	const typename _Clock::time_point __c_entry = _Clock::now();
 	const __clock_t::time_point __s_entry = __clock_t::now();
 	const auto __delta = __atime - __c_entry;
-	const auto __s_atime = __s_entry + __delta;
+	const auto __s_atime = __s_entry +
+	  chrono::__detail::ceil<__s_dur>(__delta);
 
 	if (__wait_until_impl(__lock, __s_atime) == cv_status::no_timeout)
 	  return cv_status::no_timeout;
@@ -166,10 +168,9 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	       const chrono::duration<_Rep, _Period>& __rtime)
       {
 	using __dur = typename steady_clock::duration;
-	auto __reltime = chrono::duration_cast<__dur>(__rtime);
-	if (__reltime < __rtime)
-	  ++__reltime;
-	return wait_until(__lock, steady_clock::now() + __reltime);
+	return wait_until(__lock,
+			  steady_clock::now() +
+			  chrono::__detail::ceil<__dur>(__rtime));
       }
 
     template<typename _Rep, typename _Period, typename _Predicate>
@@ -179,10 +180,9 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	       _Predicate __p)
       {
 	using __dur = typename steady_clock::duration;
-	auto __reltime = chrono::duration_cast<__dur>(__rtime);
-	if (__reltime < __rtime)
-	  ++__reltime;
-	return wait_until(__lock, steady_clock::now() + __reltime,
+	return wait_until(__lock,
+			  steady_clock::now() +
+			  chrono::__detail::ceil<__dur>(__rtime),
 			  std::move(__p));
       }
 
diff --git a/libstdc++-v3/testsuite/30_threads/condition_variable/members/68519.cc b/libstdc++-v3/testsuite/30_threads/condition_variable/members/68519.cc
index 9a70713..739f74c 100644
--- a/libstdc++-v3/testsuite/30_threads/condition_variable/members/68519.cc
+++ b/libstdc++-v3/testsuite/30_threads/condition_variable/members/68519.cc
@@ -26,25 +26,72 @@ 
 
 // PR libstdc++/68519
 
-bool val = false;
-std::mutex mx;
-std::condition_variable cv;
-
 void
-test01()
+test_wait_for()
 {
+  std::mutex mx;
+  std::condition_variable cv;
+
   for (int i = 0; i < 3; ++i)
   {
     std::unique_lock<std::mutex> l(mx);
     auto start = std::chrono::system_clock::now();
-    cv.wait_for(l, std::chrono::duration<float>(1), [] { return val; });
+    cv.wait_for(l, std::chrono::duration<float>(1), [] { return false; });
     auto t = std::chrono::system_clock::now();
     VERIFY( (t - start) >= std::chrono::seconds(1) );
   }
 }
 
+// In order to ensure that the delta calculated in the arbitrary clock overload
+// of condition_variable::wait_until fits accurately in a float, but the result
+// of adding it to steady_clock with a float duration does not, this clock
+// needs to use a more recent epoch.
+struct recent_epoch_float_clock
+{
+  using rep = std::chrono::duration<float>::rep;
+  using period = std::chrono::duration<float>::period;
+  using time_point = std::chrono::time_point<recent_epoch_float_clock,
+    std::chrono::duration<float>>;
+  static constexpr bool is_steady = true;
+
+  static const std::chrono::steady_clock::time_point epoch;
+
+  static time_point now()
+  {
+    const auto steady = std::chrono::steady_clock::now();
+    return time_point{steady - epoch};
+  }
+};
+
+const std::chrono::steady_clock::time_point recent_epoch_float_clock::epoch =
+  std::chrono::steady_clock::now();
+
+void
+test_wait_until()
+{
+  using clock = recent_epoch_float_clock;
+
+  std::mutex mx;
+  std::condition_variable cv;
+
+  for (int i = 0; i < 3; ++i)
+  {
+    std::unique_lock<std::mutex> l(mx);
+    const auto start = clock::now();
+    const auto wait_time = start + std::chrono::duration<float>{1.0};
+
+    // In theory we could get a spurious wakeup, but in practice we won't.
+    const auto result = cv.wait_until(l, wait_time);
+
+    VERIFY( result == std::cv_status::timeout );
+    const auto elapsed = clock::now() - start;
+    VERIFY( elapsed >= std::chrono::seconds(1) );
+  }
+}
+
 int
 main()
 {
-  test01();
+  test_wait_for();
+  test_wait_until();
 }