[PATCHv3 6/6] Extra async tests, not for merging

[PATCHv3 6/6] Extra async tests, not for merging

[Mike Crowe]

These tests show that changing the system clock has an effect on
std::future::wait_until when using std::chrono::system_clock but not when
using std::chrono::steady_clock. Unfortunately these tests have a number of
downsides:

1. Nothing that is attempting to keep the clock set correctly (ntpd,
   systemd-timesyncd) can be running at the same time.

2. The test process requires the CAP_SYS_TIME capability (although, as it's
   written it checks for being root.)

3. Other processes running concurrently may misbehave when the clock darts
   back and forth.

4. They are slow to run.

As such, I don't think they are suitable for merging. I include them here
because I wanted to document how I had tested the changes in the previous
commits.
---
 libstdc++-v3/testsuite/30_threads/async/async.cc | 70 ++++++++++++++++++++++++
 1 file changed, 70 insertions(+)

diff --git a/libstdc++-v3/testsuite/30_threads/async/async.cc b/libstdc++-v3/testsuite/30_threads/async/async.cc
index 755c95cbea6..4f547fb5a75 100644
--- a/libstdc++-v3/testsuite/30_threads/async/async.cc
+++ b/libstdc++-v3/testsuite/30_threads/async/async.cc
@@ -24,6 +24,7 @@

 #include <future>
 #include <testsuite_hooks.h>
+#include <sys/time.h>

 using namespace std;

@@ -157,6 +158,71 @@ void test04()
   }
 }

+void perturb_system_clock(const std::chrono::seconds &seconds)
+{
+  struct timeval tv;
+  if (gettimeofday(&tv, NULL))
+    abort();
+
+  tv.tv_sec += seconds.count();
+  if (settimeofday(&tv, NULL))
+    abort();
+}
+
+// Ensure that advancing CLOCK_REALTIME doesn't make any difference
+// when we're waiting on std::chrono::steady_clock.
+void test05()
+{
+  auto const start = chrono::steady_clock::now();
+  future<void> f1 = async(launch::async, []() {
+      std::this_thread::sleep_for(std::chrono::seconds(10));
+    });
+
+  perturb_system_clock(chrono::seconds(20));
+
+  std::future_status status;
+  status = f1.wait_for(std::chrono::seconds(4));
+  VERIFY( status == std::future_status::timeout );
+
+  status = f1.wait_until(start + std::chrono::seconds(6));
+  VERIFY( status == std::future_status::timeout );
+
+  status = f1.wait_until(start + std::chrono::seconds(12));
+  VERIFY( status == std::future_status::ready );
+
+  auto const elapsed = chrono::steady_clock::now() - start;
+  VERIFY( elapsed >= std::chrono::seconds(10) );
+  VERIFY( elapsed < std::chrono::seconds(15) );
+
+  perturb_system_clock(chrono::seconds(-20));
+}
+
+// Ensure that advancing CLOCK_REALTIME does make a difference when
+// we're waiting on std::chrono::system_clock.
+void test06()
+{
+  auto const start = chrono::system_clock::now();
+  auto const start_steady = chrono::steady_clock::now();
+
+  future<void> f1 = async(launch::async, []() {
+      std::this_thread::sleep_for(std::chrono::seconds(5));
+      perturb_system_clock(chrono::seconds(60));
+      std::this_thread::sleep_for(std::chrono::seconds(5));
+    });
+  future_status status;
+  status = f1.wait_until(start + std::chrono::seconds(60));
+  VERIFY( status == std::future_status::timeout );
+
+  auto const elapsed_steady = chrono::steady_clock::now() - start_steady;
+  VERIFY( elapsed_steady >= std::chrono::seconds(5) );
+  VERIFY( elapsed_steady < std::chrono::seconds(10) );
+
+  status = f1.wait_until(start + std::chrono::seconds(75));
+  VERIFY( status == std::future_status::ready );
+
+  perturb_system_clock(chrono::seconds(-60));
+}
+
 int main()
 {
   test01();
@@ -165,5 +231,9 @@ int main()
   test03<std::chrono::steady_clock>();
   test03<steady_clock_copy>();
   test04();
+  if (geteuid() == 0) {
+    test05();
+    test06();
+  }
   return 0;
 }
--
2.11.0

BrightSign considers your privacy to be very important. The emails you send to us will be protected and secured. Furthermore, we will only use your email and contact information for the reasons you sent them to us and for tracking how effectively we respond to your requests.

[PATCHv3 0/6] std::future::wait_* improvements

[Mike Crowe]

v2 of this series was originally posted back in January (see
https://gcc.gnu.org/ml/libstdc++/2018-01/msg00035.html )

Apart from minor log message tweaks, the changes since that version are:

* [1/6] Improve libstdc++-v3 async test

  Speed up the tests at the risk of more sporadic failures on loaded
  machines.

  Use lambda rather than separate function for asynchronous routine.

* [2/6] libstdc++ futex: Use FUTEX_CLOCK_REALTIME for wait

  Fall back to using gettimeofday and FUTEX_WAIT if FUTEX_WAIT_BITSET and
  FUTEX_CLOCK_REALTIME are not available.

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

  Fall back to using clock_gettime (or the sycall directly if necessary)
  and FUTEX_WAIT if FUTEX_WAIT_BITSET is unavailable.

* [4/6] libstdc++ atomic_futex: Use std::chrono::steady_clock as reference clock

  No changes

* [5/6] libstdc++ futex: Loop when waiting against arbitrary clock

  New patch. My work on std::condition_variable::wait_until made me realise
  that there's a risk of indicating a timeout too early when using a
  non-standard clock.

* [6/6] Extra async tests, not for merging

  Use lambdas rather than separate functions for asynchronous routines.


Torvald Riegel had some objections to my design, but did not respond when I
attempted to justify it and attempted to change my implementation based on
his suggestions (see https://gcc.gnu.org/ml/libstdc++/2018-01/msg00071.html
.)

It looks like https://gcc.gnu.org/bugzilla/show_bug.cgi?id=68519 could
apply equally well to __atomic_futex_unsigned::_M_load_when_equal_for.
I plan to look at that next.

I set up a Debian 4.0 "Etch" system (v2.6.18 kernel, glibc 2.3.6.)
Unfortunately, its GCC 4.1.2 is unable to compile GCC master (as of
5ba044fc3a443274462527eed385732f7ecee3a8) because hash-map.h appears to be
trying to put a reference in a std::pair.) The above patches cherry-pick
cleanly back to GCC 7.3, and that version does build after adding a few
constants to the system headers. I've confirmed that the lack of support
for FUTEX_WAIT_BITSET is detected correctly and that the code falls back to
using FUTEX_WAIT. This test also shows that the fallback to calling the
clock_gettime system call directly is working too. The async.cc tests all
passed.

