On Sun, 2 Nov 2025 at 17:04, Mike Crowe <[email protected]> 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.
>
> 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
I don't think we want this #error here, otherwise this looks good.
> +#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
>
