libstdc++: shared_mutex: Respond consistently to errors and deadlock

Make the shared_mutex::try_lock(), shared_timed_mutex::try_lock_until()
and shared_timed_mutex::try_lock_shared_until() all handle errors from
pthread functions consistently by returning false to indicate that the
lock could not be taken. If _GLIBCXX_ASSERTIONS is defined then
unexpected errors, such as EDEADLK and EINVAL will cause an assertion
failure. If _GLIBCXX_ASSERTIONS is not defined then these functions no
longer ever return true incorrectly indicating that they have taken the
lock.

This removes the previous behaviour of looping on EDEADLK in
try_lock_shared_until() and no longer returns true on EINVAL in all of
these functions. (In theory at least it should not be possible to
trigger EINVAL since 5dba17a3e709859968f939354e6e5e8d796012d3.)

Unfortunately my reading of POSIX is that pthread_rwlock_clockrdlock[1],
pthread_rwlock_timedrdlock pthread_rwlock_clockwrlock[2] and
pthread_rwlock_timedwrlock are allowed to deadlock rather than return
EDEADLK when trying to take a lock a second time from the same
thread. This means that the deadlock tests cannot be enabled by
default. I believe that the tests do work with glibc (2.31 & 2.36) and
with the __shared_mutex_cv implementation though.

[1] https://pubs.opengroup.org/onlinepubs/9799919799/functions/pthread_rwlock_clockrdlock.html
[2] https://pubs.opengroup.org/onlinepubs/9799919799/functions/pthread_rwlock_clockwrlock.html

libstdc++-v3/ChangeLog:

	* include/std/shared_mutex (try_lock, try_lock_until)
	(try_lock_shared_until): Respond consistently to errors and
	deadlocks.
	* testsuite/30_threads/shared_timed_mutex/try_lock_until/116586.cc:
	Test deadlock behaviour if possible.

Signed-off-by: Mike Crowe <mac@mcrowe.com>

diff --git a/libstdc++-v3/include/std/shared_mutex b/libstdc++-v3/include/std/shared_mutex
index a267ad7c4f66f674c0ac03b088ff68e8a1b5da73..92f6227dafb70fa390c021b3b3bb1502d7c43c5f 100644 (file)
--- a/libstdc++-v3/include/std/shared_mutex
+++ b/libstdc++-v3/include/std/shared_mutex
@@ -208,10 +208,14 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     try_lock()
     {
       int __ret = __glibcxx_rwlock_trywrlock(&_M_rwlock);
-      if (__ret == EBUSY) return false;
-      // Errors not handled: EINVAL
+      if (__ret == 0)
+       return true;
+      if (__ret == EBUSY)
+       return false;
+      // Errors not handled: EINVAL, EDEADLK
       __glibcxx_assert(__ret == 0);
-      return true;
+      // try_lock() is not permitted to throw
+      return false;
     }
 
     void
@@ -522,13 +526,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       {
        struct timespec __ts = chrono::__to_timeout_timespec(__atime);
        int __ret = __glibcxx_rwlock_timedwrlock(&_M_rwlock, &__ts);
-       // On self-deadlock, we just fail to acquire the lock.  Technically,
-       // the program violated the precondition.
-       if (__ret == ETIMEDOUT || __ret == EDEADLK)
+       if (__ret == 0)
+         return true;
+       if (__ret == ETIMEDOUT)
          return false;
-       // Errors not handled: EINVAL
+       // Errors not handled: EINVAL, EDEADLK
        __glibcxx_assert(__ret == 0);
-       return true;
+       return false;
       }
 
 #ifdef _GLIBCXX_USE_PTHREAD_RWLOCK_CLOCKLOCK
@@ -541,13 +545,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
        struct timespec __ts = chrono::__to_timeout_timespec(__atime);
        int __ret = pthread_rwlock_clockwrlock(&_M_rwlock, CLOCK_MONOTONIC,
                                               &__ts);
-       // On self-deadlock, we just fail to acquire the lock.  Technically,
-       // the program violated the precondition.
-       if (__ret == ETIMEDOUT || __ret == EDEADLK)
+       if (__ret == 0)
+         return true;
+       if (__ret == ETIMEDOUT)
          return false;
-       // Errors not handled: EINVAL
+       // Errors not handled: EINVAL, EDEADLK
        __glibcxx_assert(__ret == 0);
-       return true;
+       return false;
       }
 #endif
 
@@ -592,18 +596,16 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
        // acquire the lock even if it would be logically free; however, this
        // is allowed by the standard, and we made a "strong effort"
        // (see C++14 30.4.1.4p26).
-       // For cases where the implementation detects a deadlock we
-       // intentionally block and timeout so that an early return isn't
-       // mistaken for a spurious failure, which might help users realise
-       // there is a deadlock.
        do
          __ret = __glibcxx_rwlock_timedrdlock(&_M_rwlock, &__ts);