I haven't made any attempt to add entries to the .abilist files. I'm not
sure whether I'm supposed to do that as part of the patches, or not.


Mike Crowe (6):
  Improve libstdc++-v3 async test
  libstdc++ futex: Use FUTEX_CLOCK_REALTIME for wait
  libstdc++ futex: Support waiting on std::chrono::steady_clock directly
  libstdc++ atomic_futex: Use std::chrono::steady_clock as reference
    clock
  libstdc++ futex: Loop when waiting against arbitrary clock
  Extra async tests, not for merging

 libstdc++-v3/include/bits/atomic_futex.h         |  89 ++++++++++--
 libstdc++-v3/src/c++11/futex.cc                  | 113 +++++++++++++++
 libstdc++-v3/testsuite/30_threads/async/async.cc | 172 +++++++++++++++++++++++
 3 files changed, 364 insertions(+), 10 deletions(-)

--
2.11.0

BrightSign considers your privacy to be very important. The emails you send to us will be protected and secured. Furthermore, we will only use your email and contact information for the reasons you sent them to us and for tracking how effectively we respond to your requests.

[PATCHv3 5/6] libstdc++ futex: Loop when waiting against arbitrary clock

[Mike Crowe]

If std::future::wait_until is passed a time point measured against a clock
that is neither std::chrono::steady_clock nor std::chrono::system_clock
then the generic implementation of
__atomic_futex_unsigned::_M_load_when_equal_until is called which
calculates the timeout based on __clock_t and calls the
_M_load_when_equal_until method for that clock to perform the actual wait.

There's no guarantee that __clock_t is running at the same speed as
__clock_t, so if the underlying wait times out timeout we need to check the
timeout against the caller's clock again before potentially looping.

Also add two extra tests to the testsuite's async.cc:

* run test03 with steady_clock_copy, which behaves identically to
  std::chrono::steady_clock, but isn't std::chrono::steady_clock. This
  causes the overload of __atomic_futex_unsigned::_M_load_when_equal_until
  that takes an arbitrary clock to be called.

* invent test04 which uses a deliberately slow running clock in order to
  exercise the looping behaviour o
  __atomic_futex_unsigned::_M_load_when_equal_until described above.
---
 libstdc++-v3/include/bits/atomic_futex.h         | 15 ++++--
 libstdc++-v3/testsuite/30_threads/async/async.cc | 69 ++++++++++++++++++++++++
 2 files changed, 79 insertions(+), 5 deletions(-)

diff --git a/libstdc++-v3/include/bits/atomic_futex.h b/libstdc++-v3/include/bits/atomic_futex.h
index a35020aef4f..b7ffb7fb191 100644
--- a/libstdc++-v3/include/bits/atomic_futex.h
+++ b/libstdc++-v3/include/bits/atomic_futex.h
@@ -229,11 +229,16 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       _M_load_when_equal_until(unsigned __val, memory_order __mo,
          const chrono::time_point<_Clock, _Duration>& __atime)
       {
-       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;
-       return _M_load_when_equal_until(__val, __mo, __s_atime);
+       typename _Clock::time_point __c_entry = _Clock::now();
+       do {
+         const __clock_t::time_point __s_entry = __clock_t::now();
+         const auto __delta = __atime - __c_entry;
+         const auto __s_atime = __s_entry + __delta;
+         if (_M_load_when_equal_until(__val, __mo, __s_atime))
+           return true;
+         __c_entry = _Clock::now();
+       } while (__c_entry < __atime);
+       return false;
       }

     // Returns false iff a timeout occurred.
diff --git a/libstdc++-v3/testsuite/30_threads/async/async.cc b/libstdc++-v3/testsuite/30_threads/async/async.cc
index 015bcce0c2c..755c95cbea6 100644
--- a/libstdc++-v3/testsuite/30_threads/async/async.cc
+++ b/libstdc++-v3/testsuite/30_threads/async/async.cc
@@ -63,6 +63,27 @@ void test02()
   VERIFY( status == std::future_status::ready );
 }

+// This clock is used to ensure that the
+// __atomic_futex_unsigned::_M_load_when_equal_until overload that
+// takes an arbitrary clock is exercised. Without it, only the
+// specific std::chrono::steady_clock and std::chrono::realtime_clock
+// overloads are exercised.
+
+struct steady_clock_copy
+{
+  using rep = std::chrono::steady_clock::rep;
+  using period = std::chrono::steady_clock::period;
+  using duration = std::chrono::steady_clock::duration;
+  using time_point = std::chrono::time_point<steady_clock_copy, duration>;
+  static constexpr bool is_steady = true;
+
+  static time_point now()
+  {
+    auto steady = std::chrono::steady_clock::now();
+    return time_point{steady.time_since_epoch()};
+  }
+};
+
 // This test is prone to failures if run on a loaded machine where the
 // kernel decides not to schedule us for several seconds. It also
 // assumes that no-one will warp CLOCK whilst the test is
