[SystemZ][ZOS] Provide CLOCK_MONOTONIC alternative

[zibi]

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.

[mclow.lists]

Starting in C++11, the standard library requires a monotonic clock.

There was a discussion about this at the time in the standards committee, with the question asked:
"You realize that you're requiring systems to have a monotonic clock, right"?
and it was answered: "Yes, we realize that and we're fine with that"

That being said, Libc++ has a mode which does not require a monotonic clock.
The configuration is guarded by _LIBCPP_HAS_NO_THREADS

[zibi]

In D93542#2463142, @mclow.lists wrote:

That being said, Libc++ has a mode which does not require a monotonic clock.
The configuration is guarded by _LIBCPP_HAS_NO_THREADS

Thank you Marshall.
Yes, I know about _LIBCPP_HAS_NO_THREADS but I'm not sure if we want to go that route and what implication that would mean because we do have threads so disabling them seems like a BIG hammer approach.

[mclow.lists]

If you don't have a steady clock, then none of the timed synchronization calls can work.

[zibi]

Adding gettimeofday() equivalent for CLOCK_MONOTONIC for z/OS.

[ldionne]

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.

[ldionne]

In D93542#2495987, @ldionne wrote:

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]

In D93542#2495987, @ldionne wrote:

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]

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

[zibi]

Removing trailing character and resubmitting.

[ldionne]

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

227

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

[hubert.reinterpretcast]

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

232

[hubert.reinterpretcast]

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

232

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]

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]

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

[zibi]

checking CI

[zibi]

format + tidy

[hubert.reinterpretcast]

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

29

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

[zibi]

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.

[hubert.reinterpretcast]

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

libcxx/include/support/ibm/gettod_zos.h

11–12

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

libcxx/src/chrono.cpp

14

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]

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

14

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.

[hubert.reinterpretcast]

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]

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]

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]

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]

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

[hubert.reinterpretcast]

libcxx/src/chrono.cpp

14

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

[zibi]

synch with __support rename

[hubert.reinterpretcast]

libcxx/src/chrono.cpp

14

Missed update here?

[zibi]

Oops, thx Hubert.

[hubert.reinterpretcast]

From my end, LGTM.

[ldionne]

libcxx/src/CMakeLists.txt

5–10

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]

libcxx/src/CMakeLists.txt

5–10

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

[zibi]

Moving macro to chrono.cpp.

[zibi]

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

libcxx/src/CMakeLists.txt

5–10

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]

@ldionne Can I integrate this?

[ldionne]

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

libcxx/include/CMakeLists.txt

