-
Notifications
You must be signed in to change notification settings - Fork 6.7k
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
Add coding guidelines on C standard library function usage #57598
Add coding guidelines on C standard library function usage #57598
Conversation
a371282
to
6bec06c
Compare
I'm working to make Picolibc expose the "Zephyr" C library API directly. This will also enable the removal of |
6bec06c
to
18691c2
Compare
18691c2
to
235eee7
Compare
235eee7
to
672e4ee
Compare
672e4ee
to
e3b5ce8
Compare
@keith-packard For some reason, I cannot add comments to your review comment threads, so I am starting a new one:
Yes, they are indeed links. If you scroll down a bit more, you will see the link URLs.
Yes, because the Zephyr kernel testing needs to be done for every architecture and also the minimal libc can be useful during the initial bring-up of a new SoC/board.
Not necessarily. The
Sure, I will add a comment about introducing a new C standard library function to the Zephyr kernel scope/minimal libc -- that adding a new function to the list is allowed as long as it is justifiable (reviewed case-by-case). The goal is to regulate, not restrict unnecessarily. |
putchar(),ISO/IEC 9899:2011 | ||
puts(),ISO/IEC 9899:2011 | ||
qsort(),ISO/IEC 9899:2011 | ||
`qsort_r()`_,GNU |
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.
As per the discussion in the Toolchain WG on 2023-05-15, qsort_r()
, reallocarray()
and strerror_r()
will be removed from the list of allowed non-standard functions and made default n
in the minimal libc because they are not currently used anywhere.
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.
Sounds like I should remove them from the _ZEPHYR_SOURCE_
set in picolibc as well.
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.
That leaves strtok_r
and strnlen
as the only non ISO-C functions in this list. strnlen
is used once in lib/os/cbprintf_complete.c (which is only used if CONFIG_CBPRINTF_COMPLETE
is defined). There's a spurious prototype in that file because it doesn't define _POSIX_C_SOURCE 200809
. strtok_r
does not appear to be used at all. Looks like strtok_r
still needs to get moved to lib/libc/common.
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.
I cleaned up cbprintf_complete.c as part of #57619
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.
Sounds like I should remove them from the
_ZEPHYR_SOURCE_
set in picolibc as well.
For now, yes for the purpose of simplicity and consistency.
Note, however, that the set of non-standard functions allowed to be used in the Zephyr codebase (i.e. outside the "Zephyr kernel") does not necessarily have to match the set of non-standard functions allowed to be used in the "Zephyr kernel" given that the latter is a subset of the former.
strtok_r
does not appear to be used at all.
strtok_r
is used in some drivers and subsystems but not by any of the components that belong in the "Zephyr kernel" scope, so it could technically be removed from the Rule A.4.
This is actually where "the latter is a subset of the former" case comes in -- the Rule A.5 (and subsequently Picolibc with _ZEPHYR_SOURCE_
) includes strtok_r
, but not the Rule A.4.
Looks like strtok_r still needs to get moved to lib/libc/common.
Correct.
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.
Yes, that's helpful, and I think I understand the rules for code using C library functions, but I'm a lot more interested in the C library implementation itself and how it is supposed to advertise these functions, along with how we extend the lists of functions in both Rules A.4 and A.5.
Adding functions to the list in A.4 seems pretty easy -- provide an implementation of the function for the minimal C library and then just start using it. You'd need to get approval during the PR process. Any other C library will already provide an implementation and API declaration under the existing compilation rules, so there's nothing needed to use the new function with them.
Adding functions under A.5 seems more complicated, and potentially hard enough to discourage people. Similar to A.4, you need to provide an implementation for the common C library and get approval during the PR process. However, to start using this with any other C library, you need to provide a declaration of the API for every supported C library using the #include_next
mechanism that is being proposed in #57619, even for libraries which support the relevant standard (be that BSD, GNU or POSIX). For these libraries, there is a potential for skew between the Zephyr internal declarations and the library, which could lead to an accidental ABI difference and subsequent application failures, either at compile time or run time.
I'd like to consider a slightly modified version of #57619.
- Where the external library does not provide a definition of the relevant API, we have a Zephyr-specific version of the relevant header which
#include
s a common C library headers declaring the requested APIs along with a#include_next
to pull in the external library version of the file (although, that's problematic given that#include_next
is a GCC extension). - Where the external library does provide a definition of the relevant API, there is no Zephyr-specific version of the relevant header. Instead, the source code using the function would include the necessary
#define _*_SOURCE
statement so that the external library headers would include the declaration of the desired API. This would avoid any possibility of ABI skew between the Zephyr declaration and the library definition. - When using the picolibc module, that code would always be synchronized with the set of allowed APIs in A.5 under
_ZEPHYR_SOURCE
, so the#define _*_SOURCE
statement would not be required.
Making the #define _*_SOURCE
conditional upon whether the picolibc module was being used would allow verification of the source code to ensure it wasn't using functions beyond the scope of these rules without also adding a layer of non-standard C preprocessor usage -- the verification would only occur when using the picolibc module or C library that doesn't provide an API beyond ISO/IEC C.
Hrm. Maybe we should just create header files that declare the functions in A.5? Something like include/zephyr/libc.h
, or even include/zephyr/string.h
et al. We could hide all of the library-specific goo there without relying on #include_next
? That could check for the relevant config entries pulling implementations out of the common C library and insert declarations for those functions. For picolibc, _ZEPHYR_SOURCE_
could then omit declarations for functions that are pulled from the common C library (malloc, et al). This would catch use of malloc family functions not provided in the common C library implementation.
With the current plan, I don't see any reason to include support for _ZEPHYR_SOURCE
in picolibc -- Zephyr will always provide wrapping include files that declare the Zephyr API on top of the picolibc one, code for this in picolibc would be redundant.
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.
Adding functions under A.5 seems more complicated, and potentially hard enough to discourage people.
It can be a bit more involved than A.4 indeed; though, this only applies to non-standard functions since the standard functions are allowed by default by A.5.
Similar to A.4, you need to provide an implementation for the common C library and get approval during the PR process.
Correct.
However, to start using this with any other C library, you need to provide a declaration of the API for every supported C library using the
#include_next
mechanism that is being proposed in #57619, even for libraries which support the relevant standard (be that BSD, GNU or POSIX).
That would be the case for the libraries that cannot be modified to support a Zephyr-specific feature test macro (e.g. _ZEPHYR_SOURCE
).
For these libraries, there is a potential for skew between the Zephyr internal declarations and the library, which could lead to an accidental ABI difference and subsequent application failures, either at compile time or run time.
Correct. But, we require tests for all functions added to our LIBCs, so any "accidental ABI difference" that is a real problem would be caught by such tests.
I'd like to consider a slightly modified version of #57619.
- Where the external library does not provide a definition of the relevant API, we have a Zephyr-specific version of the relevant header which
#include
s a common C library headers declaring the requested APIs along with a#include_next
to pull in the external library version of the file (although, that's problematic given that#include_next
is a GCC extension).
Yes, that sounds reasonable.
Re: #include_next
, it is one of the few extensions that we take for granted (we should codify and document these one day ...). The expectation is that all C compilers (preprocessors), even for its own sake, should support #include_next
or have something equivalent to it since it would be pretty hard to write a C library without such a directive. Even IAR, the infamous compiler that is very conservative and lacks the support for various de facto standard C extensions, supports #include_next
.
- Where the external library does provide a definition of the relevant API, there is no Zephyr-specific version of the relevant header. Instead, the source code using the function would include the necessary
#define _*_SOURCE
statement so that the external library headers would include the declaration of the desired API. This would avoid any possibility of ABI skew between the Zephyr declaration and the library definition.
IMHO, this requires a further discussion. I have no problem with either approach (declaring local prototypes vs. using the feature test macros) for the C libraries other than Picolibc since they are not used for enforcing the Rule A.5 in our CI.
Some points I want to add is:
- Your concern about the ABI skew between the Zephyr declaration and the library declaration should be addressed by the C library test.
- If we decide to go with using the feature test macros, the macros must not be defined by each source file using the functions covered by such macros; instead, these macros should be defined by the libc integration headers under
lib/libc/*/include
. The rationale is that the feature test macros like_GNU_SOURCE
are not standardised and may vary across different LIBCs, and we do not want to clutter the general source files in the Zephyr codebase with these LIBC-specific feature test macros.
- When using the picolibc module, that code would always be synchronized with the set of allowed APIs in A.5 under
_ZEPHYR_SOURCE
, so the#define _*_SOURCE
statement would not be required.
Correct.
With the current plan, I don't see any reason to include support for
_ZEPHYR_SOURCE
in picolibc -- Zephyr will always provide wrapping include files that declare the Zephyr API on top of the picolibc one, code for this in picolibc would be redundant.
If you are referring to the changes in #57619, the lib/libc/picolibc/include/*.h
providing the Zephyr-side prototype declarations for the non-standard functions is only a temporary workaround until we update the minimum required Zephyr SDK version to a version that contains a more up-to-date Picolibc with _ZEPHYR_SOURCE
support -- once the minimum required Zephyr SDK version is updated, the Picolibc workarounds can be removed entirely and we can rely on _ZEPHYR_SOURCE
.
Hrm. Maybe we should just create header files that declare the functions in A.5? Something like
include/zephyr/libc.h
, or eveninclude/zephyr/string.h
et al. We could hide all of the library-specific goo there without relying on#include_next
? That could check for the relevant config entries pulling implementations out of the common C library and insert declarations for those functions. For picolibc,_ZEPHYR_SOURCE_
could then omit declarations for functions that are pulled from the common C library (malloc, et al). This would catch use of malloc family functions not provided in the common C library implementation.
I am not sure if I understand what you are suggesting here.
Are you saying that, for instance, we define our own string.h
with all standard and non-standard string.h
functions so that we do not need to include the LIBC string.h
at all?
But either way, as I explained above, we currently take #include_next
for granted and the LIBC would not be the first thing to fail if we come across a compiler that does not support #include_next
, so I would not worry too much about it.
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.
However, to start using this with any other C library, you need to provide a declaration of the API for every supported C library using the
#include_next
mechanism that is being proposed in #57619, even for libraries which support the relevant standard (be that BSD, GNU or POSIX).That would be the case for the libraries that cannot be modified to support a Zephyr-specific feature test macro (e.g.
_ZEPHYR_SOURCE
).
Not entirely -- we will need to add support in Zephyr to support older SDK versions and/or external versions of said libraries. Just as #57619 is adding picolibc support.
Re:
#include_next
, it is one of the few extensions that we take for granted (we should codify and document these one day ...). The expectation is that all C compilers (preprocessors), even for its own sake, should support#include_next
or have something equivalent to it since it would be pretty hard to write a C library without such a directive. Even IAR, the infamous compiler that is very conservative and lacks the support for various de facto standard C extensions, supports#include_next
.
Good to hear (btw, picolibc doesn't use #include_next
internally, but glibc sure makes a lot of use of it).
1. Your concern about the ABI skew between the Zephyr declaration and the library declaration should be addressed by the C library test.
Sounds like there's a requirement to add tests within Zephyr for all functions added in this fashion? Does that need to be part of this document?
2. If we decide to go with using the feature test macros, the macros must _not_ be defined by each source file using the functions covered by such macros; instead, these macros should be defined by the libc integration headers under `lib/libc/*/include`. The rationale is that the feature test macros like `_GNU_SOURCE` are not standardised and may vary across different LIBCs, and we do not want to clutter the general source files in the Zephyr codebase with these LIBC-specific feature test macros.
I think all of these feature test macros are well defined in their respective standards. _POSIX_C_SOURCE
is extensively described in the POSIX specs (https://pubs.opengroup.org/onlinepubs/9699919799/). Similarly, _GNU_SOURCE
is described in each Linux manual page for the affected functions (see the Glibc qsort_r()
manual for an example: https://man7.org/linux/man-pages/man3/qsort_r.3.html) along with the general glibc documentation https://ftp.gnu.org/old-gnu/Manuals/glibc-2.2.3/html_node/libc_13.html. Unfortunately, _BSD_SOURCE
is not described in any BSD documentation I can find, only in the glibc docs. The BSD community doesn't appear to be interested in having a C library provide a way to express different APIs? That's a big part of why I think using them directly in the C source file is a good idea -- that's how those functions are documented to be used.
Sadly, you cannot place them anywhere else -- you cannot change the C library API once the first header file has been processed so the way these feature macros work is incompatible with placing them in a header file.
Hrm. Now I realize a flaw in my plan -- a single C source file can only use a single API definition. If you define both _POSIX_C_SOURCE
and _GNU_SOURCE
in a source file, the glibc, picolibc and newlib will all expose the GNU API to the application. This means you cannot have both the POSIX strerror_r()
and GNU qsort_r()
functions accessible at the same time.
If you are referring to the changes in #57619, the
lib/libc/picolibc/include/*.h
providing the Zephyr-side prototype declarations for the non-standard functions is only a temporary workaround until we update the minimum required Zephyr SDK version to a version that contains a more up-to-date Picolibc with_ZEPHYR_SOURCE
support -- once the minimum required Zephyr SDK version is updated, the Picolibc workarounds can be removed entirely and we can rely on_ZEPHYR_SOURCE
.
I think it will be a permanent workaround to handle version skew between the picolibc module and the SDK as functions are added in the process defined by Rule A.5.
Are you saying that, for instance, we define our own
string.h
with all standard and non-standardstring.h
functions so that we do not need to include the LIBCstring.h
at all?
Not quite -- create a zephyr/string.h include file that makes sure all of the extended function declarations are available, perhaps also doing a #include <string.h>
to pull in the API available from the underlying C library. When using the common C library function, the extended function declarations could be in this file, or perhaps in a file provided by the common C library, possibly protected by a _*_SOURCE
check so that they weren't available unless the application requested them. When using another C library, this file wouldn't need to do anything extra as the C library string.h would be expected to provide the requested API.
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.
Not entirely -- we will need to add support in Zephyr to support older SDK versions and/or external versions of said libraries. Just as #57619 is adding picolibc support.
Once we make Picolibc the default libc for Zephyr, I think it would be reasonable to update the minimum required Zephyr SDK version when a change is made to the Rule A.5 (and subsequently the Picolibc _ZEPHYR_SOURCE
), in which case we would not need hacks like #57619 for Picolibc.
Sounds like there's a requirement to add tests within Zephyr for all functions added in this fashion? Does that need to be part of this document?
Yes, there needs to be an associated test when you introduce a new feature to Zephyr -- this is more or less a general project-wide requirement than a libc-specific one.
I think all of these feature test macros are well defined in their respective standards.
_POSIX_C_SOURCE
is extensively described in the POSIX specs (https://pubs.opengroup.org/onlinepubs/9699919799/). Similarly,_GNU_SOURCE
is described in each Linux manual page for the affected functions (see the Glibcqsort_r()
manual for an example: https://man7.org/linux/man-pages/man3/qsort_r.3.html) along with the general glibc documentation https://ftp.gnu.org/old-gnu/Manuals/glibc-2.2.3/html_node/libc_13.html. Unfortunately,_BSD_SOURCE
is not described in any BSD documentation I can find, only in the glibc docs. The BSD community doesn't appear to be interested in having a C library provide a way to express different APIs? That's a big part of why I think using them directly in the C source file is a good idea -- that's how those functions are documented to be used.
Neither the Linux man page nor the glibc documentation is a "standard." Not to mention the _BSD_SOURCE
is not clearly defined anywhere as you pointed out. The only real "standard" here is the POSIX for _POSIX_C_SOURCE
.
In that sense, it is completely up to each LIBC to decide whether they want to respect _GNU_SOURCE
and _BSD_SOURCE
(e.g. a LIBC may very well decide to use __USE_QSORT_R
instead of _GNU_SOURCE
), and we do not want to end up defining both _GNU_SOURCE
and __USE_QSORT_R
in every C file that uses these functions.
Sadly, you cannot place them anywhere else -- you cannot change the C library API once the first header file has been processed so the way these feature macros work is incompatible with placing them in a header file.
Hrm. Now I realize a flaw in my plan -- a single C source file can only use a single API definition. If you define both
_POSIX_C_SOURCE
and_GNU_SOURCE
in a source file, the glibc, picolibc and newlib will all expose the GNU API to the application. This means you cannot have both the POSIXstrerror_r()
and GNUqsort_r()
functions accessible at the same time.
That is indeed a problem, as we have already seen. At that point, unless we modify the LIBC to support _ZEPHYR_SOURCE
or handle the conflicts otherwise, I believe the best we can do is what is done in #57619.
I think it will be a permanent workaround to handle version skew between the picolibc module and the SDK as functions are added in the process defined by Rule A.5.
We only need to keep that workaround until we update the minimum required Zephyr SDK version, and as per the pointed I made above, I think it is reasonable to update the minimum required Zephyr SDK version when a change is made to the Rule A.5 and subsequently the Picolibc _ZEPHYR_SOURCE
.
Not quite -- create a zephyr/string.h include file that makes sure all of the extended function declarations are available, perhaps also doing a
#include <string.h>
to pull in the API available from the underlying C library. When using the common C library function, the extended function declarations could be in this file, or perhaps in a file provided by the common C library, possibly protected by a_*_SOURCE
check so that they weren't available unless the application requested them. When using another C library, this file wouldn't need to do anything extra as the C library string.h would be expected to provide the requested API.
I am still 100% clear on what you are suggesting here. How is (include/)zephyr/string.h
supposed to be included? Are you saying that Zephyr application should include zephyr/string.h
instead of string.h
?
Maybe a more discrete example would help.
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.
Neither the Linux man page nor the glibc documentation is a "standard." Not to mention the
_BSD_SOURCE
is not clearly defined anywhere as you pointed out. The only real "standard" here is the POSIX for_POSIX_C_SOURCE
.
We could argue semantics about what makes a standard, but given that the goal is for Zephyr to be able to mix-n-match functions from a variety of sources, it's clear it won't matter in the end -- we don't know anything better than #57619 that can be used to describe the Zephyr API for C libraries that don't provide _ZEPHYR_SOURCE
support.
We only need to keep that workaround until we update the minimum required Zephyr SDK version, and as per the pointed I made above, I think it is reasonable to update the minimum required Zephyr SDK version when a change is made to the Rule A.5 and subsequently the Picolibc
_ZEPHYR_SOURCE
.
I suspect there will be an occasional need to support multiple picolibc versions for Zephyr, whether to allow use of a range of SDK versions or to allow use of an external toolchain with picolibc support (like armclang).
If we can add validation of the non ISO/IEC C library APIs provided by _ZEPHYR_SOURCE
using the technique I suggested in #57619, then I think we'll catch API mismatches during testing, which reduces the concerns I had over adding C library specific declarations for functions within the Zephyr code base.
Architecture WG:
|
@nashif @evgeniy-paltsev As discussed in the Architecture WG meeting today, the purpose of the "Rule A.4: C Standard Library Usage Restrictions in Zephyr Kernel" is to restrict the usage of the libc functions in the "Zephyr kernel" to what is provided by the minimal libc, and not necessarily to give the green light on these functions being used in the "Zephyr kernel" -- that would be subject to a different set of rules (i.e. secure and safe coding practices, and most importantly the common sense). I will add a comment in the "Rule A.4: C Standard Library Usage Restrictions in Zephyr Kernel" to clarify that the use of the C standard library functions listed in the rule are subject to the secure and safe coding practices, and the functions being listed in it does not imply that their use in the "Zephyr kernel" is unconditionally justified or permitted. |
Oof. I just learned about |
In the past, we discussed adopting the optional Annex K alongside the safeclib in the Zephyr codebase, but it was rejected by the Security WG on the grounds that the Annex K is too controversial. |
The general direction of making Picolibc the default libc for Zephyr and introducing the new coding guidelines rules to regulate the C standard library function usage have been approved by the TSC on 2023-05-17. The exact phrasing of the coding guidelines rules still need to be finalised before this PR can be merged. |
This commit adds the "Rule A.4: C Standard Library Usage Restrictions in Zephyr kernel" to the coding guidelines. Signed-off-by: Stephanos Ioannidis <[email protected]>
This commit adds the "Rule A.5: C Standard Library Usage Restrictions in Zephyr Codebase" to the coding guidelines. The initial list of allowed non-ISO C libc functions is based on the non-ISO C functions available in the minimal libc. Signed-off-by: Stephanos Ioannidis <[email protected]>
This commit adds the "Functions" section to the minimal libc documentation that describes the scope of the C standard library functions available in the minimal libc. Signed-off-by: Stephanos Ioannidis <[email protected]>
e3b5ce8
to
216528d
Compare
UPDATES
|
@keith-packard @cfriedt @galak @keith-short All of the concerns raised earlier should be addressed now. Please take another look. |
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.
This looks good to me now.
Merging since this has been approved by both the TSC and the Coding Guidelines maintainer. |
This series adds the coding guidelines rules on the usage of C standard library functions in the Zephyr codebase, in preparation for making Picolibc the default libc.
It adds two new rules:
Once these rules are accepted, additional testcases and CI checks will be implemented to enforce them.
Preview: https://builds.zephyrproject.io/zephyr/pr/57598/docs/index.html
Part of #49922