This is an archive of the discontinued LLVM Phabricator instance.

[SystemZ][ZOS] Porting the time functions within libc++ to z/OS
ClosedPublic

Authored by ldionne on Sep 18 2020, 1:41 PM.

Details

Summary

This patch is one part of many steps required to build libc++ and libc++abi libraries on z/OS. This particular deals with time related functions and consists of the following 3 parts.

  1. Initialization of :timeval within libc++ library need to be adjusted to work on z/OS.

The following is z/OS definition from time.h which includes additional aggregate member.
typedef signed int suseconds_t;
struct timeval {
time_t tv_sec;
char tv_usec_pad[4];
suseconds_t tv_usec;
};

In contracts the following is definition from time.h on Linux.

typedef long int suseconds_t;
struct timeval
{
time_t tv_sec;
__suseconds_t tv_usec;
};

  1. In addition, retrieving ::timespec within libc++ library needs to be adjusted to compensate the difference of some of the members of ::stat depending of the target host.

Here are the 2 members in conflict on z/OS extracted from stat.h.
struct stat {
...
time_t st_atime;
time_t st_mtime;
...
};
In contract here is Linux equivalent from stat.h.
struct stat
{
...
struct timespec st_atim;
struct timespec st_mtim;
...
};

  1. On Linux both members are of type timespec whereas on z/OS an object of type timespec need to be constructed first before retrieving it within libc++ library.

The libc++ header file __threading_support calls nanosleep, which is not available on z/OS.
The equivalent functionality will be implemented by using both sleep() and usleep().

Diff Detail

Event Timeline

zibi created this revision.Sep 18 2020, 1:41 PM
Herald added a reviewer: Restricted Project. · View Herald TranscriptSep 18 2020, 1:41 PM
zibi requested review of this revision.Sep 18 2020, 1:41 PM
ldionne requested changes to this revision.Sep 18 2020, 1:52 PM

Assuming you already have them, I'd love to see the rest of the patches in the queue to know what we're getting into.

Also, if we're supporting a new platform, it needs to be tested. This means you need to add a bot testing libcxx on your system to the llvm builders.

libcxx/include/__threading_support
5

Can z/OS implement nanosleep() like this, then? Sounds better to implement it once in your system library than in all the places it might be used.

libcxx/src/filesystem/filesystem_common.h
15

So IIUC, timeval on z/OS is non-conforming to https://pubs.opengroup.org/onlinepubs/007908775/xsh/systime.h.html?

If we start having such differences, I think it would be better to introduce a __libcpp_timeval typedef and a __libcpp_make_timeval(sec, nsec) function. We have similar "abstraction" layers in the threading library.

This revision now requires changes to proceed.Sep 18 2020, 1:52 PM
zibi updated this revision to Diff 293284.Sep 21 2020, 4:48 PM
zibi marked 2 inline comments as done.EditedSep 21 2020, 4:48 PM

I introduced TimeVal type and make_timeval(TimeSpec const&).

I did not introduced __libcpp_timeval since that would be struct ::timeval on all plafromts anyway.. Definition of ::timeval on z/OS is conforming to the standard since it provides 2 required members. Please refer to the actual definition in description.

zibi planned changes to this revision.Sep 21 2020, 5:15 PM

@ldionne Louis please have a look again. I was not sure if you got the notification.

libcxx/include/__threading_support
5

Yes, this is our goal to eventually have nanosleep() part of the OS and eventually remove this workaround.

libcxx/src/filesystem/filesystem_common.h
15

I will experiment with your suggestion and post a new patch if successful.

As for the non-conforming I'm not sure because z/OS do provide both tv_sev and tv_usec. It all boils down to the underlying type of suseconds_t.

zibi requested review of this revision.Sep 21 2020, 5:15 PM
zibi added a comment.Sep 23 2020, 6:11 AM

There are multiple Pre-merge checks warnings coming from clang-tidy. Half of them are for existing function posix_utimes and another half for new function make_timeval as follows:

  1. defining function in the header
  2. invalid case for function
  3. invalid case for parameter

I intend to ignore them otherwise, the style of the file will be mixed and hard to read. Perhaps this file should be excluded from tidy checks.

There is also the following build failure not related to this patch which I'm not sure how to report it to eventually fix it. It looks like it has been happening for couple months now.

error: ld.lld: error: unable to find library -lcxxabi_shared
hubert.reinterpretcast added inline comments.
libcxx/include/__threading_support
538–541

Minor comment: Wording suggestion.