set(files
   stdlib.h
   streambuf
   string
   string.h
   string_view
   strstream
   support/android/locale_bionic.h
   support/fuchsia/xlocale.h
+  support/ibm/gettod_zos.h
   support/ibm/limits.h
   support/ibm/locale_mgmt_aix.h
   support/ibm/nanosleep.h
   support/ibm/support.h
   support/ibm/xlocale.h
   support/musl/xlocale.h
   support/newlib/xlocale.h
   support/nuttx/xlocale.h

libcxx/include/support/ibm/gettod_zos.h

+// -*- C++ -*-
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef _LIBCPP_SUPPORT_IBM_GETTOD_ZOS_H
+#define _LIBCPP_SUPPORT_IBM_GETTOD_ZOS_H
+
+#include <time.h>
+
+static inline int gettimeofdayMonotonic(struct timespec64* Output) {
+
+  // The POSIX gettimeofday() function is not available on z/OS. Therefore,
+  // we will call stcke and other hardware instructions in implement equivalent.
+  // Note that nanoseconds alone will overflow when reaching new epoch in 2042.
+
+  struct _t {
+    uint64_t Hi;
+    uint64_t Lo;
+  };
+  struct _t Value = {0, 0};
+  uint64_t CC = 0;
+  asm(" stcke %0\n"
+      " ipm %1\n"
+      " srlg %1,%1,28\n"
+      : "=m"(Value), "+r"(CC)::);
+
+  if (CC != 0) {
+    errno = EMVSTODNOTSET;
+    return CC;
+  }
+  uint64_t us = (Value.Hi >> 4);
+  uint64_t ns = ((Value.Hi & 0x0F) << 8) + (Value.Lo >> 56);
+  ns = (ns * 1000) >> 12;
+  us = us - 2208988800000000;
+
+  register uint64_t DivPair0 asm("r0"); // dividend (upper half), remainder
+  DivPair0 = 0;
+  register uint64_t DivPair1 asm("r1"); // dividend (lower half), quotient
+  DivPair1 = us;
+  uint64_t Divisor = 1000000;
+  asm(" dlgr %0,%2" : "+r"(DivPair0), "+r"(DivPair1) : "r"(Divisor) :);
+
+  Output->tv_sec = DivPair1;
+  Output->tv_nsec = DivPair0 * 1000 + ns;
+  return 0;
+}
+
+#endif // _LIBCPP_SUPPORT_IBM_GETTOD_ZOS_H

libcxx/src/CMakeLists.txt

 set(LIBCXX_LIB_CMAKEFILES_DIR "${CMAKE_CURRENT_BINARY_DIR}${CMAKE_FILES_DIRECTORY}"  PARENT_SCOPE)
 
+# As part of monotonic clock support on z/OS we need macro _LARGE_TIME_API
+# to be defined before any system header is included for struct timespec64.
+if (ZOS)
+  set_source_files_properties(
+    chrono.cpp
+    PROPERTIES COMPILE_DEFINITIONS "_LARGE_TIME_API"
+    )
+endif()
+
 # Get sources
 set(LIBCXX_SOURCES
   algorithm.cpp
   any.cpp
   atomic.cpp
   barrier.cpp
   bind.cpp
   charconv.cpp

libcxx/src/chrono.cpp

 //===------------------------- chrono.cpp ---------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "chrono"
 #include "cerrno"        // errno
 #include "system_error"  // __throw_system_error
+
+#if defined(__MVS__)
+#include <support/ibm/gettod_zos.h> // gettimeofdayMonotonic
+#endif
+
 #include <time.h>        // clock_gettime and CLOCK_{MONOTONIC,REALTIME,MONOTONIC_RAW}
 #include "include/apple_availability.h"
 
 #if __has_include(<unistd.h>)
 # include <unistd.h>
 #endif
 
 #if __has_include(<sys/time.h>)
 # include <sys/time.h> // for gettimeofday and timeval
 #endif
 
 #if !defined(__APPLE__) && _POSIX_TIMERS > 0
 # define _LIBCPP_USE_CLOCK_GETTIME
 #endif
 
 #if defined(_LIBCPP_WIN32API)
 #  define WIN32_LEAN_AND_MEAN
 #  define VC_EXTRA_LEAN
 #  include <windows.h>
 #  if _WIN32_WINNT >= _WIN32_WINNT_WIN8
 #    include <winapifamily.h>
static steady_clock::time_point __libcpp_steady_clock_now() {
   LARGE_INTEGER counter;
   (void) QueryPerformanceCounter(&counter);
   auto seconds = counter.QuadPart / freq.QuadPart;
   auto fractions = counter.QuadPart % freq.QuadPart;
   auto dur = seconds * nano::den + fractions * nano::den / freq.QuadPart;
   return steady_clock::time_point(steady_clock::duration(dur));
 }
 
+#elif defined(__MVS__)
+
+static steady_clock::time_point __libcpp_steady_clock_now() {
+  struct timespec64 ts;
+  if (0 != gettimeofdayMonotonic(&ts))
+    __throw_system_error(errno, "failed to obtain time of day");
+
+  return steady_clock::time_point(seconds(ts.tv_sec) + nanoseconds(ts.tv_nsec));
+}
+
 #elif defined(CLOCK_MONOTONIC)
 
 static steady_clock::time_point __libcpp_steady_clock_now() {
     struct timespec tp;
     if (0 != clock_gettime(CLOCK_MONOTONIC, &tp))
         __throw_system_error(errno, "clock_gettime(CLOCK_MONOTONIC) failed");
     return steady_clock::time_point(seconds(tp.tv_sec) + nanoseconds(tp.tv_nsec));
 }