implement clock_gettime, clock_getres, clock_settime by kevinwern · Pull Request #1100 · nix-rust/nix

kevinwern · 2019-07-24T04:24:43Z

See: https://pubs.opengroup.org/onlinepubs/007908799/xsh/clock_settime.html
Again related to: https://github.com/CraneStation/wasi-common/issues/16

Conditional inclusion of clock IDs based on looking through libc.

asomers · 2019-07-24T14:49:57Z

src/time.rs

+}
+
+pub fn clock_getres(clk_id: ClockId) -> Result<TimeSpec> {
+    let mut c_time = libc::timespec {tv_sec: 0, tv_nsec: 0};


Please use MaybeUninit instead of zero-initializing. That will cause the test to fail for now, but they'll pass as soon as #1096 is fixed.

MaybeUninit is the correct thing to do here, not zero-initialization.

asomers · 2019-07-24T14:55:09Z

test/test_time.rs

+pub fn test_clock_settime() {
+    require_capability!(CAP_SYS_TIME);
+    let ts = TimeSpec::from(timespec{tv_sec: 10000000, tv_nsec: 100});
+    let res = clock_settime(ClockId::CLOCK_REALTIME, ts).unwrap();


Lol, this will set the system time back to April 26, 1970! That's not a safe test. There's probably little you can do to test clock_settime. Perhaps it would be ok to set it to the current time.

asomers · 2019-07-24T14:55:58Z

test/test_time.rs

+
+#[test]
+pub fn test_clock_settime() {
+    require_capability!(CAP_SYS_TIME);


require_capability is a Linux-only macro.

kevinwern · 2019-07-31T18:56:07Z

Was making some of the changes but getting on a flight now...will follow up with the rest

asomers · 2019-08-07T01:48:35Z

src/time.rs

 libc_enum! {
-    #[repr(i32)]
+    #[cfg_attr(target_os = "darwin", repr(i32))]
+    #[cfg_attr(not(target_os = "darwin"), repr(u32))]


This isn't correct. Check the libc crate to see what the actual size is on various platforms:

> rg type.clockid_t linux_like/mod.rs 4:pub type clockid_t = ::c_int; newlib/mod.rs 4:pub type clockid_t = ::c_ulong; uclibc/mod.rs 6:pub type clockid_t = ::c_int; solarish/mod.rs 5:pub type clockid_t = ::c_int; haiku/mod.rs 9:pub type clockid_t = i32; redox/mod.rs 9:pub type clockid_t = ::c_int; hermit/mod.rs 39:pub type clockid_t = c_ulong; bsd/netbsdlike/mod.rs 10:pub type clockid_t = ::c_int; bsd/apple/mod.rs 16:pub type clockid_t = ::c_uint; bsd/freebsdlike/dragonfly/mod.rs 8:pub type clockid_t = ::c_ulong; bsd/freebsdlike/freebsd/mod.rs 6:pub type clockid_t = ::c_int;

asomers · 2019-08-07T01:52:49Z

src/time.rs

+}
+
+pub fn clock_getres(clk_id: ClockId) -> Result<TimeSpec> {
+    let mut c_time = libc::timespec {tv_sec: 0, tv_nsec: 0};


MaybeUninit is the correct thing to do here, not zero-initialization.

kevinwern · 2019-08-27T05:04:27Z

Sorry, ended up not having time for a while after pushing up some of the changes...will mark things as a WIP in that situation in the future. I should be around now, though.

Anyway, I updated the conditional compilation to reflect what is in libc and used MaybeUninit where we need to create and pass a timespec ourselves instead of expecting one from the caller.

Also decided to just ditch the set_* tests. Don't think there's much value in that, and I couldn't find a consistent, safe thing to use for all platforms (my computer uses NTP so I didn't catch that I had changed the clock ID to CLOCK_REALTIME in my stupid tinkering to create a "successful" test). I think I've been overtesting in my PRs anyway.

Let me know what you think. Sorry again for falling off for a bit.

asomers · 2019-09-03T22:04:35Z

src/time.rs

+                target_os = "illumos",
+                all(target_env = "newlib", target_arch = "arm"),
+               ), repr(i32))]
+    #[cfg_attr(target_os = "macos", repr(u32))]


Don't forget ios

asomers · 2019-09-03T22:05:47Z

src/time.rs

+                target_os = "emscripten",
+                target_os = "solaris",
+                target_os = "illumos",
+                all(target_env = "newlib", target_arch = "arm"),


Why the special case for target_arch on newlib? I don't see that in libc.

For newlib, the supported target_archs are only arm and aarch64. While the type clockid_t is defined here:
https://github.com/rust-lang/libc/blob/49c1d138e3b4719e21ee7cbce7dd903543cda80d/src/unix/newlib/mod.rs#L4

c_ulong is defined for each arch in their respective modules:
https://github.com/rust-lang/libc/blob/49c1d138e3b4719e21ee7cbce7dd903543cda80d/src/unix/newlib/arm/mod.rs#L5
https://github.com/rust-lang/libc/blob/49c1d138e3b4719e21ee7cbce7dd903543cda80d/src/unix/newlib/aarch64/mod.rs#L5

Just realized that I had put the "arm" case under i32 rather than u32. Hopefully makes more sense now.

asomers · 2019-09-03T22:09:40Z

src/time.rs

