[Bug libstdc++/53841] New: [C++11] condition_variable::wait_until() fails with high resolution clocks

[Bug libstdc++/53841] New: [C++11] condition_variable::wait_until() fails with high resolution clocks

[redi at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53841

             Bug #: 53841
           Summary: [C++11] condition_variable::wait_until() fails with
                    high resolution clocks
    Classification: Unclassified
           Product: gcc
           Version: 4.8.0
            Status: UNCONFIRMED
          Keywords: rejects-valid
          Severity: normal
          Priority: P3
         Component: libstdc++
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: redi@gcc.gnu.org


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.

[Bug libstdc++/53841] [C++11] condition_variable::wait_until() fails with high resolution clocks

[redi at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53841

--- Comment #1 from Jonathan Wakely <redi at gcc dot gnu.org> 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.

[Bug libstdc++/53841] [C++11] condition_variable::wait_until() fails with high resolution clocks

[redi at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53841

--- Comment #2 from Jonathan Wakely <redi at gcc dot gnu.org> 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

[Bug libstdc++/53841] [C++11] condition_variable::wait_until() fails with high resolution clocks

[richard-gccbugzilla at metafoo dot co.uk]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53841

Richard Smith <richard-gccbugzilla at metafoo dot co.uk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |richard-gccbugzilla at
                   |                            |metafoo dot co.uk

--- Comment #3 from Richard Smith <richard-gccbugzilla at metafoo dot co.uk> 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).

[Bug libstdc++/53841] [C++11] condition_variable::wait_until() fails with high resolution clocks

[redi at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53841

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2012-11-14
     Ever Confirmed|0                           |1

--- Comment #4 from Jonathan Wakely <redi at gcc dot gnu.org> 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.

[Bug libstdc++/53841] [C++11] condition_variable::wait_until() fails with high resolution clocks

[redi at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53841

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
         AssignedTo|unassigned at gcc dot       |redi at gcc dot gnu.org
                   |gnu.org                     |

[Bug libstdc++/53841] [C++11] condition_variable::wait_until() fails with high resolution clocks

[redi at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53841

--- Comment #5 from Jonathan Wakely <redi at gcc dot gnu.org> 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

[Bug libstdc++/53841] [C++11] condition_variable::wait_until() fails with high resolution clocks

[redi at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53841

--- Comment #6 from Jonathan Wakely <redi at gcc dot gnu.org> 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

[Bug libstdc++/53841] [C++11] condition_variable::wait_until() fails with high resolution clocks

[redi at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53841

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|                            |FIXED
   Target Milestone|---                         |4.7.3
      Known to fail|                            |4.6.3, 4.7.2

--- Comment #7 from Jonathan Wakely <redi at gcc dot gnu.org> 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!