User account creation filtered due to spam.

Bug 53841 - [C++11] condition_variable::wait_until() fails with high resolution clocks
Summary: [C++11] condition_variable::wait_until() fails with high resolution clocks
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: libstdc++ (show other bugs)
Version: 4.8.0
: P3 normal
Target Milestone: 4.7.3
Assignee: Jonathan Wakely
URL:
Keywords: rejects-valid
Depends on:
Blocks:
 
Reported: 2012-07-03 15:58 UTC by Jonathan Wakely
Modified: 2012-11-15 01:58 UTC (History)
2 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail: 4.6.3, 4.7.2
Last reconfirmed: 2012-11-14 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Wakely 2012-07-03 15:58:47 UTC
GCC's condition_variable::wait_until knows how to convert from unknown clocks to system_clock, although it has a problem:

	// DR 887 - Sync unknown clock to known clock.
	const typename _Clock::time_point __c_entry = _Clock::now();
	const __clock_t::time_point __s_entry = __clock_t::now();
	const chrono::nanoseconds __delta = __atime - __c_entry;

That last line fails to compile if _Clock is higher resolution than system_clock (i.e. has a smaller period or uses a floating point representation).

Here's a clock that corresponds to system_clock but uses duration<double,ratio<1>> as its duration type:

#include <chrono>
#include <mutex>
#include <condition_variable>

namespace ch = std::chrono;

struct FPClock : ch::system_clock
{
    typedef double rep;
    typedef std::ratio<1> period;
    typedef ch::duration<rep, period> duration;
    typedef ch::time_point<FPClock> time_point;

    static time_point now()
    { return time_point(duration(system_clock::now().time_since_epoch())); }
};

void f()
{
    std::mutex mx;
    std::unique_lock<std::mutex> l(mx);
    std::condition_variable cv;
    cv.wait_until(l, FPClock::now());
}

This fails to compile.

It could be fixed with duration_cast:

    const chrono::nanoseconds __delta
      = chrono::duration_cast<chrono::nanoseconds>(__atime - __c_entry);

but that loses precision and truncates the value, e.g. FPClock::duration(9e-10) is converted to nanoseconds(0), but nanoseconds(1) would be better.

I think it needs to use a version of duration_cast that rounds up to the next value representable as chrono::nanoseconds.
Comment 1 Jonathan Wakely 2012-07-03 16:05:50 UTC
Also, I don't think the _Clock template parameter for __wait_until_impl is needed. That function is always passed a time_point<__clock_t, D> so _Clock is always deduced as _Clock.
Comment 2 Jonathan Wakely 2012-07-03 16:15:38 UTC
(In reply to comment #1)
> Also, I don't think the _Clock template parameter for __wait_until_impl is
> needed. That function is always passed a time_point<__clock_t, D> so _Clock is
> always deduced as _Clock.

Oops, I mean always deduced as __clock_t
Comment 3 Richard Smith 2012-11-14 23:06:07 UTC
This code has more problems with duration conversions:

	// DR 887 - Sync unknown clock to known clock.
	const typename _Clock::time_point __c_entry = _Clock::now();
	const __clock_t::time_point __s_entry = __clock_t::now();
	const chrono::nanoseconds __delta = __atime - __c_entry;
	const __clock_t::time_point __s_atime = __s_entry + __delta;

The last line attempts to implicitly convert from time_point<[...], nanoseconds> to system_clock::time_point, which may be in microseconds or even in seconds.

Suggested fix:

-	const chrono::nanoseconds __delta = __atime - __c_entry;
-	const __clock_t::time_point __s_atime = __s_entry + __delta;
+	const auto __delta = __atime - __c_entry;
+	const auto __s_atime = __s_entry + __delta;

Clang trunk currently rejects this code (prior to instantiation, even) due to this invalid conversion if _GLIBCXX_USE_CLOCK_REALTIME is not defined (as seems to be the case on several popular linux distributions).
Comment 4 Jonathan Wakely 2012-11-14 23:14:22 UTC
(In reply to comment #3)
> -    const chrono::nanoseconds __delta = __atime - __c_entry;
> -    const __clock_t::time_point __s_atime = __s_entry + __delta;
> +    const auto __delta = __atime - __c_entry;
> +    const auto __s_atime = __s_entry + __delta;

Thanks for this.

> Clang trunk currently rejects this code (prior to instantiation, even) due to
> this invalid conversion if _GLIBCXX_USE_CLOCK_REALTIME is not defined (as seems
> to be the case on several popular linux distributions).

That's not defined on GNU/Linux unless you build GCC with --enable-libstdcxx-time=rt, which has performance implications.
Comment 5 Jonathan Wakely 2012-11-15 01:38:23 UTC
Author: redi
Date: Thu Nov 15 01:38:17 2012
New Revision: 193523

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=193523
Log:
	PR libstdc++/53841
	* include/std/condition_variable (condition_variable::wait_until):
	Handle clocks with higher resolution than __clock_t.
	(condition_variable::__wait_until_impl): Remove unnecessary _Clock
	parameter.
	* testsuite/30_threads/condition_variable/members/53841.cc: New.

Added:
    trunk/libstdc++-v3/testsuite/30_threads/condition_variable/members/53841.cc
Modified:
    trunk/libstdc++-v3/ChangeLog
    trunk/libstdc++-v3/include/std/condition_variable
Comment 6 Jonathan Wakely 2012-11-15 01:56:13 UTC
Author: redi
Date: Thu Nov 15 01:56:05 2012
New Revision: 193528

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=193528
Log:
	PR libstdc++/53841
	* include/std/condition_variable (condition_variable::wait_until):
	Handle clocks with higher resolution than __clock_t.
	* testsuite/30_threads/condition_variable/members/53841.cc: New.

Added:
    branches/gcc-4_7-branch/libstdc++-v3/testsuite/30_threads/condition_variable/members/53841.cc
Modified:
    branches/gcc-4_7-branch/libstdc++-v3/ChangeLog
    branches/gcc-4_7-branch/libstdc++-v3/include/std/condition_variable
Comment 7 Jonathan Wakely 2012-11-15 01:58:01 UTC
Fixed for 4.7.3 and 4.8.0

Thanks for prodding me to fix this, I'd forgotten all about it!