Remove explicit -D_POSIX_C_SOURCE global compiler options when building with picolibc

[keith-packard]

Picolibc now has explicit Zephyr support so we no longer need _POSIX_C_SOURCE defined to get the complete Zephyr C Library API

[zephyrbot]

The following west manifest projects have been modified in this Pull Request:

Name	Old Revision	New Revision	Diff
picolibc	zephyrproject-rtos/picolibc@1a5c603	zephyrproject-rtos/picolibc@5415d37 (zephyr-no-posix-define)	zephyrproject-rtos/picolibc@1a5c603b..5415d37a

Note: This message is automatically posted and updated by the Manifest GitHub Action.

[keith-packard]

Ideally, the one for the toolchain would depend on the version of picolibc provided, but I have no idea how to make that work.

[sylvioalves]

This can be removed as there is no non-standard posix call in Espressif's sources.

[keith-packard]

Thanks! I'll mix that in once I've got things building.

[keith-packard]

Looks like that worked!

[keith-packard]

The compliance check failure is from the strerror_r test -- strerror_r is not part of the Zephyr C library interface, and so the test must define _POSIX_C_SOURCE=200809L to gain access to that symbol. I could remove this test, or we could add the function to the Zephyr API. Anyone have a preference?

[aescolar]

The compliance check failure is from the strerror_r test -- strerror_r is not part of the Zephyr C library interface, and so the test must define _POSIX_C_SOURCE=200809L to gain access to that symbol. I could remove this test, or we could add the function to the Zephyr API. Anyone have a preference?

I guess adding it to the Zephyr API is a bit against the point (of these rules https://docs.zephyrproject.org/latest/contribute/coding_guidelines/index.html#rule-a-5-c-standard-library-usage-restrictions-in-zephyr-codebase )
I would not have a strong objection to removing that part of the test.
Though I guess another option would be to ignore the conformance check warning? (this PR is not adding the function use, it is just exposing it)

[keith-packard]

My personal thinking would be that code in Zephyr should expect to need to set the same macros as in other systems when using C library extension functions. And that having that requirement would relax the requirement on the C libraries (be it newlib, or glibc, or..) or their integration to have special treatment for Zephyr.

Oh, that's a very interesting idea. While we say that the Zephyr C library API includes a few POSIX extensions, we simply require that applications using those functions request them through the "normal" mechanism -- #define _POSIX_C_SOURCE 200809L, instead of expecting them to be present automatically. That would make integration with other C libraries far simpler; ones with limited POSIX support (like picolibc and newlib) would not need to do anything special. For those without POSIX support, Zephyr would provide wrapper headers that checked for _POSIX_C_SOURCE before adding the non ISO-C APIs as well as using #include_next to pull in the underlying C library headers.

One downside is that this would make it very difficult to detect code which is using non-ISO C APIs which are not part of the Zephyr API. Another is that we would have to stick #define _POSIX_C_SOURCE 200809L defines in some core Zephyr code which is using these APIs, which makes the first downside leak into the Zephyr core.

[cfriedt]

Darn - I guess this line might still be an issue because it changes cflags globally for the entire build, which would violate rules A.4 and likely A.5 of the Coding Guidelines.

[aescolar]

There is another PR fixing this. It does not need to be done in one go.

[cfriedt]

Is it possible to use this instead? That would change the definition to a local one rather than a global one (which would conflict in many places).

Suggested change

	zephyr_compile_definitions(_POSIX_C_SOURCE=200809L)
	zephyr_library_compile_definitions(_POSIX_C_SOURCE=200809L)

[keith-packard]

No, the whole point of this define is to ensure that newlib exposes the Zephyr C API as defined by Rule A.4 in the Coding Guidelines. Unless and until newlib itself is fixed to provide Zephyr compatibility, we can either

1. Abandon Rule A.4 and assert that Zephyr only uses ISO C interfaces
2. Define _POSIX_C_SOURCE=200809L globally
3. Find some alternative mechanism to get newlib to expose the extended functions.

[aescolar]

@keith-packard would it be an option to only expose by default the ISO C interface, but still allow those extra functions defined in Rule A.4 (thru individual waives in PRs that need to use them, where setting the _POSIX_C_SOURCE macro is allowed in particular cases). Effectively what you described here: #67040 (comment)

[keith-packard]

Are you saying that we require applications define _POSIX_C_SOURCE=200809L to gain access to the extended functions? Because if it's not required, then we're adding implicit dependencies on using only a C library with Zephyr compatibility (picolibc or the minimal C library).

That would certainly be easier for C library developers, but it seems like it will require careful auditing of changes in any file with such a definition to ensure that it doesn't accidentally start depending on additional POSIX functionality.

[aescolar]

Are you saying that we require ..

Yes

[keith-packard]

It seems like that would also require allowing core Zephyr files to contain such definitions; have tooling that prevents valid code from being merged will slow down development. Because once the definition is present in a source file, no future changes to that file will see the same review, hence allowing non-approved functions to leak in. Alternatively, more sophisticated tooling that detected the use of POSIX functions beyond the approved list.

[aescolar]

You are right that in general that would be the case, and that would be very far from ideal.

Though today a quick check seems to show:

• there is nothing in the "zephyr kernel" using strnlen() (the only exception in A.4).
• for more peripheral code in the tree, there is a few uses of strnlen() and strtok_r() (rule A.5 exceptions).

But given that today, at least for the tests we are running, all those built with just this
123bdfb
I wonder if it would be best to, by now, postpone this particular point (I would very much like to have @stephanosio on this discussion), and not do this particular line change (or that other commit) in this PR; The rest of the PR seems fully uncontroversial and an overall improvement.

[keith-packard]

We can't remove instances of #define _POSIX_C_SOURCE in most places without somehow dealing with newlib's lack of Zephyr support. Otherwise, building code with newlib that uses strnlen will fail.

[cfriedt]

It would still be slightly more ideal to remove application conformance macros from global cflags.

[keith-packard]

It would still be slightly more ideal to remove application conformance macros from global cflags.

That requires that upstream newlib be fixed to provide Zephyr support.

[keith-packard]

This steals @cfriedt 's idea to wrap string.h and add the non ISO-C declarations there, but is newlib specific and so doesn't risk breaking other C libaries.

[aescolar]

Hi @keith-packard , this PR now has a very different content and target than originally. That can be quite confusing for reviewers, as the previous comments and commits are unrelated. Moreover some may have already muted this PR, and the merge criteria does not properly account for PRs that morph.
The old PR content, commits and comments were relevant as they documented a discussion, and were linked from several other PRs.
May I ask you to create a new PR for the new content, push back the old commits to this branch, revert the PR title and original description and close this PR so it remains as an archive of the discussion?

Setting the DNM label due to this.

[cfriedt]

This looks fine to me. Thanks @keith-packard

Or if you prefer to make a separate PR, sure. I have no issues with the current incantation though.

Not totally necessary, but FWIR there are some CI errors right now that this will fix, so it might be a good idea to file a bug report.

CI errors are priority: high, so that will ensure it gets merged with the release (IIRC, we require zero high priority issues with a release).

[keith-packard]

I've reset this PR back to the state before it became just patches for newlib compatibility. It's no longer relevant for anything but historical purposes.