@@ -90,11 +111,59 @@ void test03()
   VERIFY( elapsed < std::chrono::seconds(5) );
 }

+// This clock is supposed to run at a tenth of normal speed, but we
+// don't have to worry about rounding errors causing us to wake up
+// slightly too early below if we actually run it at an eleventh of
+// normal speed.
+struct slow_clock
+{
+  using rep = std::chrono::steady_clock::rep;
+  using period = std::chrono::steady_clock::period;
+  using duration = std::chrono::steady_clock::duration;
+  using time_point = std::chrono::time_point<slow_clock, duration>;
+  static constexpr bool is_steady = true;
+
+  static time_point now()
+  {
+    auto steady = std::chrono::steady_clock::now();
+    return time_point{steady.time_since_epoch() / 11};
+  }
+};
+
+void test04()
+{
+  auto const slow_start = slow_clock::now();
+  future<void> f1 = async(launch::async, []() {
+      std::this_thread::sleep_for(std::chrono::seconds(2));
+    });
+
+  // Wait for ~1s
+  {
+    auto const steady_begin = std::chrono::steady_clock::now();
+    auto const status = f1.wait_until(slow_start + std::chrono::milliseconds(100));
+    VERIFY(status == std::future_status::timeout);
+    auto const elapsed = std::chrono::steady_clock::now() - steady_begin;
+    VERIFY(elapsed >= std::chrono::seconds(1));
+    VERIFY(elapsed < std::chrono::seconds(2));
+  }
+
+  // Wait for up to ~2s more
+  {
+    auto const steady_begin = std::chrono::steady_clock::now();
+    auto const status = f1.wait_until(slow_start + std::chrono::milliseconds(300));
+    auto const elapsed = std::chrono::steady_clock::now() - steady_begin;
+    VERIFY(status == std::future_status::ready);
+    VERIFY(elapsed < std::chrono::seconds(2));
+  }
+}
+
 int main()
 {
   test01();
   test02();
   test03<std::chrono::system_clock>();
   test03<std::chrono::steady_clock>();
+  test03<steady_clock_copy>();
+  test04();
   return 0;
 }
--
2.11.0

BrightSign considers your privacy to be very important. The emails you send to us will be protected and secured. Furthermore, we will only use your email and contact information for the reasons you sent them to us and for tracking how effectively we respond to your requests.

[PATCHv3 2/6] libstdc++ futex: Use FUTEX_CLOCK_REALTIME for wait

[Mike Crowe]

The futex system call supports waiting for an absolute time if
FUTEX_WAIT_BITSET is used rather than FUTEX_WAIT. Doing so provides two
benefits:

1. The call to gettimeofday is not required in order to calculate a
   relative timeout.

2. If someone changes the system clock during the wait then the futex
   timeout will correctly expire earlier or later. Currently that only
   happens if the clock is changed prior to the call to gettimeofday.

According to futex(2), support for FUTEX_CLOCK_REALTIME was added in the
v2.6.28 Linux kernel and FUTEX_WAIT_BITSET was added in v2.6.25. To ensure
that the code still works correctly with earlier kernel versions, an ENOSYS
error from futex[1] results in the futex_clock_realtime_unavailable being
flag being set. This flag is used to avoid the unnecessary unsupported
futex call in the future and to fall back to the previous gettimeofday and
relative time implementation.

glibc applied an equivalent switch in pthread_cond_timedwait to use
FUTEX_CLOCK_REALTIME and FUTEX_WAIT_BITSET rather than FUTEX_WAIT for
glibc-2.10 back in 2009. See glibc:cbd8aeb836c8061c23a5e00419e0fb25a34abee7

The futex_clock_realtime_unavailable flag is accessed using
std::memory_order_relaxed to stop it becoming a bottleneck. If the first
two calls to _M_futex_wait_until happen to happen simultaneously then the
only consequence is that both will try to use FUTEX_CLOCK_REALTIME, both
risk discovering that it doesn't work and, if so, both set the flag.

[1] This is how glibc's nptl-init.c determines whether these flags are
    supported.
---
 libstdc++-v3/src/c++11/futex.cc | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/libstdc++-v3/src/c++11/futex.cc b/libstdc++-v3/src/c++11/futex.cc
index 278a5a80902..72062a4285e 100644
--- a/libstdc++-v3/src/c++11/futex.cc
+++ b/libstdc++-v3/src/c++11/futex.cc
@@ -35,8 +35,16 @@

 // Constants for the wait/wake futex syscall operations
 const unsigned futex_wait_op = 0;
+const unsigned futex_wait_bitset_op = 9;
+const unsigned futex_clock_realtime_flag = 256;
+const unsigned futex_bitset_match_any = ~0;
 const unsigned futex_wake_op = 1;

+namespace
+{
+  std::atomic<bool> futex_clock_realtime_unavailable;
+}
+
 namespace std _GLIBCXX_VISIBILITY(default)
 {
 _GLIBCXX_BEGIN_NAMESPACE_VERSION
@@ -58,6 +66,32 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       }
     else
       {
+       if (!futex_clock_realtime_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_realtime_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;
+               if (errno == ENOSYS)
+                 {
+                   futex_clock_realtime_unavailable.store(true, std::memory_order_relaxed);
+                   // Fall through to legacy implementation if the system
+                   // call is unavailable.
+                 }
+               else
+                 return true;
+             }
+           else
+             return true;
+         }
+
+       // We only get to here if futex_clock_realtime_unavailable was
+       // true or has just been set to true.
        struct timeval tv;
        gettimeofday (&tv, NULL);
        // Convert the absolute timeout value to a relative timeout
--
2.11.0

BrightSign considers your privacy to be very important. The emails you send to us will be protected and secured. Furthermore, we will only use your email and contact information for the reasons you sent them to us and for tracking how effectively we respond to your requests.

