Page MenuHomePhabricator

[SystemZ][ZOS] Provide CLOCK_MONOTONIC alternative
ClosedPublic

Authored by zibi on Dec 18 2020, 7:23 AM.

Details

Summary

We need CLOCK_MONOTONIC equivalent implementation for z/OS within libc++. The default implementation is asserting.

On z/OS the lack of 'clock_gettime()' and 'time_point()' force us to look for alternatives.
The current proposal is to use gettimeofday() for CLOCK_MONOTONIC which is also used in CLOCK_REALTIME. This will allow us to skip the assertion with compromised CLOCK_MONOTONIC implementation which will not guarantee to never go back in time because it will use gettimeofday() but only when it's set.

Is this a good compromise for platforms which does not support monotonic clock?
Hopefully this will spark the discussion and agreement how to proceed in this situation.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
ldionne requested changes to this revision.Jan 13 2021, 8:30 AM

Unless I'm heavily mistaken, gettimeofday() isn't monotonic. It's not a good idea to implement a feature in a way that we know to be broken from the start.

I think it would be best for z/OS to provide a monotonic clock, or for libc++ on z/OS to acknowledge the lack of such support and use _LIBCPP_HAS_NO_THREADS.

This revision now requires changes to proceed.Jan 13 2021, 8:30 AM

Unless I'm heavily mistaken, gettimeofday() isn't monotonic. It's not a good idea to implement a feature in a way that we know to be broken from the start.

I think it would be best for z/OS to provide a monotonic clock, or for libc++ on z/OS to acknowledge the lack of such support and use _LIBCPP_HAS_NO_THREADS.

In other words, I'm echoing Marshall's comment and saying that this won't make progress unless we have a non-broken monotonic clock.

zibi added a comment.Jan 13 2021, 8:37 AM

Unless I'm heavily mistaken, gettimeofday() isn't monotonic. It's not a good idea to implement a feature in a way that we know to be broken from the start.

Sorry, for confusion. We hashed it out internally and this is our monotonic implementation on z/OS.

zibi updated this revision to Diff 316427.Jan 13 2021, 9:24 AM

Not sure why pre. build check failed with trailing spaces. I could not find any.
Resubmitting to check if it goes this time.

zibi updated this revision to Diff 316439.Jan 13 2021, 10:06 AM

Removing trailing character and resubmitting.

ldionne requested changes to this revision.Jan 18 2021, 10:56 AM

If you say it's monotonic..

I would like to reiterate what I've said before: without a build bot, this code will rot. I believe the right order here would be to add a build bot (and have it fail), and then work towards making it green.

