Use clock_gettime()'s CLOCK_REALTIME instead of gettimeofday() where possible

[ed]

Summary

The system_clock::now() function currently uses gettimeofday(). The problem with gettimeofday() is that it is an obsolete XSI function, hence unavailable on CloudABI. See:

http://pubs.opengroup.org/onlinepubs/9699919799/functions/gettimeofday.html

Change this code to use clock_gettime() with CLOCK_REALTIME instead, which is more consistent, as clock_gettime() is already used for steady_clock.

A previous version of this change actually attempted to change system_clock::duration, but I reverted this part as it breaks the existing ABI.

[jroelofs]

I think the new bits here need to be guarded on a check for #if (defined(_POSIX_TIMERS) && _POSIX_TIMERS > 0), and otherwise fall back on the original implementation.

[ed]

In D8253#138862, @jroelofs wrote:

I think the new bits here need to be guarded on a check for #if (defined(_POSIX_TIMERS) && _POSIX_TIMERS > 0), and otherwise fall back on the original implementation.

Checking against _POSIX_TIMERS would be a bit problematic in my specific case. The issue with _POSIX_TIMERS is that it actually covers two separate features:

• The clock_*() API (gettime, getres, nanosleep),
• The timer_*() API.

CloudABI only supports the former (as there is no signal handling), meaning we had to set _POSIX_TIMERS to -1. We could use:

#if (defined(_POSIX_TIMERS) && _POSIX_TIMERS > 0) || defined(__CloudABI__)

But that might be overdoing it. Maybe it is sufficient to just use:

#ifdef CLOCK_REALTIME

I could also invert the logic for the monotonic clock. Test against CLOCK_MONOTONIC first. Only if that one is absent, resort to testing against __APPLE__. Otherwise, throw a compiler error.

Would that be all right?

[mclow.lists]

Please include me on all libc++ code reviews.

src/chrono.cpp

42

Apple does not provide clock_gettime, but It's not clear to me that this should be an "Apple vs. everyone else" test.

Are there other systems that do not provide that call?

[ed]

src/chrono.cpp

42

While reading the code I assumed that every operating system supported by this source file except OS X had clock_gettime, but I forgot to take into account that the monotonic clock is optional through a compilation switch _LIBCPP_HAS_NO_MONOTONIC_CLOCK.

The updated patch no longer makes this false assumption. It will only call clock_gettime when CLOCK_REALTIME or CLOCK_MONOTONIC are set.

[jroelofs]

Please upload your diffs with more context (i.e. git diff -U999)... it'll make the reviews a bit easier.

