[PATCHv3 3/6] libstdc++ futex: Support waiting on std::chrono::steady_clock directly

[Jonathan Wakely]

On 01/08/18 14:19 +0100, Mike Crowe wrote:
>The user-visible effect of this change is for std::future::wait_until to
>use CLOCK_MONOTONIC when passed a timeout of std::chrono::steady_clock
>type. This makes it immune to any changes made to the system clock
>CLOCK_REALTIME.
>
>Add an overload of __atomic_futex_unsigned::_M_load_and_text_until_impl
>that accepts a std::chrono::steady_clock, and correctly passes this through
>to __atomic_futex_unsigned_base::_M_futex_wait_until_steady which uses
>CLOCK_MONOTONIC for the timeout within the futex system call. These
>functions are mostly just copies of the std::chrono::system_clock versions
>with small tweaks.
>
>Prior to this commit, a std::chrono::steady timeout would be converted via
>std::chrono::system_clock which risks reducing or increasing the timeout if
>someone changes CLOCK_REALTIME whilst the wait is happening. (The commit
>immediately prior to this one increases the window of opportunity for that
>from a short period during the calculation of a relative timeout, to the
>entire duration of the wait.)
>
>FUTEX_WAIT_BITSET was added in kernel v2.6.25. If futex reports ENOSYS to
>indicate that this operation is not supported then the code falls back to
>using clock_gettime(2) to calculate a relative time to wait for.
>
>I believe that I've added this functionality in a way that it doesn't break
>ABI compatibility, but that has made it more verbose and less type safe. I
>believe that it would be better to maintain the timeout as an instance of
>the correct clock type all the way down to a single _M_futex_wait_until
>function with an overload for each clock. The current scheme of separating
>out the seconds and nanoseconds early risks accidentally calling the wait
>function for the wrong clock.

Surely that would just be a programming error? Users aren't calling
these functions, and we only call them in a very limited number of
places, so the risk of calling the wrong one seems manageable. Just
don't do that.


>Unfortunately, doing this would break code
>that compiled against the old header.

We could add the new functions taking steady_clock::time_point and
system_clock::time_point, and as long as the existing function is also
still exported from the library nothing would break, would it?

We could make the existing function call the new one, or vice versa,
to avoid duplicating code.

I'm nto sure we actually want to do that, but I don't see why it
wouldn't be possible to do without breaking existing code.


>diff --git a/libstdc++-v3/src/c++11/futex.cc b/libstdc++-v3/src/c++11/futex.cc
>index 72062a4285e..5c02f0f55ed 100644
>--- a/libstdc++-v3/src/c++11/futex.cc
>+++ b/libstdc++-v3/src/c++11/futex.cc
>@@ -118,6 +125,78 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>       }
>   }
>
>+  bool
>+  __atomic_futex_unsigned_base::_M_futex_wait_until_steady(unsigned *__addr,
>+      unsigned __val,
>+      bool __has_timeout, chrono::seconds __s, chrono::nanoseconds __ns)
>+  {
>+    if (!__has_timeout)
>+      {
>+       // Ignore whether we actually succeeded to block because at worst,
>+       // we will fall back to spin-waiting.  The only thing we could do
>+       // here on errors is abort.
>+       int ret __attribute__((unused));
>+       ret = syscall (SYS_futex, __addr, futex_wait_op, __val, nullptr);
>+       _GLIBCXX_DEBUG_ASSERT(ret == 0 || errno == EINTR || errno == EAGAIN);
>+       return true;
>+      }
>+    else
>+      {
>+       if (!futex_clock_monotonic_unavailable.load(std::memory_order_relaxed))
>+         {
>+           struct timespec rt;
>+           rt.tv_sec = __s.count();
>+           rt.tv_nsec = __ns.count();
>+
>+           if (syscall (SYS_futex, __addr, futex_wait_bitset_op | futex_clock_monotonic_flag, __val, &rt, nullptr, futex_bitset_match_any) == -1)
>+             {
>+               _GLIBCXX_DEBUG_ASSERT(errno == EINTR || errno == EAGAIN
>+                                     || errno == ETIMEDOUT || errno == ENOSYS);
>+               if (errno == ETIMEDOUT)
>+                 return false;
>+               else if (errno == ENOSYS)
>+                 {
>+                   futex_clock_monotonic_unavailable.store(true, std::memory_order_relaxed);
>+                   // Fall through to legacy implementation if the system
>+                   // call is unavailable.
>+                 }
>+               else
>+                 return true;
>+             }
>+         }
>+
>+       // We only get to here if futex_clock_realtime_unavailable was

This should say futex_clock_monotonic_unavailable.

I'm also replacing the _GLIBCXX_DEBUG_ASSERT checks in that file with
__glibcxx_assert checks, because that file is never going to be
compiled with Debug Mode.