Skip to content
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

Make picolibc the default C library for Zephyr #49922

Closed
60 tasks done
Tracked by #45776
stephanosio opened this issue Sep 5, 2022 · 40 comments · Fixed by #57340
Closed
60 tasks done
Tracked by #45776

Make picolibc the default C library for Zephyr #49922

stephanosio opened this issue Sep 5, 2022 · 40 comments · Fixed by #57340
Assignees
Labels
area: C Library C Standard Library area: picolibc Picolibc C Standard Library Meta A collection of features, enhancements or bugs RFC Request For Comments: want input from the community

Comments

@stephanosio
Copy link
Member

stephanosio commented Sep 5, 2022

#44096 introduced the picolibc, a full-featured yet light-weight C library and fork of newlib, as a module for Zephyr that can be configured and fine-tuned at the Zephyr build time to minimise its footprint.

This issue describes the tasks required for making the picolibc the default C library support in the Zephyr RTOS.

Tasks

Related discussions

@stephanosio stephanosio added area: C Library C Standard Library RFC Request For Comments: want input from the community Meta A collection of features, enhancements or bugs area: picolibc Picolibc C Standard Library labels Sep 5, 2022
@stephanosio stephanosio mentioned this issue Sep 5, 2022
6 tasks
@keith-packard
Copy link
Collaborator

I think we should use an SDK version that includes picolibc -- that way we can run both C and C++ tests and use the picolibc module and the SDK version. I think that means getting picolibc into a version of the SDK that we can use from the github CI system?

@stephanosio
Copy link
Member Author

I think we should use an SDK version that includes picolibc -- that way we can run both C and C++ tests and use the picolibc module and the SDK version. I think that means getting picolibc into a version of the SDK that we can use from the github CI system?

Yes, zephyrproject-rtos/sdk-ng#287 will need to be completed as part of this.

@stephanosio
Copy link
Member Author

cc @nashif @carlescufi @tejlmand

@stephanosio
Copy link
Member Author

Toolchain WG on 2022-09-20

  • @nashif: This will need to be discussed and reviewed at the project level (TSC).
  • @nashif: In general, no strong objection to the general direction of making picolibc the default C library; however, there are some concerns regarding the subsequent effects that this can have:
    • @nashif: How do we prevent the core parts of Zephyr (e.g. kernel) from abusing all the available C library functions that may lead to overall footprint increase?
      • @stephanosio: We can define a minimal set of C standard library functions that are sanctioned to be used in the core parts of Zephyr and guard any extraneous functions by a Kconfig such that they are not available for use by default.
    • @nashif: Using the C standard library functions that are supported by the picolibc but not by the minimal libc will render Zephyr unusable with the minimal libc. How do we prevent that?
      • @stephanosio: We can take a similar approach as the above. But, the minimal libc is TOO minimal and it should not be the "golden standard" to which all Zephyr subsystems are doomed.
      • @stephanosio: For kernel, limiting the availability of the C standard library functions to what is currently provided in the minimal libc seems reasonable. For other subsystems, more flexibility should be allowed even at the cost of breaking the compatibility with the minimal libc.

@keith-packard
Copy link
Collaborator

Is the eventual plan to get rid of the minimal libc?

@stephanosio
Copy link
Member Author

Is the eventual plan to get rid of the minimal libc?

Not in the foreseeable future (it will be too controversial). The minimal libc will stay to provide somewhat self-contained? way of testing the kernel.

To quote what @nashif posted in the Teams chat:

[1:10 AM] Nashif, Anas
in zephyr 1.0.0....

[1:10 AM] Nashif, Anas
config MINIMAL_LIBC        bool        prompt "Build minimal c library"        help        Build integrated minimal c library. This integrated library is available        to support kernel functionality and test cases. It is not designed to be        used with applications. For applications, please use an external C        library such as newlib.

@nashif
Copy link
Member

nashif commented Sep 19, 2022

Is the eventual plan to get rid of the minimal libc?

Not in the foreseeable future (it will be too controversial). The minimal libc will stay to provide somewhat self-contained? way of testing the kernel.