[PATCHv3 4/6] libstdc++ atomic_futex: Use std::chrono::steady_clock as reference clock

[Mike Crowe]

The user-visible effect of this change is that std::future::wait_for now
uses std::chrono::steady_clock to determine the timeout. This makes it
immune to changes made to the system clock. It also means that anyone using
their own clock types with std::future::wait_until will have the timeout
converted to std::chrono::steady_clock rather than
std::chrono::system_clock.

Now that use of both std::chrono::steady_clock and
std::chrono::system_clock are correctly supported for the wait timeout, I
believe that std::chrono::steady_clock is a better choice for the reference
clock that all other clocks are converted to since it is guaranteed to
advance steadily. The previous behaviour of converting to
std::chrono::system_clock risks timeouts changing dramatically when the
system clock is changed.
---
 libstdc++-v3/include/bits/atomic_futex.h | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/libstdc++-v3/include/bits/atomic_futex.h b/libstdc++-v3/include/bits/atomic_futex.h
index 47ecd329ea9..a35020aef4f 100644
--- a/libstdc++-v3/include/bits/atomic_futex.h
+++ b/libstdc++-v3/include/bits/atomic_futex.h
@@ -71,7 +71,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
   template <unsigned _Waiter_bit = 0x80000000>
   class __atomic_futex_unsigned : __atomic_futex_unsigned_base
   {
-    typedef chrono::system_clock __clock_t;
+    typedef chrono::steady_clock __clock_t;

     // This must be lock-free and at offset 0.
     atomic<unsigned> _M_data;
@@ -169,7 +169,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     unsigned
     _M_load_and_test_until_impl(unsigned __assumed, unsigned __operand,
        bool __equal, memory_order __mo,
-       const chrono::time_point<__clock_t, _Dur>& __atime)
+       const chrono::time_point<std::chrono::system_clock, _Dur>& __atime)
     {
       auto __s = chrono::time_point_cast<chrono::seconds>(__atime);
       auto __ns = chrono::duration_cast<chrono::nanoseconds>(__atime - __s);
@@ -229,7 +229,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       _M_load_when_equal_until(unsigned __val, memory_order __mo,
          const chrono::time_point<_Clock, _Duration>& __atime)
       {
-       // DR 887 - Sync unknown clock to known clock.
        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;
@@ -241,7 +240,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     template<typename _Duration>
     _GLIBCXX_ALWAYS_INLINE bool
     _M_load_when_equal_until(unsigned __val, memory_order __mo,
-       const chrono::time_point<__clock_t, _Duration>& __atime)
+       const chrono::time_point<std::chrono::system_clock, _Duration>& __atime)
     {
       unsigned __i = _M_load(__mo);
       if ((__i & ~_Waiter_bit) == __val)
--
2.11.0

BrightSign considers your privacy to be very important. The emails you send to us will be protected and secured. Furthermore, we will only use your email and contact information for the reasons you sent them to us and for tracking how effectively we respond to your requests.

[PATCHv3 1/6] Improve libstdc++-v3 async test

[Mike Crowe]

Add tests for waiting for the future using both std::chrono::steady_clock
and std::chrono::system_clock in preparation for dealing with those clocks
properly in futex.cc.
---
 libstdc++-v3/testsuite/30_threads/async/async.cc | 33 ++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/libstdc++-v3/testsuite/30_threads/async/async.cc b/libstdc++-v3/testsuite/30_threads/async/async.cc
index 4c2cdd1a534..015bcce0c2c 100644
--- a/libstdc++-v3/testsuite/30_threads/async/async.cc
+++ b/libstdc++-v3/testsuite/30_threads/async/async.cc
@@ -51,17 +51,50 @@ void test02()
   VERIFY( status == std::future_status::timeout );
   status = f1.wait_until(std::chrono::system_clock::now());
   VERIFY( status == std::future_status::timeout );
+  status = f1.wait_until(std::chrono::steady_clock::now());
+  VERIFY( status == std::future_status::timeout );
   l.unlock();  // allow async thread to proceed
   f1.wait();   // wait for it to finish
   status = f1.wait_for(std::chrono::milliseconds(0));
   VERIFY( status == std::future_status::ready );
   status = f1.wait_until(std::chrono::system_clock::now());
   VERIFY( status == std::future_status::ready );
+  status = f1.wait_until(std::chrono::steady_clock::now());
+  VERIFY( status == std::future_status::ready );
+}
+
+// This test is prone to failures if run on a loaded machine where the
+// kernel decides not to schedule us for several seconds. It also
+// assumes that no-one will warp CLOCK whilst the test is
+// running.
+template<typename CLOCK>
+void test03()
+{
+  auto const start = CLOCK::now();
+  future<void> f1 = async(launch::async, []() {
+      std::this_thread::sleep_for(std::chrono::seconds(2));
+    });
+  std::future_status status;
+
+  status = f1.wait_for(std::chrono::milliseconds(500));
+  VERIFY( status == std::future_status::timeout );
+
+  status = f1.wait_until(start + std::chrono::seconds(1));
+  VERIFY( status == std::future_status::timeout );
+
+  status = f1.wait_until(start + std::chrono::seconds(5));
+  VERIFY( status == std::future_status::ready );
+
+  auto const elapsed = CLOCK::now() - start;
+  VERIFY( elapsed >= std::chrono::seconds(2) );
+  VERIFY( elapsed < std::chrono::seconds(5) );
 }

 int main()
 {
   test01();
   test02();
+  test03<std::chrono::system_clock>();
+  test03<std::chrono::steady_clock>();
   return 0;
 }
--
2.11.0

BrightSign considers your privacy to be very important. The emails you send to us will be protected and secured. Furthermore, we will only use your email and contact information for the reasons you sent them to us and for tracking how effectively we respond to your requests.

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

[Mike Crowe]

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. Unfortunately, doing this would break code
that compiled against the old header.
---
 libstdc++-v3/include/bits/atomic_futex.h | 67 ++++++++++++++++++++++++++-
 libstdc++-v3/src/c++11/futex.cc          | 79 ++++++++++++++++++++++++++++++++
 2 files changed, 145 insertions(+), 1 deletion(-)

diff --git a/libstdc++-v3/include/bits/atomic_futex.h b/libstdc++-v3/include/bits/atomic_futex.h
index ecf5b02031a..47ecd329ea9 100644
--- a/libstdc++-v3/include/bits/atomic_futex.h
+++ b/libstdc++-v3/include/bits/atomic_futex.h
@@ -52,11 +52,18 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 #if defined(_GLIBCXX_HAVE_LINUX_FUTEX) && ATOMIC_INT_LOCK_FREE > 1
   struct __atomic_futex_unsigned_base
   {
-    // Returns false iff a timeout occurred.
+    // __s and __ns are measured against CLOCK_REALTIME. Returns false
+    // iff a timeout occurred.
     bool
     _M_futex_wait_until(unsigned *__addr, unsigned __val, bool __has_timeout,
        chrono::seconds __s, chrono::nanoseconds __ns);

+    // __s and __ns are measured against CLOCK_MONOTONIC. Returns
+    // false iff a timeout occurred.
+    bool
+    _M_futex_wait_until_steady(unsigned *__addr, unsigned __val, bool __has_timeout,
+       chrono::seconds __s, chrono::nanoseconds __ns);
+
     // This can be executed after the object has been destroyed.
     static void _M_futex_notify_all(unsigned* __addr);
   };
@@ -86,6 +93,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     // value if equal is false.
     // The assumed value is the caller's assumption about the current value
     // when making the call.
+    // __s and __ns are measured against CLOCK_REALTIME.
     unsigned
     _M_load_and_test_until(unsigned __assumed, unsigned __operand,
        bool __equal, memory_order __mo, bool __has_timeout,
@@ -110,6 +118,36 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
        }
     }

