This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] Threading support: Attempt to externalize system_clock::now() and steady_clock::now() implementations
Needs ReviewPublic

Authored by rmaprath on Feb 10 2017, 4:15 AM.

Details

Summary

These implementations depend heavily on the underlying platform. It would be good to put them behind a porting function within the threading API as quite a bit of threading functionality depend on the clocks.

The steady_clock::now() implementation was difficult to pluck out for __APPLE__ because it uses some static functions within the library sources.

I have tested these changes on linux, I will test them on Windows and Mac before committing (if this gets approved).

Diff Detail

Event Timeline

rmaprath created this revision.Feb 10 2017, 4:15 AM
rmaprath updated this revision to Diff 88190.Feb 13 2017, 6:06 AM

Re-based.

EricWF requested changes to this revision.Feb 16 2017, 5:36 PM

I really don't like this change (and D29757) because they start to include a ton of extra headers, and because they lift complex configuration logic into what should be a very simple header.
These functions are much better suited to being define out-of-line for a number of reasons.

Is there another way we can allow "external implementations" of these functions without lifting everything into a header?

This revision now requires changes to proceed.Feb 16 2017, 5:36 PM
rmaprath updated this revision to Diff 89665.Feb 24 2017, 6:41 AM
rmaprath edited edge metadata.

Different approach: Externalize these functions only for the external-thread-api library variant.

(Similar to D29757)

rmaprath updated this revision to Diff 90025.Feb 28 2017, 7:01 AM

Fixed minor omission.

ed added a subscriber: ed.Mar 14 2017, 2:02 PM

Worth mentioning: the latest version of macOS now supports clock_gettime(). Maybe better to leave the code as is and simply axe the Mach time code at some point in the future?

In D29818#700949, @ed wrote:

Worth mentioning: the latest version of macOS now supports clock_gettime(). Maybe better to leave the code as is and simply axe the Mach time code at some point in the future?

Supporting only the latest and greatest is unfriendly to folks who are stuck on particular versions...

I really dislike that __libcpp_clock_monotonic and __libcpp_clock_realtime are never declared, and are expected to be magically defined by the external threading header.
This makes the configuration seem incorrect and unused.

Also the previous threading support changes were beneficial to libc++ because they cleaned up and centralized the threading interface, making it easier to port libc++ to different
threading environments. This change has no such benefit.

Why should libc++ upstream this ARM specific configuration need?

Hi @EricWF

Apologies for the delay in replying to this.

I really dislike that __libcpp_clock_monotonic and __libcpp_clock_realtime are never declared, and are expected to be magically defined by the external threading header.
This makes the configuration seem incorrect and unused.

This I can address by conditionally declaring these functions in the __threading_support header. But I think the the following assessment is not right:

Also the previous threading support changes were beneficial to libc++ because they cleaned up and centralized the threading interface, making it easier to port libc++ to different
threading environments. This change has no such benefit.

Why should libc++ upstream this ARM specific configuration need?

Currently chrono.cpp depends on more than what a standard C library provides (clock_gettime() is specific to Unix, it is not part of the C standard), so this is about general portability of libc++ and not ARM specific.

Anyone porting libc++ into a bare-metal (or RTOS) environment will have to deal with this, so it would make sense to provide an external API for this? Of course, the approach proposed in this patch might not be the only way (or the best way) to do it.

WDYT?

/ Asiri