My point above was that we need to just be cautious with such a move (picolibc as the default libc, dropping minimal, etc.) and think about all possible consequences that might affect users. Last time we tried to make such a move (make newlib default), there were countless issues and that did not happen. (#3102)

@keith-packard
Copy link
Collaborator

Last time we tried to make such a move (make newlib default), there were countless issues and that did not happen. (#3102)

Thanks for that link, really useful to be able to review what happened in the past. It looks like picolibc has a few benefits that should help resolve some of the issues from the past:

  • picolibc's printf is smaller than printk, so we can use printf everywhere and save space.
  • picolibc uses TLS for all thread-specific data, avoiding reentrancy issues and the large struct _reent
  • picolibc can be built as a module, instead of being tied to the toolchain.

We also need to ensure that picolibc sees more testing and that bugs related to using picolibc are resolved. I had been working on this slowly during the early picolibc integration work, and many of the patches required to work with picolibc have been merged.

We also need to get Zephyr-specific changes integrated into picolibc -- to make sure the right POSIX APIs are exposed in Picolibc headers when compiling Zephyr code. There are already bits for a few operating systems present, so it's easy and natural to add another one.

Finally, we need to get the Zephyr SDK built to include picolibc along with libstdc++ support. When building with C++, it would be quite a bit of work to get libstdc++ built as a module, so instead we should use the existing crosstool-ng support to get libstdc++ built for picolibc and then use picolibc from the SDK instead of as a module. That's blocked on deciding how to package the SDK to include picolibc. Simply adding picolibc will increase the SDK size quite a bit, potentially going over 2GB. Add to that the requests for including debug symbols in the SDK to ease development.

@nashif
Copy link
Member

nashif commented Sep 21, 2022

@stephanosio @keith-packard how far can we get by enabling picolibc as the default in Zephyr as a module? Would this work with all supported toolchains? Adding picolibc to the SDK is fine, however we need to make sure other non-SDK toolchains still work and are not left behind and there need to be parity. We should not get into the situation where using zephyr with some features enabled forces the use the Zephyr SDK.

@stephanosio
Copy link
Member Author

stephanosio commented Sep 21, 2022

Would this work with all supported toolchains

Noting that even the minimal libc seems to be having issues with some proprietary toolchains (e.g. with armclang as mentioned by @tejlmand during the meeting), it will likely need some tweaking.

But, in principle, it is no different to the minimal libc in that it can be adapted to work with any toolchains we support now and in the future because it is integrated to the Zephyr build process and we build it from source.

however we need to make sure other non-SDK toolchains still work and are not left behind and there need to be parity. We should not get into the situation where using zephyr with some features enabled forces the use the Zephyr SDK.

As long as the toolchain itself allows using a custom C library (i.e. not linking to the default C library embedded in the toolchain), there is nothing fundamentally preventing the picolibc module from working with them (basically, the same requirement as the minimal libc).

p.s. one of the reasons for adding picolibc as a module was to allow it to be used in a toolchain agnostic way (see #44143).

@keith-packard
Copy link
Collaborator

What kind of per-architecture testing are you looking for? Do you want external picolibc tests? Or internal Zephyr tests? External picolibc tests rely on architecture-specific support for stand-alone applications to be included in picolibc, which would be nice, but is a pretty big lift for some architectures. Picolibc currently has this for arm, aarch64, power9, risc-v and x86. Internal zephyr tests don't use much of the library, so coverage is weak. I'd be happy to include additional architectures in the picolibc test suite, if someone wants to provide the code.

@evgeniy-paltsev
Copy link
Collaborator

Just a note for the case of ARC (both arc and arc64) & ARC MWDT toolchain we want to keep Zephyr minimal libc as a default - as we doesn't support piclolib with MWDT toolchain.

Not fully sure if we want to switch ARC & GNU toolchain to have picolibc as default (as we don't support it for ARC officially)

@stephanosio
Copy link
Member Author

During the discussion of #57340 in the TSC meeting on 2023-05-03, the following concerns were raised:

  1. What is the scope of the "Zephyr kernel" that is supposed to be compatible with the minimal libc?
  2. How do we prevent people from committing code that uses non-standard functions available in Picolibc.

I will look into addressing these concerns by:

  1. Documenting the scope of the "Zephyr kernel."
  2. Implementing a test to ensure that "Zephyr kernel" does not make use of any functions that are not available in the minimal libc.
  3. Documenting the list of allowed standard C library functions that can be used in the broader Zephyr codebase.
  4. Implementing a CI check to ensure that disallowed standard C library functions are not used.

@keith-packard
Copy link
Collaborator

keith-packard commented May 3, 2023

  1. Implementing a test to ensure that "Zephyr kernel" does not make use of any functions that are not available in the minimal libc.

I think this duplicates 4. below?

3. Documenting the list of _allowed_ standard C library functions that can be used in the broader Zephyr codebase.

I suggest that this should be explicit about whether Zephyr expects these functions to follow the POSIX or C standard, as well as a general policy about whether functions that exist only in the POSIX standard could be considered for inclusion in this list.

4. Implementing a CI check to ensure that _disallowed_ standard C library functions are not used.

Should this instead say "that only allowed standard C library functions are used"? Seems like an allow list will be easier to manage than a disallow list.

@andyross
Copy link
Contributor

andyross commented May 3, 2023

Should this instead say "that only allowed standard C library functions are used"? Seems like an allow list will be easier to manage than a disallow list.

Or just "the allowed list is whatever is in minimal libc", and the needed CI check is "build everything in tests/kernel with minimal libc".

@keith-packard
Copy link
Collaborator

Or just "the allowed list is whatever is in minimal libc", and the needed CI check is "build everything in tests/kernel with minimal libc".

That's certainly easier than scanning code for use of symbols present in picolibc but not present in the minimal C library.

@keith-packard
Copy link
Collaborator

keith-packard commented May 4, 2023

I hacked up a script that generates a list of symbols exported by picolibc and not exported by the minimal C library by scanning the public header files from both. It's a terrible combination of coccinelle and classic Unix tools; there's probably a much cleaner way of doing this ...

keith-packard@310e7ed

It reads the picolibc headers from the SDK and the minimal C library headers from Zephyr sources.

@stephanosio
Copy link
Member Author

Should this instead say "that only allowed standard C library functions are used"? Seems like an allow list will be easier to manage than a disallow list.

Or just "the allowed list is whatever is in minimal libc", and the needed CI check is "build everything in tests/kernel with minimal libc".

That was the original plan and the TSC was not satisfied with that.

As mentioned in #49922 (comment), first of all, we need to clearly define the scope of "Zephyr kernel" because limiting its scope to kernel/ is not enough. After that, we can document the policy and CI check for that -- now that is the first part.

The second part is to come up with an allow list of the allowed standard C library functions (and naturally a disallow list of the non-standard and deprecated standard C library functions available in Picolibc and other common "full" LIBCs) for the broader Zephyr codebase (i.e. not just the kernel), because we do not want people abusing the non-standard functions and the deprecated standard functions provided in Picolibc.

The first part is quite straight forward once we clearly define the scope of "Zephyr kernel" (though, that it is by no means an easy task because everyone will have a slightly different opinion on what "Zephyr kernel" or "core parts of Zephyr" really is).

The second part can be implemented in a few different ways; for example:

  1. Modifying Picolibc to self-limit the scope of functions available (something like __ZEPHYR_VISIBLE).
  2. Code scan (CI check) using the disallow list of non-standard and deprecated standard C library functions

I am leaning towards the second option because it is a more general solution (as in not libc-specific) and can be used to enforce the our "libc function usage policy" across multiple LIBCs, which is important because we may have CI workflows for third-party toolchains that do not use Picolibc in the future.

@stephanosio
Copy link
Member Author

The second part can be implemented in a few different ways; for example:

  1. Modifying Picolibc to self-limit the scope of functions available (something like __ZEPHYR_VISIBLE).
  2. Code scan (CI check) using the disallow list of non-standard and deprecated standard C library functions

I am leaning towards the second option because it is a more general solution (as in not libc-specific) and can be used to enforce the our "libc function usage policy" across multiple LIBCs, which is important because we may have CI workflows for third-party toolchains that do not use Picolibc in the future.

Another simpler option could be to:

  1. Use Picolibc with _ANSI_SOURCE so that only the standard functions are available.
  2. Define the prototypes of the allowed extension functions in the Zephyr-side local Picolibc headers to only permit the allowed extension functions to be used (Add coding guidelines on C standard library function usage #57598).
  3. Add a CI check to prevent any upstream source files from defining the feature test macros (e.g. _GNU_SOURCE).

This will effectively allow us to restrict the Picolibc function usage to what is proposed in #57598, without coming up with an explicit disallow list for the CI check.

Given that any other C standard libraries implementing extensions will have some form of feature test macros, this should also be adaptable across multiple LIBCs.

@keith-packard
Copy link
Collaborator

Another simpler option could be to:

1. Use Picolibc with `_ANSI_SOURCE` so that only the _standard_ functions are available.

2. Define the prototypes of the allowed extension functions in the Zephyr-side local Picolibc headers to only permit the allowed extension functions to be used ([Add coding guidelines on C standard library function usage #57598](https://github.com/zephyrproject-rtos/zephyr/pull/57598)).

Alternatively, adding support for _ZEPHYR_SOURCE to picolibc so that it would expose the Zephyr API set directly, allowing code integrated into Zephyr to define _POSIX_SOURCE, _BSD_SOURCE, _DEFAULT_SOURCE or _GNU_SOURCE as desired. As we want to move to that solution in the future anyways, how about we just get there now instead? It's all the same work -- auditing the set of functions exposed by the library, but placing the results in the picolibc source code will ensure that it's handled uniformly wrt the other _SOURCE macros.

3. Add a CI check to prevent any upstream source files from defining the feature test macros (e.g. `_GNU_SOURCE`).

There are existing uses of _DEFAULT_SOURCE, _GNU_SOURCE, _BSD_SOURCE and _POSIX_C_SOURCE in the code, which would need to get fixed. Fortunately not many though.

This will effectively allow us to restrict the Picolibc function usage to what is proposed in #57598, without coming up with an explicit disallow list for the CI check.

This would also allow code which happens to use these names not get caught by the disallow list on accident. It wouldn't catch code which includes explicit prototypes for these functions from gaining access to the underlying C library function. Which error would you prefer?

Given that any other C standard libraries implementing extensions will have some form of feature test macros, this should also be adaptable across multiple LIBCs.

What other C libraries are relevant here? Aside from newlib, I thought most of the others were basic C libraries without POSIX support? And I would not be surprised if we could get newlib to adopt the _ZEPHYR_SOURCE changes.

@stephanosio
Copy link
Member Author

stephanosio commented May 5, 2023

Alternatively, adding support for _ZEPHYR_SOURCE to picolibc so that it would expose the Zephyr API set directly, allowing code integrated into Zephyr to define _POSIX_SOURCE, _BSD_SOURCE, _DEFAULT_SOURCE or _GNU_SOURCE as desired. As we want to move to that solution in the future anyways, how about we just get there now instead? It's all the same work -- auditing the set of functions exposed by the library, but placing the results in the picolibc source code will ensure that it's handled uniformly wrt the other _SOURCE macros.

No problem with this approach whatsoever; in fact, I prefer this to #57619. I am kind of rushing because I want to get this done in one way or another before the 3.4 feature freeze.

There are existing uses of _DEFAULT_SOURCE, _GNU_SOURCE, _BSD_SOURCE and _POSIX_C_SOURCE in the code, which would need to get fixed. Fortunately not many though.

Yes, fixing those should be quite trivial (in theory).

This would also allow code which happens to use these names not get caught by the disallow list on accident.

Correct.

It wouldn't catch code which includes explicit prototypes for these functions from gaining access to the underlying C library function. Which error would you prefer?

Sure; but, brute-force attempts like that will be easily noticed and rejected during a human code review.

What other C libraries are relevant here? Aside from newlib, I thought most of the others were basic C libraries without POSIX support?

This was more or less hypothetical -- if we ever want to add support for a libc that includes extensions in the future.

And I would not be surprised if we could get newlib to adopt the _ZEPHYR_SOURCE changes.

@keith-packard
Copy link
Collaborator

keith-packard commented May 21, 2023

Which kernel tests do you want built with the minimal C library? I'm working on a PR to add kernel tests that use CONFIG_MINIMAL_LIBC=y to select the minimal C library.

@JordanYates
Copy link
Collaborator

JordanYates commented Jun 9, 2023

Potential issue: printing 64bit integers (long long support) using the pre-built libraries requires using the full floating point library variant. Currently PICOLIBC_IO_LONG_LONG doesn't do anything for pre-built libraries, and values are silently truncated to 32bits.

@nashif nashif removed this from the v3.4.0 milestone Jun 12, 2023
@nashif nashif moved this from v3.4 to v3.5 in Release Plan Jun 14, 2023
keith-packard added a commit to keith-packard/zephyr that referenced this issue Jul 17, 2023
All code in the Zephyr core must use only the Zephyr C library API
according to rules A.4 and A.5. Such code is not permitted to request API
extensions from the C library via any of the API request mechanisms.

This addition to checkpatch.pl verifies that patches don't #define
any of these:

	__STRICT_ANSI__
	_POSIX_SOURCE
	_POSIX_C_SOURCE
	_XOPEN_SOURCE
	_ISOC99_SOURCE
	_ISOC11_SOURCE
	_ATFILE_SOURCE
	_GNU_SOURCE
	_BSD_SOURCE
	_SVID_SOURCE
	_DEFAULT_SOURCE

Reference: zephyrproject-rtos#49922

Signed-off-by: Keith Packard <[email protected]>
keith-packard added a commit to keith-packard/zephyr that referenced this issue Jul 17, 2023
All code in the Zephyr core must use only the Zephyr C library API
according to rules A.4 and A.5. Such code is not permitted to request API
extensions from the C library via any of the API request mechanisms.

This addition to checkpatch.pl verifies that patches don't #define
any of these:

	__STRICT_ANSI__
	_POSIX_SOURCE
	_POSIX_C_SOURCE
	_XOPEN_SOURCE
	_ISOC99_SOURCE
	_ISOC11_SOURCE
	_ATFILE_SOURCE
	_GNU_SOURCE
	_BSD_SOURCE
	_SVID_SOURCE
	_DEFAULT_SOURCE

Reference: zephyrproject-rtos#49922

Signed-off-by: Keith Packard <[email protected]>
keith-packard added a commit to keith-packard/zephyr that referenced this issue Jul 17, 2023
All code in the Zephyr core must use only the Zephyr C library API
according to rules A.4 and A.5. Such code is not permitted to request API
extensions from the C library via any of the API request mechanisms.

This addition to checkpatch.pl verifies that patches don't #define
any of these:

	__STRICT_ANSI__
	_POSIX_SOURCE
	_POSIX_C_SOURCE
	_XOPEN_SOURCE
	_ISOC99_SOURCE
	_ISOC11_SOURCE
	_ATFILE_SOURCE
	_GNU_SOURCE
	_BSD_SOURCE
	_SVID_SOURCE
	_DEFAULT_SOURCE

Reference: zephyrproject-rtos#49922

Signed-off-by: Keith Packard <[email protected]>
fabiobaltieri pushed a commit that referenced this issue Jul 18, 2023
All code in the Zephyr core must use only the Zephyr C library API
according to rules A.4 and A.5. Such code is not permitted to request API
extensions from the C library via any of the API request mechanisms.

This addition to checkpatch.pl verifies that patches don't #define
any of these:

	__STRICT_ANSI__
	_POSIX_SOURCE
	_POSIX_C_SOURCE
	_XOPEN_SOURCE
	_ISOC99_SOURCE
	_ISOC11_SOURCE
	_ATFILE_SOURCE
	_GNU_SOURCE
	_BSD_SOURCE
	_SVID_SOURCE
	_DEFAULT_SOURCE

Reference: #49922

Signed-off-by: Keith Packard <[email protected]>
kunoh pushed a commit to kunoh/zephyr that referenced this issue Aug 7, 2023
All code in the Zephyr core must use only the Zephyr C library API
according to rules A.4 and A.5. Such code is not permitted to request API
extensions from the C library via any of the API request mechanisms.

This addition to checkpatch.pl verifies that patches don't #define
any of these:

	__STRICT_ANSI__
	_POSIX_SOURCE
	_POSIX_C_SOURCE
	_XOPEN_SOURCE
	_ISOC99_SOURCE
	_ISOC11_SOURCE
	_ATFILE_SOURCE
	_GNU_SOURCE
	_BSD_SOURCE
	_SVID_SOURCE
	_DEFAULT_SOURCE

Reference: zephyrproject-rtos#49922

Signed-off-by: Keith Packard <[email protected]>
@tejlmand
Copy link
Collaborator

Toolchain wg: @keith-packard anything left before this is ready for merge ?
All issues in #57340 are resolved.

@keith-packard
Copy link
Collaborator

Toolchain wg: @keith-packard anything left before this is ready for merge ? All issues in #57340 are resolved.

The last run of #57340 appears to have hit a regression, so we should get that fixed. Otherwise, yes, this is ready to merge.

@keith-packard
Copy link
Collaborator

The regression isn't related to picolibc at all; it's caused by a change in how the microbit emulator is run. #61719 has the details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: C Library C Standard Library area: picolibc Picolibc C Standard Library Meta A collection of features, enhancements or bugs RFC Request For Comments: want input from the community
Projects
Status: Done
Archived in project
Development

Successfully merging a pull request may close this issue.

9 participants