From e05ff30078e80869f2bf3af6dbdbea134c252158 Mon Sep 17 00:00:00 2001 From: Mike Crowe Date: Fri, 11 Sep 2020 14:25:00 +0100 Subject: [PATCH] libstdc++: Avoid rounding errors on custom clocks in condition_variable 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/ChangeLog: * 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. * 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 +++--- .../condition_variable/members/68519.cc | 61 ++++++++++++++++--- 2 files changed, 63 insertions(+), 16 deletions(-) diff --git a/libstdc++-v3/include/std/condition_variable b/libstdc++-v3/include/std/condition_variable index a08cfc62705..1abec546320 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 @@ -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 2a6ecb95fd1..c86ca2ccdbf 100644 --- a/libstdc++-v3/testsuite/30_threads/condition_variable/members/68519.cc +++ b/libstdc++-v3/testsuite/30_threads/condition_variable/members/68519.cc @@ -25,25 +25,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 l(mx); auto start = std::chrono::system_clock::now(); - cv.wait_for(l, std::chrono::duration(1), [] { return val; }); + cv.wait_for(l, std::chrono::duration(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::rep; + using period = std::chrono::duration::period; + using time_point = std::chrono::time_point>; + 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 l(mx); + const auto start = clock::now(); + const auto wait_time = start + std::chrono::duration{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(); } -- 2.30.2