posix: headers: fix remaining newlib picolibc zephyr pthread inconsistencies #52316
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cfriedt
requested review from
enjiamai,
stephanosio,
KangJianX,
jeremybettis and
keith-packard
This was referenced Nov 17, 2022
Merged
cfriedt
changed the title
This was referenced Nov 17, 2022
cfriedt
force-pushed
the
fix-remaining-newlib-zephyr-pthread-inconsistencies
branch
15 times, most recently
from
aad0ed9 to
e19e209
Compare
cfriedt
requested review from
rlubos,
tbursztyka,
pfalcon and
nashif
as code owners
|
cfriedt
force-pushed
the
fix-remaining-newlib-zephyr-pthread-inconsistencies
branch
from
0d507d8 to
58ea904
Compare
|
@keith-packard - do you have an opportunity to re-review? |
There was a second call to `pthread_attr_init()` that reallly had no sense being there. Also, it seems that there was a call to `pthread_attr_destroy()` out of perhaps paranoia. The duplicate call and `pthread_attr_destroy()` can be removed. Signed-off-by: Chris Friedt <[email protected]>
The `sys/stat.h` header has never been a part of ISO C so move it to `zephyr/include/posix/sys/`. To ensure a smooth migration, leave a stub header in `lib/libc/minimal/include/sys/` that prints a deprecation warning suggesting developers either include `<zephyr/posix/sys/stat.h>` or use `CONFIG_POSIX_API=y`. Signed-off-by: Chris Friedt <[email protected]>
The `fcntl.h` header has never been a part of ISO C so move it to `include/zephyr/posix`. To ensure a smooth migration, a header was left in `lib/libc/minimal/include` that prints a deprecation warning. Users should either include `<zephyr/posix/fcntl.h>` or switch to `CONFIG_POSIX_API=y`. Signed-off-by: Chris Friedt <[email protected]>
The `fcntl.h` header should declare the `fcntl()` function, according to the spec. Signed-off-by: Chris Friedt <[email protected]>
Define `PTHREAD_CREATE_DETACHED` and `PTHREAD_CREATE_JOINABLE` to be compatible with the Newlib definitions. This is a temporary workaround for zephyrproject-rtos#51211 until Newlib headers are pulled in. Signed-off-by: Chris Friedt <[email protected]>
Part of the POSIX Roadmap for LTSv3 is to adopt POSIX-related headers from Newlib. The strategy to get there is to first make the existing POSIX subsystem work in the presence of Newlib POSIX types. Part of the strategy involved adopting Newlib's `uint32_t` abstraction for some Zephyr POSIX types. However, some types are declared as structures. Luckily, the API only passes those structures around in the form of pointers, and the API only mutates those structures via global functions. With that, we are able to alias Newlib POSIX types as Zephyr POSIX structures. One of the caveats to doing that without introducing stack corruption is to ensure that the Zephyr POSIX types are <= their respective Newlib counterparts. There was only one Zephyr structure for which that requirement did not hold: `pthread_attr_t`. We left `pthread_attr_t` as the Newlib definition, and named the Zephyr variant `struct pthread_attr`. On 32-bit machines, both structures were 32-bytes. ``` sizeof(pthread_attr_t): 32 sizeof(struct pthread_attr): 32 ``` However, on 64-bit machines, `pthread_attr_t` was 40 bytes, while `struct pthread_attr` was 48 bytes. ``` sizeof(pthread_attr_t): 40 sizeof(struct pthread_attr): 48 ``` That triggered the following assertion. ``` BUILD_ASSERT(sizeof(pthread_attr_t) >= sizeof(struct pthread_attr)); ``` The `stacksize` field was subsequently changed from `size_t` to `uint32_t`, and that reduced the latter to 40 bytes as well, solving the last real problem. ``` sizeof(pthread_attr_t): 40 sizeof(struct pthread_attr): 40 ``` Signed-off-by: Chris Friedt <[email protected]>
Zephyr's `SCHED_RR` and `SCHED_FIFO` definitions were slightly different than Newlib's. Additionally, the test had hard-coded magic numbers instead of using symbolic values. Signed-off-by: Chris Friedt <[email protected]>
Just to match the filename remove the `SYS_` from `POSIX_SYS_TYPES_H_`. Signed-off-by: Chris Friedt <[email protected]>
Avoid directly accessing fields in `pthread_attr_t` and only access / mutate them with POSIX functions. Signed-off-by: Chris Friedt <[email protected]>
`POSIX__SIGNAL_H` is not representative of the filename. Signed-off-by: Chris Friedt <[email protected]>
The remaining types that needed to be harmonized between Newlib and Zephyr's POSIX definitions are: * `struct sched_param` - don't re-define if using minimal libc * `pthread_attr_t` - convert to `struct pthread_attr` - define type if using minimal libc - assert acceptible object size * `pthread_mutexattr_t` - convert to `struct pthread_mutexattr` - define type if using minimal libc - assert acceptible object size * `pthred_condattr_t` - convert to `struct pthread_condattr` - define type if using minimal libc - assert acceptible object size * `pthread_once_t` - adopt newlib definition - define type if using minimal libc Signed-off-by: Chris Friedt <[email protected]>
PicoLibC defines `O_CREAT` (really `_FCREAT`) as 0x0040. Otherwise, these constants are identical. Signed-off-by: Chris Friedt <[email protected]>
Previously, `<sys/stat.h>` was declaring the following constants which should be declared in `<fcntl.h>` according to POSIX. Signed-off-by: Chris Friedt <[email protected]>
The `SEEK_SET`, `SEEK_CUR`, and `SEEK_END` constants are defined in `<stdio.h>`, not in `<sys/stat.h>`. Signed-off-by: Chris Friedt <[email protected]>
This may be getting ahead of the curve a bit, but here we adopt the `<sys/stat.h>` header from picolibc. It is mostly the same as that of newlib but the picolibc variant seemed to be slightly more "inclusive". Removed the `<_ansi.h>` include. The commit immediately following this declares a few missing types only if they are not already declared. Signed-off-by: Chris Friedt <[email protected]>
The original format of this file as imported from PicoLibC was not compatible with Zephyr's coding stytle. This change fixes compliance issues listed below: - whitespace formatting - block comment formatting - named parameters Introduction of new typedefs should be considered false-negatives as these are existing standard POSIX types. Signed-off-by: Chris Friedt <[email protected]>
After adopting the `<sys/stat.h>` header from picolibc, there is a possibility that the following types are not defined. ```cpp typedef int dev_t; typedef int ino_t; typedef unsigned short nlink_t; typedef unsigned short uid_t; typedef unsigned short gid_t; typedef unsigned long blksize_t; typedef unsigned long blkcnt_t; ``` Of the above missing types, the oonly ones that are used today in Zephyr are `blksize_t` and `blkcnt_t`. Signed-off-by: Chris Friedt <[email protected]>
cfriedt
force-pushed
the
fix-remaining-newlib-zephyr-pthread-inconsistencies
branch
from
58ea904 to
6d921c7
Compare
|
Just a reminder, this needs to be merged sooner rather than later as it's blocking the remaining PRs (in draft) for #51620 (Apache Thrift module) which will be upstreamed as part of the 3.3 release. Would be great to get it merged this week so the last couple of tasks can be completed. @enjiamai |
mbolivar-nordic
approved these changes
Jan 6, 2023
|
Afraid I'm unlikely to have time to review this patch in any detail; please don't wait on me. |
jeremybettis
approved these changes
Jan 9, 2023
stephanosio
approved these changes
Jan 9, 2023
|
Compliance check failures are false positives: Scancode failure has been sanctioned by the TSC: |
cfriedt
deleted the
fix-remaining-newlib-zephyr-pthread-inconsistencies
branch
13 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
17 commits that close the last posix-related gaps from Zephyr to Newlib.
Note: Compliance failures are false positives.