Skip to content

[DNM] why you should care about _POSIX_C_SOURCE #68436

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

Conversation

cfriedt
Copy link
Member

@cfriedt cfriedt commented Feb 1, 2024

DO NOT MERGE
DO NOT REVIEW (this is for demonstration purposes only)

This is a proof-of-concept PR that demonstrates the scale of breakage we will be looking at by not addressing the issue of managing _POSIX_C_SOURCE in an intelligent and scalable way.

twister --build-only -p qemu_x86 -T tests/posix/ \
  -T samples/posix -T samples/net -T tests/lib/c_lib -T tests/net/
...
- Total complete:  403/ 403  100%  skipped: 146, failed: 0, error:  103
INFO    - 404 test scenarios (403 test instances) selected, 146 configurations skipped (139 by static filter, 7 at runtime).
INFO    - 154 of 403 test configurations passed (59.92%), 0 failed, 103 errored, 146 skipped with 0 warnings in 265.66 seconds
INFO    - 0 test configurations executed on platforms, 257 test configurations were only built.
Screenshot 2024-02-01 at 1 51 18 PM

DO NOT MERGE

This is a proof-of-concept PR that demonstrates the scale of
breakage we will be looking at by not addressing the issue
of managing _POSIX_C_SOURCE in an intelligent and scalable way.

twister --build-only -p qemu_x86 -T tests/posix/ \
  -T samples/posix -T samples/net -T tests/lib/c_lib -T tests/net/
...
- Total complete:  403/ 403  100%  skipped: 146, failed: 0, error:  103
INFO    - 404 test scenarios (403 test instances) selected, 146 configurations skipped (139 by static filter, 7 at runtime).
INFO    - 154 of 403 test configurations passed (59.92%), 0 failed, 103 errored, 146 skipped with 0 warnings in 265.66 seconds
INFO    - 0 test configurations executed on platforms, 257 test configurations were only built.

Signed-off-by: Christopher Friedt <[email protected]>
@kartben
Copy link
Collaborator

kartben commented Feb 2, 2024

Any chance this could be made a "minimal repro"? It's hard to answer the "why you should care about _POSIX_C_SOURCE?" question with what seems to be more changes than necessary to make your point. Would definitely be helpful to people like me with limited POSIX expertise :) Thanks!

@cfriedt
Copy link
Member Author

cfriedt commented Feb 4, 2024

Any chance this could be made a "minimal repro"?

I actually do need to make this entire change (or something similar to it) very soon, but a "minimal repro" version could be the following:

@kartben - please let me know if this needs any adjustment, or simply adjust as needed if you think it can be worded better.

  • declaring POSIX function prototypes conditionally based on application conformance feature test macros is a planned change (for conformance purposes). As a contrived example, pthread_create() became part of the POSIX specification in 2001

Problem No. 1

  • because there was no deprecation process for the hard-coded / global definition of _POSIX_C_SOURCE=200809, we will see warnings and errors if it is not declared.
  • Save the patch below as /tmp/foo.patch and apply with git apply /tmp/foo.patch
diff --git a/include/zephyr/posix/pthread.h b/include/zephyr/posix/pthread.h
index 9b34c319b57..adc2eded24f 100644
--- a/include/zephyr/posix/pthread.h
+++ b/include/zephyr/posix/pthread.h
@@ -436,8 +436,10 @@ void pthread_exit(void *retval);
 int pthread_join(pthread_t thread, void **status);
 int pthread_cancel(pthread_t pthread);
 int pthread_detach(pthread_t thread);
+#if _POSIX_C_SOURCE >= 200112L
 int pthread_create(pthread_t *newthread, const pthread_attr_t *attr,
                   void *(*threadroutine)(void *), void *arg);
+#endif
 int pthread_setcancelstate(int state, int *oldstate);
 int pthread_setcanceltype(int type, int *oldtype);
 void pthread_testcancel(void);
  • attempt to build e.g. samples/posix/philosophers and observe several "implicit function declaration" warnings
west build -p qemu_riscv64 samples/posix/philosophers
...
/home/cfriedt/zephyrproject/zephyr/samples/posix/philosophers/src/main.c: In function 'start_threads':
/home/cfriedt/zephyrproject/zephyr/samples/posix/philosophers/src/main.c:220:23: warning: implicit declaration of function 'pthread_create'; did you mean 'k_thread_create'? [-Wimplicit-function-declaration]
  220 |                 ret = pthread_create(&threads[i], NULL, philosopher, INT_TO_POINTER(i));
      |                       ^~~~~~~~~~~~~~
      |                       k_thread_create
