std.posix: Rewriting enums to seperate clock IDs for clock_gettime() and timerfd_create() #21347

chrboesch · 2024-09-08T19:01:44Z

In the linux kernel header file (time.h) is the note, that the driver implementing the clock IDs

SGI_CYCLE
TAI
got removed. Therefore, calling timerfd_create() with these IDs will result in an error. So I disabled them.

In the linux kernel header file (time.h) is the note, that the driver implementing the clock IDs - SGI_CYCLE - TAI got removed. Therefore, calling timerfd_create() with these IDs will result in an error. So I disabled them.

alexrp

This looks fine to me.

However, I do not believe this fixes #20334 because there are still more clocks defined here than timerfd_create() will accept. And it seems like there needs to be some discussion on that issue as to how that should be addressed.

chrboesch · 2024-09-10T08:24:51Z

I have tried every other clock ID on my linux computer and it works fine and as expected. And since the clock IDs are defined as enums, contrary to the assumption of the thread creator, you can only select the ones entered. So it will definitely fix the issue.

alexrp · 2024-09-10T08:30:27Z

So just to be clear, even clocks like PROCESS_CPUTIME_ID and THREAD_CPUTIME_ID work with timerfd_create()?

chrboesch · 2024-09-10T08:45:33Z

The fact that you ask means it doesn't work on your computer. Let me do some testing on different computers.

alexrp · 2024-09-10T09:01:25Z

No no, I haven't actually tried. I'm only asking because those specifically are somewhat special clocks, so I just wouldn't take it for granted that they work with timerfd_create(). I'm happy to take your word for it if you've tested them and they worked.

chrboesch · 2024-09-10T10:06:13Z

First of all, thanks for your skepticism. It was my bad that I did some quick tests in C on my Gentoo with self-compiled kernel. And that I mixed timerfd_create with getclock_time because there is no separation in the header file time.h. Interestingly, the timers even work in Zig even though they shouldn't. But only on my self-compiled kernel. And sometimes only with root rights.

However, the man page timerfd_create(2) says there are actually only two valid timers: CLOCK_REALTIME and CLOCK_MONOTONIC. So, I think we should limit the enum of these clock IDs, because it doesn't make sense if a timer sometimes works and sometimes doesn't. And it's hard to explain that you can achieve this with some special kernel settings. 😉

What surprises me a little, however, is that the equivalent POSIX command is 'timer_create'. And it also knows CLOCK_PROCESS_CPUTIME_ID and CLOCK_THREAD_CPUTIME_ID, but is triggered by different Linux commands. In other words, the over all change should not only be limited to restricting the enum, but also to correctly mapping the POSIX command.

TL;DR
My suggestion: Firstly, we limit the enum to the actually possible clock IDs (that fixes the issue) and secondly, we should correct the POSIX command later. What do you think?

Addendum: Since clock_id is also used in Zig for clock_gettime, this is a bit more complicated. We therefore need to have a second enum just for timercreate_fd. That's probably what the issue creator meant?

Addendum 2: To add to the confusion, Zig uses clockid and clockid_t equivalently, which is incorrect, as Linux distinguishes between them. This makes it difficult to simply add an enum clockid.

Addendum 3: It's not Zig's fault, it's Linux's fault. There, the term 'clockid' is used in the same way in different contexts, which contributes to this confusion. These are the things that contribute to instability and errors in operation because developers mistakenly confuse the two. I'll think about how we can make this better for Zig.

alexrp · 2024-09-10T10:43:47Z

First of all, thanks for your skepticism. It was my bad that I did some quick tests in C on my Gentoo with self-compiled kernel. And that I mixed timerfd_create with getclock_time because there is no separation in the header file time.h. Interestingly, the timers even work in Zig even though they shouldn't. But only on my self-compiled kernel. And sometimes only with root rights.

It wouldn't surprise me if some distros disable those clocks I mentioned for non-superusers out of security concerns or something like that. But I'm just guessing here.

However, the man page timerfd_create(2) says there are actually only two valid timers: CLOCK_REALTIME and CLOCK_MONOTONIC. So, I think we should limit the enum of these clock IDs, because it doesn't make sense if a timer sometimes works and sometimes doesn't. And it's hard to explain that you can achieve this with some special kernel settings. 😉

