Make clock API POSIX-compliant and implement clock IDs by adamgreloch · Pull Request #538 · phoenix-rtos/libphoenix

adamgreloch · 2026-09-30T12:09:56Z

TASK: RTOS-1472

Fixes: phoenix-rtos/phoenix-rtos-project#1685

Description

Motivation and Context

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Chore (refactoring, style fixes, git/CI config, submodule management, no code logic changes)

How Has This Been Tested?

Already covered by automatic testing.
New test added: (add PR link here).
Tested by hand on: (list targets here).

Checklist:

My change requires a change to the documentation.
I have updated the documentation accordingly.
I have added tests to cover my changes.
All new and existing linter checks and tests passed.
My changes generate no new compilation warnings for any of the targets.

Special treatment

This PR needs additional PRs to work (list the PRs, preferably in merge-order).
!proc: track thread/process sys/user time, add sys_cpuclockGet and sys_times syscalls phoenix-rtos-kernel#847
I will merge this PR by myself when appropriate.

gemini-code-assist

Code Review

This pull request introduces support for process and thread CPU-time clocks, implementing POSIX-compliant APIs such as pthread_getcpuclockid, clock_getcpuclockid, and clock_getres, while expanding clock_gettime and clock_nanosleep capabilities. It also refactors internal priority and tick interval handling, and updates the times system call implementation. Feedback on these changes highlights critical safety and portability improvements: first, pthread_getcpuclockid should verify thread existence via the thread list to prevent segmentation faults on invalid thread IDs; second, clock_nanosleep must validate that the req timespec pointer is non-NULL before dereferencing; and third, the CPU clock macros in common/cpuclock.h should cast the clock ID to an unsigned type to avoid implementation-defined behavior during right-shift operations.

github-actions · 2026-09-30T12:49:01Z

Unit Test Results

13 884 tests +154 13 024 ✅ +153 1h 6m 27s ⏱️ + 9m 55s
791 suites + 11 859 💤 ± 0
1 files ± 0 1 ❌ + 1

For more details on these failures, see this check.

Results for commit ef2d465. ± Comparison against base commit 9bffcee.

♻️ This comment has been updated with latest results.

Darchiv

Left a few comments.

Darchiv · 2026-10-01T13:46:13Z

@@ -564,15 +564,29 @@ int nanosleep(const struct timespec *req, struct timespec *rem)
 /* clock_nanosleep() shall return **positive** errors codes directly, without setting errno */
 int clock_nanosleep(clockid_t clock, int flags, const struct timespec *req, struct timespec *rem)


Function parameter names are different than in POSIX

I see that there are multiple functions with argument naming deviating from POSIX spec. Will do this in a separate PR in one sweep.

Darchiv · 2026-10-01T13:46:50Z

+#include <sys/time.h>
 #include <sys/types.h>
 #include <sys/times.h>
+#include <time.h>


Any reason not to sort these alphabetically?

Applies in other places too

maybe let's do it for the whole libphoenix in one sweep in a separate PR

Darchiv · 2026-10-02T08:15:46Z

