[patch] Fix shared_timed_mutex::try_lock_until() et al

[patch] Fix shared_timed_mutex::try_lock_until() et al

[Jonathan Wakely]

[-- Attachment #1: Type: text/plain, Size: 1419 bytes --]

When I implemented the C++14 type std::shared_timed_mutex (or
std::shared_mutex as it was originally called) I must have been half
asleep, because I completely messed up the timed locking functions,
try_lock_for, try_lock_until, try_shared_lock_for and
try_shared_lock_until. I wrote them to only use the timeout when
trying to lock the internal mutex object, but that is not the actual
lock, it just protects access to shared_timed_mutex::_M_state, so it
is only held for very short periods and is usually not heavily
contended. That means the internal lock gets taken immediately, but
then if the actual reader or writer lock can't be taken the functions
return immediately, instead of retrying for the specified time.

This fixes those functions to block on the condition variables until
the specified time, and adds a test to confirm that the timed
functions keep trying instead of returning immediately.

I've also cleaned the code up considerably, documenting the algorithm,
replacing explicit loops around condition variable waits with the form
of waiting that takes a predicate, and replacing lots of fiddly bit
twiddling with much simpler operations.

Tested x86_64-linux, powerpc64le-linux and x86_64-dragonfly3.6 with
hacked config to test the code changed in this patch as well as the
default pthread_rwlock_t implementation.

This only affects C++14, so I'd like to commit it to trunk and the 4.9
branch.


[-- Attachment #2: patch.txt --]
[-- Type: text/x-patch, Size: 11166 bytes --]

commit 9622a4b4672ba14cefec2ffae5cf0a2c91f5f5ae
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Thu Apr 2 19:50:50 2015 +0100

    	* include/std/shared_mutex (shared_timed_mutex): Add comments to
    	explain the logic.
    	(_M_n_readers): Rename to _S_n_readers.
    	(_M_write_entered, _M_readers): New convenience functions.
    	(lock, lock_shared, try_lock_shared, unlock_shared): Use convenience
    	functions. Use predicates with condition variables. Simplify bitwise
    	operations.
    	(try_lock_for, try_shared_lock_for): Convert duration to time_point
    	and call try_lock_until or try_shared_lock_until respectively.
    	(try_lock_until, try_shared_lock_until): Wait on the condition
    	variables until the specified time passes.
    	* testsuite/30_threads/shared_timed_mutex/try_lock/3.cc: New.

diff --git a/libstdc++-v3/include/std/shared_mutex b/libstdc++-v3/include/std/shared_mutex
index ab1b45b..7391f11 100644
--- a/libstdc++-v3/include/std/shared_mutex
+++ b/libstdc++-v3/include/std/shared_mutex
@@ -268,7 +268,12 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 
 #else // ! _GLIBCXX_USE_PTHREAD_RWLOCK_T
 
+    // Must use the same clock as condition_variable
+    typedef chrono::system_clock	__clock_t;
+
 #if _GTHREAD_USE_MUTEX_TIMEDLOCK
+    // Can't use std::timed_mutex with std::condition_variable, so define
+    // a new timed mutex type that derives from std::mutex.
     struct _Mutex : mutex, __timed_mutex_impl<_Mutex>
     {
       template<typename _Rep, typename _Period>
@@ -285,16 +290,44 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     typedef mutex _Mutex;
 #endif
 
-    // Based on Howard Hinnant's reference implementation from N2406
-
+    // Based on Howard Hinnant's reference implementation from N2406.
+
+    // The high bit of _M_state is the write-entered flag which is set to
+    // indicate a writer has taken the lock or is queuing to take the lock.
+    // The remaining bits are the count of reader locks.
+    //
+    // To take a reader lock block on gate1 while the write-entered flag is
+    // set or the maximum number of reader locks is held, then increment the
+    // reader lock count.
+    // To release decrement the count, then if the write-entered flag is set
+    // and the count is zero then signal gate2 to wake a queued writer,
+    // otherwise if the maximum number of reader locks was held signal gate1
+    // to wake a reader.
+    //
+    // To take a writer lock block on gate1 while the write-entered flag is
+    // set, then set the write-entered flag to start queueing, then block on
+    // gate2 while the number of reader locks is non-zero.
+    // To release unset the write-entered flag and signal gate1 to wake all
+    // blocked readers and writers.
+
+    // Only locked when accessing _M_state or waiting on condition variables.
     _Mutex		_M_mut;
+    // Used to block while write-entered is set or reader count at maximum.
     condition_variable	_M_gate1;
+    // Used to block queued writers while reader count is non-zero.
     condition_variable	_M_gate2;
+    // The write-entered flag and reader count.
     unsigned		_M_state;
 
     static constexpr unsigned _S_write_entered
       = 1U << (sizeof(unsigned)*__CHAR_BIT__ - 1);
-    static constexpr unsigned _M_n_readers = ~_S_write_entered;
+    static constexpr unsigned _S_n_readers = ~_S_write_entered;
+
+    // Test whether the write-entered flag is set. _M_mut must be locked.
+    bool _M_write_entered() const { return _M_state & _S_write_entered; }
+
+    // The number of reader locks currently held. _M_mut must be locked.
+    unsigned _M_readers() const { return _M_state & _S_n_readers; }
 
   public:
     shared_timed_mutex() : _M_state(0) {}
@@ -313,11 +346,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     lock()
     {
       unique_lock<mutex> __lk(_M_mut);
-      while (_M_state & _S_write_entered)
-	_M_gate1.wait(__lk);
+      _M_gate1.wait(__lk, [=]{ return !_M_write_entered(); });
       _M_state |= _S_write_entered;
-      while (_M_state & _M_n_readers)
-	_M_gate2.wait(__lk);
+      _M_gate2.wait(__lk, [=]{ return _M_readers() == 0; });
     }
 
     bool
@@ -337,26 +368,30 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       bool
       try_lock_for(const chrono::duration<_Rep, _Period>& __rel_time)
       {
-	unique_lock<_Mutex> __lk(_M_mut, __rel_time);
-	if (__lk.owns_lock() && _M_state == 0)
-	  {
-	    _M_state = _S_write_entered;
-	    return true;
-	  }
-	return false;
+	return try_lock_until(__clock_t::now() + __rel_time);
       }
 
     template<typename _Clock, typename _Duration>
       bool
       try_lock_until(const chrono::time_point<_Clock, _Duration>& __abs_time)
       {
-	unique_lock<_Mutex> __lk(_M_mut, __abs_time);
-	if (__lk.owns_lock() && _M_state == 0)
+	if (!_M_mut.try_lock_until(__abs_time))
+	  return false;
+	unique_lock<mutex> __lk(_M_mut, adopt_lock);
+	if (!_M_gate1.wait_until(__lk, __abs_time,
+				 [=]{ return !_M_write_entered(); }))
 	  {
-	    _M_state = _S_write_entered;
-	    return true;
+	    return false;
 	  }
-	return false;
+	_M_state |= _S_write_entered;
+	if (!_M_gate2.wait_until(__lk, __abs_time,
+				 [=]{ return _M_readers() == 0; }))
+	  {
+	    _M_state ^= _S_write_entered;
+	    _M_gate1.notify_all();
+	    return false;
+	  }
+	return true;
       }
 #endif
 
@@ -364,7 +399,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     unlock()
     {
       {
-	lock_guard<_Mutex> __lk(_M_mut);
+	lock_guard<mutex> __lk(_M_mut);
 	_M_state = 0;
       }
       _M_gate1.notify_all();
@@ -376,27 +411,19 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     lock_shared()
     {
       unique_lock<mutex> __lk(_M_mut);
-      while ((_M_state & _S_write_entered)
-	  || (_M_state & _M_n_readers) == _M_n_readers)
-	{
-	  _M_gate1.wait(__lk);
-	}
-      unsigned __num_readers = (_M_state & _M_n_readers) + 1;
-      _M_state &= ~_M_n_readers;
-      _M_state |= __num_readers;
+      _M_gate1.wait(__lk, [=]{ return _M_state < _S_n_readers; });
+      ++_M_state;
     }
 
     bool
     try_lock_shared()
     {
-      unique_lock<_Mutex> __lk(_M_mut, try_to_lock);
-      unsigned __num_readers = _M_state & _M_n_readers;
-      if (__lk.owns_lock() && !(_M_state & _S_write_entered)
-	  && __num_readers != _M_n_readers)
+      unique_lock<mutex> __lk(_M_mut, try_to_lock);
+      if (!__lk.owns_lock())
+	return false;
+      if (_M_state < _S_n_readers)
 	{
-	  ++__num_readers;
-	  _M_state &= ~_M_n_readers;
-	  _M_state |= __num_readers;
+	  ++_M_state;
 	  return true;
 	}
       return false;
@@ -407,20 +434,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       bool
       try_lock_shared_for(const chrono::duration<_Rep, _Period>& __rel_time)
       {
-	unique_lock<_Mutex> __lk(_M_mut, __rel_time);
-	if (__lk.owns_lock())
-	  {
-	    unsigned __num_readers = _M_state & _M_n_readers;
-	    if (!(_M_state & _S_write_entered)
-		&& __num_readers != _M_n_readers)
-	      {
-		++__num_readers;
-		_M_state &= ~_M_n_readers;
-		_M_state |= __num_readers;
-		return true;
-	      }
-	  }
-	return false;
+	return try_lock_shared_until(__clock_t::now() + __rel_time);
       }
 
     template <typename _Clock, typename _Duration>
@@ -428,38 +442,32 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       try_lock_shared_until(const chrono::time_point<_Clock,
 						     _Duration>& __abs_time)
       {
-	unique_lock<_Mutex> __lk(_M_mut, __abs_time);
-	if (__lk.owns_lock())
+	if (!_M_mut.try_lock_until(__abs_time))
+	  return false;
+	unique_lock<mutex> __lk(_M_mut, adopt_lock);
+	if (!_M_gate1.wait_until(__lk, __abs_time,
+				 [=]{ return _M_state < _S_n_readers; }))
 	  {
-	    unsigned __num_readers = _M_state & _M_n_readers;
-	    if (!(_M_state & _S_write_entered)
-		&& __num_readers != _M_n_readers)
-	      {
-		++__num_readers;
-		_M_state &= ~_M_n_readers;
-		_M_state |= __num_readers;
-		return true;
-	      }
+	    return false;
 	  }
-	return false;
+	++_M_state;
+	return true;
       }
 #endif
 
     void
     unlock_shared()
     {
-      lock_guard<_Mutex> __lk(_M_mut);
-      unsigned __num_readers = (_M_state & _M_n_readers) - 1;
-      _M_state &= ~_M_n_readers;
-      _M_state |= __num_readers;
-      if (_M_state & _S_write_entered)
+      lock_guard<mutex> __lk(_M_mut);
+      auto __prev = _M_state--;
+      if (_M_write_entered())
 	{
-	  if (__num_readers == 0)
+	  if (_M_readers() == 0)
 	    _M_gate2.notify_one();
 	}
       else
 	{
-	  if (__num_readers == _M_n_readers - 1)
+	  if (__prev == _S_n_readers)
 	    _M_gate1.notify_one();
 	}
     }
diff --git a/libstdc++-v3/testsuite/30_threads/shared_timed_mutex/try_lock/3.cc b/libstdc++-v3/testsuite/30_threads/shared_timed_mutex/try_lock/3.cc
new file mode 100644
index 0000000..e9f728e
--- /dev/null
+++ b/libstdc++-v3/testsuite/30_threads/shared_timed_mutex/try_lock/3.cc
@@ -0,0 +1,75 @@
+// { dg-do run { target *-*-freebsd* *-*-dragonfly* *-*-netbsd* *-*-linux* *-*-solaris* *-*-cygwin *-*-darwin* powerpc-ibm-aix* } }
+// { dg-options " -std=gnu++14 -pthread" { target *-*-freebsd* *-*-dragonfly* *-*-netbsd* *-*-linux* powerpc-ibm-aix* } }
+// { dg-options " -std=gnu++14 -pthreads" { target *-*-solaris* } }
+// { dg-options " -std=gnu++14 " { target *-*-cygwin *-*-darwin* } }
+// { dg-require-cstdint "" }
+// { dg-require-gthreads "" }
+
+// Copyright (C) 2013-2015 Free Software Foundation, Inc.
+//
+// This file is part of the GNU ISO C++ Library.  This library is free
+// software; you can redistribute it and/or modify it under the
+// terms of the GNU General Public License as published by the
+// Free Software Foundation; either version 3, or (at your option)
+// any later version.
+
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License along
+// with this library; see the file COPYING3.  If not see
+// <http://www.gnu.org/licenses/>.
+
+
+#include <shared_mutex>
+#include <thread>
+#include <system_error>
+#include <testsuite_hooks.h>
+
+int main()
+{
+  bool test __attribute__((unused)) = true;
+  typedef std::shared_timed_mutex mutex_type;
+
+  try
+    {
+      mutex_type m;
+      m.lock();
+      bool b;
+
+      std::thread t([&] {
+        try
+          {
+            using namespace std::chrono;
+            auto timeout = 100ms;
+            auto start = system_clock::now();
+            b = m.try_lock_for(timeout);
+            auto t = system_clock::now() - start;
+            VERIFY( !b );
+            VERIFY( t >= timeout );
+
+            start = system_clock::now();
+            b = m.try_lock_until(start + timeout);
+            t = system_clock::now() - start;
+            VERIFY( !b );
+            VERIFY( t >= timeout );
+          }
+        catch (const std::system_error& e)
+          {
+            VERIFY( false );
+          }
+      });
+      t.join();
+      m.unlock();
+    }
+  catch (const std::system_error& e)
+    {
+      VERIFY( false );
+    }
+  catch (...)
+    {
+      VERIFY( false );
+    }
+}

Re: [patch] Fix shared_timed_mutex::try_lock_until() et al

[Torvald Riegel]

There is an correctness issue related to mutex destruction.  The added
documentation is a good start, but I'd still add some more for the
complicated pieces of reasoning.  Details inline below.

On Tue, 2015-04-07 at 15:28 +0100, Jonathan Wakely wrote:
> diff --git a/libstdc++-v3/include/std/shared_mutex b/libstdc
> ++-v3/include/std/shared_mutex
> index ab1b45b..7391f11 100644
> --- a/libstdc++-v3/include/std/shared_mutex
> +++ b/libstdc++-v3/include/std/shared_mutex
> @@ -268,7 +268,12 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>  
>  #else // ! _GLIBCXX_USE_PTHREAD_RWLOCK_T
>  
> +    // Must use the same clock as condition_variable
> +    typedef chrono::system_clock       __clock_t;
> +
>  #if _GTHREAD_USE_MUTEX_TIMEDLOCK
> +    // Can't use std::timed_mutex with std::condition_variable, so
> define
> +    // a new timed mutex type that derives from std::mutex.
>      struct _Mutex : mutex, __timed_mutex_impl<_Mutex>
>      {
>        template<typename _Rep, typename _Period>
> @@ -285,16 +290,44 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>      typedef mutex _Mutex;
>  #endif
>  
> -    // Based on Howard Hinnant's reference implementation from N2406
> -
> +    // Based on Howard Hinnant's reference implementation from N2406.
> +
> +    // The high bit of _M_state is the write-entered flag which is
> set to
> +    // indicate a writer has taken the lock or is queuing to take the
> lock.
> +    // The remaining bits are the count of reader locks.
> +    //
> +    // To take a reader lock block on gate1 while the write-entered
> flag is

I think this is easier to parse if you add a comma: "To take a reader
lock, block ...".  Same for other sentences below.

> +    // set or the maximum number of reader locks is held, then
> increment the
> +    // reader lock count.
> +    // To release decrement the count, then if the write-entered flag
> is set
> +    // and the count is zero then signal gate2 to wake a queued
> writer,
> +    // otherwise if the maximum number of reader locks was held
> signal gate1
> +    // to wake a reader.
> +    //
> +    // To take a writer lock block on gate1 while the write-entered
> flag is
> +    // set, then set the write-entered flag to start queueing, then
> block on
> +    // gate2 while the number of reader locks is non-zero.
> +    // To release unset the write-entered flag and signal gate1 to
> wake all
> +    // blocked readers and writers.

Perhaps it would also be useful to have a sentence on how writers and
readers are prioritized (e.g., writers preferred if readers already hold
the lock, all are equal when writer unlocks, ...).

> +
> +    // Only locked when accessing _M_state or waiting on condition
> variables.
>      _Mutex             _M_mut;
> +    // Used to block while write-entered is set or reader count at
> maximum.
>      condition_variable _M_gate1;
> +    // Used to block queued writers while reader count is non-zero.
>      condition_variable _M_gate2;
> +    // The write-entered flag and reader count.
>      unsigned           _M_state;
>  
>      static constexpr unsigned _S_write_entered
>        = 1U << (sizeof(unsigned)*__CHAR_BIT__ - 1);
> -    static constexpr unsigned _M_n_readers = ~_S_write_entered;
> +    static constexpr unsigned _S_n_readers = ~_S_write_entered;

Rename this to _S_max_readers or such?

> +
> +    // Test whether the write-entered flag is set. _M_mut must be
> locked.
> +    bool _M_write_entered() const { return _M_state &
> _S_write_entered; }
> +
> +    // The number of reader locks currently held. _M_mut must be
> locked.
> +    unsigned _M_readers() const { return _M_state & _S_n_readers; }
>  
>    public:
>      shared_timed_mutex() : _M_state(0) {}
> @@ -313,11 +346,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>      lock()
>      {
>        unique_lock<mutex> __lk(_M_mut);
> -      while (_M_state & _S_write_entered)
> -       _M_gate1.wait(__lk);

Add something like this:?

// We first wait until we acquire the writer-part of the lock, and
// then wait until no readers hold the lock anymore.

> +      _M_gate1.wait(__lk, [=]{ return !_M_write_entered(); });
>        _M_state |= _S_write_entered;
> -      while (_M_state & _M_n_readers)
> -       _M_gate2.wait(__lk);
> +      _M_gate2.wait(__lk, [=]{ return _M_readers() == 0; });
>      }
>  
>      bool
> @@ -337,26 +368,30 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>        bool
>        try_lock_for(const chrono::duration<_Rep, _Period>& __rel_time)
>        {
> -       unique_lock<_Mutex> __lk(_M_mut, __rel_time);
> -       if (__lk.owns_lock() && _M_state == 0)
> -         {
> -           _M_state = _S_write_entered;
> -           return true;
> -         }
> -       return false;
> +       return try_lock_until(__clock_t::now() + __rel_time);
>        }
>  
>      template<typename _Clock, typename _Duration>
>        bool
>        try_lock_until(const chrono::time_point<_Clock, _Duration>&
> __abs_time)
>        {
> -       unique_lock<_Mutex> __lk(_M_mut, __abs_time);
> -       if (__lk.owns_lock() && _M_state == 0)
> +       if (!_M_mut.try_lock_until(__abs_time))
> +         return false;

I think a non-timed acquisition for the internal lock should be fine.
The internal lock is supposed to protect critical sections of finite
(and short) length, and the condvars also don't do acquisition with time
outs.

> +       unique_lock<mutex> __lk(_M_mut, adopt_lock);
> +       if (!_M_gate1.wait_until(__lk, __abs_time,
> +                                [=]{ return !_M_write_entered(); }))
>           {
> -           _M_state = _S_write_entered;
> -           return true;
> +           return false;
>           }
> -       return false;
> +       _M_state |= _S_write_entered;
> +       if (!_M_gate2.wait_until(__lk, __abs_time,
> +                                [=]{ return _M_readers() == 0; }))
> +         {

I'd add a comment saying that you have to mimic a full write unlock, so
that's why those steps are necessary.

> +           _M_state ^= _S_write_entered;
> +           _M_gate1.notify_all();
> +           return false;
> +         }
> +       return true;
>        }
>  #endif
>  
> @@ -364,7 +399,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>      unlock()
>      {
>        {
> -       lock_guard<_Mutex> __lk(_M_mut);
> +       lock_guard<mutex> __lk(_M_mut);
>         _M_state = 0;
>        }
>        _M_gate1.notify_all();

The notification must be *inside* of the critical section.  Otherwise,
you're violating the mutex destruction requirements (see 30.4.1.3.1p3):
After you set _M_state to 0 and release _M_mut, another thread can go
it, acquire, assume it's the last one to own the mutex if that's what
the program ensures, and destroy; then, destruction would be concurrent
with the call to notify_all.  (Perhaps check other wake-ups / unlocks as
well.)

> @@ -376,27 +411,19 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>      lock_shared()
>      {
>        unique_lock<mutex> __lk(_M_mut);
> -      while ((_M_state & _S_write_entered)
> -         || (_M_state & _M_n_readers) == _M_n_readers)
> -       {
> -         _M_gate1.wait(__lk);
> -       }
> -      unsigned __num_readers = (_M_state & _M_n_readers) + 1;
> -      _M_state &= ~_M_n_readers;
> -      _M_state |= __num_readers;
> +      _M_gate1.wait(__lk, [=]{ return _M_state < _S_n_readers; });
> +      ++_M_state;
>      }
>  
>      bool
>      try_lock_shared()
>      {
> -      unique_lock<_Mutex> __lk(_M_mut, try_to_lock);
> -      unsigned __num_readers = _M_state & _M_n_readers;
> -      if (__lk.owns_lock() && !(_M_state & _S_write_entered)
> -         && __num_readers != _M_n_readers)
> +      unique_lock<mutex> __lk(_M_mut, try_to_lock);
> +      if (!__lk.owns_lock())
> +       return false;
> +      if (_M_state < _S_n_readers)
>         {
> -         ++__num_readers;
> -         _M_state &= ~_M_n_readers;
> -         _M_state |= __num_readers;
> +         ++_M_state;
>           return true;
>         }
>        return false;
> @@ -407,20 +434,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>        bool
>        try_lock_shared_for(const chrono::duration<_Rep, _Period>&
> __rel_time)
>        {
> -       unique_lock<_Mutex> __lk(_M_mut, __rel_time);
> -       if (__lk.owns_lock())
> -         {
> -           unsigned __num_readers = _M_state & _M_n_readers;
> -           if (!(_M_state & _S_write_entered)
> -               && __num_readers != _M_n_readers)
> -             {
> -               ++__num_readers;
> -               _M_state &= ~_M_n_readers;
> -               _M_state |= __num_readers;
> -               return true;
> -             }
> -         }
> -       return false;
> +       return try_lock_shared_until(__clock_t::now() + __rel_time);
>        }
>  
>      template <typename _Clock, typename _Duration>
> @@ -428,38 +442,32 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>        try_lock_shared_until(const chrono::time_point<_Clock,
>                                                      _Duration>&
> __abs_time)
>        {
> -       unique_lock<_Mutex> __lk(_M_mut, __abs_time);
> -       if (__lk.owns_lock())
> +       if (!_M_mut.try_lock_until(__abs_time))
> +         return false;

See above.

> +       unique_lock<mutex> __lk(_M_mut, adopt_lock);
> +       if (!_M_gate1.wait_until(__lk, __abs_time,
> +                                [=]{ return _M_state <
> _S_n_readers; }))
>           {
> -           unsigned __num_readers = _M_state & _M_n_readers;
> -           if (!(_M_state & _S_write_entered)
> -               && __num_readers != _M_n_readers)
> -             {
> -               ++__num_readers;
> -               _M_state &= ~_M_n_readers;
> -               _M_state |= __num_readers;
> -               return true;
> -             }
> +           return false;
>           }
> -       return false;
> +       ++_M_state;
> +       return true;
>        }
>  #endif
>  
>      void
>      unlock_shared()
>      {
> -      lock_guard<_Mutex> __lk(_M_mut);
> -      unsigned __num_readers = (_M_state & _M_n_readers) - 1;
> -      _M_state &= ~_M_n_readers;
> -      _M_state |= __num_readers;
> -      if (_M_state & _S_write_entered)
> +      lock_guard<mutex> __lk(_M_mut);
> +      auto __prev = _M_state--;
> +      if (_M_write_entered())
>         {
> -         if (__num_readers == 0)
> +         if (_M_readers() == 0)
>             _M_gate2.notify_one();
>         }
>        else
>         {
> -         if (__num_readers == _M_n_readers - 1)
> +         if (__prev == _S_n_readers)
>             _M_gate1.notify_one();
>         }

I think this needs documentation why we can't miss a wake-up in case
_M_write_entered is true and we have a reader overflow.  I think it's
not quite obvious why this works.
If there is a writer we don't notify gate1 in unlock_shared(), so there
is potential to "loose" that information in interleavings with a writer.
If the writer acquires the lock eventually, things will work because
then, the writer will do the gate1 notification in its unlock path.  If
the writer times out, it still does the gate1 notification regardless of
the the number of readers registered (because if there is no reader, it
will never time out but simply acquire the lock).  Thus, when there is a
writer, the writer becomes responsible for gate1 notification, and
things work.

Re: [patch] Fix shared_timed_mutex::try_lock_until() et al

[Jonathan Wakely]

[-- Attachment #1: Type: text/plain, Size: 6089 bytes --]

On 08/04/15 18:59 +0200, Torvald Riegel wrote:
>> +    // set or the maximum number of reader locks is held, then
>> increment the
>> +    // reader lock count.
>> +    // To release decrement the count, then if the write-entered flag
>> is set
>> +    // and the count is zero then signal gate2 to wake a queued
>> writer,
>> +    // otherwise if the maximum number of reader locks was held
>> signal gate1
>> +    // to wake a reader.
>> +    //
>> +    // To take a writer lock block on gate1 while the write-entered
>> flag is
>> +    // set, then set the write-entered flag to start queueing, then
>> block on
>> +    // gate2 while the number of reader locks is non-zero.
>> +    // To release unset the write-entered flag and signal gate1 to
>> wake all
>> +    // blocked readers and writers.
>
>Perhaps it would also be useful to have a sentence on how writers and
>readers are prioritized (e.g., writers preferred if readers already hold
>the lock, all are equal when writer unlocks, ...).

How about:

// This means that when no reader locks are held readers and writers get
// equal priority. When one or more reader locks is held a writer gets
// priority and no more reader locks can be taken while the writer is
// queued.

>> +    static constexpr unsigned _S_n_readers = ~_S_write_entered;
>
>Rename this to _S_max_readers or such?

Done.

>>      template<typename _Clock, typename _Duration>
>>        bool
>>        try_lock_until(const chrono::time_point<_Clock, _Duration>&
>> __abs_time)
>>        {
>> -       unique_lock<_Mutex> __lk(_M_mut, __abs_time);
>> -       if (__lk.owns_lock() && _M_state == 0)
>> +       if (!_M_mut.try_lock_until(__abs_time))
>> +         return false;
>
>I think a non-timed acquisition for the internal lock should be fine.
>The internal lock is supposed to protect critical sections of finite
>(and short) length, and the condvars also don't do acquisition with time
>outs.

Good point about the condvars re-acquiring the mutex.

We can get rid of the _Mutex type then, and just use std::mutex, and
that also means we can provide the timed locking functions even when
!defined(_GTHREAD_USE_MUTEX_TIMEDLOCK).

And so maybe we should use this fallback implementation instead of the
pthread_rwlock_t one when !defined(_GTHREAD_USE_MUTEX_TIMEDLOCK), so
that they have a complete std::shared_timed_mutex (this applies to at
least Darwin, not sure which other targets).

Maybe we should also provide a fallback std::timed_mutex based on a
condvar, I'll put that on the TODO list for stage 1.

>> +       unique_lock<mutex> __lk(_M_mut, adopt_lock);
>> +       if (!_M_gate1.wait_until(__lk, __abs_time,
>> +                                [=]{ return !_M_write_entered(); }))
>>           {
>> -           _M_state = _S_write_entered;
>> -           return true;
>> +           return false;
>>           }
>> -       return false;
>> +       _M_state |= _S_write_entered;
>> +       if (!_M_gate2.wait_until(__lk, __abs_time,
>> +                                [=]{ return _M_readers() == 0; }))
>> +         {
>
>I'd add a comment saying that you have to mimic a full write unlock, so
>that's why those steps are necessary.

OK, like this:

// Wake all threads blocked while the write-entered flag was set.

>> +           _M_state ^= _S_write_entered;
>> +           _M_gate1.notify_all();
>> +           return false;
>> +         }
>> +       return true;
>>        }
>>  #endif
>>
>> @@ -364,7 +399,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>>      unlock()
>>      {
>>        {
>> -       lock_guard<_Mutex> __lk(_M_mut);
>> +       lock_guard<mutex> __lk(_M_mut);
>>         _M_state = 0;
>>        }
>>        _M_gate1.notify_all();
>
>The notification must be *inside* of the critical section.  Otherwise,
>you're violating the mutex destruction requirements (see 30.4.1.3.1p3):
>After you set _M_state to 0 and release _M_mut, another thread can go
>it, acquire, assume it's the last one to own the mutex if that's what
>the program ensures, and destroy; then, destruction would be concurrent
>with the call to notify_all.  (Perhaps check other wake-ups / unlocks as
>well.)

Thanks. I'll add this comment too:

// call notify_all() while mutex is held so that another thread can't
// lock and unlock the mutex then destroy *this before we make the call.

>>      void
>>      unlock_shared()
>>      {
>> -      lock_guard<_Mutex> __lk(_M_mut);
>> -      unsigned __num_readers = (_M_state & _M_n_readers) - 1;
>> -      _M_state &= ~_M_n_readers;
>> -      _M_state |= __num_readers;
>> -      if (_M_state & _S_write_entered)
>> +      lock_guard<mutex> __lk(_M_mut);
>> +      auto __prev = _M_state--;
>> +      if (_M_write_entered())
>>         {
>> -         if (__num_readers == 0)
>> +         if (_M_readers() == 0)
>>             _M_gate2.notify_one();
>>         }
>>        else
>>         {
>> -         if (__num_readers == _M_n_readers - 1)
>> +         if (__prev == _S_n_readers)
>>             _M_gate1.notify_one();
>>         }
>
>I think this needs documentation why we can't miss a wake-up in case
>_M_write_entered is true and we have a reader overflow.  I think it's
>not quite obvious why this works.
>If there is a writer we don't notify gate1 in unlock_shared(), so there
>is potential to "loose" that information in interleavings with a writer.
>If the writer acquires the lock eventually, things will work because
>then, the writer will do the gate1 notification in its unlock path.  If
>the writer times out, it still does the gate1 notification regardless of
>the the number of readers registered (because if there is no reader, it
>will never time out but simply acquire the lock).  Thus, when there is a
>writer, the writer becomes responsible for gate1 notification, and
>things work.

Is this sufficient?

// No need to notify gate1 because we give priority to the queued
// writer, and that writer will eventually notify gate1 after it
// clears the write-entered flag.


I've attached a diff showing just the changes since the last patch, as
well as the complete patch against the trunk.


[-- Attachment #2: incremental.txt --]
[-- Type: text/plain, Size: 8396 bytes --]

diff --git a/libstdc++-v3/ChangeLog b/libstdc++-v3/ChangeLog
index 5871716..39d6866 100644
--- a/libstdc++-v3/ChangeLog
+++ b/libstdc++-v3/ChangeLog
@@ -2,7 +2,8 @@
 
 	* include/std/shared_mutex (shared_timed_mutex): Add comments to
 	explain the logic.
-	(_M_n_readers): Rename to _S_n_readers.
+	(_Mutex): Remove redundant type.
+	(_M_n_readers): Rename to _S_max_readers.
 	(_M_write_entered, _M_readers): New convenience functions.
 	(lock, lock_shared, try_lock_shared, unlock_shared): Use convenience
 	functions. Use predicates with condition variables. Simplify bitwise
@@ -11,7 +12,8 @@
 	and call try_lock_until or try_shared_lock_until respectively.
 	(try_lock_until, try_shared_lock_until): Wait on the condition
 	variables until the specified time passes.
-	(unlock, unlock_shared): Add Debug Mode assertions.
+	(unlock): Add Debug Mode assertion.
+	(unlock_shared): Add Debug Mode assertion.
 	* testsuite/30_threads/shared_timed_mutex/try_lock/3.cc: New.
 
 2015-03-30  Jonathan Wakely  <jwakely@redhat.com>
diff --git a/libstdc++-v3/include/std/shared_mutex b/libstdc++-v3/include/std/shared_mutex
index feb5022..7f26465 100644
--- a/libstdc++-v3/include/std/shared_mutex
+++ b/libstdc++-v3/include/std/shared_mutex
@@ -271,47 +271,33 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     // Must use the same clock as condition_variable
     typedef chrono::system_clock	__clock_t;
 
-#if _GTHREAD_USE_MUTEX_TIMEDLOCK
-    // Can't use std::timed_mutex with std::condition_variable, so define
-    // a new timed mutex type that derives from std::mutex.
-    struct _Mutex : mutex, __timed_mutex_impl<_Mutex>
-    {
-      template<typename _Rep, typename _Period>
-	bool
-	try_lock_for(const chrono::duration<_Rep, _Period>& __rtime)
-	{ return _M_try_lock_for(__rtime); }
-
-      template<typename _Clock, typename _Duration>
-	bool
-	try_lock_until(const chrono::time_point<_Clock, _Duration>& __atime)
-	{ return _M_try_lock_until(__atime); }
-    };
-#else
-    typedef mutex _Mutex;
-#endif
-
     // Based on Howard Hinnant's reference implementation from N2406.
 
     // The high bit of _M_state is the write-entered flag which is set to
     // indicate a writer has taken the lock or is queuing to take the lock.
     // The remaining bits are the count of reader locks.
     //
-    // To take a reader lock block on gate1 while the write-entered flag is
+    // To take a reader lock, block on gate1 while the write-entered flag is
     // set or the maximum number of reader locks is held, then increment the
     // reader lock count.
-    // To release decrement the count, then if the write-entered flag is set
+    // To release, decrement the count, then if the write-entered flag is set
     // and the count is zero then signal gate2 to wake a queued writer,
     // otherwise if the maximum number of reader locks was held signal gate1
     // to wake a reader.
     //
-    // To take a writer lock block on gate1 while the write-entered flag is
+    // To take a writer lock, block on gate1 while the write-entered flag is
     // set, then set the write-entered flag to start queueing, then block on
     // gate2 while the number of reader locks is non-zero.
-    // To release unset the write-entered flag and signal gate1 to wake all
+    // To release, unset the write-entered flag and signal gate1 to wake all
     // blocked readers and writers.
+    //
+    // This means that when no reader locks are held readers and writers get
+    // equal priority. When one or more reader locks is held a writer gets
+    // priority and no more reader locks can be taken while the writer is
+    // queued.
 
     // Only locked when accessing _M_state or waiting on condition variables.
-    _Mutex		_M_mut;
+    mutex		_M_mut;
     // Used to block while write-entered is set or reader count at maximum.
     condition_variable	_M_gate1;
     // Used to block queued writers while reader count is non-zero.
@@ -321,13 +307,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 
     static constexpr unsigned _S_write_entered
       = 1U << (sizeof(unsigned)*__CHAR_BIT__ - 1);
-    static constexpr unsigned _S_n_readers = ~_S_write_entered;
+    static constexpr unsigned _S_max_readers = ~_S_write_entered;
 
     // Test whether the write-entered flag is set. _M_mut must be locked.
     bool _M_write_entered() const { return _M_state & _S_write_entered; }
 
     // The number of reader locks currently held. _M_mut must be locked.
-    unsigned _M_readers() const { return _M_state & _S_n_readers; }
+    unsigned _M_readers() const { return _M_state & _S_max_readers; }
 
   public:
     shared_timed_mutex() : _M_state(0) {}
@@ -346,8 +332,10 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     lock()
     {
       unique_lock<mutex> __lk(_M_mut);
+      // Wait until we can set the write-entered flag.
       _M_gate1.wait(__lk, [=]{ return !_M_write_entered(); });
       _M_state |= _S_write_entered;
+      // Then wait until there are no more readers.
       _M_gate2.wait(__lk, [=]{ return _M_readers() == 0; });
     }
 
@@ -363,7 +351,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       return false;
     }
 
-#if _GTHREAD_USE_MUTEX_TIMEDLOCK
     template<typename _Rep, typename _Period>
       bool
       try_lock_for(const chrono::duration<_Rep, _Period>& __rel_time)
@@ -375,9 +362,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       bool
       try_lock_until(const chrono::time_point<_Clock, _Duration>& __abs_time)
       {
-	if (!_M_mut.try_lock_until(__abs_time))
-	  return false;
-	unique_lock<mutex> __lk(_M_mut, adopt_lock);
+	unique_lock<mutex> __lk(_M_mut);
 	if (!_M_gate1.wait_until(__lk, __abs_time,
 				 [=]{ return !_M_write_entered(); }))
 	  {
@@ -388,21 +373,21 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 				 [=]{ return _M_readers() == 0; }))
 	  {
 	    _M_state ^= _S_write_entered;
+	    // Wake all threads blocked while the write-entered flag was set.
 	    _M_gate1.notify_all();
 	    return false;
 	  }
 	return true;
       }
-#endif
 
     void
     unlock()
     {
-      {
-	lock_guard<mutex> __lk(_M_mut);
-	_GLIBCXX_DEBUG_ASSERT( _M_write_entered() );
-	_M_state = 0;
-      }
+      lock_guard<mutex> __lk(_M_mut);
+      _GLIBCXX_DEBUG_ASSERT( _M_write_entered() );
+      _M_state = 0;
+      // call notify_all() while mutex is held so that another thread can't
+      // lock and unlock the mutex then destroy *this before we make the call.
       _M_gate1.notify_all();
     }
 
@@ -412,7 +397,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     lock_shared()
     {
       unique_lock<mutex> __lk(_M_mut);
-      _M_gate1.wait(__lk, [=]{ return _M_state < _S_n_readers; });
+      _M_gate1.wait(__lk, [=]{ return _M_state < _S_max_readers; });
       ++_M_state;
     }
 
@@ -422,7 +407,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       unique_lock<mutex> __lk(_M_mut, try_to_lock);
       if (!__lk.owns_lock())
 	return false;
-      if (_M_state < _S_n_readers)
+      if (_M_state < _S_max_readers)
 	{
 	  ++_M_state;
 	  return true;
@@ -430,7 +415,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       return false;
     }
 
-#if _GTHREAD_USE_MUTEX_TIMEDLOCK
     template<typename _Rep, typename _Period>
       bool
       try_lock_shared_for(const chrono::duration<_Rep, _Period>& __rel_time)
@@ -443,18 +427,15 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       try_lock_shared_until(const chrono::time_point<_Clock,
 						     _Duration>& __abs_time)
       {
-	if (!_M_mut.try_lock_until(__abs_time))
-	  return false;
-	unique_lock<mutex> __lk(_M_mut, adopt_lock);
+	unique_lock<mutex> __lk(_M_mut);
 	if (!_M_gate1.wait_until(__lk, __abs_time,
-				 [=]{ return _M_state < _S_n_readers; }))
+				 [=]{ return _M_state < _S_max_readers; }))
 	  {
 	    return false;
 	  }
 	++_M_state;
 	return true;
       }
-#endif
 
     void
     unlock_shared()
@@ -464,12 +445,17 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       auto __prev = _M_state--;
       if (_M_write_entered())
 	{
+	  // Wake the queued writer if there are no more readers.
 	  if (_M_readers() == 0)
 	    _M_gate2.notify_one();
+	  // No need to notify gate1 because we give priority to the queued
+	  // writer, and that writer will eventually notify gate1 after it
+	  // clears the write-entered flag.
 	}
       else
 	{
-	  if (__prev == _S_n_readers)
+	  // Wake any thread that was blocked on reader overflow.
+	  if (__prev == _S_max_readers)
 	    _M_gate1.notify_one();
 	}
     }

[-- Attachment #3: patch.txt --]
[-- Type: text/x-patch, Size: 12423 bytes --]

commit 11422fedbc04016a519e58e4c965fadbaef7851e
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Thu Apr 2 19:50:50 2015 +0100

    	* include/std/shared_mutex (shared_timed_mutex): Add comments to
    	explain the logic.
    	(_Mutex): Remove redundant type.
    	(_M_n_readers): Rename to _S_max_readers.
    	(_M_write_entered, _M_readers): New convenience functions.
    	(lock, lock_shared, try_lock_shared, unlock_shared): Use convenience
    	functions. Use predicates with condition variables. Simplify bitwise
    	operations.
    	(try_lock_for, try_shared_lock_for): Convert duration to time_point
    	and call try_lock_until or try_shared_lock_until respectively.
    	(try_lock_until, try_shared_lock_until): Wait on the condition
    	variables until the specified time passes.
    	(unlock): Add Debug Mode assertion.
    	(unlock_shared): Add Debug Mode assertion.
    	* testsuite/30_threads/shared_timed_mutex/try_lock/3.cc: New.

diff --git a/libstdc++-v3/include/std/shared_mutex b/libstdc++-v3/include/std/shared_mutex
index ab1b45b..7f26465 100644
--- a/libstdc++-v3/include/std/shared_mutex
+++ b/libstdc++-v3/include/std/shared_mutex
@@ -268,33 +268,52 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 
 #else // ! _GLIBCXX_USE_PTHREAD_RWLOCK_T
 
-#if _GTHREAD_USE_MUTEX_TIMEDLOCK
-    struct _Mutex : mutex, __timed_mutex_impl<_Mutex>
-    {
-      template<typename _Rep, typename _Period>
-	bool
-	try_lock_for(const chrono::duration<_Rep, _Period>& __rtime)
-	{ return _M_try_lock_for(__rtime); }
-
-      template<typename _Clock, typename _Duration>
-	bool
-	try_lock_until(const chrono::time_point<_Clock, _Duration>& __atime)
-	{ return _M_try_lock_until(__atime); }
-    };
-#else
-    typedef mutex _Mutex;
-#endif
-
-    // Based on Howard Hinnant's reference implementation from N2406
+    // Must use the same clock as condition_variable
+    typedef chrono::system_clock	__clock_t;
 
-    _Mutex		_M_mut;
+    // Based on Howard Hinnant's reference implementation from N2406.
+
+    // The high bit of _M_state is the write-entered flag which is set to
+    // indicate a writer has taken the lock or is queuing to take the lock.
+    // The remaining bits are the count of reader locks.
+    //
+    // To take a reader lock, block on gate1 while the write-entered flag is
+    // set or the maximum number of reader locks is held, then increment the
+    // reader lock count.
+    // To release, decrement the count, then if the write-entered flag is set
+    // and the count is zero then signal gate2 to wake a queued writer,
+    // otherwise if the maximum number of reader locks was held signal gate1
+    // to wake a reader.
+    //
+    // To take a writer lock, block on gate1 while the write-entered flag is
+    // set, then set the write-entered flag to start queueing, then block on
+    // gate2 while the number of reader locks is non-zero.
+    // To release, unset the write-entered flag and signal gate1 to wake all
+    // blocked readers and writers.
+    //
+    // This means that when no reader locks are held readers and writers get
+    // equal priority. When one or more reader locks is held a writer gets
+    // priority and no more reader locks can be taken while the writer is
+    // queued.
+
+    // Only locked when accessing _M_state or waiting on condition variables.
+    mutex		_M_mut;
+    // Used to block while write-entered is set or reader count at maximum.
     condition_variable	_M_gate1;
+    // Used to block queued writers while reader count is non-zero.
     condition_variable	_M_gate2;
+    // The write-entered flag and reader count.
     unsigned		_M_state;
 
     static constexpr unsigned _S_write_entered
       = 1U << (sizeof(unsigned)*__CHAR_BIT__ - 1);
-    static constexpr unsigned _M_n_readers = ~_S_write_entered;
+    static constexpr unsigned _S_max_readers = ~_S_write_entered;
+
+    // Test whether the write-entered flag is set. _M_mut must be locked.
+    bool _M_write_entered() const { return _M_state & _S_write_entered; }
+
+    // The number of reader locks currently held. _M_mut must be locked.
+    unsigned _M_readers() const { return _M_state & _S_max_readers; }
 
   public:
     shared_timed_mutex() : _M_state(0) {}
@@ -313,11 +332,11 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     lock()
     {
       unique_lock<mutex> __lk(_M_mut);
-      while (_M_state & _S_write_entered)
-	_M_gate1.wait(__lk);
+      // Wait until we can set the write-entered flag.
+      _M_gate1.wait(__lk, [=]{ return !_M_write_entered(); });
       _M_state |= _S_write_entered;
-      while (_M_state & _M_n_readers)
-	_M_gate2.wait(__lk);
+      // Then wait until there are no more readers.
+      _M_gate2.wait(__lk, [=]{ return _M_readers() == 0; });
     }
 
     bool
@@ -332,41 +351,43 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       return false;
     }
 
-#if _GTHREAD_USE_MUTEX_TIMEDLOCK
     template<typename _Rep, typename _Period>
       bool
       try_lock_for(const chrono::duration<_Rep, _Period>& __rel_time)
       {
-	unique_lock<_Mutex> __lk(_M_mut, __rel_time);
-	if (__lk.owns_lock() && _M_state == 0)
-	  {
-	    _M_state = _S_write_entered;
-	    return true;
-	  }
-	return false;
+	return try_lock_until(__clock_t::now() + __rel_time);
       }
 
     template<typename _Clock, typename _Duration>
       bool
       try_lock_until(const chrono::time_point<_Clock, _Duration>& __abs_time)
       {
-	unique_lock<_Mutex> __lk(_M_mut, __abs_time);
-	if (__lk.owns_lock() && _M_state == 0)
+	unique_lock<mutex> __lk(_M_mut);
+	if (!_M_gate1.wait_until(__lk, __abs_time,
+				 [=]{ return !_M_write_entered(); }))
 	  {
-	    _M_state = _S_write_entered;
-	    return true;
+	    return false;
 	  }
-	return false;
+	_M_state |= _S_write_entered;
+	if (!_M_gate2.wait_until(__lk, __abs_time,
+				 [=]{ return _M_readers() == 0; }))
+	  {
+	    _M_state ^= _S_write_entered;
+	    // Wake all threads blocked while the write-entered flag was set.
+	    _M_gate1.notify_all();
+	    return false;
+	  }
+	return true;
       }
-#endif
 
     void
     unlock()
     {
-      {
-	lock_guard<_Mutex> __lk(_M_mut);
-	_M_state = 0;
-      }
+      lock_guard<mutex> __lk(_M_mut);
+      _GLIBCXX_DEBUG_ASSERT( _M_write_entered() );
+      _M_state = 0;
+      // call notify_all() while mutex is held so that another thread can't
+      // lock and unlock the mutex then destroy *this before we make the call.
       _M_gate1.notify_all();
     }
 
@@ -376,51 +397,29 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     lock_shared()
     {
       unique_lock<mutex> __lk(_M_mut);
-      while ((_M_state & _S_write_entered)
-	  || (_M_state & _M_n_readers) == _M_n_readers)
-	{
-	  _M_gate1.wait(__lk);
-	}
-      unsigned __num_readers = (_M_state & _M_n_readers) + 1;
-      _M_state &= ~_M_n_readers;
-      _M_state |= __num_readers;
+      _M_gate1.wait(__lk, [=]{ return _M_state < _S_max_readers; });
+      ++_M_state;
     }
 
     bool
     try_lock_shared()
     {
-      unique_lock<_Mutex> __lk(_M_mut, try_to_lock);
-      unsigned __num_readers = _M_state & _M_n_readers;
-      if (__lk.owns_lock() && !(_M_state & _S_write_entered)
-	  && __num_readers != _M_n_readers)
+      unique_lock<mutex> __lk(_M_mut, try_to_lock);
+      if (!__lk.owns_lock())
+	return false;
+      if (_M_state < _S_max_readers)
 	{
-	  ++__num_readers;
-	  _M_state &= ~_M_n_readers;
-	  _M_state |= __num_readers;
+	  ++_M_state;
 	  return true;
 	}
       return false;
     }
 
-#if _GTHREAD_USE_MUTEX_TIMEDLOCK
     template<typename _Rep, typename _Period>
       bool
       try_lock_shared_for(const chrono::duration<_Rep, _Period>& __rel_time)
       {
-	unique_lock<_Mutex> __lk(_M_mut, __rel_time);
-	if (__lk.owns_lock())
-	  {
-	    unsigned __num_readers = _M_state & _M_n_readers;
-	    if (!(_M_state & _S_write_entered)
-		&& __num_readers != _M_n_readers)
-	      {
-		++__num_readers;
-		_M_state &= ~_M_n_readers;
-		_M_state |= __num_readers;
-		return true;
-	      }
-	  }
-	return false;
+	return try_lock_shared_until(__clock_t::now() + __rel_time);
       }
 
     template <typename _Clock, typename _Duration>
@@ -428,38 +427,35 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       try_lock_shared_until(const chrono::time_point<_Clock,
 						     _Duration>& __abs_time)
       {
-	unique_lock<_Mutex> __lk(_M_mut, __abs_time);
-	if (__lk.owns_lock())
+	unique_lock<mutex> __lk(_M_mut);
+	if (!_M_gate1.wait_until(__lk, __abs_time,
+				 [=]{ return _M_state < _S_max_readers; }))
 	  {
-	    unsigned __num_readers = _M_state & _M_n_readers;
-	    if (!(_M_state & _S_write_entered)
-		&& __num_readers != _M_n_readers)
-	      {
-		++__num_readers;
-		_M_state &= ~_M_n_readers;
-		_M_state |= __num_readers;
-		return true;
-	      }
+	    return false;
 	  }
-	return false;
+	++_M_state;
+	return true;
       }
-#endif
 
     void
     unlock_shared()
     {
-      lock_guard<_Mutex> __lk(_M_mut);
-      unsigned __num_readers = (_M_state & _M_n_readers) - 1;
-      _M_state &= ~_M_n_readers;
-      _M_state |= __num_readers;
-      if (_M_state & _S_write_entered)
+      lock_guard<mutex> __lk(_M_mut);
+      _GLIBCXX_DEBUG_ASSERT( _M_readers() > 0 );
+      auto __prev = _M_state--;
+      if (_M_write_entered())
 	{
-	  if (__num_readers == 0)
+	  // Wake the queued writer if there are no more readers.
+	  if (_M_readers() == 0)
 	    _M_gate2.notify_one();
+	  // No need to notify gate1 because we give priority to the queued
+	  // writer, and that writer will eventually notify gate1 after it
+	  // clears the write-entered flag.
 	}
       else
 	{
-	  if (__num_readers == _M_n_readers - 1)
+	  // Wake any thread that was blocked on reader overflow.
+	  if (__prev == _S_max_readers)
 	    _M_gate1.notify_one();
 	}
     }
diff --git a/libstdc++-v3/testsuite/30_threads/shared_timed_mutex/try_lock/3.cc b/libstdc++-v3/testsuite/30_threads/shared_timed_mutex/try_lock/3.cc
new file mode 100644
index 0000000..e9f728e
--- /dev/null
+++ b/libstdc++-v3/testsuite/30_threads/shared_timed_mutex/try_lock/3.cc
@@ -0,0 +1,75 @@
+// { dg-do run { target *-*-freebsd* *-*-dragonfly* *-*-netbsd* *-*-linux* *-*-solaris* *-*-cygwin *-*-darwin* powerpc-ibm-aix* } }
+// { dg-options " -std=gnu++14 -pthread" { target *-*-freebsd* *-*-dragonfly* *-*-netbsd* *-*-linux* powerpc-ibm-aix* } }
+// { dg-options " -std=gnu++14 -pthreads" { target *-*-solaris* } }
+// { dg-options " -std=gnu++14 " { target *-*-cygwin *-*-darwin* } }
+// { dg-require-cstdint "" }
+// { dg-require-gthreads "" }
+
+// Copyright (C) 2013-2015 Free Software Foundation, Inc.
+//
+// This file is part of the GNU ISO C++ Library.  This library is free
+// software; you can redistribute it and/or modify it under the
+// terms of the GNU General Public License as published by the
+// Free Software Foundation; either version 3, or (at your option)
+// any later version.
+
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License along
+// with this library; see the file COPYING3.  If not see
+// <http://www.gnu.org/licenses/>.
+
+
+#include <shared_mutex>
+#include <thread>
+#include <system_error>
+#include <testsuite_hooks.h>
+
+int main()
+{
+  bool test __attribute__((unused)) = true;
+  typedef std::shared_timed_mutex mutex_type;
+
+  try
+    {
+      mutex_type m;
+      m.lock();
+      bool b;
+
+      std::thread t([&] {
+        try
+          {
+            using namespace std::chrono;
+            auto timeout = 100ms;
+            auto start = system_clock::now();
+            b = m.try_lock_for(timeout);
+            auto t = system_clock::now() - start;
+            VERIFY( !b );
+            VERIFY( t >= timeout );
+
+            start = system_clock::now();
+            b = m.try_lock_until(start + timeout);
+            t = system_clock::now() - start;
+            VERIFY( !b );
+            VERIFY( t >= timeout );
+          }
+        catch (const std::system_error& e)
+          {
+            VERIFY( false );
+          }
+      });
+      t.join();
+      m.unlock();
+    }
+  catch (const std::system_error& e)
+    {
+      VERIFY( false );
+    }
+  catch (...)
+    {
+      VERIFY( false );
+    }
+}

Re: [patch] Fix shared_timed_mutex::try_lock_until() et al

[Jonathan Wakely]

[-- Attachment #1: Type: text/plain, Size: 674 bytes --]

On 08/04/15 20:11 +0100, Jonathan Wakely wrote:
>We can get rid of the _Mutex type then, and just use std::mutex, and
>that also means we can provide the timed locking functions even when
>!defined(_GTHREAD_USE_MUTEX_TIMEDLOCK).
>
>And so maybe we should use this fallback implementation instead of the
>pthread_rwlock_t one when !defined(_GTHREAD_USE_MUTEX_TIMEDLOCK), so
>that they have a complete std::shared_timed_mutex (this applies to at
>least Darwin, not sure which other targets).

Here's a further patch to do that (which really needs to go into 5.0
too, so we don't switch Darwin to the new pthread_rwlock_t version and
then have to swtich it back again in 6.0).

[-- Attachment #2: patch.txt --]
[-- Type: text/x-patch, Size: 4770 bytes --]

commit 20f08d3eac6ec88c83becb8f0cb2e65c10d3fe20
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Wed Apr 8 20:25:45 2015 +0100

    	* include/std/shared_mutex (shared_timed_mutex): Only use
    	pthread_rwlock_t when the POSIX Timeouts option is supported.
    	* testsuite/30_threads/shared_lock/cons/5.cc: Remove
    	dg-require-gthreads-timed.
    	* testsuite/30_threads/shared_lock/cons/6.cc: Likewise.
    	* testsuite/30_threads/shared_lock/locking/3.cc: Likewise.
    	* testsuite/30_threads/shared_lock/locking/4.cc: Likewise.

diff --git a/libstdc++-v3/include/std/shared_mutex b/libstdc++-v3/include/std/shared_mutex
index 7f26465..351a4f6 100644
--- a/libstdc++-v3/include/std/shared_mutex
+++ b/libstdc++-v3/include/std/shared_mutex
@@ -57,7 +57,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
   /// shared_timed_mutex
   class shared_timed_mutex
   {
-#ifdef _GLIBCXX_USE_PTHREAD_RWLOCK_T
+#if defined(_GLIBCXX_USE_PTHREAD_RWLOCK_T) && _GTHREAD_USE_MUTEX_TIMEDLOCK
     typedef chrono::system_clock	__clock_t;
 
 #ifdef PTHREAD_RWLOCK_INITIALIZER
@@ -116,7 +116,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       return true;
     }
 
-#if _GTHREAD_USE_MUTEX_TIMEDLOCK
     template<typename _Rep, typename _Period>
       bool
       try_lock_for(const chrono::duration<_Rep, _Period>& __rel_time)
@@ -158,7 +157,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	const auto __s_atime = __s_entry + __delta;
 	return try_lock_until(__s_atime);
       }
-#endif
 
     void
     unlock()
@@ -200,7 +198,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       return true;
     }
 
-#if _GTHREAD_USE_MUTEX_TIMEDLOCK
     template<typename _Rep, typename _Period>
       bool
       try_lock_shared_for(const chrono::duration<_Rep, _Period>& __rel_time)
@@ -258,7 +255,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	const auto __s_atime = __s_entry + __delta;
 	return try_lock_shared_until(__s_atime);
       }
-#endif
 
     void
     unlock_shared()
@@ -266,7 +262,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       unlock();
     }
 
-#else // ! _GLIBCXX_USE_PTHREAD_RWLOCK_T
+#else // ! (_GLIBCXX_USE_PTHREAD_RWLOCK_T && _GTHREAD_USE_MUTEX_TIMEDLOCK)
 
     // Must use the same clock as condition_variable
     typedef chrono::system_clock	__clock_t;
@@ -459,7 +455,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	    _M_gate1.notify_one();
 	}
     }
-#endif // ! _GLIBCXX_USE_PTHREAD_RWLOCK_T
+#endif // _GLIBCXX_USE_PTHREAD_RWLOCK_T && _GTHREAD_USE_MUTEX_TIMEDLOCK
   };
 #endif // _GLIBCXX_HAS_GTHREADS
 
diff --git a/libstdc++-v3/testsuite/30_threads/shared_lock/cons/5.cc b/libstdc++-v3/testsuite/30_threads/shared_lock/cons/5.cc
index 63ab514..9ec0498 100644
--- a/libstdc++-v3/testsuite/30_threads/shared_lock/cons/5.cc
+++ b/libstdc++-v3/testsuite/30_threads/shared_lock/cons/5.cc
@@ -3,7 +3,6 @@
 // { dg-options " -std=gnu++14 -pthreads" { target *-*-solaris* } }
 // { dg-options " -std=gnu++14 " { target *-*-cygwin *-*-darwin* } }
 // { dg-require-cstdint "" }
-// { dg-require-gthreads-timed "" }
 
 // Copyright (C) 2013-2015 Free Software Foundation, Inc.
 //
diff --git a/libstdc++-v3/testsuite/30_threads/shared_lock/cons/6.cc b/libstdc++-v3/testsuite/30_threads/shared_lock/cons/6.cc
index 8b2bcd5..2074bbe 100644
--- a/libstdc++-v3/testsuite/30_threads/shared_lock/cons/6.cc
+++ b/libstdc++-v3/testsuite/30_threads/shared_lock/cons/6.cc
@@ -3,7 +3,6 @@
 // { dg-options " -std=gnu++14 -pthreads" { target *-*-solaris* } }
 // { dg-options " -std=gnu++14 " { target *-*-cygwin *-*-darwin* } }
 // { dg-require-cstdint "" }
-// { dg-require-gthreads-timed "" }
 
 // Copyright (C) 2013-2015 Free Software Foundation, Inc.
 //
diff --git a/libstdc++-v3/testsuite/30_threads/shared_lock/locking/3.cc b/libstdc++-v3/testsuite/30_threads/shared_lock/locking/3.cc
index b67022a..4b653ea 100644
--- a/libstdc++-v3/testsuite/30_threads/shared_lock/locking/3.cc
+++ b/libstdc++-v3/testsuite/30_threads/shared_lock/locking/3.cc
@@ -3,7 +3,6 @@
 // { dg-options " -std=gnu++14 -pthreads" { target *-*-solaris* } }
 // { dg-options " -std=gnu++14 " { target *-*-cygwin *-*-darwin* } }
 // { dg-require-cstdint "" }
-// { dg-require-gthreads-timed "" }
 
 // Copyright (C) 2013-2015 Free Software Foundation, Inc.
 //
diff --git a/libstdc++-v3/testsuite/30_threads/shared_lock/locking/4.cc b/libstdc++-v3/testsuite/30_threads/shared_lock/locking/4.cc
index e87d0dd..afeefa2 100644
--- a/libstdc++-v3/testsuite/30_threads/shared_lock/locking/4.cc
+++ b/libstdc++-v3/testsuite/30_threads/shared_lock/locking/4.cc
@@ -3,7 +3,6 @@
 // { dg-options " -std=gnu++14 -pthreads" { target *-*-solaris* } }
 // { dg-options " -std=gnu++14 " { target *-*-cygwin *-*-darwin* } }
 // { dg-require-cstdint "" }
-// { dg-require-gthreads-timed "" }
 
 // Copyright (C) 2013-2015 Free Software Foundation, Inc.
 //

Re: [patch] Fix shared_timed_mutex::try_lock_until() et al

[Torvald Riegel]

On Wed, 2015-04-08 at 20:11 +0100, Jonathan Wakely wrote:
> On 08/04/15 18:59 +0200, Torvald Riegel wrote:
> >> +    // set or the maximum number of reader locks is held, then
> >> increment the
> >> +    // reader lock count.
> >> +    // To release decrement the count, then if the write-entered flag
> >> is set
> >> +    // and the count is zero then signal gate2 to wake a queued
> >> writer,
> >> +    // otherwise if the maximum number of reader locks was held
> >> signal gate1
> >> +    // to wake a reader.
> >> +    //
> >> +    // To take a writer lock block on gate1 while the write-entered
> >> flag is
> >> +    // set, then set the write-entered flag to start queueing, then
> >> block on
> >> +    // gate2 while the number of reader locks is non-zero.
> >> +    // To release unset the write-entered flag and signal gate1 to
> >> wake all
> >> +    // blocked readers and writers.
> >
> >Perhaps it would also be useful to have a sentence on how writers and
> >readers are prioritized (e.g., writers preferred if readers already hold
> >the lock, all are equal when writer unlocks, ...).
> 
> How about:
> 
> // This means that when no reader locks are held readers and writers get
> // equal priority. When one or more reader locks is held a writer gets
> // priority and no more reader locks can be taken while the writer is
> // queued.
> 
> >> +    static constexpr unsigned _S_n_readers = ~_S_write_entered;
> >
> >Rename this to _S_max_readers or such?
> 
> Done.
> 
> >>      template<typename _Clock, typename _Duration>
> >>        bool
> >>        try_lock_until(const chrono::time_point<_Clock, _Duration>&
> >> __abs_time)
> >>        {
> >> -       unique_lock<_Mutex> __lk(_M_mut, __abs_time);
> >> -       if (__lk.owns_lock() && _M_state == 0)
> >> +       if (!_M_mut.try_lock_until(__abs_time))
> >> +         return false;
> >
> >I think a non-timed acquisition for the internal lock should be fine.
> >The internal lock is supposed to protect critical sections of finite
> >(and short) length, and the condvars also don't do acquisition with time
> >outs.
> 
> Good point about the condvars re-acquiring the mutex.
> 
> We can get rid of the _Mutex type then, and just use std::mutex, and
> that also means we can provide the timed locking functions even when
> !defined(_GTHREAD_USE_MUTEX_TIMEDLOCK).
> 
> And so maybe we should use this fallback implementation instead of the
> pthread_rwlock_t one when !defined(_GTHREAD_USE_MUTEX_TIMEDLOCK), so
> that they have a complete std::shared_timed_mutex (this applies to at
> least Darwin, not sure which other targets).
> 
> Maybe we should also provide a fallback std::timed_mutex based on a
> condvar, I'll put that on the TODO list for stage 1.
> 
> >> +       unique_lock<mutex> __lk(_M_mut, adopt_lock);
> >> +       if (!_M_gate1.wait_until(__lk, __abs_time,
> >> +                                [=]{ return !_M_write_entered(); }))
> >>           {
> >> -           _M_state = _S_write_entered;
> >> -           return true;
> >> +           return false;
> >>           }
> >> -       return false;
> >> +       _M_state |= _S_write_entered;
> >> +       if (!_M_gate2.wait_until(__lk, __abs_time,
> >> +                                [=]{ return _M_readers() == 0; }))
> >> +         {
> >
> >I'd add a comment saying that you have to mimic a full write unlock, so
> >that's why those steps are necessary.
> 
> OK, like this:
> 
> // Wake all threads blocked while the write-entered flag was set.
> 
> >> +           _M_state ^= _S_write_entered;
> >> +           _M_gate1.notify_all();
> >> +           return false;
> >> +         }
> >> +       return true;
> >>        }
> >>  #endif
> >>
> >> @@ -364,7 +399,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> >>      unlock()
> >>      {
> >>        {
> >> -       lock_guard<_Mutex> __lk(_M_mut);
> >> +       lock_guard<mutex> __lk(_M_mut);
> >>         _M_state = 0;
> >>        }
> >>        _M_gate1.notify_all();
> >
> >The notification must be *inside* of the critical section.  Otherwise,
> >you're violating the mutex destruction requirements (see 30.4.1.3.1p3):
> >After you set _M_state to 0 and release _M_mut, another thread can go
> >it, acquire, assume it's the last one to own the mutex if that's what
> >the program ensures, and destroy; then, destruction would be concurrent
> >with the call to notify_all.  (Perhaps check other wake-ups / unlocks as
> >well.)
> 
> Thanks. I'll add this comment too:
> 
> // call notify_all() while mutex is held so that another thread can't
> // lock and unlock the mutex then destroy *this before we make the call.
> 
> >>      void
> >>      unlock_shared()
> >>      {
> >> -      lock_guard<_Mutex> __lk(_M_mut);
> >> -      unsigned __num_readers = (_M_state & _M_n_readers) - 1;
> >> -      _M_state &= ~_M_n_readers;
> >> -      _M_state |= __num_readers;
> >> -      if (_M_state & _S_write_entered)
> >> +      lock_guard<mutex> __lk(_M_mut);
> >> +      auto __prev = _M_state--;
> >> +      if (_M_write_entered())
> >>         {
> >> -         if (__num_readers == 0)
> >> +         if (_M_readers() == 0)
> >>             _M_gate2.notify_one();
> >>         }
> >>        else
> >>         {
> >> -         if (__num_readers == _M_n_readers - 1)
> >> +         if (__prev == _S_n_readers)
> >>             _M_gate1.notify_one();
> >>         }
> >
> >I think this needs documentation why we can't miss a wake-up in case
> >_M_write_entered is true and we have a reader overflow.  I think it's
> >not quite obvious why this works.
> >If there is a writer we don't notify gate1 in unlock_shared(), so there
> >is potential to "loose" that information in interleavings with a writer.
> >If the writer acquires the lock eventually, things will work because
> >then, the writer will do the gate1 notification in its unlock path.  If
> >the writer times out, it still does the gate1 notification regardless of
> >the the number of readers registered (because if there is no reader, it
> >will never time out but simply acquire the lock).  Thus, when there is a
> >writer, the writer becomes responsible for gate1 notification, and
> >things work.
> 
> Is this sufficient?
> 
> // No need to notify gate1 because we give priority to the queued
> // writer, and that writer will eventually notify gate1 after it
> // clears the write-entered flag.
> 
> 
> I've attached a diff showing just the changes since the last patch, as
> well as the complete patch against the trunk.
> 

LGTM.  Thanks.

Re: [patch] Fix shared_timed_mutex::try_lock_until() et al

[Torvald Riegel]

On Wed, 2015-04-08 at 20:38 +0100, Jonathan Wakely wrote:
> On 08/04/15 20:11 +0100, Jonathan Wakely wrote:
> >We can get rid of the _Mutex type then, and just use std::mutex, and
> >that also means we can provide the timed locking functions even when
> >!defined(_GTHREAD_USE_MUTEX_TIMEDLOCK).
> >
> >And so maybe we should use this fallback implementation instead of
> the
> >pthread_rwlock_t one when !defined(_GTHREAD_USE_MUTEX_TIMEDLOCK), so
> >that they have a complete std::shared_timed_mutex (this applies to at
> >least Darwin, not sure which other targets).
> 
> Here's a further patch to do that (which really needs to go into 5.0
> too, so we don't switch Darwin to the new pthread_rwlock_t version and
> then have to swtich it back again in 6.0).

I understand why a mutex with timeouts isn't required anymore, but why
do you now add it to the USE_PTHREAD_RWLOCK_T condition?  If
pthread_rwlock_t is available, why would we need a normal mutex with
timeouts?

For example, this chunk here (and others too):

> diff --git a/libstdc++-v3/include/std/shared_mutex b/libstdc
> ++-v3/include/std/shared_mutex
> index 7f26465..351a4f6 100644
> --- a/libstdc++-v3/include/std/shared_mutex
> +++ b/libstdc++-v3/include/std/shared_mutex
> @@ -57,7 +57,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>    /// shared_timed_mutex
>    class shared_timed_mutex
>    {
> -#ifdef _GLIBCXX_USE_PTHREAD_RWLOCK_T
> +#if defined(_GLIBCXX_USE_PTHREAD_RWLOCK_T) &&
> _GTHREAD_USE_MUTEX_TIMEDLOCK
>      typedef chrono::system_clock       __clock_t;
>  
>  #ifdef PTHREAD_RWLOCK_INITIALIZER 

Re: [patch] Fix shared_timed_mutex::try_lock_until() et al

[Jonathan Wakely]

On 10/04/15 02:16 +0200, Torvald Riegel wrote:
>On Wed, 2015-04-08 at 20:38 +0100, Jonathan Wakely wrote:
>> On 08/04/15 20:11 +0100, Jonathan Wakely wrote:
>> >We can get rid of the _Mutex type then, and just use std::mutex,
>> >and that also means we can provide the timed locking functions
>> >even when !defined(_GTHREAD_USE_MUTEX_TIMEDLOCK).
>> >
>> >And so maybe we should use this fallback implementation instead of
>> the
>> >pthread_rwlock_t one when !defined(_GTHREAD_USE_MUTEX_TIMEDLOCK),
>> >so that they have a complete std::shared_timed_mutex (this applies
>> >to at least Darwin, not sure which other targets).
>>
>> Here's a further patch to do that (which really needs to go into
>> 5.0 too, so we don't switch Darwin to the new pthread_rwlock_t
>> version and then have to swtich it back again in 6.0).
>
>I understand why a mutex with timeouts isn't required anymore, but
>why do you now add it to the USE_PTHREAD_RWLOCK_T condition?  If
>pthread_rwlock_t is available, why would we need a normal mutex with
>timeouts?

Darwin and HPUX support pthread_rwlock_t but not the timed lock
functions, see https://gcc.gnu.org/PR64847 and (part of)
https://gcc.gnu.org/PR64368 which were "fixed" by
https://gcc.gnu.org/r220161 which just disables the timed lock
functions on those targets, using #if _GTHREAD_USE_MUTEX_TIMEDLOCK.

That means Darwin and HPUX have an incomplete shared_timed_mutex that
doesn't support the timed functions (i.e. what might get added to
C++17 as std::shared_mutex).

The patch below gives Darwin and HPUX a fully conforming (albeit
slower) shared_timed_mutex, by ensuring we don't use pthread_rwlock_t
for targets that can't use timed functions with pthread_rwlock_t.

If std::shared_mutex is added to the standard we could use
pthread_rwlock_t for that even on Darwin and HPUX, because it provides
everything needed for a non-timed std::shared_mutex.

>For example, this chunk here (and others too):
>
>> diff --git a/libstdc++-v3/include/std/shared_mutex b/libstdc
>> ++-v3/include/std/shared_mutex
>> index 7f26465..351a4f6 100644
>> --- a/libstdc++-v3/include/std/shared_mutex
>> +++ b/libstdc++-v3/include/std/shared_mutex
>> @@ -57,7 +57,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>>    /// shared_timed_mutex
>>    class shared_timed_mutex
>>    {
>> -#ifdef _GLIBCXX_USE_PTHREAD_RWLOCK_T
>> +#if defined(_GLIBCXX_USE_PTHREAD_RWLOCK_T) &&
>> _GTHREAD_USE_MUTEX_TIMEDLOCK
>>      typedef chrono::system_clock       __clock_t;
>>
>>  #ifdef PTHREAD_RWLOCK_INITIALIZER
>

Re: [patch] Fix shared_timed_mutex::try_lock_until() et al

[Torvald Riegel]

On Fri, 2015-04-10 at 09:37 +0100, Jonathan Wakely wrote:
> On 10/04/15 02:16 +0200, Torvald Riegel wrote:
> >On Wed, 2015-04-08 at 20:38 +0100, Jonathan Wakely wrote:
> >> On 08/04/15 20:11 +0100, Jonathan Wakely wrote:
> >> >We can get rid of the _Mutex type then, and just use std::mutex,
> >> >and that also means we can provide the timed locking functions
> >> >even when !defined(_GTHREAD_USE_MUTEX_TIMEDLOCK).
> >> >
> >> >And so maybe we should use this fallback implementation instead of
> >> the
> >> >pthread_rwlock_t one when !defined(_GTHREAD_USE_MUTEX_TIMEDLOCK),
> >> >so that they have a complete std::shared_timed_mutex (this applies
> >> >to at least Darwin, not sure which other targets).
> >>
> >> Here's a further patch to do that (which really needs to go into
> >> 5.0 too, so we don't switch Darwin to the new pthread_rwlock_t
> >> version and then have to swtich it back again in 6.0).
> >
> >I understand why a mutex with timeouts isn't required anymore, but
> >why do you now add it to the USE_PTHREAD_RWLOCK_T condition?  If
> >pthread_rwlock_t is available, why would we need a normal mutex with
> >timeouts?
> 
> Darwin and HPUX support pthread_rwlock_t but not the timed lock
> functions, see https://gcc.gnu.org/PR64847 and (part of)
> https://gcc.gnu.org/PR64368 which were "fixed" by
> https://gcc.gnu.org/r220161 which just disables the timed lock
> functions on those targets, using #if _GTHREAD_USE_MUTEX_TIMEDLOCK.
> 
> That means Darwin and HPUX have an incomplete shared_timed_mutex that
> doesn't support the timed functions (i.e. what might get added to
> C++17 as std::shared_mutex).
> 
> The patch below gives Darwin and HPUX a fully conforming (albeit
> slower) shared_timed_mutex, by ensuring we don't use pthread_rwlock_t
> for targets that can't use timed functions with pthread_rwlock_t.
> 
> If std::shared_mutex is added to the standard we could use
> pthread_rwlock_t for that even on Darwin and HPUX, because it provides
> everything needed for a non-timed std::shared_mutex.

Ah, right.  I was confused by the name, thinking those targets don't
support pthread_mutex_timedlock, not pthread_rwlock_timedrdlock etc.

Re: [patch] Fix shared_timed_mutex::try_lock_until() et al

[Jonathan Wakely]

On 10/04/15 11:56 +0200, Torvald Riegel wrote:
>Ah, right.  I was confused by the name, thinking those targets don't
>support pthread_mutex_timedlock, not pthread_rwlock_timedrdlock etc.

The macro actually relates to the _POSIX_TIMEOUTS macro, but was
originally added to check for pthread_mutex_timedlock support, hence
its name. Maybe a better name would be _GLIBCXX_USE_MUTEX_TIMEOUTS.

Both pthread_mutex_timedlock and pthread_rwlock_timed??lock were
originally part of the POSIX Timeouts option, so on older POSIX
systems they are only available when _POSIX_TIMEOUTS is defined.
(They are part of the base spec in the current POSIX standard, but
Darwin and HPUX don't implement that and still treat them as
optional.)

Re: [patch] Fix shared_timed_mutex::try_lock_until() et al

[Jonathan Wakely]

On 08/04/15 20:11 +0100, Jonathan Wakely wrote:
>diff --git a/libstdc++-v3/ChangeLog b/libstdc++-v3/ChangeLog
>index 5871716..39d6866 100644
>--- a/libstdc++-v3/ChangeLog
>+++ b/libstdc++-v3/ChangeLog
>@@ -2,7 +2,8 @@
>
> 	* include/std/shared_mutex (shared_timed_mutex): Add comments to
> 	explain the logic.
>-	(_M_n_readers): Rename to _S_n_readers.
>+	(_Mutex): Remove redundant type.
>+	(_M_n_readers): Rename to _S_max_readers.
> 	(_M_write_entered, _M_readers): New convenience functions.
> 	(lock, lock_shared, try_lock_shared, unlock_shared): Use convenience
> 	functions. Use predicates with condition variables. Simplify bitwise
>@@ -11,7 +12,8 @@
> 	and call try_lock_until or try_shared_lock_until respectively.
> 	(try_lock_until, try_shared_lock_until): Wait on the condition
> 	variables until the specified time passes.
>-	(unlock, unlock_shared): Add Debug Mode assertions.
>+	(unlock): Add Debug Mode assertion.
>+	(unlock_shared): Add Debug Mode assertion.
> 	* testsuite/30_threads/shared_timed_mutex/try_lock/3.cc: New.

This has been committed to trunk and 4.9 (because on the 4.9 branch we
only had the non-pthread_rwlock_t version with broken timed lock
functions).