Generally speaking, the Linux kernel code is the source of truth; the man pages are sometimes incomplete or wrong. I think we should base our decision on what the kernel code is written to allow. If it is the case (and I don't know if it is) that the kernel can return EINVAL for some clock IDs depending on factors outside of the programmer's control, then EINVAL should be considered an expected error and be converted to a Zig error accordingly.

I think all the Linux system calls that take clock IDs should be checked to see what's allowed before we decide what to do here (other than removing SGI_CYCLE and TAI, which should be uncontroversial).

chrboesch · 2024-09-10T10:49:59Z

Ok, I removed the "fixes #20334", because it doesn't. So the PR is valid. And then we should discuss in the issue what we should do to solve the problem in general.

…h clock_gettime() As explained in ziglang#2034 and also discussed here, now the complete adjustment to resolve the issue.

calling for clock_gettime() for getting the timespec now as return value.

forgotten changes for local testing This reverts commit 6c0a56d.

error

system.timerfd_create should get the right integer param.

rootbeer

I like the cleanup of clock_gettime(). I've got a couple drive-by comments, but feel free to ignore them if they don't make sense to you.

rootbeer · 2024-09-11T20:02:10Z

lib/std/os/linux.zig

+    // * The driver implementing this got removed. The clock ID is kept as a
+    // * place holder. Do not reuse!
+    // Therefore, calling clock_gettime() with these IDs will result in an error.
+    // SGI_CYCLE = 10,


Technically these could be useful if someone wants to compile a Zig program to run against an old Linux kernel that supports these old clocks? (I suspect that's really unlikely in this case though?) Anyway, it seems a bit odd for Zig to only track what is currently supported, so maybe its just sufficient to add a comment that these clocks are not supported on modern systems?

SGI_CYCLE = 10, // Linux dropped support for this clock in the v4.13 timeframe TAI = 11, // Linux dropped support for this clock in the v4.13 timeframe

I may be missing some context on these clocks, so feel free to ignore this if its not compelling to you.

TAI_CLOCK was designed as CLOCK_REALTIME(UTC) + tai_offset, but tai_offset was always 0. So there is no point in using this clock, and no one did.
SGI_CYCLE was for Silicon Graphics (SGI) workstations, which are probably no longer in use, so it makes sense to disable these two.

rootbeer · 2024-09-11T20:07:51Z

lib/std/os/linux.zig

+
+pub const CLOCK_ID = clock_id;
+
+pub const clock_id = enum(u32) {


Can you add a short comment here and on clockid_t differentiating them? (I think just labelling which API/syscall they're aimed at would be sufficient? clock_gettime vs timerfd_create?)

yes, makes sense.

rootbeer · 2024-09-11T20:16:12Z

lib/std/posix/test.zig

@@ -1121,7 +1121,7 @@ test "access smoke test" {
 test "timerfd" {
    if (native_os != .linux) return error.SkipZigTest;

-    const tfd = try posix.timerfd_create(.MONOTONIC, .{ .CLOEXEC = true });
+    const tfd = try posix.timerfd_create(posix.clock_id.MONOTONIC, .{ .CLOEXEC = true });


The posix.clock_id. prefix shouldn't be necessary here, Zig should pick the right .MONOTONIC from the type signature of timerfd_create.

…it. So the declaration of the enum must be i32 for the C export.

chrboesch · 2024-09-12T14:16:24Z

Now this PR fixes #20334.

alexrp · 2024-09-14T15:55:27Z

lib/std/os/linux.zig

@@ -2054,10 +2054,10 @@ pub fn eventfd(count: u32, flags: u32) usize {
    return syscall2(.eventfd2, count, flags);
 }

-pub fn timerfd_create(clockid: clockid_t, flags: TFD) usize {


I haven't looked at the PR in a few days so I might be missing some context here - why are we changing this to an integer?

I based my work on the kernel sources, there it is an integer.

I didn't mean it quite that literally when I said the kernel code should be the basis for our decision here. 🙂 We do use enums in syscall wrappers when the set of acceptable values is known ahead of time - which I think is the case here?

Was unsure but can change that.

Disable no longer used clock IDs in Linux

cfcda2b

In the linux kernel header file (time.h) is the note, that the driver implementing the clock IDs - SGI_CYCLE - TAI got removed. Therefore, calling timerfd_create() with these IDs will result in an error. So I disabled them.

chrboesch changed the title ~~Disable no longer used clock IDs in Linux~~ std.os.linux: Disable no longer used clock IDs Sep 8, 2024

alexrp approved these changes Sep 10, 2024

View reviewed changes

chrboesch mentioned this pull request Sep 10, 2024

std.posix.timerfd_create can reach unreachable code #20334

Open

chrboesch added 3 commits September 11, 2024 11:43

Added enum for timerfd_create() and resolved some inconsistencies wit…

923562d

…h clock_gettime() As explained in ziglang#2034 and also discussed here, now the complete adjustment to resolve the issue.

Changed calling clock_gettime() to get return timespec

afd900f

Changed calling clock_gettime() to get return timespec

0496bd7

chrboesch requested a review from kprotty as a code owner September 11, 2024 10:01

chrboesch added 3 commits September 11, 2024 12:14

Fixed callig convention of timerfd_create() in c.zig and corrected the

afe3c39

calling for clock_gettime() for getting the timespec now as return value.

Fixed catch block for calling clock_gettime()

338b64d

Added clock_id struct for Linux and FreeBSD

224b7dd

chrboesch changed the title ~~std.os.linux: Disable no longer used clock IDs~~ std.posix: Rewriting enums to seperate clock IDs for clock_gettime() and timerfd_create() Sep 11, 2024

chrboesch added 7 commits September 11, 2024 16:06

Posix test adjusted

1e5ff2c

Resolving the struct 'clock_id' via the system path

6c0a56d

Revert "Resolving the struct 'clock_id' via the system path", because of

4069e01

forgotten changes for local testing This reverts commit 6c0a56d.

Resolving the new struct 'clock_id' with system path, to fix the testing

f44a471

error

Merge branch 'master' into i20334

ff42ad0

Moved the @intFromEnum convertion to posix.timerfd_create, because of

39a76f6

system.timerfd_create should get the right integer param.

Merge remote-tracking branch 'refs/remotes/origin/i20334' into i20334

9e65d26

rootbeer reviewed Sep 11, 2024

View reviewed changes

chrboesch added 3 commits September 11, 2024 23:29

Fixed the linux test and added some comments.

b1787f7

Fixed incorrectly declared enum (as u32 instead of usize)

2ad5f17

Added @intcast to syscall, because on a 32bit machine usize isn't 64b…

ed2d918

…it. So the declaration of the enum must be i32 for the C export.

chrboesch added 2 commits September 12, 2024 12:29

Merge branch 'master' into i20334

ee09b1d

Fiexed forgotten usize changing in i32

b8a9cb4

alexrp reviewed Sep 14, 2024

View reviewed changes

chrboesch added 3 commits September 15, 2024 17:58

Merge branch 'master' into i20334

4b8686b

Changed parameter type from int to enum.

08c5c4c

Merge branch 'master' into i20334

af8f912

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

std.posix: Rewriting enums to seperate clock IDs for clock_gettime() and timerfd_create() #21347

std.posix: Rewriting enums to seperate clock IDs for clock_gettime() and timerfd_create() #21347

chrboesch commented Sep 8, 2024 •

edited

Loading

alexrp left a comment

chrboesch commented Sep 10, 2024

alexrp commented Sep 10, 2024

chrboesch commented Sep 10, 2024

alexrp commented Sep 10, 2024 •

edited

Loading

chrboesch commented Sep 10, 2024 •

edited

Loading

alexrp commented Sep 10, 2024

chrboesch commented Sep 10, 2024

rootbeer left a comment

rootbeer Sep 11, 2024

chrboesch Sep 11, 2024

rootbeer Sep 11, 2024

chrboesch Sep 11, 2024 •

edited

Loading

rootbeer Sep 11, 2024

chrboesch Sep 11, 2024 •

edited

Loading

chrboesch commented Sep 12, 2024

alexrp Sep 14, 2024

chrboesch Sep 14, 2024

alexrp Sep 14, 2024

chrboesch Sep 15, 2024


		pub const CLOCK_ID = clock_id;

		pub const clock_id = enum(u32) {

std.posix: Rewriting enums to seperate clock IDs for clock_gettime() and timerfd_create() #21347

Are you sure you want to change the base?

std.posix: Rewriting enums to seperate clock IDs for clock_gettime() and timerfd_create() #21347

Conversation

chrboesch commented Sep 8, 2024 • edited Loading

alexrp left a comment

Choose a reason for hiding this comment

chrboesch commented Sep 10, 2024

alexrp commented Sep 10, 2024

chrboesch commented Sep 10, 2024

alexrp commented Sep 10, 2024 • edited Loading

chrboesch commented Sep 10, 2024 • edited Loading

alexrp commented Sep 10, 2024

chrboesch commented Sep 10, 2024

rootbeer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrboesch Sep 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrboesch Sep 11, 2024 • edited Loading

Choose a reason for hiding this comment

chrboesch commented Sep 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrboesch commented Sep 8, 2024 •

edited

Loading

alexrp commented Sep 10, 2024 •

edited

Loading

chrboesch commented Sep 10, 2024 •

edited

Loading

chrboesch Sep 11, 2024 •

edited

Loading

chrboesch Sep 11, 2024 •

edited

Loading