https://gcc.gnu.org/bugzilla/show_bug.cgi?id=116586
Mike Crowe <mac at mcrowe dot com> changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |mac at mcrowe dot com
--- Comment #3 from Mike Crowe <mac at mcrowe dot com> ---
The particularly nasty thing about this for shared_timed_mutex is that if
_GLIBCXX_ASSERTIONS is not defined then
std::shared_timed_mutex::try_lock_until() returns true even though it hasn't
successfully taken the lock! That feels like a particularly nasty failure mode
for such a bug. :(
IMO returning success when we know that _something_ went wrong isn't really a
great idea. Since we're checking the return value anyway it feels like
returning false for unrecognised errors would be safer and barely any more
expensive. It looks like I just blindly copied the _timedlock code in
ab40695a46c664 to exacerbate this problem. :(
I'd be interested in views on whether std::shared_timed_mutex::try_lock_until()
should return false on all unrecognised errors if the assert allows it to
continue. Perhaps something like:
--8<--
int __ret = pthread_rwlock_clockwrlock(&_M_rwlock, CLOCK_MONOTONIC,
&__ts);
if (__ret == 0) /// ***NEW***
return true; /// ***NEW***
// On self-deadlock, we just fail to acquire the lock. Technically,
// the program violated the precondition.
if (__ret == ETIMEDOUT || __ret == EDEADLK)
return false;
// Errors not handled: EINVAL
__glibcxx_assert(__ret == 0);
return false; /// ***INVERTED***
-->8--
As would be expected, this appears to generate ever-so-slightly-less-efficient
code with -O2 and _GLIBCXX_ASSERTIONS[1], but more efficient code without
_GLIBCXX_ASSERTIONS[2] because the compiler can throw away the checks against
the exact value of __ret.
In the meantime I'll write test cases for at least the other functions I added
_clock* versions of and ensure that they cope with negative timeouts.
[1] https://godbolt.org/z/b3dKGWeW6
[2] https://godbolt.org/z/a59G7nE7j
