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.

Sign up for GitHub

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

Jump to bottom

libc: common: add support for iso c11 threads #60759

Merged
merged 11 commits into from
Nov 14, 2023
Merged

libc: common: add support for iso c11 threads #60759

merged 11 commits into from
Nov 14, 2023

Conversation

cfriedt
Copy link
Member

@cfriedt cfriedt commented Jul 25, 2023 •
edited
Loading

Note: Compliance issues are false positives

Capitalize on newly added support for dynamic thread stacks and the existing pthread support to provide an implementation of ISO C11 <threads.h>.

For more information about the ISO C standard, please see
https://www.iso.org/standard/74528.html

@cfriedt cfriedt added area: Tests Issues related to a particular existing or missing test area: POSIX POSIX API Library area: Minimal libc Minimal C Standard Library labels Jul 25, 2023
@cfriedt cfriedt requested review from ceolin and keith-packard July 25, 2023 01:48
@cfriedt

This comment was marked as outdated.

Sign in to view
@cfriedt cfriedt force-pushed the c11-threads branch 2 times, most recently from 681d636 to 59d811d Compare July 25, 2023 01:58
@cfriedt
Copy link
Member Author

cfriedt commented Jul 25, 2023

Just looking for some early feedback

keith-packard
keith-packard reviewed Jul 25, 2023
View reviewed changes
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 fine. Is there any reason to depend upon the C library thread.h? Or could Zephyr provide its own version of that file directly?

@cfriedt
Copy link
Member Author

cfriedt commented Jul 25, 2023

This is looking fine. Is there any reason to depend upon the C library thread.h? Or could Zephyr provide its own version of that file directly?

No reason to explicitly not make it common in Zephyr that I can think of - @keith-packard, were you thinking it might be better to put this into lib/libc/common/?

@keith-packard
Copy link
Collaborator

No reason to explicitly not make it common in Zephyr that I can think of - @keith-packard, were you thinking it might be better to put this into lib/libc/common/?

It's almost something more common than libc/common -- shouldn't we always use this code, even if the C library provides alternatives?

@cfriedt
Copy link
Member Author

cfriedt commented Jul 30, 2023 •
edited
Loading

Need to rebase after #60964 and #60961 are merged

@cfriedt cfriedt requested a review from keith-packard July 30, 2023 16:49
@cfriedt cfriedt changed the title libc: minimal: add support for iso c11 threads libc: common: add support for iso c11 threads Jul 30, 2023
@cfriedt cfriedt force-pushed the c11-threads branch 3 times, most recently from 853b7e2 to bea803e Compare August 2, 2023 15:08
@cfriedt
Copy link
Member Author

cfriedt commented Aug 2, 2023

Fixed another regression in pthread_mutex_timedlock() (moved to a separate PR).

ycsin
ycsin previously approved these changes Nov 9, 2023
View reviewed changes
jgl-meta
jgl-meta previously approved these changes Nov 9, 2023
View reviewed changes
Copy link
Collaborator

@jgl-meta jgl-meta left a comment

Choose a reason for hiding this comment

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

lgtm!

dkalowsk
dkalowsk previously approved these changes Nov 12, 2023
View reviewed changes
Copy link
Contributor

@dkalowsk dkalowsk left a comment

Choose a reason for hiding this comment

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

One minor note I saw on this review, but honestly it can wait for a future code clean up. Looks good to me!

include/zephyr/posix/sys/stat.h Show resolved Hide resolved
cfriedt added 11 commits November 13, 2023 09:58
@cfriedt
tests: lib: c_lib: separate libc tests into smaller suites
b072c30 
C library testing is mainly there to support what is
necessary to support Zephyr. We do test a variety of libcs
currently, which is where YAML comes in handy.

However, the main libc testsuite can be overkill for testing
some things, and might not be suitable for testing optional
features.

Create a 'common' subdirectory for common libc tests.

Signed-off-by: Christopher Friedt <[email protected]>
@cfriedt
libc: common: add support for iso c11 threads
017eb25 
This change capitalizes on newly added support for dynamic
thread stacks and the existing pthread support to provide
an implementation of the ISO C11 `<threads.h>` API.

Signed-off-by: Christopher Friedt <[email protected]>
@cfriedt
tests: libc: add tests for iso c11 threads
1d159a0 
Add tests to verify functionality of the C11 `<threads.h>` API.

