-
Notifications
You must be signed in to change notification settings - Fork 7.3k
posix: sysconf: match _SC* defines with newlib/picolib #88177
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
base: main
Are you sure you want to change the base?
Conversation
0124368
to
f6bc184
Compare
Is there some reason not to just use the picolibc/newlib values when those C libraries are in use? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nslowell - can we switch to #define
s instead of enum
?
The major benefit of that is getting warnings / errors if the symbol is redefined.
But also, the enum was mainly there because originally there was no specific ordering required.
And use the #defines that come from libc when available? |
That could be achieved with something like |
Yup, although the standard file is <unistd.h>, so Zephyr should use that if possible. If that's not possible, then using <sys/unistd.h> could work. |
f6bc184
to
6a76659
Compare
Appreciate the feedback @cfriedt and @keith-packard. Switching from enum to #defines that match newlib/picolib seemed doable for POSIX_SYSCONF_IMPL_FULL. I did have to adjust how z_sysconf() is used to prevent it from breaking due to preprocessor expansion, like I mentioned in #87833 (comment)
However, for POSIX_SYSCONF_IMPL_MACRO, there is no way around the sysconf breakage, so the #defines have to be wrapped within #ifdef POSIX_SYSCONF_IMPL_FULL. Secondly, I really thought getting this code to reference newlib/picolib's unistd.h was going to be a bit of a headache, but I guess I got lucky. It's possible there's some scenario this breaks, but it seemed to work with all libc options (minimal, newlib, and picolib). I'm hoping #88195 will merge soon, so this can run the full test suite and find any potential issues with this change. Thanks again for the feedback. |
82bba72
to
86be8bf
Compare
PTHREAD_STACK_MIN is one of the very few posix features referenced in sysconf that is not defined in posix_features.h Move it to minimize header dependencies. Signed-off-by: Nicholas Lowell <[email protected]>
static inline gethostname() in unistd.h can cause declaration collisions. we should just move it to a normal function definition like the rest of the network functions. Signed-off-by: Nicholas Lowell <[email protected]>
match standard by having input void *buf parameter for pwrite() marked as const, and avoid any potential declaration conflicts Signed-off-by: Nicholas Lowell <[email protected]>
86be8bf
to
9cd89da
Compare
@keith-packard, @cfriedt Secondly, there's no way to maintain the macro version of sysconf() and have successful tests without isolating it and keeping the original enum declaration for it. The new #defines that match newlib/picolib values will be only for the full implementation. Hope this is sufficient. I've got all tests passing (except for timeouts which are unrelated and pass when run locally) |
9cd89da
to
f0e6999
Compare
It's possible for newlib/picolib libc libraries to internally call sysconf() which would execute zephyr's implementation. However, if the _SC* defines do not have matching values, then the incorrect switch case executes. This issue arises when using newlib/picolib libc that includes sysconf implementation for ARM. With current defaults, the zephyr sysconf() overrides the original libc sysconf() so we must ensure proper operation. We will switch to the #define list just like newlib/picolib. We can't currently use their unistd.h directly due to a domino of declaration conflicts. For the "small" macro implementation, we have to drop using CONCAT to prevent pre-expansion of the new #defines Signed-off-by: Nicholas Lowell <[email protected]>
f0e6999
to
133d00b
Compare
@cfriedt @keith-packard I think this is good to go now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - thanks for all of your help @nslowell 🙏
@@ -72,7 +64,8 @@ size_t confstr(int name, char *buf, size_t len); | |||
#endif | |||
|
|||
#ifdef CONFIG_POSIX_SYSCONF_IMPL_MACRO | |||
#define sysconf(x) (long)CONCAT(__z_posix_sysconf, x) | |||
/* Can't use CONCAT(), must concat directly to prevent expansion of 'x' */ | |||
#define sysconf(x) (long)__z_posix_sysconf##x |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In case there is another revision, I would just bracket (...)
this entire expression out of paranoia, but not blocking.
It's possible for newlib/picolib libc libraries to internally call sysconf() which would execute zephyr's implementation. However, if the _SC* defines do not have matching values, then the incorrect switch case executes.
This issue arises when using newlib/picolib libc that includes sysconf implementation for ARM. With current defaults, the zephyr sysconf() overrides the original libc sysconf() so we must ensure proper operation.