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.
libstdc++-v3/ChangeLog:
* include/std/shared_mutex (try_lock, try_lock_until,
try_lock_shared_until): Respond consistently to errors and deadlock.
* testsuite/30_threads/shared_timed_mutex/try_lock_until/116586.cc:
test deadlock behaviour if possible
Signed-off-by: Mike Crowe <[email protected]>
[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/include/std/shared_mutex | 55 ++++++++++---------
.../try_lock_until/116586.cc | 24 ++++++++
2 files changed, 54 insertions(+), 25 deletions(-)
History:
v1: https://gcc.gnu.org/pipermail/libstdc++/2025-November/064216.html
v2: Remove debug #error left in by accident
diff --git a/libstdc++-v3/include/std/shared_mutex
b/libstdc++-v3/include/std/shared_mutex
index a267ad7c4f6..92f6227dafb 100644
--- 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 5736b7dc047..7847a08f57b 100644
---
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,21 @@ 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)
+#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 +45,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 +64,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 +78,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 +92,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));
}
--
2.47.3
