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

[Jonathan Wakely <jwakely at redhat dot com>]

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!