546–547

If it was worthwhile to write the test as > 999999, then it is worthwhile to write the reduction in microseconds as -= 1000000.
Also, minor nit: Indentation.

552

Unfortunately, if a signal occurs, we have no convenient way of knowing how much time is left. We can still loop though.

libcxx/src/filesystem/filesystem_common.h
408–417

Let's use a more generic change here instead of a change that regresses z/OS under 31-bit.

zibi updated this revision to Diff 294364.Sep 25 2020, 10:52 AM
zibi marked 4 inline comments as done.

Thank you Hubert for comments. I specially like the part which eliminated #if statement.

libcxx/include/__threading_support
552

This is the loop I mentioned. It ensures we do sleep as long as requested but has the problem of not knowing how long we have already slept for.

zibi updated this revision to Diff 294701.Sep 28 2020, 7:46 AM
zibi marked an inline comment as done.

Thank you Hubert, now usleep will loop in case it was unsuccessful.

@ldionne, does this patch look okay to you? In terms of changes being made for z/OS, it's mainly going to be:

  • lack of LSB and BSD or GNU extension APIs,
  • older UNIX conformance level,
  • EBCDIC/ASCII mode support, and
  • versioning for IEEE floating point.

I understand that a lot of context would be needed for the latter two, but I believe that the first two is not particularly special and introduces only the usual amount of maintenance burden.
Has it been a norm to request a full set of patches from the get-go before accepting patches that adjust for API differences?

zibi added a comment.Oct 5 2020, 7:30 AM

ping

libcxx/include/__threading_support
552

This is the loop I mentioned. It ensures we do sleep as long as requested but has the problem of not knowing how long we have already slept for.

Thx, Hubert I was not sure if you wanted to make that change when you first time commented out.

@ldionne, does this patch look okay to you? In terms of changes being made for z/OS, it's mainly going to be:

I'm okay with the changes, but I don't think it makes sense to implement nanosleep() upstream. Are you willing to keep this workaround downstream until z/OS implements nanosleep? This also gives you a nice incentive to implement nanosleep(). :-)

  • lack of LSB and BSD or GNU extension APIs,
  • older UNIX conformance level,
  • EBCDIC/ASCII mode support, and
  • versioning for IEEE floating point.

I understand that a lot of context would be needed for the latter two, but I believe that the first two is not particularly special and introduces only the usual amount of maintenance burden.
Has it been a norm to request a full set of patches from the get-go before accepting patches that adjust for API differences?

There's been no norm so far, and libc++ has greatly suffered from that laxness. We now "support" several platforms that are not tested and have nobody working on them. That situation is bad for everyone: the platforms are not properly supported, and there's dead (or not dead?) code to account for these platforms everywhere. We can go one patch at a time, but be aware that we might push back on some changes if (1) they are too intrusive and (2) we don't have good ways to isolate those from the rest of the code, and hence they add a lot of unavoidable complexity. We'll try to find solutions to make it reasonable, of course.

I'm trying to start being more principled around new configurations/platform support. For example, are you able to provide a build bot to build on your system?

libcxx/src/filesystem/filesystem_common.h
414

I don't understand this comment.

zibi updated this revision to Diff 297319.Oct 9 2020, 1:21 PM

Removing confusing comment is the only delta.

@ldionne There are plans to have an external buildbot setup on z/OS. I’ll update with more details after discussing with the people trying to figure out the logistics.

As for the nanosleep() we would like to keep it upstream it should not be a burden since it's consolidated in one location and clearly guarded for z/OS only. The nanosleep() is on our list to implement downstream so the fact we are upstreaming will not minimize our effort to make that function available as part of OS.

Removing confusing comment is the only delta.

@ldionne There are plans to have an external buildbot setup on z/OS. I’ll update with more details after discussing with the people trying to figure out the logistics.

As for the nanosleep() we would like to keep it upstream it should not be a burden since it's consolidated in one location and clearly guarded for z/OS only. The nanosleep() is on our list to implement downstream so the fact we are upstreaming will not minimize our effort to make that function available as part of OS.

If you want to keep nanosleep() upstream, please:

  1. Put it in a separate header in src/zos/nanosleep.h and call the function nanosleep
  2. Just include the header in filesystem_common.h

That way, we won't need the #if in the middle of the logic. I might appear overly strict on not adding new #ifs, but these are the number one thing making libc++'s code complicated.

zibi updated this revision to Diff 298689.Oct 16 2020, 11:27 AM