libcxx/CMakeLists.txt
536 ↗(On Diff #316439)

Can you bury this into gettod.h?

libcxx/include/support/ibm/gettod.h
19 ↗(On Diff #316439)

Can you please use a name that conveys more meaning? Something like gettimeofday_monotonic maybe?

libcxx/src/chrono.cpp
233

Don't #include here -- you're #includeing inside a namespace. Please place this include at the top of the file, with other includes.

This revision now requires changes to proceed.Jan 18 2021, 10:56 AM
hubert.reinterpretcast added inline comments.
libcxx/CMakeLists.txt
536 ↗(On Diff #316439)

That may be insufficient if the system headers with the affected interfaces get included before gettod.h is reached.

libcxx/include/support/ibm/gettod.h
26 ↗(On Diff #316439)

Are the unsigned longs in this file supposed to differ in size by address mode? If not, I suggest using the uintN_t typedefs.

libcxx/src/chrono.cpp
238
libcxx/include/support/ibm/gettod.h
13–17 ↗(On Diff #316439)

This seems to be a lot of header includes to get the definition of timespec64. I don't see a use of assert in the code.

37 ↗(On Diff #316439)

Please do not name a variable representing microseconds as ms. Use us (see std::chrono_literals).

41 ↗(On Diff #316439)

Would lldiv help here?

libcxx/src/chrono.cpp
238

I don't see where errno is populated to indicate the error represented by a non-zero condition code in the implementation of __gettod. Is it meaningful to use errno here?

zibi marked 10 inline comments as done.Jan 25 2021, 12:32 PM

Thank you louis and Hubert for review and suggestions.

libcxx/CMakeLists.txt
536 ↗(On Diff #316439)

This has to be defined here so the macro is defined before any system headers is included as stated in the comment. We took this same approach in clang code were similar macros needed to be defined with same requirements.

libcxx/include/support/ibm/gettod.h
13–17 ↗(On Diff #316439)

The original code was reduced which does not require all the header. Let me prune the list.

19 ↗(On Diff #316439)

sure

26 ↗(On Diff #316439)

Thanks, what will help with 32-bit run time down the road.

41 ↗(On Diff #316439)

Will use HLASM dlgr to get better performance.

zibi updated this revision to Diff 319093.Jan 25 2021, 12:37 PM
zibi marked 5 inline comments as done.

Please see a new patch according to the comments I received.

zibi updated this revision to Diff 319094.Jan 25 2021, 12:39 PM

checking CI

zibi updated this revision to Diff 319100.Jan 25 2021, 1:49 PM

format + tidy

libcxx/include/support/ibm/gettod.h
15 ↗(On Diff #319100)

Adding an external linkage symbol is a layer of complication that I don't think is needed here.

33 ↗(On Diff #319100)

It seems there's specifically EMVSTODNOTSET

41–42 ↗(On Diff #319100)

Comments could help.

libcxx/src/chrono.cpp
35

Should the header be top-level for "support/ibm/"? Perhaps it should be named to indicate that it is for z/OS.

zibi updated this revision to Diff 319310.Jan 26 2021, 8:44 AM
zibi marked 4 inline comments as done.

Hubert, I incorporated your suggestions. Please note that currently pre-build checks fails because of unrelated issues with Apple back-deployment macosx10.9 test bucket.

LGTM from my end with one comment. Need @ldionne's input.

libcxx/include/support/ibm/gettod_zos.h
10–11 ↗(On Diff #319310)

Minor nit: Update header guard to match the filename/path/etc. once that is settled (see comment below).

libcxx/src/chrono.cpp
20

Not sure what @ldionne's opinion is here, and sorry for not noticing earlier. There's a libcxx/src/include/ directory for what I understand to be headers to be used for the library build (as opposed to headers containing interfaces meant to face the library user). I think the header here might fit that first category,

ldionne requested changes to this revision.Feb 2 2021, 12:26 PM
ldionne added inline comments.
libcxx/CMakeLists.txt
536 ↗(On Diff #316439)

I'm really unhappy about adding random defines to make platforms behave sanely. Why doesn't z/OS provide timespec64 when _LARGE_TIME_API isn't defined?

Also, you should be using target_add_definitions() here instead, to avoid polluting the build flags globally.

libcxx/src/chrono.cpp
20

I think it's fine to leave it under libcxx/include even though it's only used in the build. I think it would also make sense to move the current contents of libcxx/src/include somewhere else to try and colocate headers. I personally don't mind shipping those headers - they are never used until one needs them, but they don't hurt either since they are under libcxx/include/support.

@zibi Watch out for an upcoming change where I'll rename libcxx/include/support to libcxx/include/__support to make sure it doesn't conflict with user-provided directories having the same name. You'll have to do a minor rebasing.

This revision now requires changes to proceed.Feb 2 2021, 12:26 PM
libcxx/CMakeLists.txt
536 ↗(On Diff #316439)

It seems reasonable to provide timespec64 through time.h (which provides timespec). However, time.h is a standard header, and timespec64 does not match any of the standardized forms of identifiers that are reserved (or at least it does not clearly look like a reserved identifier to me). It is a valid design choice to use opt-in (as opposed to opt-out) macros for controlling extensions.

ldionne added inline comments.Feb 2 2021, 1:22 PM
libcxx/CMakeLists.txt
536 ↗(On Diff #316439)

It's a valid design choice, but it's both inconvenient and inconsistent with what other platforms do. Anyway, this is OK but we need to use target_add_definitions.

zibi added inline comments.Feb 2 2021, 1:27 PM
libcxx/CMakeLists.txt
536 ↗(On Diff #316439)

Believe me, I'm totally with you on this but, unfortunately I have no control over visibility of timespec64. We could hide this in one of the wrapper tools but this would not be fare for not being able to build library using just the repo.

However the good news is we might remove the entire patch once we get one we get monotomic clock shipped with OS.

zibi marked 3 inline comments as done.Feb 2 2021, 3:15 PM
zibi added inline comments.
libcxx/CMakeLists.txt
536 ↗(On Diff #316439)

It's a valid design choice, but it's both inconvenient and inconsistent with what other platforms do. Anyway, this is OK but we need to use target_add_definitions.

Are you suggesting to define new macro target_add_definitions and use it instead of add_definitions? Just to make sure you want to define '_LARGE_TIME_API ' only for building chrono.cpp.p`, right?

zibi updated this revision to Diff 321302.Feb 3 2021, 7:56 PM
zibi marked an inline comment as done.
zibi edited the summary of this revision. (Show Details)

Making _LARGE_TIME_API locale, renaming header guard and removing NR asm constriants.

libcxx/src/chrono.cpp
20

@zibi, the rename has landed. The header needs to be moved to be under __support.

zibi updated this revision to Diff 321741.Feb 5 2021, 6:39 AM

synch with __support rename

libcxx/src/chrono.cpp
20

Missed update here?

zibi updated this revision to Diff 321804.Feb 5 2021, 9:34 AM

Oops, thx Hubert.

zibi marked 2 inline comments as done.Feb 5 2021, 9:46 AM

From my end, LGTM.

ldionne added inline comments.Fri, Feb 5, 12:18 PM
libcxx/src/CMakeLists.txt
5–10 ↗(On Diff #321804)

If we're doing it this way, then why not add a #define at the top of chrono.cpp? I'd like to avoid adding complexity to the build system.

ldionne requested changes to this revision.Fri, Feb 5, 12:19 PM
ldionne added inline comments.
libcxx/src/CMakeLists.txt
5–10 ↗(On Diff #321804)

So please either use a define in chrono.cpp or implement my suggestion of target_compile_definitions(), but not this.

This revision now requires changes to proceed.Fri, Feb 5, 12:19 PM
zibi updated this revision to Diff 321865.Fri, Feb 5, 1:35 PM
zibi marked 2 inline comments as done.

Moving macro to chrono.cpp.

zibi added a comment.Fri, Feb 5, 1:39 PM

Moving to chrono.cpp is fine, otherwise please provide the exact suggestion for target_compile_definitions() .

libcxx/src/CMakeLists.txt
5–10 ↗(On Diff #321804)

If we're doing it this way, then why not add a #define at the top of chrono.cpp? I'd like to avoid adding complexity to the build system.

zibi added a comment.Fri, Feb 12, 7:25 AM

@ldionne Can I integrate this?

ldionne accepted this revision.Fri, Feb 12, 9:12 AM

This LGTM now.

otherwise please provide the exact suggestion for target_compile_definitions() .

Just FYI: https://cmake.org/cmake/help/latest/command/target_compile_definitions.html

This revision is now accepted and ready to land.Fri, Feb 12, 9:12 AM
This revision was automatically updated to reflect the committed changes.