https://gcc.gnu.org/g:a39fec8c74c8cd58754053dba807d68e2398ca83
commit r16-5400-ga39fec8c74c8cd58754053dba807d68e2398ca83 Author: Mike Crowe <[email protected]> Date: Tue Nov 18 13:06:53 2025 +0000 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 <[email protected]> Diff: --- libstdc++-v3/include/std/shared_mutex | 55 ++++++++++++---------- .../shared_timed_mutex/try_lock_until/116586.cc | 25 ++++++++++ 2 files changed, 55 insertions(+), 25 deletions(-) diff --git a/libstdc++-v3/include/std/shared_mutex b/libstdc++-v3/include/std/shared_mutex index a267ad7c4f66..92f6227dafb7 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 5736b7dc0470..da9b65395655 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,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)); }