@ldionne Sorry, I was away for few days. I hid nanosleep() in a separated header to make code in __threading_support cleaner. Please have a look.

zibi updated this revision to Diff 298695.Oct 16 2020, 11:39 AM

removing traces

zibi added inline comments.Oct 16 2020, 11:42 AM
libcxx/src/filesystem/filesystem_common.h
414

I don't understand this comment.

I don't understand either. The diff is misleading since this is NOT a new comment. I will delete this comment.

ldionne requested changes to this revision.Oct 28 2020, 10:41 AM
ldionne added inline comments.
libcxx/include/CMakeLists.txt
165 ↗(On Diff #298695)

Please put it below in the if (LIBCXX_INSTALL_SUPPORT_HEADERS), and not dependent on ZOS.

libcxx/include/support/ibm/nanosleep.h
16

This needs to be marked inline to avoid ODR violations.

libcxx/src/filesystem/filesystem_common.h
387

inline.

391

inline.

401

inline.

414

Let's take this occasion to make this inline.

This revision now requires changes to proceed.Oct 28 2020, 10:41 AM
zibi updated this revision to Diff 301724.Oct 29 2020, 1:20 PM
zibi marked 7 inline comments as done.

Thank you Louis for the review and suggestions. Please have a look again.

ldionne accepted this revision.Nov 10 2020, 4:46 AM

LGTM except nitpick about header guard. Please rebase onto master, let CI run, and then we're good to go.

libcxx/include/support/ibm/nanosleep.h
11

The header include guard should be _LIBCPP_SUPPORT_IBM_NANOSLEEP_H.

This revision is now accepted and ready to land.Nov 10 2020, 4:46 AM
zibi updated this revision to Diff 304286.Nov 10 2020, 11:55 AM
zibi marked an inline comment as done.

Thank you Louis, I updated as requested.

zibi updated this revision to Diff 304292.Nov 10 2020, 12:09 PM

set correct repo

zibi updated this revision to Diff 304615.Nov 11 2020, 11:53 AM

I'm resubmitting the same patch to redo CI to see if it will regressions.

zibi updated this revision to Diff 304824.Nov 12 2020, 7:09 AM

Fixing CI.

thakis added a subscriber: thakis.Nov 12 2020, 9:17 AM
thakis added inline comments.
libcxx/include/CMakeLists.txt
163 ↗(On Diff #304849)

Did you mean to remove the support headers up here? We now include all of them twice in files (except for nanosleep, which is added only below, not up here)

ldionne reopened this revision.Nov 12 2020, 10:38 AM

Looks like you had some problems with git.

This revision is now accepted and ready to land.Nov 12 2020, 10:38 AM
ldionne commandeered this revision.Nov 12 2020, 10:38 AM
ldionne edited reviewers, added: zibi; removed: ldionne.
This revision now requires review to proceed.Nov 12 2020, 10:38 AM
ldionne updated this revision to Diff 304900.Nov 12 2020, 10:45 AM

Re-upload a corrected patch. I'll let CI run and then I'll re-apply it myself.

ldionne updated this revision to Diff 304901.Nov 12 2020, 10:46 AM

Restore commit author to the real author, not me (for credit).

In the future, please also try to distil your commit message to something that explains the gist of the change. Going into many details might be useful for the Phabricator review, but not in the commit message itself.

ldionne updated this revision to Diff 304903.Nov 12 2020, 10:52 AM

Add missing nanosleep.h header to the header list. This must have been the cause of the confusion when checking-in the patch the first time around.

zibi added a comment.EditedNov 12 2020, 10:53 AM

Thank you Louis for resubmitting it, I already had the patch ready. Also thank you to Nico for spotting the issue.

zibi added a comment.Nov 12 2020, 1:06 PM

@ldionne Louis, please use the following as commit message:

This patch is one part of many steps required to build libc++ and libc++abi libraries on z/OS.
This particular deals with time related functions and consists of the following:

  1. Initialization of struct imeval within libc++ library need to be adjusted for additional aggregate member.
  2. Retrieving struct timespec have to be compensated for the difference of some of the members within `struct stat.
  3. The nanosleep() is not available on z/OS. This patch will provide equivalent functionality using both sleep() and usleep().
This revision was not accepted when it landed; it landed in state Needs Review.Nov 13 2020, 7:48 AM
Closed by commit rG6a8099e0f61f: [libc++] Port the time functions to z/OS (authored by zibi, committed by ldionne). · Explain Why
This revision was automatically updated to reflect the committed changes.