[PATCH v5 6/8] libstdc++ atomic_futex: Avoid rounding errors in std::future::wait_* [PR91486]

[Jonathan Wakely]

On 29/05/20 07:17 +0100, Mike Crowe via Libstdc++ wrote:
>Convert the specified duration to the target clock's duration type
>before adding it to the current time in
>__atomic_futex_unsigned::_M_load_when_equal_for and
>_M_load_when_equal_until.  This removes the risk of the timeout being
>rounded down to the current time resulting in there being no wait at
>all when the duration type lacks sufficient precision to hold the
>steady_clock current time.
>
>Rather than using the style of fix from PR68519, let's expose the C++17
>std::chrono::ceil function as std::chrono::__detail::ceil so that it can
>be used in code compiled with earlier standards versions and simplify
>the fix. This was suggested by John Salmon in
>https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91486#c5 .
>
>This problem has become considerably less likely to trigger since I
>switched the __atomic__futex_unsigned::__clock_t reference clock from
>system_clock to steady_clock and added the loop, but the consequences of
>triggering it have changed too.
>
>By my calculations it takes just over 194 days from the epoch for the
>current time not to be representable in a float. This means that
>system_clock is always subject to the problem (with the standard 1970
>epoch) whereas steady_clock with float duration only runs out of
>resolution machine has been running for that long (assuming the Linux
>implementation of CLOCK_MONOTONIC.)
>
>The recently-added loop in
>__atomic_futex_unsigned::_M_load_when_equal_until turns this scenario
>into a busy wait.
>
>Unfortunately the combination of both of these things means that it's
>not possible to write a test case for this occurring in
>_M_load_when_equal_until as it stands.
>
>	* libstdc++-v3/include/std/chrono: (__detail::ceil) Move
>          implementation of std::chrono::ceil into private namespace so
>          that it's available to pre-C++17 code.
>
>	* libstdc++-v3/include/bits/atomic_futex.h:
>	  (__atomic_futex_unsigned::_M_load_when_equal_for,
>	  __atomic_futex_unsigned::_M_load_when_equal_until): Use
>	  __detail::ceil to convert delta to the reference clock
>	  duration type to avoid resolution problems
>
>	* libstdc++-v3/testsuite/30_threads/async/async.cc: (test_pr91486):
>          New test for __atomic_futex_unsigned::_M_load_when_equal_for.
>---
> libstdc++-v3/include/bits/atomic_futex.h         |  6 +++--
> libstdc++-v3/include/std/chrono                  | 19 +++++++++++++----
> libstdc++-v3/testsuite/30_threads/async/async.cc | 15 +++++++++++++-
> 3 files changed, 34 insertions(+), 6 deletions(-)
>
>diff --git a/libstdc++-v3/include/bits/atomic_futex.h b/libstdc++-v3/include/bits/atomic_futex.h
>index 5f95ade..aa137a7 100644
>--- a/libstdc++-v3/include/bits/atomic_futex.h
>+++ b/libstdc++-v3/include/bits/atomic_futex.h
>@@ -219,8 +219,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>       _M_load_when_equal_for(unsigned __val, memory_order __mo,
> 	  const chrono::duration<_Rep, _Period>& __rtime)
>       {
>+	using __dur = typename __clock_t::duration;
> 	return _M_load_when_equal_until(__val, __mo,
>-					__clock_t::now() + __rtime);
>+		    __clock_t::now() + chrono::__detail::ceil<__dur>(__rtime));
>       }
>
>     // Returns false iff a timeout occurred.
>@@ -233,7 +234,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> 	do {
> 	  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<_Duration>(__delta);
> 	  if (_M_load_when_equal_until(__val, __mo, __s_atime))
> 	    return true;
> 	  __c_entry = _Clock::now();
>diff --git a/libstdc++-v3/include/std/chrono b/libstdc++-v3/include/std/chrono
>index 6d78f32..4257c7c 100644
>--- a/libstdc++-v3/include/std/chrono
>+++ b/libstdc++-v3/include/std/chrono
>@@ -299,6 +299,20 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> #endif
> #endif // C++20
>
>+    // We want to use ceil even when compiling for earlier standards versions
>+    namespace __detail
>+    {
>+      template<typename _ToDur, typename _Rep, typename _Period>
>+        constexpr __enable_if_is_duration<_ToDur>
>+        ceil(const duration<_Rep, _Period>& __d)
>+        {
>+	  auto __to = chrono::duration_cast<_ToDur>(__d);
>+	  if (__to < __d)
>+	    return __to + _ToDur{1};
>+	  return __to;
>+	}
>+    }
>+
> #if __cplusplus >= 201703L
> # define __cpp_lib_chrono 201611
>
>@@ -316,10 +330,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>       constexpr __enable_if_is_duration<_ToDur>
>       ceil(const duration<_Rep, _Period>& __d)
>       {
>-	auto __to = chrono::duration_cast<_ToDur>(__d);
>-	if (__to < __d)
>-	  return __to + _ToDur{1};
>-	return __to;
>+	return __detail::ceil<_ToDur>(__d);

This isn't valid in C++11, a constexpr function needs to be just a
return statement. Fix incoming ...