Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lib: libc: common: provide a common clock() implementation #57800

Conversation

cfriedt
Copy link
Member

@cfriedt cfriedt commented May 11, 2023

The clock() function was originally part of ISO C89 and it is also a POSIX extension of the C standard.

Both newlib and picolibc have implementations of clock() that reference the POSIX times() function. However, POSIX is optional in Zephyr while C is non-optional. Due to this, the newlib and picolibc implementations inadvertantly cause a layering violation that would otherwise not exist on full POSIX operating systems like Linux or BSD.

Provide a simple implementation here that is independent of POSIX.

Fixes #51978

Compliance issue is a false positive

@cfriedt cfriedt requested a review from nashif as a code owner May 11, 2023 15:38
@zephyrbot zephyrbot added the area: C Library C Standard Library label May 11, 2023
@cfriedt cfriedt force-pushed the common-libc-clock-implementation branch from 1e957e5 to 8163da8 Compare May 11, 2023 15:53
lib/libc/common/Kconfig Outdated Show resolved Hide resolved
lib/libc/minimal/include/time.h Show resolved Hide resolved
lib/libc/common/source/time/clock.c Outdated Show resolved Hide resolved
@cfriedt cfriedt force-pushed the common-libc-clock-implementation branch 3 times, most recently from 4cf381d to 5362f66 Compare May 11, 2023 16:26
lib/libc/Kconfig Outdated Show resolved Hide resolved
@cfriedt cfriedt force-pushed the common-libc-clock-implementation branch 3 times, most recently from 884aa45 to 9e367cf Compare May 11, 2023 19:05
@cfriedt cfriedt requested review from stephanosio and galak May 11, 2023 19:07
@cfriedt
Copy link
Member Author

cfriedt commented May 11, 2023

We'll see how this does in CI. @evgeniy-paltsev - I can add something here for ARC if you like as well.

@cfriedt cfriedt force-pushed the common-libc-clock-implementation branch 2 times, most recently from dfa3101 to f24ec69 Compare May 12, 2023 00:00
Copy link
Collaborator

@keith-packard keith-packard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that newlib and picolibc use unsigned long for _CLOCK_T_.

lib/libc/minimal/include/sys/_types.h Show resolved Hide resolved
lib/libc/minimal/include/time.h Show resolved Hide resolved
@cfriedt cfriedt force-pushed the common-libc-clock-implementation branch from f24ec69 to 03d7858 Compare May 13, 2023 14:05
@cfriedt
Copy link
Member Author

cfriedt commented May 13, 2023

  • used uint64_t for clock_t in the minimal libc
  • ensure clock() always succeeds in the common libc by rolling over (clock_t)-1
  • check for (clock_t)-1 (failure) in test

Note: in the future it may be possible for clock() to fail if there is an underlying hardware or software issue.

@cfriedt cfriedt force-pushed the common-libc-clock-implementation branch from 03d7858 to c0bb3cf Compare May 13, 2023 14:09
keith-packard
keith-packard previously approved these changes May 13, 2023
@keith-packard
Copy link
Collaborator

Coding guidelines check is currently failing due to this patch defining clock. Compliance checks are failing due to adding the clock_t typedef. Both are spurious.

@abrodkin abrodkin added area: ARC ARC Architecture and removed area: ARC ARC Architecture labels May 22, 2023
@cfriedt
Copy link
Member Author

cfriedt commented Aug 5, 2023

Doing a minor respin. I might as well add the POSIX bits as well, it's so simple to implement. Until we have k_clock_get() and friends though, the per-process and per-thread clock ids will not work, so tests using those should be skipped.

@cfriedt cfriedt force-pushed the common-libc-clock-implementation branch from c0c3b78 to d72469e Compare August 5, 2023 13:20
Copy link
Collaborator

@keith-packard keith-packard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking good; there's an unresolved comment in test_clock about getting a reasonable delay like you used in test_times. And, of course, the compliance checker has 'some issues'. Seems like it's almost ready ?

Copy link
Member Author

@cfriedt cfriedt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some comments..

lib/libc/common/source/time/clock.c Outdated Show resolved Hide resolved
lib/posix/clock.c Outdated Show resolved Hide resolved
lib/posix/clock.c Outdated Show resolved Hide resolved
clock_t now;

then = clock();
k_sleep(K_TICKS(1));
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep - I'll change that shortly.

@cfriedt cfriedt force-pushed the common-libc-clock-implementation branch from d72469e to 201957c Compare August 7, 2023 14:33
@cfriedt
Copy link
Member Author

cfriedt commented Aug 7, 2023

Note: this is back in draft, so no need to review yet.

@github-actions
Copy link

github-actions bot commented Oct 7, 2023

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@github-actions github-actions bot added the Stale label Oct 7, 2023
@github-actions github-actions bot closed this Oct 21, 2023
@cfriedt cfriedt reopened this Oct 26, 2023
@github-actions github-actions bot removed the Stale label Oct 27, 2023
@cfriedt cfriedt force-pushed the common-libc-clock-implementation branch 2 times, most recently from 949f69b to 1afcb03 Compare November 1, 2023 02:42
The `clock()` function was originally part of ISO C89, and it is
also a POSIX extension of the C standard.

Both newlib and picolibc have implementations of `clock()` that
reference the POSIX `times()` function.  However, POSIX is
optional in Zephyr while C is non-optional. Due to this, the
newlib and picolibc implementations cause a layering violation.

Provide a simple implementation here that is independent of
POSIX to avoid the layering violation.

Signed-off-by: Christopher Friedt <cfriedt@meta.com>
Add a test for the ISO C `clock()` function, which has been
present since C89.

Signed-off-by: Christopher Friedt <cfriedt@meta.com>
@cfriedt cfriedt force-pushed the common-libc-clock-implementation branch from 1afcb03 to 52d7a66 Compare November 1, 2023 11:39
Copy link

github-actions bot commented Jan 1, 2024

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@github-actions github-actions bot added the Stale label Jan 1, 2024
@cfriedt cfriedt removed the Stale label Jan 1, 2024
Copy link

github-actions bot commented Mar 2, 2024

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@github-actions github-actions bot added the Stale label Mar 2, 2024
@github-actions github-actions bot closed this Mar 16, 2024
@cfriedt cfriedt reopened this Mar 16, 2024
@github-actions github-actions bot removed the Stale label Mar 17, 2024
Copy link

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@github-actions github-actions bot added the Stale label May 17, 2024
@cfriedt cfriedt removed the Stale label May 17, 2024
Copy link

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@github-actions github-actions bot added the Stale label Jul 17, 2024
@github-actions github-actions bot closed this Aug 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: C Library C Standard Library Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add POSIX times() support
9 participants