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

[aescolar]

Background:

1. 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.
2. 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.
3. 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 #67040 (comment) , #67040 (comment) , #67040 (comment) )

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:

• https://docs.zephyrproject.org/latest/contribute/coding_guidelines/index.html#rule-a-4-c-standard-library-usage-restrictions-in-zephyr-kernel
• https://docs.zephyrproject.org/latest/contribute/coding_guidelines/index.html#rule-a-5-c-standard-library-usage-restrictions-in-zephyr-codebase
• Add coding guidelines on C standard library function usage #57598
• Remove explicit -D_POSIX_C_SOURCE global compiler options when building with picolibc #67040
• Several fixes around POSIX_SOURCE, remove global definitions of POSIX_SOURCE #67052
• Make picolibc the default C library for Zephyr #49922
  • Specifically this part of the discussion Make picolibc the default C library for Zephyr #49922 (comment)
• scripts/checkpatch: Check for patches adding #defines for libc APIs #60487
• libc: newlib/picolibc: define prototypes for non-ansi C functions #57619

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.

[aescolar]

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

[cfriedt]

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.

Wow - this sounds an awful lot like someone is being told what they think, rather than their opinion simply being asked of them..

Can you tag the developers you are referring to, here? So that maybe they could share their opinion themselves?

[aescolar]

Can you tag the developers you are referring to

They have been tagged. You are welcome to elaborate on your point of view. This is an RFC.

[keith-packard]

I've modified #67040 so that it makes newlib expose the additional APIs by wrapping string.h. It seems like we should be able to do the same for any other C library? Even the native C library?

[aescolar]

I've modified #67040 so that it makes newlib expose the additional APIs by wrapping string.h. It seems like we should be able to do the same for any other C library? Even the native C library?

Thanks @keith-packard , but that assumes an outcome for this RFC which is not given. I would prefer if we could discuss here first if we should expect C libraries integration to do this. If you think that it should be yes, you are more than welcome to motivate it.

[aescolar]

It seems like we should be able to do the same for any other C library? Even the native C library?

For the libraries Zephyr provides (either as source or as part of the SDK) we can. Though we create a source relationship which architecturally I would think is best to avoid. (i.e. "Is it good that any file in the OS side knows about C library implementation details instead of just relaying on the standard way of configuring it?")
An important thing to note here is that we do not necessarily know what library the user uses (Zephyr is used by some with their own C library), so as such that creates that problem for those users.
(For the native C library, although typically that is glibc I don't think we can assume it in general. And even for glibc we cannot know the version)
So as such, the complexity we add both to ourselves and our users would be an argument against assuming the C library needs to expose these 2 extra function prototypes by default.
To me, it seems the main argument for exposing these 2 prototypes is one of minor convenience for developers and avoiding tripping checkpatch (if the users adds _POSIX_C_SOURCE to expose them in .c files, instead of in cmake).
So I lean quite strongly towards not assuming the C library is meant to expose these by default, neither by special treatment of Zephyr in the C library, or its integration.

[aescolar]

As an attempt to collect this conversation in one place, here a verbatim copy of @stephanosio comment: #68381 (comment)

---

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:

1. It is too "noisy."
2. 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).
3. 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."

[cfriedt]

2. 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.

Maybe I've misunderstood, but I do not believe point 2 accurately describes picolibc.

It also seems that nobody has come forward to confirm their understanding is as described above otherwise.

Just a suggestion, but it might be better to reword point 2 so that it only includes facts.

E.g., if I were to reword point number 2, I would write

2. the minimal libc does not conditionally declare POSIX functions based on application conformance macros

[cfriedt]

Can you tag the developers you are referring to

They have been tagged. You are welcome to elaborate on your point of view. This is an RFC.

Who was tagged specifically? I don't know anyone with the understanding described in point 2.

[cfriedt]

Personally, I would be happy to have application conformance macros in the minimal libc (as well as the posix headers) to conditionally declare various functions. Implementation conformance macros as well. That is part of #51211 already.

defined(_ZEPHYR_SOURCE) || _POSIX_C_SOURCE >= level seems like a good option to enable visibility of the few POSIX functions that are required by A.4 or A 5 (strnlen, strtok_r, maybe gmtime_r?)

[aescolar]

Thanks @stephanosio , to document my counters

Enforcing the POSIX C extension usage throughout Zephyr codebase becomes much more difficult because

Today the detection/enforcement through checkpatch is quite flawed: it does not detect usage of not allowed ISO functions, it does not detect SOURCE macros being set in cmake files; and setting these macros is anyhow technically correct and not in violation of these rules. I think it would be better fixing this detection anyhow, and I don't think it should be based on these macros being set.

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..

As you mentioned it is a known standard. I would like to clarify that I am not proposing to sprinkle our source with defines to work around quirky C libraries or compilers. Setting _POSIX_C_SOURCE in this case is expected and necessary when building with pico, newlibc and glibc when setting the compiler in strict iso mode (-std=c<xx> as we do in Zephyr) targeting other systems/OSs.

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."

