From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from avasout07.plus.net (avasout07.plus.net [84.93.230.235]) by sourceware.org (Postfix) with ESMTPS id 78D45388C02E for ; Fri, 29 May 2020 06:18:24 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 78D45388C02E Received: from deneb ([80.229.24.9]) by smtp with ESMTP id eYLTj2a0w0wwMeYLUj60DY; Fri, 29 May 2020 07:18:24 +0100 X-Clacks-Overhead: "GNU Terry Pratchett" X-CM-Score: 0.00 X-CNFS-Analysis: v=2.3 cv=b/4pHuOx c=1 sm=1 tr=0 a=E/9URZZQ5L3bK/voZ0g0HQ==:117 a=E/9URZZQ5L3bK/voZ0g0HQ==:17 a=sTwFKg_x9MkA:10 a=6I8-TjbVYSU-v1zY1DsA:9 Received: from mac by deneb with local (Exim 4.92) (envelope-from ) id 1jeYL4-00065l-Iv; Fri, 29 May 2020 07:17:54 +0100 From: Mike Crowe To: libstdc++@gcc.gnu.org, gcc-patches@gcc.gnu.org Subject: [PATCH v5 7/8] libstdc++ condition_variable: Avoid rounding errors on custom clocks Date: Fri, 29 May 2020 07:17:33 +0100 Message-Id: <55a17a4c2254cbbcccb036e898aa27ace663a908.1590732962.git-series.mac@mcrowe.com> X-Mailer: git-send-email 2.26.2 In-Reply-To: References: MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-CMAE-Envelope: MS4wfEbWl1NxTwgD1IVbYgxoArUFWfF/4aeNyuPiZCl5+jNRH4ruJfZaJxTT9Z/PgB77SzocrYV/CB1BSc+LFYVDKrBI3P90hyVnctQmturiq0PN701YutoT x0KMjqe60GLMcBZvOPKy6LHv5X1PHpcd4EI= X-Spam-Status: No, score=-12.8 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: libstdc++@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libstdc++ mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 29 May 2020 06:18:26 -0000 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(-) 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 @@ -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 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(); } -- git-series 0.9.1