This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by ed on Mar 11 2015, 7:30 AM.

Details

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.

Diff Detail

Event Timeline

ed updated this revision to Diff 21717.Mar 11 2015, 7:30 AM
ed retitled this revision from to Use clock_gettime()'s CLOCK_REALTIME instead of gettimeofday() where possible.
ed updated this object.
ed edited the test plan for this revision. (Show Details)
ed added a reviewer: jroelofs.
ed set the repository for this revision to rL LLVM.
ed added a subscriber: Unknown Object (MLST).
jroelofs edited edge metadata.Mar 11 2015, 7:56 AM

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 added a comment.EditedMar 11 2015, 8:43 AM

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?

ed updated this revision to Diff 21731.Mar 11 2015, 8:57 AM
ed edited edge metadata.
ed removed rL LLVM as the repository for this revision.

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 updated this revision to Diff 21732.Mar 11 2015, 8:59 AM
ed added inline comments.Mar 11 2015, 9:04 AM
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.

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 added inline comments.
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 added a comment.Mar 11 2015, 9:32 AM

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.

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 added inline comments.Mar 11 2015, 9:55 AM
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

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.

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

@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 added a comment.Mar 26 2015, 8:07 AM

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 added a comment.May 10 2015, 1:22 PM

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 accepted this revision.May 11 2015, 5:21 PM
jroelofs edited edge metadata.

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

This revision is now accepted and ready to land.May 11 2015, 5:21 PM
ed closed this revision.May 19 2015, 11:16 AM