Signed-off-by: Christopher Friedt <[email protected]>
@cfriedt
lib: libc: common: add C11 mutex implementation
3210493 
Add support for C11 mutexes to go with C11 threads.

Signed-off-by: Christopher Friedt <[email protected]>
@cfriedt
tests: lib: c_lib: add tests for C11 mutexes
f05b33d 
Add tests to cover C11 mutexes

Signed-off-by: Christopher Friedt <[email protected]>
@cfriedt
libc: common: support for C11 condition variables
599d143 
Add C11 condition variable support to go with C11 threads
and mutexes.

Signed-off-by: Christopher Friedt <[email protected]>
@cfriedt
tests: lib: c_lib: tests for C11 condition variables
195eb46 
Add tests for C11 condition variables.

Signed-off-by: Christopher Friedt <[email protected]>
@cfriedt
libc: common: support for C11 thread-specific storage
478661e 
Add C11 thread-specific storage (tss) support to go with C11 threads
and mutexes.

Signed-off-by: Christopher Friedt <[email protected]>
@cfriedt
tests: lib: c_lib: tests for C11 thread-specific storage
edfa948 
Add tests for C11 thread-specific storage.

Signed-off-by: Christopher Friedt <[email protected]>
@cfriedt
libc: common: support for C11 call_once()
c44373f 
Add C11 call_once() support to go with C11 threads
and mutexes.

Signed-off-by: Christopher Friedt <[email protected]>
@cfriedt
tests: lib: c_lib: tests for C11 call_once()
0f4ebef 
Add tests for C11 call_once().

Signed-off-by: Christopher Friedt <[email protected]>
@cfriedt cfriedt dismissed stale reviews from dkalowsk, jgl-meta, ycsin, and jhedberg via 0f4ebef November 13, 2023 15:00
@cfriedt
Copy link
Member Author

cfriedt commented Nov 13, 2023

  • rebased (again) to resolve conflicts
  • @carlescufi @nashif @stephanosio - can someone please merge this? It really just had merge conflict after merge conflict, has been rebased, re-approved about a dozen times over the last almost 4 months with practically zero code changes.

@aescolar
Copy link
Member

aescolar commented Nov 14, 2023 •
edited
Loading

  • rebased (again) to resolve conflicts

    • @carlescufi @nashif @stephanosio - can someone please merge this? It really just had merge conflict after merge conflict, has been rebased, re-approved about a dozen times over the last almost 4 months with practically zero code changes.

@cfriedt note that only admins can override the compliance check failure, so that does not work in this PR favor

stephanosio
stephanosio approved these changes Nov 14, 2023
View reviewed changes
Copy link
Member

@stephanosio stephanosio left a comment

Choose a reason for hiding this comment

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

LGTM

@stephanosio stephanosio merged commit 2fc19aa into zephyrproject-rtos:main Nov 14, 2023
34 of 35 checks passed
@cfriedt cfriedt deleted the c11-threads branch November 14, 2023 09:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aescolar aescolar aescolar approved these changes

@stephanosio stephanosio stephanosio approved these changes

@dkalowsk dkalowsk dkalowsk approved these changes

@ycsin ycsin ycsin approved these changes

@ceolin ceolin Awaiting requested review from ceolin

@andyross andyross Awaiting requested review from andyross

@keith-packard keith-packard Awaiting requested review from keith-packard

@MaureenHelm MaureenHelm Awaiting requested review from MaureenHelm

@golowanow golowanow Awaiting requested review from golowanow

@gopiotr gopiotr Awaiting requested review from gopiotr

@hakehuang hakehuang Awaiting requested review from hakehuang

@PerMac PerMac Awaiting requested review from PerMac

@nashif nashif Awaiting requested review from nashif

@galak galak Awaiting requested review from galak

@carlescufi carlescufi Awaiting requested review from carlescufi

@jhedberg jhedberg Awaiting requested review from jhedberg

@jgl-meta jgl-meta Awaiting requested review from jgl-meta

Assignees

@keith-packard keith-packard

@stephanosio stephanosio

Labels
Architecture Review Discussion in the Architecture WG required area: C Library C Standard Library area: POSIX POSIX API Library area: Tests Issues related to a particular existing or missing test
Projects
Architecture Review
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.