I think it's worth noting that:
a) I expect many users out there would be happy enough with their own C library not being Zephyr aware as they would not be using more "advanced" functionality that require these (for ex. even malloc is a quite advanced feature many do not use in embedded)
b) Even if a C library needs some level of integration with the OS, limiting in how many ways it needs to be customized would seem sensible. Moreover, one could hope a complete enough C library would either provide appropriate hooks {for acquiring mutexes, setting a putchar backend,..}; or that if not, that the maintainers of the library could be convinced to provide those. But it seems much less likely we could expect or be able to convince library maintainers to expose a different subset of APIs when compiling for Zephyr (even though @keith-packard has been really accommodating by modifying picolibc to do so). So while the first could be eventually solved in an architecturally clean manner, the second probably wouldn't.
My overall thinking is that there are too many C libraries out there, some proprietary and optimized for some architectures and compilers, so we can not expect them to accommodate Zephyr in quirky ways. And we should not set demands on them or their integration that either expect them to do unlikely things or cannot be solved in an architecturally sound way, specially if there is not a very good argument for these.

My overall feeling is that by going in this direction (so we require things like #68381 both in and out of tree) we are choosing a problematic path for both us and the users that will cause significantly more trouble, for a very limited benefit.

[aescolar]

As an extra motivation for this proposal, note the concern raised in
#68414 (comment) due to #68414 .
That is, that without doing what is proposed in this RFC but instead assuming "2." , the picolibc version, and therefore the Zephyr SDK version, are linked to the Zephyr version (what is allowed in rules A.4 & A.5) a bit too tight, and therefore changes in these rules require updates to those to keep Zephyr building warning free (unless what is proposed in this RFC is also done).

That is, that the alternative for what is proposed in this RFC (i.e. assuming 2.), in its best case, couples the C library too tightly to Zephyr.

[stephanosio]

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?

[cfriedt]

I just spent 4.5 hours responding to somewhat related comments in another PR and have appointments and 8 hours of actual work to attend to at this point. Not to mention other obligations.

This also seems to duplicate an issue that was filed prior to it #67283.

Unfortunately, it may take some time for me to re-review this, so thanks for your patience.

[cfriedt]

Just at a glance, the proposed solution in #67283 seems to be quite conservative - i.e. simply adjust checkpatch.pl to reduce false positives.

I have to re-read this again at some point, but it seems like it's going far beyond .. also, there has been an RFC on the topic of POSIX application conformance macros for several weeks that seems to be blocked for some reason by @aescolar.

I would hate to see the process of the other RFC circumvented. To me that does not seem right.

[stephanosio]

@cfriedt It was my understanding that you, the POSIX API layer maintainer, wanted to ensure _POSIX_C_SOURCE can be used in individual Zephyr C source files, according to the POSIX standard. Is that not correct?

[cfriedt]

after all, that is the standard compliant and hence the most hassle-free way forward.

I'm not sure if I necessarily agree.

We should stop pretending that hard-coding _POSIX_C_SOURCE in a source file is the only way to define application conformance. The standard specifies that _POSIX_C_SOURCE should be declared before including any header file. We all know that macros can be defined (and undefined) on the command line (-D_POSIX_C_SOURCE=200809L) before any header file is included. We all know that headers can be included on the command line (e.g. -include autoconf.h) before any header is included in source code. Some macros can even be defined in spec files.

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

This is true. However, we can also simply declare them in order to meet Coding Guidelines Rules A.4 and A.5, as we have always done. Since this is documented (and realistically is done due to shortcomings in < ISO C2x), it's a perfectly acceptable deviation.

There are already conformant strnlen, strtok_r, and gmtime_r declarations and implementions in-tree. I think having those 3 functions declared in common libc headers is perfectly fine, with the implementations living in either common libc or lib/posix.

It would be trivial to add a default-y Kconfig option to declare them so that systems with a conflicting definition (be it macro, inline, etc) can be accommodated (to explicitly n-select the declaration).

It certainly would scale better (O(1) vs O(n)).

Doing it this way means we still do not expose the kernel and core parts of the OS to the rest of the POSIX API.

@cfriedt It was my understanding that you, the POSIX API layer maintainer, wanted to ensure _POSIX_C_SOURCE can be used in individual Zephyr C source files, according to the POSIX standard. Is that not correct?

Thank you for asking me for my opinion.

_POSIX_C_SOURCE should only be hard-coded manually in source files as a last-resort, and likely not with application-facing code.

This is true for many reasons and here are just a few:

• the application should be able to adjust _POSIX_C_SOURCE and _XOPEN_SOURCE (i.e. application conformance feature test macros) as necessary in a consistent way to avoid unspecified behavior
• manually defining reserved macros is prone to human error and scales poorly
• it's much easier to scale consistently by making _POSIX_C_SOURCE configurable (i.e. use Kconfig to configure software)

Manually redefining _POSIX_C_SOURCE wherever needed scales linearly (at best), while Kconfig is constant complexity (O(n) vs O(1)).

Even just in terms of usability - if the application is required to use a specific _POSIX_C_SOURCE version and as a result, the user needs to adjust the hard-coded version in 300 different places, the user cannot say they are using e.g. zephyr v3.6.1 because they've needed to modify it.

For non-application-facing code (in particular, arch/posix) hard-code away - as long as it is not a global cflag. I mean, even there I would encourage a consistent way of managing versions, but I do not want to overreach.

[aescolar]

One thing to note is that declaring the prototypes of these for other libaries is problematic.
For example, for the host C libray we have no control of which version the user has
(people running on 5 years OSs is not abnormal), or even what C library.
The only reliable solution for the host C library, and the one other would likely attempt first
with other C libraries would be to just define this macro globally in the build files.

.. _POSIX_C_SOURCE in a source file is the only way ..

Even though it is not the only way, there is a benefit of doing so:
Defining them after undefining them in the C source file avoids any possible issue with redefinitions,
as it is the last possible definition. (A user may have defined the macro to be added to the build command line)