This is an archive of the discontinued LLVM Phabricator instance.

[SystemZ][z/OS] correct rc and errno within nanosleep()
ClosedPublic

Authored by zibi on Mar 25 2021, 1:16 PM.

Details

Summary

This patch fixes rc and errno within nanosleep(). It also updates __rem parameter as well it introduces cast to handle conversions from long into unsigned int to avoid warnings.

Diff Detail

Event Timeline

zibi requested review of this revision.Mar 25 2021, 1:16 PM
zibi created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptMar 25 2021, 1:16 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
zibi updated this revision to Diff 333566.Mar 26 2021, 9:02 AM

Fix issues with rc and errno as well adding cast for conversion warrnings.

zibi edited the summary of this revision. (Show Details)Mar 26 2021, 11:07 AM
zibi updated this revision to Diff 333597.Mar 26 2021, 11:09 AM

fixing CI failure

Mordante accepted this revision as: Mordante.Mar 31 2021, 12:06 PM
Mordante added a subscriber: Mordante.

Some minor issues, other than that LGTM.

libcxx/include/__support/ibm/nanosleep.h
46

rc -> __rc.

49

Should we add a comment this value might be too large?

ldionne requested changes to this revision.Mar 31 2021, 2:42 PM

This commit message is highly misleading. This is rewriting the function, not only tuning down warning levels. Can you please delimit more clearly what your change is doing if you want a meaningful review? It might be a good idea to break it down into more than one patch, too.

This revision now requires changes to proceed.Mar 31 2021, 2:42 PM

By the way, my comments are made in the spirit of encouraging good review practices more than anything. I otherwise don't have a stake in this patch, since this is z/OS which is not officially supported anyway (since it lacks CI).

zibi updated this revision to Diff 334696.Apr 1 2021, 7:52 AM
zibi marked 2 inline comments as done.

addressing comments

zibi added a comment.Apr 1 2021, 7:57 AM

This commit message is highly misleading.

Sorry about that, I fixed the description so it should be clear now.

zibi retitled this revision from [SystemZ][z/OS] tune down warning about unused parameter on nanosleep() to [SystemZ][z/OS] correct rc and errno within nanosleep().Apr 1 2021, 8:03 AM
zibi edited the summary of this revision. (Show Details)
zibi edited the summary of this revision. (Show Details)Apr 1 2021, 8:08 AM
Mordante added inline comments.Apr 1 2021, 9:28 AM
libcxx/include/__support/ibm/nanosleep.h
40

Please use complete sentences starting with a capital and ending with a full stop.

50

I think I wasn't clear enough regarding what I meant with too large.
When nanosleep fails with EINTR it should set the remaining time in __rem.
When it fails during sleep it sets the proper value (except rounding up).
When it fails during usleep with EINTR it acts as if it slept for 0 µs, which might be wrong. AFAIK it's not possible to fix this unless usleep on z/OS provides more information.

So I like the mentioning of the rounding up, but I would also like some information regarding ignoring the time slept.

Mordante requested changes to this revision.Apr 1 2021, 9:29 AM
This revision now requires changes to proceed.Apr 1 2021, 9:29 AM
zibi updated this revision to Diff 334759.Apr 1 2021, 10:41 AM
zibi edited the summary of this revision. (Show Details)

adding short comments about implementation

zibi marked an inline comment as done.Apr 1 2021, 10:43 AM
zibi added inline comments.
libcxx/include/__support/ibm/nanosleep.h
50

Thanks, have a look at new comments.

Mordante accepted this revision as: Mordante.Apr 4 2021, 1:49 AM

Thanks for the update. LGTM!

curdeius added inline comments.
libcxx/include/__support/ibm/nanosleep.h
50

Typo. But this comment sounds a bit awkward to me.

zibi marked 2 inline comments as done.Apr 6 2021, 8:35 AM
zibi added inline comments.
libcxx/include/__support/ibm/nanosleep.h
50

Thx for catching this. I will rewrite the comment and land this patch.

This revision was not accepted when it landed; it landed in state Needs Review.Apr 6 2021, 8:37 AM
This revision was automatically updated to reflect the committed changes.
zibi marked an inline comment as done.

@zibi Please do *not* commit patches when the libcxx review group is not green. The policy for committing to libc++ is clear - you have to wait for the libcxx group to be green, or you will get reverted.

This is OK for now and I'm sure you did not intend to do wrong, but please follow the process in the future.

zibi added a comment.Apr 9 2021, 5:27 AM

@zibi Please do *not* commit patches when the libcxx review group is not green. The policy for committing to libc++ is clear - you have to wait for the libcxx group to be green, or you will get reverted.

This is OK for now and I'm sure you did not intend to do wrong, but please follow the process in the future.

@ldionne Yes, sorry about that, I'm still learning the process.
The last commit was [NFC] so I thought it will be fine but will wait in the future and thank you for brining this up to my attention.