Re: [PATCH 07/11] PR libstdc++/91906 Fix timed_mutex::try_lock_until on arbitrary clock

[François Dumont <frs dot dumont at gmail dot com>]

Sorry if I am totally out of subject here as I don't know this part of 
the lib outside the patches I see sometimes on this mailing list.

On 9/28/19 10:44 AM, Mike Crowe wrote:
> A non-standard clock may tick more slowly than std::chrono::steady_clock.
> This means that we risk returning false early when the specified timeout
> may not have expired. This can be avoided by looping until the timeout time
> as reported by the non-standard clock has been reached.
>
> Unfortunately, we have no way to tell whether the non-standard clock ticks
> more quickly that std::chrono::steady_clock. If it does then we risk
> returning later than would be expected, but that is unavoidable and
> permitted by the standard.
>
> 	* include/std/mutex (_M_try_lock_until): Loop until the absolute
> 	timeout time is reached as measured against the appropriate clock.
> 	* testsuite/30_threads/timed_mutex/try_lock_until/3.cc:
> 	Also run test using slow_clock to test above fix.
> 	* testsuite/30_threads/recursive_timed_mutex/try_lock_until/3.cc:
> 	Likewise.
> ---
>   libstdc++-v3/include/std/mutex                                              | 13 +++++++++++--
>   libstdc++-v3/testsuite/30_threads/recursive_timed_mutex/try_lock_until/3.cc |  2 ++
>   libstdc++-v3/testsuite/30_threads/timed_mutex/try_lock_until/3.cc           |  2 ++
>   3 files changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/libstdc++-v3/include/std/mutex b/libstdc++-v3/include/std/mutex
> index e06d286..bb3a41b 100644
> --- a/libstdc++-v3/include/std/mutex
> +++ b/libstdc++-v3/include/std/mutex
> @@ -189,8 +189,17 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>   	bool
>   	_M_try_lock_until(const chrono::time_point<_Clock, _Duration>& __atime)
>   	{
> -	  auto __rtime = __atime - _Clock::now();
> -	  return _M_try_lock_for(__rtime);
> +          // The user-supplied clock may not tick at the same rate as
> +          // steady_clock, so we must loop in order to guarantee that
> +          // the timeout has expired before returning false.
> +          auto __now = _Clock::now();

Isn't it possible here that __now == __atime ?

In this case the loop won't even be entered giving no chance for 
_M_try_lock_for to return true as it used to be done in the previous code.

So a do/while seems better to me. Or at least a __atime >= __now if the 
Standard says that false shall be returned when __atime < __now even 
without trying to lock.

> +          while (__atime > __now) {
> +            auto __rtime = __atime - __now;
> +            if (_M_try_lock_for(__rtime))
> +              return true;
> +            __now = _Clock::now();
> +          }
> +          return false;
>   	}
>       };