+    // If a timeout occurs, returns a current value after the timeout;
+    // otherwise, returns the operand's value if equal is true or a different
+    // value if equal is false.
+    // The assumed value is the caller's assumption about the current value
+    // when making the call.
+    // __s and __ns are measured against CLOCK_MONOTONIC.
+    unsigned
+    _M_load_and_test_until_steady(unsigned __assumed, unsigned __operand,
+       bool __equal, memory_order __mo, bool __has_timeout,
+       chrono::seconds __s, chrono::nanoseconds __ns)
+    {
+      for (;;)
+       {
+         // Don't bother checking the value again because we expect the caller
+         // to have done it recently.
+         // memory_order_relaxed is sufficient because we can rely on just the
+         // modification order (store_notify uses an atomic RMW operation too),
+         // and the futex syscalls synchronize between themselves.
+         _M_data.fetch_or(_Waiter_bit, memory_order_relaxed);
+         bool __ret = _M_futex_wait_until_steady((unsigned*)(void*)&_M_data,
+                                          __assumed | _Waiter_bit,
+                                          __has_timeout, __s, __ns);
+         // Fetch the current value after waiting (clears _Waiter_bit).
+         __assumed = _M_load(__mo);
+         if (!__ret || ((__operand == __assumed) == __equal))
+           return __assumed;
+         // TODO adapt wait time
+       }
+    }
+
     // Returns the operand's value if equal is true or a different value if
     // equal is false.
     // The assumed value is the caller's assumption about the current value
@@ -140,6 +178,19 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
          true, __s.time_since_epoch(), __ns);
     }

+    template<typename _Dur>
+    unsigned
+    _M_load_and_test_until_impl(unsigned __assumed, unsigned __operand,
+       bool __equal, memory_order __mo,
+       const chrono::time_point<std::chrono::steady_clock, _Dur>& __atime)
+    {
+      auto __s = chrono::time_point_cast<chrono::seconds>(__atime);
+      auto __ns = chrono::duration_cast<chrono::nanoseconds>(__atime - __s);
+      // XXX correct?
+      return _M_load_and_test_until_steady(__assumed, __operand, __equal, __mo,
+         true, __s.time_since_epoch(), __ns);
+    }
+
   public:

     _GLIBCXX_ALWAYS_INLINE unsigned
@@ -200,6 +251,20 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       return (__i & ~_Waiter_bit) == __val;
     }

