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

[Mike Crowe <mac at mcrowe dot com>]

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?

> 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.

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 ?

Thanks for looking at this.

Mike.