+                target_os = "netbsd",
+                target_os = "openbsd",
+                target_os = "haiku",
+                target_os = "linux",


This should be all(target_os = "linux", not(target_env = "newlib")) and likewise for android and emscripten.

Ah, that's very tricky. Added that in the case of everything that appears after newlib/uclibc. In some cases it had to be all(target_os = "<x>", not(target_env = "newlib", target_env = "uclibc")).

asomers · 2019-09-20T02:12:19Z

You should be able to fix the test failures by rebasing now.

asomers · 2019-09-28T16:30:55Z

Ok, I think this is good, but you need to squash before we can merge.

kevinwern · 2019-12-02T23:07:50Z

Squashed a while ago and just rebased again for the changelog. Hopefully good to merge now.

asomers

bors r+

1100: implement clock_gettime, clock_getres, clock_settime r=asomers a=kevinwern See: https://pubs.opengroup.org/onlinepubs/007908799/xsh/clock_settime.html Again related to: https://github.com/CraneStation/wasi-common/issues/16 Conditional inclusion of clock IDs based on looking through libc. Co-authored-by: Kevin Wern <kevin.m.wern@gmail.com>

bors · 2019-12-22T00:50:59Z

Build failed

continuous-integration/travis-ci/push

asomers · 2019-12-22T01:30:48Z

I've seen this before. I think it's a bug in glibc. It's certainly nothing to do with your PR.

bors retry

1100: implement clock_gettime, clock_getres, clock_settime r=asomers a=kevinwern See: https://pubs.opengroup.org/onlinepubs/007908799/xsh/clock_settime.html Again related to: https://github.com/CraneStation/wasi-common/issues/16 Conditional inclusion of clock IDs based on looking through libc. Co-authored-by: Kevin Wern <kevin.m.wern@gmail.com>

bors · 2019-12-22T01:57:59Z

Build failed

continuous-integration/travis-ci/push

asomers · 2019-12-23T03:21:48Z

Please rebase to fix the musl failure.

xonatius · 2020-07-26T04:38:09Z

I'm interested in some of these functions too for #1275. @kevinwern do you mind if I pick up this PR to implement them?

@kevinwern

1281: Added clock_gettime, clock_getres, clock_settime, clock_getcpuclockid r=asomers a=xonatius Picked up #1100 and added `clock_getcpuclockid` call as well. Credits to @kevinwern for the initial version. https://www.man7.org/linux/man-pages/man2/clock_gettime.2.html https://www.man7.org/linux/man-pages/man3/clock_getcpuclockid.3.html Closes #1275 Co-authored-by: Daniil Bondarev <xonatius@gmail.com>

cemeyer · 2021-09-07T02:34:43Z

Can we close this one after #1281?

asomers · 2021-09-07T23:50:46Z

Yep

kevinwern force-pushed the feature/implement-time-funcs branch 3 times, most recently from 14fce4c to 0b16e15 Compare July 24, 2019 13:50

asomers requested changes Jul 24, 2019

View reviewed changes

asomers reviewed Aug 7, 2019

View reviewed changes

asomers reviewed Sep 3, 2019

View reviewed changes

kevinwern force-pushed the feature/implement-time-funcs branch from e1b481f to e480d1e Compare September 17, 2019 02:38

kevinwern added a commit to kevinwern/nix that referenced this pull request Sep 17, 2019

CHANGELOG: move nix-rust#1100 (this) to unreleased after rebase

2a30f35

kevinwern added a commit to kevinwern/nix that referenced this pull request Sep 22, 2019

CHANGELOG: move nix-rust#1100 (this) to unreleased after rebase

d8fc27d

kevinwern force-pushed the feature/implement-time-funcs branch from 19f57d4 to 7a10cae Compare September 22, 2019 22:07

kevinwern force-pushed the feature/implement-time-funcs branch from 7a10cae to 8051751 Compare September 30, 2019 03:18

kevinwern force-pushed the feature/implement-time-funcs branch 2 times, most recently from d2a1c8d to e84c05b Compare December 2, 2019 22:04

implement clock_gettime, clock_getres, clock_settime

c9b4a36

kevinwern force-pushed the feature/implement-time-funcs branch from e84c05b to c9b4a36 Compare December 2, 2019 22:08

asomers approved these changes Dec 22, 2019

View reviewed changes

xonatius mentioned this pull request Aug 1, 2020

Added clock_gettime, clock_getres, clock_settime, clock_getcpuclockid #1281

Merged

asomers closed this Sep 7, 2021

Conversation

kevinwern commented Jul 24, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevinwern commented Jul 31, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevinwern commented Aug 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asomers commented Sep 20, 2019

Uh oh!

asomers commented Sep 28, 2019

Uh oh!

kevinwern commented Dec 2, 2019

Uh oh!

asomers left a comment

Choose a reason for hiding this comment

Uh oh!

bors bot commented Dec 22, 2019

Build failed

Uh oh!

asomers commented Dec 22, 2019

Uh oh!

bors bot commented Dec 22, 2019

Build failed

Uh oh!

asomers commented Dec 23, 2019

Uh oh!

xonatius commented Jul 26, 2020

Uh oh!

cemeyer commented Sep 7, 2021

Uh oh!

asomers commented Sep 7, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kevinwern commented Aug 27, 2019 •

edited

Loading