zephyr: [RFC] On the way C libraries (don't) expose all functions defined in coding guideline rules A.4 and A.5

Background:

Today the Zephyr coding guidelines A.4 & A.5 specify what functions can be used in the kernel and in-tree. 1.1) Most of these functions are part of the base C11 standard, two are (POSIX.1-2008) extensions.
Some developers have the understanding that this requires the used C libraries to expose these functions without the need for the user files (C / cmake) to define any SOURCE macros. Picolibc and minimallibc do this. Newlib, the integration with the host C library for the native targets, or other libraries out there don’t.
Checkpatch today has a simple check of A.4 and A.5 by warning on definitions of SOURCE macros in .c files. This has two problems: 4.1) it does not detect definitions in build files. 4.2) It blocks legitimate uses of these macros.

This RFC does NOT try to discuss what should be in 1). If focuses on 2)

Clarification needed

Has there been any kind of authoritative decision on 2) (i.e. TSC vote), or is this just an understanding reached in some PR discussion by a subset of developers?

Discussion on 2)

Today PicolibC ensures that these 2 extra functions prototypes are defined by treating Zephyr specially, with extra Zephyr specific ifdefs to detect and expose them. Newlib does not, and one cannot expect other C libraries the user may use to provide these. The expectation that these 2 are exposed, sets a weird requirement on the C library integration (See https://github.com/zephyrproject-rtos/zephyr/pull/67040#issuecomment-1871855097 , https://github.com/zephyrproject-rtos/zephyr/pull/67040#issuecomment-1896476000 , https://github.com/zephyrproject-rtos/zephyr/pull/67040#discussion_r1462200259 )

Under the assumption that 2) is the way forward, a simple way to work around this issue for C libraries without special Zephyr handling is to define globally the _POSIX_C_SOURCE macro. This is what PicolibC did before, and what #67040 proposed doing for Newlib. There is the minor disadvantage of doing this, that we can cause this macro to be redefined, causing a redefinition warning, either in tree or in users code.

So far the only issue with assuming that 2) is not the way forward, is that the _POSIX_C_SOURCE macro needs to be defined where those 2 functions are used. As such there is no drawback of this beyond a) _POSIX_C_SOURCE needs to be defined in those files , and b) That checkpatch is triggered if that is done in the C source files themselves. a) is something one would need to do with another OS or C library, so something users should expect and technically correct. Avoiding to do this due to that checkpatch check seems the wrong reason, as checkpatch detection of this rules violation is anyhow both very limited and overeager.

Proposal

C libraries are not expected to expose the extra functions defined in A.4 and A.5 by default
Source code in tree defines the required _POSIX_C_SOURCE macro locally (in the C file or build for that file/library) where it uses these extra functions.
Checkpatch detection of the _POSIX_C_SOURCE macro usage in .c files is fixed (i.e. does not warn for this use when only these 2 functions are used).
To avoid people assuming the oposite, coding guidelines rules A.4 & A.5 would state that C libraries may require _POSIX_C_SOURCE be set for those 2 functions prototypes to be visible.

Benefit of the proposal:

We stop requiring extra integration and adding complexity around integration of C libraries. We just do what we would be expected to do and is technically correct. This would simplify the integration of other C libraries both in and out of tree, and avoid issues derived from the workarounds that extra integration requires (There is no nice way of solving this issue without modifying the C library source).
PicolibC can stop treating Zephyr specially if desired (i.e. remove _ZEPHYR_VISIBLE)
Checkpatch check for this rule needs refining anyhow, as today’s detection is faulty both in false positives and false negatives.

References:

Additional notes

Please do not use this RFC to discuss what functions should be part of A.4 & A.5. Create other RFCs for that.

Edit: In proposal replaced “SOURCE macros” with “_POSIX_C_SOURCE macro” as this is the only one I really intended to discuss about here.

About this issue

Original URL
State: open
Created 5 months ago
Comments: 18 (17 by maintainers)

Most upvoted comments

After having a further discussion with @aescolar on this matter and giving more thought on the POSIX compliance and portability aspects (especially in regards to compatibility with the host C library for native POSIX and supporting other C libraries implementing these POSIX C extension functions), I think it may be beneficial to define _POSIX_C_SOURCE where needed as required by the POSIX standard – after all, that is the standard compliant and hence the most hassle-free way forward.

The local definition of the feature test macros, however, should be strictly limited to _POSIX_C_SOURCE, which is part of the POSIX standard; any other non-standard feature test macros (e.g. _GNU_SOURCE) must not be used.

This means:

POSIX C functions listed in the Rule A.4 and Rule A.5 should be made available based on the value of the _POSIX_C_SOURCE macro
non-POSIX C non-ISO C functions listed in the Rule A.4 and Rule A.5 should still be made available unconditionally – the libc integration layer (or the libc itself in case of Picolibc) will be responsible for ensuring this.

@aescolar @cfriedt @keith-packard What do you think?

stephanosio on Feb 2, 2024

As an attempt to collect this conversation in one place, here a verbatim copy of @stephanosio comment: https://github.com/zephyrproject-rtos/zephyr/pull/68381#issuecomment-1920335446

Just reiterating the bits from the conversation I had offline with @aescolar for the record

If the conclusion of #68278 is that C libraries should indeed expose these 2 functions by default

I would prefer not to clutter individual Zephyr source files with the libc feature test macros because:

It is too “noisy.”
Enforcing the POSIX C extension usage throughout Zephyr codebase becomes much more difficult because defining _POSIX_C_SOURCE pulls in a lot more than just strnlen and some other functions from the POSIX standard that we allow in the Coding Guidelines (i.e. we basically have to manually list and block all POSIX C extension functions in the CI compliance check, as opposed to just having one check that blocks _POSIX_C_SOURCE).
Defining _POSIX_C_SOURCE in individual Zephyr source files sets the pattern for defining libc feature test macros in individual Zephyr source files. While _POSIX_C_SOURCE is a rather special case being part of the POSIX standard, other common feature test macros like _GNU_SOURCE and _BSD_SOURCE are not part of any “standards” and their (equivalent) names and interpretations may vary across different libcs (at this time, we only have two POSIX functions in the allowed non-ISO C function list; but, it is foreseeable that the list will grow in the future).

It is a bit not nice though to have the OS be so aware of newlib implementation details (so we couple to some degree the newlib version from the SDK with the Zephyr version). Architecturally it does not feel nice. Though this concern may be a bit theoretical, and I do not have in my mind a specially better option.

I think libc awareness from the OS side and vice versa is something inevitable for optimal integration between the two (e.g. things like thread safety, reentrancy, atomics and threads all require awareness from both sides). Without it, the libc integration would not be “complete.”

aescolar on Feb 1, 2024

CC @stephanosio @keith-packard @cfriedt @henrikbrixandersen @kartben

aescolar on Jan 30, 2024