On Sunday 02 November 2025 at 17:03:08 +0000, Mike Crowe wrote: > 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.
...and I've now tested with glibc 2.41 too. All on x86_64 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 | 25 +++++++++ > 2 files changed, 55 insertions(+), 25 deletions(-) > > I'm not really sure whether the DEADLOCK_VERIFY tests are worth the > complexity and the risk of relying on specific implementations. I'd be > happy to remove them. > > 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..dc11be8a7f4 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) > +#define DEADLOCK_VERIFY(X) \ > + VERIFY(X) > +#else > +#error not glibc > +#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)); > } > > -- > 2.39.5 > > Ping. Mike.