+    // Returns false iff a timeout occurred.
+    template<typename _Duration>
+    _GLIBCXX_ALWAYS_INLINE bool
+    _M_load_when_equal_until(unsigned __val, memory_order __mo,
+       const chrono::time_point<std::chrono::steady_clock, _Duration>& __atime)
+    {
+      unsigned __i = _M_load(__mo);
+      if ((__i & ~_Waiter_bit) == __val)
+       return true;
+      // TODO Spin-wait first.  Ignore effect on timeout.
+      __i = _M_load_and_test_until_impl(__i, __val, true, __mo, __atime);
+      return (__i & ~_Waiter_bit) == __val;
+    }
+
     _GLIBCXX_ALWAYS_INLINE void
     _M_store_notify_all(unsigned __val, memory_order __mo)
     {
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
@@ -33,9 +33,15 @@
 #include <errno.h>
 #include <debug/debug.h>

+#ifdef _GLIBCXX_USE_CLOCK_GETTIME_SYSCALL
+#include <unistd.h>
+#include <sys/syscall.h>
+#endif
+
 // Constants for the wait/wake futex syscall operations
 const unsigned futex_wait_op = 0;
 const unsigned futex_wait_bitset_op = 9;
+const unsigned futex_clock_monotonic_flag = 0;
 const unsigned futex_clock_realtime_flag = 256;
 const unsigned futex_bitset_match_any = ~0;
 const unsigned futex_wake_op = 1;
@@ -43,6 +49,7 @@ const unsigned futex_wake_op = 1;
 namespace
 {
   std::atomic<bool> futex_clock_realtime_unavailable;
+  std::atomic<bool> futex_clock_monotonic_unavailable;
 }

 namespace std _GLIBCXX_VISIBILITY(default)
@@ -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
+       // true or has just been set to true.
+       struct timespec ts;
+#ifdef _GLIBCXX_USE_CLOCK_GETTIME_SYSCALL
+       syscall(SYS_clock_gettime, CLOCK_MONOTONIC, &ts);
+#else
+       clock_gettime(CLOCK_MONOTONIC, &ts);
+#endif
+       // Convert the absolute timeout value to a relative timeout
+       struct timespec rt;
+       rt.tv_sec = __s.count() - ts.tv_sec;
+       rt.tv_nsec = __ns.count() - ts.tv_nsec;
+       if (rt.tv_nsec < 0)
+         {
+           rt.tv_nsec += 1000000000;
+           --rt.tv_sec;
+         }
+       // Did we already time out?
+       if (rt.tv_sec < 0)
+         return false;
+
+       if (syscall (SYS_futex, __addr, futex_wait_op, __val, &rt) == -1)
+         {
+           _GLIBCXX_DEBUG_ASSERT(errno == EINTR || errno == EAGAIN
+                                 || errno == ETIMEDOUT);
+           if (errno == ETIMEDOUT)
+             return false;
+         }
+       return true;
+      }
+  }
+
   void
   __atomic_futex_unsigned_base::_M_futex_notify_all(unsigned* __addr)
   {
--
2.11.0

BrightSign considers your privacy to be very important. The emails you send to us will be protected and secured. Furthermore, we will only use your email and contact information for the reasons you sent them to us and for tracking how effectively we respond to your requests.

Re: [PATCHv3 1/6] Improve libstdc++-v3 async test

[Jonathan Wakely]

On 01/08/18 14:19 +0100, Mike Crowe wrote:
>Add tests for waiting for the future using both std::chrono::steady_clock
>and std::chrono::system_clock in preparation for dealing with those clocks
>properly in futex.cc.
>---
> libstdc++-v3/testsuite/30_threads/async/async.cc | 33 ++++++++++++++++++++++++
> 1 file changed, 33 insertions(+)
>
>diff --git a/libstdc++-v3/testsuite/30_threads/async/async.cc b/libstdc++-v3/testsuite/30_threads/async/async.cc
>index 4c2cdd1a534..015bcce0c2c 100644
>--- a/libstdc++-v3/testsuite/30_threads/async/async.cc
>+++ b/libstdc++-v3/testsuite/30_threads/async/async.cc
>@@ -51,17 +51,50 @@ void test02()
>   VERIFY( status == std::future_status::timeout );
>   status = f1.wait_until(std::chrono::system_clock::now());
>   VERIFY( status == std::future_status::timeout );
>+  status = f1.wait_until(std::chrono::steady_clock::now());
>+  VERIFY( status == std::future_status::timeout );
>   l.unlock();  // allow async thread to proceed
>   f1.wait();   // wait for it to finish
>   status = f1.wait_for(std::chrono::milliseconds(0));
>   VERIFY( status == std::future_status::ready );
>   status = f1.wait_until(std::chrono::system_clock::now());
>   VERIFY( status == std::future_status::ready );
>+  status = f1.wait_until(std::chrono::steady_clock::now());
>+  VERIFY( status == std::future_status::ready );
>+}
>+
>+// This test is prone to failures if run on a loaded machine where the
>+// kernel decides not to schedule us for several seconds. It also
>+// assumes that no-one will warp CLOCK whilst the test is
>+// running.
>+template<typename CLOCK>
>+void test03()
>+{
>+  auto const start = CLOCK::now();
>+  future<void> f1 = async(launch::async, []() {
>+      std::this_thread::sleep_for(std::chrono::seconds(2));

Thanks for reducing these times since the last patch. I think this
test improvement can go on trunk, and if we see too many failures we
can adjust the times.

Re: [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.

Re: [PATCHv3 0/6] std::future::wait_* improvements

[Jonathan Wakely]

On 01/08/18 14:19 +0100, Mike Crowe wrote:
>v2 of this series was originally posted back in January (see
>https://gcc.gnu.org/ml/libstdc++/2018-01/msg00035.html )
>
>Apart from minor log message tweaks, the changes since that version are:
>
>* [1/6] Improve libstdc++-v3 async test
>
>  Speed up the tests at the risk of more sporadic failures on loaded
>  machines.
>
>  Use lambda rather than separate function for asynchronous routine.
>
>* [2/6] libstdc++ futex: Use FUTEX_CLOCK_REALTIME for wait
>
>  Fall back to using gettimeofday and FUTEX_WAIT if FUTEX_WAIT_BITSET and
>  FUTEX_CLOCK_REALTIME are not available.
>
>* [3/6] libstdc++ futex: Support waiting on std::chrono::steady_clock directly
>
>  Fall back to using clock_gettime (or the sycall directly if necessary)
>  and FUTEX_WAIT if FUTEX_WAIT_BITSET is unavailable.
>
>* [4/6] libstdc++ atomic_futex: Use std::chrono::steady_clock as reference clock
>
>  No changes
>
>* [5/6] libstdc++ futex: Loop when waiting against arbitrary clock
>
>  New patch. My work on std::condition_variable::wait_until made me realise
>  that there's a risk of indicating a timeout too early when using a
>  non-standard clock.
>
>* [6/6] Extra async tests, not for merging
>
>  Use lambdas rather than separate functions for asynchronous routines.
>
>
>Torvald Riegel had some objections to my design, but did not respond when I
>attempted to justify it and attempted to change my implementation based on
>his suggestions (see https://gcc.gnu.org/ml/libstdc++/2018-01/msg00071.html
>.)
>
>It looks like https://gcc.gnu.org/bugzilla/show_bug.cgi?id=68519 could
>apply equally well to __atomic_futex_unsigned::_M_load_when_equal_for.
>I plan to look at that next.
>
>I set up a Debian 4.0 "Etch" system (v2.6.18 kernel, glibc 2.3.6.)
>Unfortunately, its GCC 4.1.2 is unable to compile GCC master (as of
>5ba044fc3a443274462527eed385732f7ecee3a8) because hash-map.h appears to be
>trying to put a reference in a std::pair.)

That's https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86739

>The above patches cherry-pick
>cleanly back to GCC 7.3, and that version does build after adding a few
>constants to the system headers. I've confirmed that the lack of support
>for FUTEX_WAIT_BITSET is detected correctly and that the code falls back to
>using FUTEX_WAIT. This test also shows that the fallback to calling the
>clock_gettime system call directly is working too. The async.cc tests all
>passed.
>
>I haven't made any attempt to add entries to the .abilist files. I'm not
>sure whether I'm supposed to do that as part of the patches, or not.

Changes to gnu.ver linker script are needed, which should be in the patch.

Changes to the baseline_symbols.txt files should not be included in
the patch, those are regenerated later.


>
>Mike Crowe (6):
>  Improve libstdc++-v3 async test
>  libstdc++ futex: Use FUTEX_CLOCK_REALTIME for wait
>  libstdc++ futex: Support waiting on std::chrono::steady_clock directly
>  libstdc++ atomic_futex: Use std::chrono::steady_clock as reference
>    clock
>  libstdc++ futex: Loop when waiting against arbitrary clock
>  Extra async tests, not for merging

Most of these patches fail to apply because they use spaces where the
GCC code has tabs. I can fix it, but I'm not sure why the tabs aren't
in your patches.

Re: [PATCHv3 2/6] libstdc++ futex: Use FUTEX_CLOCK_REALTIME for wait

[Jonathan Wakely]

On 01/08/18 14:19 +0100, Mike Crowe wrote:
>The futex system call supports waiting for an absolute time if
>FUTEX_WAIT_BITSET is used rather than FUTEX_WAIT. Doing so provides two
>benefits:
>
>1. The call to gettimeofday is not required in order to calculate a
>   relative timeout.
>
>2. If someone changes the system clock during the wait then the futex
>   timeout will correctly expire earlier or later. Currently that only
>   happens if the clock is changed prior to the call to gettimeofday.
>
>According to futex(2), support for FUTEX_CLOCK_REALTIME was added in the
>v2.6.28 Linux kernel and FUTEX_WAIT_BITSET was added in v2.6.25. To ensure
>that the code still works correctly with earlier kernel versions, an ENOSYS
>error from futex[1] results in the futex_clock_realtime_unavailable being
>flag being set. This flag is used to avoid the unnecessary unsupported
>futex call in the future and to fall back to the previous gettimeofday and
>relative time implementation.
>
>glibc applied an equivalent switch in pthread_cond_timedwait to use
>FUTEX_CLOCK_REALTIME and FUTEX_WAIT_BITSET rather than FUTEX_WAIT for
>glibc-2.10 back in 2009. See glibc:cbd8aeb836c8061c23a5e00419e0fb25a34abee7
>
>The futex_clock_realtime_unavailable flag is accessed using
>std::memory_order_relaxed to stop it becoming a bottleneck. If the first
>two calls to _M_futex_wait_until happen to happen simultaneously then the
>only consequence is that both will try to use FUTEX_CLOCK_REALTIME, both
>risk discovering that it doesn't work and, if so, both set the flag.
>
>[1] This is how glibc's nptl-init.c determines whether these flags are
>    supported.
>---
> libstdc++-v3/src/c++11/futex.cc | 34 ++++++++++++++++++++++++++++++++++
> 1 file changed, 34 insertions(+)
>
>diff --git a/libstdc++-v3/src/c++11/futex.cc b/libstdc++-v3/src/c++11/futex.cc
>index 278a5a80902..72062a4285e 100644
>--- a/libstdc++-v3/src/c++11/futex.cc
>+++ b/libstdc++-v3/src/c++11/futex.cc
>@@ -35,8 +35,16 @@
>
> // Constants for the wait/wake futex syscall operations
> const unsigned futex_wait_op = 0;
>+const unsigned futex_wait_bitset_op = 9;
>+const unsigned futex_clock_realtime_flag = 256;
>+const unsigned futex_bitset_match_any = ~0;
> const unsigned futex_wake_op = 1;
>
>+namespace
>+{
>+  std::atomic<bool> futex_clock_realtime_unavailable;
>+}
>+
> namespace std _GLIBCXX_VISIBILITY(default)
> {
> _GLIBCXX_BEGIN_NAMESPACE_VERSION
>@@ -58,6 +66,32 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>       }
>     else
>       {
>+       if (!futex_clock_realtime_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_realtime_flag, __val, &rt, nullptr, futex_bitset_match_any) == -1)
>+             {
>+               _GLIBCXX_DEBUG_ASSERT(errno == EINTR || errno == EAGAIN
>+                                     || errno == ETIMEDOUT || errno == ENOSYS);

I've just replaced the use of _GLIBCXX_DEBUG_ASSERT in that file with
__glibcxx_assert, because the former is only used when Debug Mode is
active, and we'll never build libstdc++.so with Debug Mode.

The latter is enabled by -D_GLIBCXX_ASSERTIONS, which I've just added
to the default flags for the --enable-libstdcxx-debug option (which
builds a second copy of libstdc++.so built without optimisations).

I can make the corresponding changes to your patches, so this is just
FYI.

Re: [PATCHv3 0/6] std::future::wait_* improvements

[Jonathan Wakely]

On 01/08/18 20:49 +0100, Jonathan Wakely wrote:
>On 01/08/18 14:19 +0100, Mike Crowe wrote:
>>v2 of this series was originally posted back in January (see
>>https://gcc.gnu.org/ml/libstdc++/2018-01/msg00035.html )
>>
>>Apart from minor log message tweaks, the changes since that version are:
>>
>>* [1/6] Improve libstdc++-v3 async test
>>
>> Speed up the tests at the risk of more sporadic failures on loaded
>> machines.
>>
>> Use lambda rather than separate function for asynchronous routine.
>>
>>* [2/6] libstdc++ futex: Use FUTEX_CLOCK_REALTIME for wait
>>
>> Fall back to using gettimeofday and FUTEX_WAIT if FUTEX_WAIT_BITSET and
>> FUTEX_CLOCK_REALTIME are not available.
>>
>>* [3/6] libstdc++ futex: Support waiting on std::chrono::steady_clock directly
>>
>> Fall back to using clock_gettime (or the sycall directly if necessary)
>> and FUTEX_WAIT if FUTEX_WAIT_BITSET is unavailable.
>>
>>* [4/6] libstdc++ atomic_futex: Use std::chrono::steady_clock as reference clock
>>
>> No changes
>>
>>* [5/6] libstdc++ futex: Loop when waiting against arbitrary clock
>>
>> New patch. My work on std::condition_variable::wait_until made me realise
>> that there's a risk of indicating a timeout too early when using a
>> non-standard clock.
>>
>>* [6/6] Extra async tests, not for merging
>>
>> Use lambdas rather than separate functions for asynchronous routines.
>>
>>
>>Torvald Riegel had some objections to my design, but did not respond when I
>>attempted to justify it and attempted to change my implementation based on
>>his suggestions (see https://gcc.gnu.org/ml/libstdc++/2018-01/msg00071.html
>>.)
>>
>>It looks like https://gcc.gnu.org/bugzilla/show_bug.cgi?id=68519 could
>>apply equally well to __atomic_futex_unsigned::_M_load_when_equal_for.
>>I plan to look at that next.
>>
>>I set up a Debian 4.0 "Etch" system (v2.6.18 kernel, glibc 2.3.6.)
>>Unfortunately, its GCC 4.1.2 is unable to compile GCC master (as of
>>5ba044fc3a443274462527eed385732f7ecee3a8) because hash-map.h appears to be
>>trying to put a reference in a std::pair.)
>
>That's https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86739
>
>>The above patches cherry-pick
>>cleanly back to GCC 7.3, and that version does build after adding a few
>>constants to the system headers. I've confirmed that the lack of support
>>for FUTEX_WAIT_BITSET is detected correctly and that the code falls back to
>>using FUTEX_WAIT. This test also shows that the fallback to calling the
>>clock_gettime system call directly is working too. The async.cc tests all
>>passed.
>>
>>I haven't made any attempt to add entries to the .abilist files. I'm not
>>sure whether I'm supposed to do that as part of the patches, or not.
>
>Changes to gnu.ver linker script are needed, which should be in the patch.

This is the change needed for 'make check-abi' to pass:

--- a/libstdc++-v3/config/abi/pre/gnu.ver
+++ b/libstdc++-v3/config/abi/pre/gnu.ver
@@ -1902,10 +1902,9 @@ GLIBCXX_3.4.21 {
     _ZNSt7codecvtID[is]c*;
     _ZT[ISV]St7codecvtID[is]c*E;
 
-    extern "C++"
-    {
-      std::__atomic_futex_unsigned_base*;
-    };
+    # std::__atomic_futex_unsigned_base members
+    _ZNSt28__atomic_futex_unsigned_base19_M_futex_notify_all*;
+    _ZNSt28__atomic_futex_unsigned_base19_M_futex_wait_until*;
 
     # codecvt_utf8 etc.
     _ZNKSt19__codecvt_utf8_base*;
@@ -2044,6 +2043,9 @@ GLIBCXX_3.4.26 {
     _ZNSt3pmr20get_default_resourceEv;
     _ZNSt3pmr20set_default_resourceEPNS_15memory_resourceE;
 
+    # std::__atomic_futex_unsigned_base::_M_futex_wait_until_steady
+    _ZNSt28__atomic_futex_unsigned_base26_M_futex_wait_until_steady*;
+
 } GLIBCXX_3.4.25;
 
 # Symbols in the support library (libsupc++) have their own tag.



>Changes to the baseline_symbols.txt files should not be included in
>the patch, those are regenerated later.
>
>
>>
>>Mike Crowe (6):
>> Improve libstdc++-v3 async test
>> libstdc++ futex: Use FUTEX_CLOCK_REALTIME for wait
>> libstdc++ futex: Support waiting on std::chrono::steady_clock directly
>> libstdc++ atomic_futex: Use std::chrono::steady_clock as reference
>>   clock
>> libstdc++ futex: Loop when waiting against arbitrary clock
>> Extra async tests, not for merging
>
>Most of these patches fail to apply because they use spaces where the
>GCC code has tabs. I can fix it, but I'm not sure why the tabs aren't
>in your patches.
>
>
>
>

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

[Mike Crowe]

On Wednesday 01 August 2018 at 20:38:33 +0100, Jonathan Wakely wrote:
> On 01/08/18 14:19 +0100, Mike Crowe wrote:
> > 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?

Yes, but it's not one that the compiler can detect.

> 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.

I see no reason not to use the type system to help avoid mistakes in the
internal impementation of the library as well as in its external interface.
But, as you say, the function is only called from a few places so there's
little reason to change it now.

> > diff --git a/libstdc++-v3/src/c++11/futex.cc b/libstdc++-v3/src/c++11/futex.cc
[snip]
> > +       // We only get to here if futex_clock_realtime_unavailable was
> 
> This should say futex_clock_monotonic_unavailable.

Thanks. I've fixed that, but I'm not sure whether you want me to continue
to updating these patches or whether you plan to apply all the fixes
yourself before committing them.

Thanks.

Mike.