-       while (__ret == EAGAIN || __ret == EDEADLK);
+       while (__ret == EAGAIN);
+       if (__ret == 0)
+         return true;
        if (__ret == ETIMEDOUT)
          return false;
-       // Errors not handled: EINVAL
+       // Errors not handled: EINVAL, EDEADLK
        __glibcxx_assert(__ret == 0);
-       return true;
+       return false;
       }
 
 #ifdef _GLIBCXX_USE_PTHREAD_RWLOCK_CLOCKLOCK
@@ -616,13 +618,16 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
        struct timespec __ts = chrono::__to_timeout_timespec(__atime);
        int __ret = pthread_rwlock_clockrdlock(&_M_rwlock, CLOCK_MONOTONIC,
                                               &__ts);
-       // On self-deadlock, we just fail to acquire the lock.  Technically,
-       // the program violated the precondition.
-       if (__ret == ETIMEDOUT || __ret == EDEADLK)
+       // On self-deadlock, if _GLIBCXX_ASSERTIONS is not defined, we just
+       // fail to acquire the lock.  Technically, the program violated the
+       // precondition.
+       if (__ret == 0)
+         return true;
+       if (__ret == ETIMEDOUT)
          return false;
-       // Errors not handled: EINVAL
+       // Errors not handled: EINVAL, EDEADLK
        __glibcxx_assert(__ret == 0);
-       return true;
+       return false;
       }
 #endif
 

diff --git a/libstdc++-v3/testsuite/30_threads/shared_timed_mutex/try_lock_until/116586.cc b/libstdc++-v3/testsuite/30_threads/shared_timed_mutex/try_lock_until/116586.cc
index 5736b7dc04706908b2afc8b93442fe08e518dc81..da9b65395655b2f5c89625a0a184078672401c00 100644 (file)
--- a/libstdc++-v3/testsuite/30_threads/shared_timed_mutex/try_lock_until/116586.cc
+++ b/libstdc++-v3/testsuite/30_threads/shared_timed_mutex/try_lock_until/116586.cc
@@ -23,6 +23,22 @@ namespace chrono = std::chrono;
 #define VERIFY_IN_NEW_THREAD(X) \
   (void) std::async(std::launch::async, [&] { VERIFY(X); })
 
+// Unfortunately POSIX says that pthread_rwlock_clockwrlock,
+// pthread_rwlock_clockrdlock, pthread_rwlock_timedwrlock and
+// pthread_rwlock_timedrdlock are allowed to deadlock rather than return
+// EDEADLK or just time out if the thread already has the associated lock. This
+// means that we can't enable the deadlock tests by default. The glibc
+// implementation does return EDEADLK and __shared_mutex_cv times out so we can
+// test both of them.
+#if (defined(__GLIBC__) || ! (_GLIBCXX_USE_PTHREAD_RWLOCK_T && _GTHREAD_USE_MUTEX_TIMEDLOCK)) \
+  && ! defined(_GLIBCXX_ASSERTIONS)
+#define DEADLOCK_VERIFY(X) \
+  VERIFY(X)
+#else
+#define DEADLOCK_VERIFY(X) \
+  do {} while(0)
+#endif
+
 template <typename Clock>
 void
 test_exclusive_absolute(chrono::nanoseconds offset)
@@ -30,6 +46,12 @@ test_exclusive_absolute(chrono::nanoseconds offset)
   std::shared_timed_mutex stm;
   chrono::time_point<Clock> tp(offset);
   VERIFY(stm.try_lock_until(tp));
+
+  // Trying to acquire the same long on the same thread
+  DEADLOCK_VERIFY(!stm.try_lock_until(tp));
+
+  // Check that false is returned on timeouts even if the implementation
+  // returned EDEADLK above by trying to lock on a different thread.
   VERIFY_IN_NEW_THREAD(!stm.try_lock_until(tp));
 }
 
@@ -43,6 +65,7 @@ test_shared_absolute(chrono::nanoseconds offset)
   stm.unlock_shared();
 
   VERIFY(stm.try_lock_for(chrono::seconds{10}));
+  DEADLOCK_VERIFY(!stm.try_lock_shared_until(tp));
   VERIFY_IN_NEW_THREAD(!stm.try_lock_shared_until(tp));
 }
 
@@ -56,6 +79,7 @@ test_exclusive_relative(chrono::nanoseconds offset)
   std::shared_timed_mutex stm;
   const auto d = -Clock::now().time_since_epoch() + offset;
   VERIFY(stm.try_lock_for(d));
+  DEADLOCK_VERIFY(!stm.try_lock_for(d));
   VERIFY_IN_NEW_THREAD(!stm.try_lock_for(d));
 }
 
@@ -69,6 +93,7 @@ test_shared_relative(chrono::nanoseconds offset)
   stm.unlock_shared();
   // Should complete immediately
   VERIFY(stm.try_lock_for(chrono::seconds{10}));
+  DEADLOCK_VERIFY(!stm.try_lock_shared_for(d));
   VERIFY_IN_NEW_THREAD(!stm.try_lock_shared_for(d));
 }