[PATCH 2/2] PR libstdc++/41861 Add full steady_clock support to condition_variable

[Jonathan Wakely]

On 04/09/19 15:49 +0100, Mike Crowe wrote:
>On Wednesday 04 September 2019 at 14:39:35 +0100, Jonathan Wakely wrote:
>> On 15/07/19 17:47 +0100, Mike Crowe wrote:
>> > The pthread_cond_clockwait function was recently added[1] to glibc, and is
>> > due to be released in glibc 2.30. If this function is available in the C
>> > library it can be used it to fix
>> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=41861 by supporting
>> > std::chrono::steady_clock properly with std::condition_variable.
>> >
>> > This means that code using std::condition_variable::wait_for or
>> > std::condition_variable::wait_until with std::chrono::steady_clock is no
>> > longer subject to timing out early or potentially waiting for much longer
>> > if the system clock is warped at an inopportune moment.
>> >
>> > If pthread_cond_clockwait is available then std::chrono::steady_clock is
>> > deemed to be the "best" clock available which means that it is used for the
>> > relative wait_for calls and absolute wait_until calls using user-defined
>> > clocks. Calls explicitly using std::chrono::system_clock continue to use
>> > CLOCK_REALTIME via __gthread_cond_timedwait.
>> >
>> > If pthread_cond_clockwait is not available then std::chrono::system_clock
>> > is deemed to be the "best" clock available which means that the previous
>> > suboptimal behaviour remains.
>> >
>> > [1] https://sourceware.org/git/?p=glibc.git;a=commit;h=afe4de7d283ebd88157126c5494ce1796194c16e
>> >
>> > libstdc++-v3/
>> >
>> > 	* include/std/condition_variable: Add include of <bits/c++config.h>
>> > 	to make _GLIBCXX_USE_PTHREAD_COND_CLOCKWAIT available.
>> > 	(condition_variable): Split __clock_t into __system_clock_t, which
>> > 	is always system_clock and __best_clock_t, which is the clock that
>> > 	we wish to convert arbitrary other clocks to.  If we know that
>> > 	pthread_cond_clockwait is available then it is best to convert
>> > 	clocks to steady_clock, otherwise it's best to use
>> > 	system_clock. (wait_until): If pthread_cond_clockwait is available,
>> > 	provide a steady_clock overload.  Convert previous __clock_t
>> > 	overload to use __system_clock_t.  Convert generic overload to
>> > 	convert passed clock to __best_clock_t.  (wait_until_impl): Add
>> > 	steady_clock overload that calls pthread_cond_clockwait.  Convert
>> > 	previous __clock_t overload to use
>> > 	__system_clock_t. (condition_variable_any): Use steady_clock for
>> > 	__clock_t if pthread_cond_clockwait is available.
>> >
>> > 	* acinclude.m4:	Detect the availability of POSIX-proposed
>> > 	pthread_cond_clockwait function.
>> > 	* configure: Likewise.
>> > 	* configure.ac: Likewise.
>> > 	* config.h.in: Add _GLIBCXX_USE_PTHREAD_COND_CLOCKWAIT define to
>> > 	indicate presence of pthread_cond_clockwait function.
>>
>> Thanks, Mike!
>>
>> This patch is much simpler than the previous versions. Thanks for
>> perservering with POSIX and glibc to get the necessary functionality
>> into libc.
>>
>> I've attached a slightly-modified patch, which changes the names of
>> the clock typedefs in std::condition_variable. The names
>> "steady_clock" and "system_clock" are already reserved names, so we
>> can just use those (instead of the uglier __steady_clock_t forms). And
>> we can just keep the original __clock_t name to mean the best clock.
>> Does that look OK to you too?
>
>You're far more expert on that stuff than I am, but it looks fine to me.
>
>However, you've also removed the <bits/c++config.h> include. Was that
>intentional?

Yes, it's already included via <type_traits> (and probably several
other routes). Every header should already be including that. If they
didn't, the macros like _GLIBCXX_HAS_GTHREADS, _GLIBCXX_VISIBILITY,
and _GLIBCXX_BEGIN_NAMESPACE_VERSION would produce errors.


>> I've confirmed that with glibc 2.30 the new function is detected, and
>> the testsuite passes. I'm running the tests with an older glibc as
>> well.
>
>I found that it was necessary to run the test under strace to prove that
>the correct variant of futex is called too, because...
>
>> I noticed that the new tests you added in [PATCH 1/2] pass on current
>> trunk, even without [PATCH 2/2], is that expected?
>
>Unfortunately, yes. CLOCK_MONOTONIC and CLOCK_REALTIME will tick at the
>same rate, so unless someone warps CLOCK_REALTIME during the test we can't
>tell the difference between the old implementation of translating
>steady_clock to system_clock and the new implementation that uses
>steady_clock directly. :( I added the tests in the hope of finding other
>mistakes in the new implementation rather than to reproduce the original
>problem.

OK, that was what I figured was the case. Thanks for confirming.

>Maybe I should add comments to the test to make that clear along the lines
>of those found in testsuite/27_io/objects/char/3_xin.cc ?

More comments are usually useful, otherwise I'll just ask the same
question again and again :-)

All my testing passed, so I'll get this committed today. Thanks again
for perservering to finally get this fixed!