How about defining _LIBCPP_HAS_NO_REALTIME_CLOCK, similarly to the existing one for monotonic clocks in include/__config under the appropriate guards? (i.e. it would be defined on __APPLE__, and anywhere else that doesn't have clock_gettime(CLOCK_REALTIME, x))

src/chrono.cpp

72

This is already taken care of where _LIBCPP_HAS_NO_MONOTONIC_CLOCK is defined. I don't think there's a need to change anything around the steady_clock implementation.

[hfinkel]

src/chrono.cpp

42

As a general note, there are many embedded-type systems that don't provide a monotonic clock. POSIX has a separate feature category for monotonic clock support: _POSIX_MONOTONIC_CLOCK -- and you must check for that, not just that CLOCK_MONOTONIC is defined (because there are systems on which CLOCK_MONOTONIC is defined, but not _POSIX_MONOTONIC_CLOCK, and calling clock_gettime with CLOCK_MONOTONIC will return EINVAL. Another option, perhaps, is to try it, and then call back to CLOCK_REALTIME if you get EINVAL.

[ed]

In D8253#138984, @jroelofs wrote:

Please upload your diffs with more context (i.e. git diff -U999)... it'll make the reviews a bit easier.

Sure! Will do that as of now. Still trying to get used to Phabricator.

How about defining _LIBCPP_HAS_NO_REALTIME_CLOCK, similarly to the existing one for monotonic clocks in include/__config under the appropriate guards? (i.e. it would be defined on __APPLE__, and anywhere else that doesn't have clock_gettime(CLOCK_REALTIME, x))

So it is important to keep in mind that _LIBCPP_HAS_NO_MONOTONIC_CLOCK does not indicate whether CLOCK_MONOTONIC is present or not. It is used to indicate that the system provides some method of obtaining a monotonic clock. The _LIBCPP_HAS_NO_MONOTONIC_CLOCK flag is not set on OS X, even though there is a monotonic clock.

Using that logic _LIBCPP_HAS_NO_REALTIME_CLOCK would imply that the system has no real-time clock at all.

src/chrono.cpp

72

The intent of this change was to be a bit more consistent with the change to system_clock. First try to use the POSIX API (clock_gettime) and only if that one is unavailable fall back to custom APIs specific to a smaller set of operating systems.

[jroelofs]

Indeed, I added _LIBCPP_HAS_NO_MONOTONIC_CLOCK for exactly the situation @hfinkel describes: my platform doesn't have access to a monotonic clock. Originally (and in my local fork) it had checks for _POSIX_TIMERS and _POSIX_MONOTONIC_CLOCK but those seem to have gone away upstream.

Sorry about the bad name suggestions. I suppose I was conflating the "does it have X" checks with the "how should X be implemented" checks. For my platform, the latter is irrelevant because the answer to the former is "no". For yours, this distinction is more important. Now I see that the naming should reflect that.

[ed]

src/chrono.cpp

42

@hfinkel: Yes. That's good to keep in mind. There's also _POSIX_MONOTONIC_CLOCK.

Though we could test against _POSIX_MONOTONIC_CLOCK, I would actually suggest against that in this case. If a system has MONOTONIC_CLOCK but has _POSIX_MONOTONIC_CLOCK undefined or set to 0, we will just build a steady_clock that will fail at run-time. There is nothing to fall back to in those cases anyway.

Testing for _POSIX_MONOTONIC_CLOCK is a bit annoying, because it means we need to add some more litter to the code as well:

#ifdef _WIN32
#include <unistd.h> // For _POSIX_MONOTONIC_CLOCK
#endif

[mclow.lists]

Indeed, I added _LIBCPP_HAS_NO_MONOTONIC_CLOCK for exactly the situation @hfinkel describes: my platform doesn't have access to a monotonic clock.

Note that if you don't have a monotonic clock, none of the thread/mutex/etc stuff will compile.
The only way that @jroelofs does this is to also define _LIBCPP_HAS_NO_THREADS.

The C++ standard *requires* that an implementation have a monotonic clock.

[hfinkel]

In D8253#139038, @mclow.lists wrote:

Indeed, I added _LIBCPP_HAS_NO_MONOTONIC_CLOCK for exactly the situation @hfinkel describes: my platform doesn't have access to a monotonic clock.

Note that if you don't have a monotonic clock, none of the thread/mutex/etc stuff will compile.
The only way that @jroelofs does this is to also define _LIBCPP_HAS_NO_THREADS.

The C++ standard *requires* that an implementation have a monotonic clock.

Sure, but that has little to do with what POSIX calls its 'monotonic clock'. On some systems, the POSIX 'real time' clock *is* monotonic (because the system has no facilities for changing the time in a non-monotonic way). You need to employ system-specific knowledge to know that's the case, but on those systems, using the real-time clock is appropriate (and sometimes the only thing possible).

[mclow.lists]

@hfinkel wrote:

Sure, but that has little to do with what POSIX calls its 'monotonic clock'.

Sorry, I used the wrong terminology. The C++ standard requires (and large portions of libc++ depend on), the existence of std::steady_clock, which is a clock which only moves forward; and does so in real time. (so reading from a non-steady clock, and then returning the max value you've ever seen is not satisfactory).

An easy way to implement that on POSIX is to use clock_gettime(CLOCK_MONOTONIC, &tp).

[ed]

Hi all,

As the discussion got side-tracked a bit (in a good way), it's not entirely clear to me whether I got a green light to push this change in. It looks like I've replied to all of the questions. Shall I go ahead and commit this change as is?

Thanks,
Ed

[ed]

Hi there,

Could anyone please take a second look at this change and let me know whether it is all right for me to push this in?

Thanks,
Ed

[jroelofs]

After looking back at the thread, I think this is good to go... it was just my comment that side-tracked things.

src/chrono.cpp

 //===----------------------------------------------------------------------===//
 
 #include "chrono"
-#include <sys/time.h>        //for gettimeofday and timeval
-#ifdef __APPLE__
-#include <mach/mach_time.h>  // mach_absolute_time, mach_timebase_info_data_t
-#else  /* !__APPLE__ */
 #include <cerrno>  // errno
 #include <system_error>  // __throw_system_error
-#include <time.h>  // clock_gettime, CLOCK_MONOTONIC
-#endif  // __APPLE__
+#include <time.h>  // clock_gettime, CLOCK_MONOTONIC and CLOCK_REALTIME
 
+#if !defined(CLOCK_REALTIME)
+#include <sys/time.h>        // for gettimeofday and timeval
+#endif
+
+#if !defined(_LIBCPP_HAS_NO_MONOTONIC_CLOCK) && !defined(CLOCK_MONOTONIC)
+#if __APPLE__
+#include <mach/mach_time.h>  // mach_absolute_time, mach_timebase_info_data_t
+#else
+#error "Monotonic clock not implemented"
+#endif
+#endif
+
 _LIBCPP_BEGIN_NAMESPACE_STD
 
 namespace chrono

 system_clock::time_point
 system_clock::now() _NOEXCEPT
 {
+#ifdef CLOCK_REALTIME
+    struct timespec tp;
+    if (0 != clock_gettime(CLOCK_REALTIME, &tp))
+        __throw_system_error(errno, "clock_gettime(CLOCK_REALTIME) failed");
+    return time_point(seconds(tp.tv_sec) + microseconds(tp.tv_nsec / 1000));
+#else  // !CLOCK_REALTIME
     timeval tv;
     gettimeofday(&tv, 0);
     return time_point(seconds(tv.tv_sec) + microseconds(tv.tv_usec));
+#endif  // CLOCK_REALTIME
 }
 
 time_t

 
 #ifndef _LIBCPP_HAS_NO_MONOTONIC_CLOCK
 // steady_clock
+//
+// Warning:  If this is not truly steady, then it is non-conforming.  It is
+//  better for it to not exist and have the rest of libc++ use system_clock
+//  instead.
 
 const bool steady_clock::is_steady;
 
-#ifdef __APPLE__
+#ifdef CLOCK_MONOTONIC
+
+steady_clock::time_point
+steady_clock::now() _NOEXCEPT
+{
+    struct timespec tp;
+    if (0 != clock_gettime(CLOCK_MONOTONIC, &tp))
+        __throw_system_error(errno, "clock_gettime(CLOCK_MONOTONIC) failed");
+    return time_point(seconds(tp.tv_sec) + nanoseconds(tp.tv_nsec));
+}
+
+#elif defined(__APPLE__)
+
 //   mach_absolute_time() * MachInfo.numer / MachInfo.denom is the number of
 //   nanoseconds since the computer booted up.  MachInfo.numer and MachInfo.denom
 //   are run time constants supplied by the OS.  This clock has no relationship

     return time_point(duration(fp()));
 }
 
-#else  // __APPLE__
-// FIXME: if _LIBCPP_HAS_NO_MONOTONIC_CLOCK, then clock_gettime isn't going to
-// work. It may be possible to fall back on something else, depending on the system.
+#else
+#error "Monotonic clock not implemented"
+#endif
 
-// Warning:  If this is not truly steady, then it is non-conforming.  It is
-//  better for it to not exist and have the rest of libc++ use system_clock
-//  instead.
-
-steady_clock::time_point
-steady_clock::now() _NOEXCEPT
-{
-    struct timespec tp;
-    if (0 != clock_gettime(CLOCK_MONOTONIC, &tp))
-        __throw_system_error(errno, "clock_gettime(CLOCK_MONOTONIC) failed");
-    return time_point(seconds(tp.tv_sec) + nanoseconds(tp.tv_nsec));
-}
-#endif  // __APPLE__
-
 #endif // !_LIBCPP_HAS_NO_MONOTONIC_CLOCK
 
 }