+		us = __getSystickInterval();
+	}
+	else if (clock_id == CLOCK_REALTIME || clock_id == CLOCK_MONOTONIC || clock_id == CLOCK_MONOTONIC_RAW) {
+		us = 1;


Well, the actual resolution for CLOCK_REALTIME depends on the per-platform hardware clock used by the kernel. Usually this is a typical 32,768 kHz calibrated built-in RTC, so the resolution would be ~30 us. This aligns with clock_settime() POSIX behavior, which truncates the actually set time to match the resolution (e.g. a write and read from HW RTC registers will yield a truncated value).

I think platform-dependent parameters should at least be defined by kernel per-arch headers. Maybe it should also be a part of sysctl-like mechanism to query/set the kernel tick frequency and other runtime parameters.

Or maybe this can't be done due to compatibility concerns?

(Of course we could enhance the realtime clock by complementing it with a high-resolution clock in short timeframes, but I'm almost positive this is only in a sphere of dreams, so your 1 us seems not to come from that.)

True, but userspace doesn't need to know the real resolution. It must be the smallest increment by which the clock can change, so 1 us is fine for ~30 us clocks.

I agree that, long term, it would be nice to have this sorted out precisely, but from the POSIX perspective it's not required. I'd prefer to exploit this fact, since currently we don't have the API to pass the true resolution to the userspace.

Darchiv · 2026-10-02T09:43:43Z

 clock_t clock(void)
 {
-	return (clock_t)-1;
+	time_t res = cpuclockGet(0, 0);


Is this value normalized so that it can be used with CLOCKS_PER_SEC? Note that the (legacy) functions clock() and times() operate on different clocks - clock() returns the number of ticks which must be divided by CLOCKS_PER_SEC to get the time and times() returns other, "virtual" number of ticks which must be divided by the value of sysconf(_SC_CLK_TCK). to get the time.

Why "virtual" and not real ticks? If we decide that system tick interval can be changed at runtime, then the output of times() would not be comparable with other invocations' output and, more importantly, sysconf(_SC_CLK_TCK) would need to be a dynamic variable. So, times() can be implemented in the same way as clock(), just with a (possibly) different "clocks per sec" constant.

Let's treat clock() and times() as legacy functions mainly used by ports, so that their behavior does not affect how we account time in the kernel.

If you ask whether it is POSIX compliant, it is. Kernel provides microseconds, clock() is supposed to return a value sanely divisible by CLOCKS_PER_SEC which could very well be called MS_PER_SEC, since it is specified to be equal 1000000.

Let's treat clock() and times() as legacy functions mainly used by ports, so that their behavior does not affect how we account time in the kernel.

Agreed.

nsleep is a native syscall, so it should operate on PH_CLOCK_* definitions. TASK: RTOS-1472

Breaking change: depends on newly added sys_cpuTime syscall and PH_CLK_TCK definition. TASK: RTOS-1472

TASK: RTOS-1472

gemini-code-assist Bot reviewed Sep 30, 2026

View reviewed changes

Comment thread pthread/pthread.c

Comment thread time/time.c

Comment thread common/cpuclock.h Outdated

adamgreloch force-pushed the adamgreloch/RTOS-1472 branch from c0e836d to cea7694 Compare September 30, 2026 15:01

adamgreloch marked this pull request as ready for review September 30, 2026 15:01

adamgreloch requested a review from a team September 30, 2026 15:01

Darchiv reviewed Oct 2, 2026

View reviewed changes

adamgreloch force-pushed the adamgreloch/RTOS-1472 branch from cea7694 to 9b8b952 Compare October 2, 2026 16:14

adamgreloch added 5 commits October 2, 2026 18:33

!pass phx clock to nsleep, allow PH_CLOCK_REALTIME

797f6b8

nsleep is a native syscall, so it should operate on PH_CLOCK_* definitions. TASK: RTOS-1472

!sys/times: implement times()

460b1cf

Breaking change: depends on newly added sys_cpuTime syscall and PH_CLK_TCK definition. TASK: RTOS-1472

sys/sched: expose systick interval through __getSystickInterval

0708001

TASK: RTOS-1472

implement per-process and per-thread CPU clock ids

c5e1145

TASK: RTOS-1472

time/time: implement clock() with sys_cpuTime()

ef2d465

TASK: RTOS-1472

adamgreloch force-pushed the adamgreloch/RTOS-1472 branch from 9b8b952 to ef2d465 Compare October 2, 2026 16:33

		@@ -564,15 +564,29 @@ int nanosleep(const struct timespec req, struct timespec rem)
		/* clock_nanosleep() shall return positive errors codes directly, without setting errno */
		int clock_nanosleep(clockid_t clock, int flags, const struct timespec req, struct timespec rem)

Conversation

adamgreloch commented Sep 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

Types of changes

How Has This Been Tested?

Checklist:

Special treatment

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Sep 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Unit Test Results

Uh oh!

Darchiv left a comment

Choose a reason for hiding this comment

Uh oh!

Darchiv Oct 1, 2026

Choose a reason for hiding this comment

Uh oh!

adamgreloch Oct 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Darchiv Oct 1, 2026

Choose a reason for hiding this comment

Uh oh!

adamgreloch Oct 2, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Darchiv Oct 2, 2026

Choose a reason for hiding this comment

Uh oh!

adamgreloch Oct 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Darchiv Oct 2, 2026

Choose a reason for hiding this comment

Uh oh!

adamgreloch Oct 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

adamgreloch commented Sep 30, 2026 •

edited

Loading

github-actions Bot commented Sep 30, 2026 •

edited

Loading

adamgreloch Oct 2, 2026 •

edited

Loading

adamgreloch Oct 2, 2026 •

edited

Loading

adamgreloch Oct 2, 2026 •

edited

Loading