[58/131] Building C object zephyr/lib/libc/common/CMakeFiles/lib__libc__common.dir/source/thrd/thrd.c.obj
/home/cfriedt/zephyrproject/zephyr/lib/libc/common/source/thrd/thrd.c: In function 'thrd_create':
/home/cfriedt/zephyrproject/zephyr/lib/libc/common/source/thrd/thrd.c:25:17: warning: implicit declaration of function 'pthread_create'; did you mean 'k_thread_create'? [-Wimplicit-function-declaration]
   25 |         switch (pthread_create(thr, NULL, pfunc, arg)) {
      |                 ^~~~~~~~~~~~~~
      |                 k_thread_create
[87/131] Building C object zephyr/lib/posix/options/CMakeFiles/lib__posix__options.dir/mqueue.c.obj
/home/cfriedt/zephyrproject/zephyr/lib/posix/options/mqueue.c: In function 'send_message':
/home/cfriedt/zephyrproject/zephyr/lib/posix/options/mqueue.c:472:31: warning: implicit declaration of function 'pthread_create'; did you mean 'k_thread_create'? [-Wimplicit-function-declaration]
  472 |                         ret = pthread_create(&th,
      |                               ^~~~~~~~~~~~~~
      |                               k_thread_create
[118/131] Building C object zephyr/lib/posix/options/CMakeFiles/lib__posix__options.dir/timer.c.obj
/home/cfriedt/zephyrproject/zephyr/lib/posix/options/timer.c: In function 'timer_create':
/home/cfriedt/zephyrproject/zephyr/lib/posix/options/timer.c:171:23: warning: implicit declaration of function 'pthread_create'; did you mean 'k_thread_create'? [-Wimplicit-function-declaration]
  171 |                 ret = pthread_create(&timer->thread, evp->sigev_notify_attributes,
      |                       ^~~~~~~~~~~~~~
      |                       k_thread_create
  • acknowldge that warnings are promoted to errors in CI
  • To fix this, the application should specify _POSIX_C_SOURCE

Problem No. 2

  • multiply by a large number to take into account the many instances of POSIX functions in Zephyr, the number of architectures and boards supported by Zephyr, the number of downstream users of Zephyr
  • Specifying _POSIX_C_SOURCE in an ad-hoc manner where versions are hard-coded everywhere will introduce vast quantities of technical debt, unspecified behaviour, and non-conformance.
  • to avoid the above unwanted results, the application should be able to specify _POSIX_C_SOURCE in a consistent and scalable way (See posix: add Kconfig and cmake for application conformance #67132)
  • as a corollary to this (which basically brings us full-circle), see the PR posix: env: support for environ, getenv(), setenv(), unsetenv() #66762 where application conformance feature test macros need to be defined several times.

@aescolar
Copy link
Member

aescolar commented Feb 5, 2024

and observe several "implicit function declaration" warnings

The issue is about not defining that macro where it should be defined, after the headers are changed to require the macro to expose those prototypes.

For which the solution would one of:
a) Merge your PR, and for each application library that uses one of these headers add target_posix_library_compile_options() to their cmake files and set that kconfig for each application in their prj.conf (or, except for the C library provided functionality, default thru kconfig when either POSIX_API is selected, or when another library which starts only exposing these prototypes if these macros are defined is selected).
b) OR Just set target_compile_options(-D_POSIX_C_SOURCE=200809L) in the affected application cmake file.
c) OR Just #define _POSIX_C_SOURCE 200809L in the affected application source files
d) OR Just do zephyr_compile_definitions(_POSIX_C_SOURCE=200809L) in the app cmake file.
e) OR ..

a) Does not seem better than the rest to me.

@cfriedt
Copy link
Member Author

cfriedt commented Mar 3, 2024

a) Does not seem better than the rest to me.

Is it reasonable to expect that other people, companies or organizations, certification bodies, product lines, etc may have a different preference or opinion?

I guess that would assume that they might have slightly different use cases or configuration requirements.

In your opinion, how do you suggest that those people use Zephyr to accomplish what their needs are?

E.g. for example, assume a user needs to define _POSIX_C_SOURCE=200112L.

Should users be expected to patch a tagged Zephyr release in 200 different places, fork and patch modules that are tagged to a specific commit, etc, rather than using a simple Kconfig?

To me, that question has a fairly obvious and objective answer.

Copy link

github-actions bot commented May 4, 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 May 4, 2024
@github-actions github-actions bot closed this May 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants