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

Allow use of SDK version of picolibc with C++ #53338

Merged
merged 3 commits into from
Jan 20, 2023

Conversation

keith-packard
Copy link
Collaborator

SDK version 0.16 will include picolibc along with libstdc++ support. Once that is released, this short series will be needed in Zephyr to allow use of picolibc with C++ applications.

cfriedt
cfriedt previously approved these changes Jan 10, 2023
Copy link
Collaborator

@tejlmand tejlmand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the number of picolibc related settings seems to be growing for no appearent reason.
I believe reducing the number to a single user facing setting will improve usability and remove the circular dependency.

cmake/toolchain/zephyr/generic.cmake Outdated Show resolved Hide resolved
lib/libc/Kconfig Outdated Show resolved Hide resolved
lib/libc/Kconfig Outdated Show resolved Hide resolved
@keith-packard
Copy link
Collaborator Author

the number of picolibc related settings seems to be growing for no appearent reason. I believe reducing the number to a single user facing setting will improve usability and remove the circular dependency.

Agreed. Help here would be greatly appreciated. Here's what the current symbols do:

  • PICOLIBC_SELECT_MODULE -- a global symbol, default 'y', which says that if picolibc is to be used, then use the picolibc module instead of using picolibc as included in the toolchain.
  • PICOLIBC_USE_MODULE -- defined when picolibc is selected and the module should be used. In particular, this is not defined when picolibc is not selected, even if the global PICOLIBC_SELECT_MODULE value is set to the default value, 'y'.
  • PICOLIBC_MODULE -- defined when the picolibc module should be built. AFAICT, this is set to 'y' by default, and doesn't appear to depend on whether picolibc will be used as the C library.

There are some cross-system configuration adventures that need to be addressed -- the picolibc source code includes some cmake bits which check for the Zephyr-specific symbol CONFIG_PICOLIBC_USE_MODULE here https://github.com/picolibc/picolibc/blob/main/zephyr/zephyr.cmake - picolibc is using that, instead of CONFIG_PICOLIBC_MODULE which seems to be set to 'y', even if picolibc is not being used as the C library.

PICOLIBC_SELECT_MODULE cannot depend on whether picolibc is selected as the C library as that is used to determine whether picolibc is supported at all when C++ is being used, and picolibc cannot be selected as a C library for C++ apps unless it is coming from the toolchain, not built as a module (enough negatives there for you?).

How about this as a solution:

  • Fix the picolibc module to check for both CONFIG_PICOLIBC_USE_MODULE and CONFIG_PICOLIBC. This should be a no-op for the current Zephyr bits.
  • Move CONFIG_PICOLIBC_USE_MODULE up to lib/libc/Kconfig.

Alternatively, figure out how to use CONFIG_PICOLIBC_MODULE instead of CONFIG_PICOLIBC_USE_MODULE somehow? As above, we need to have a user-visible variable that selects between picolibc-module and picolibc-toolchain.

@keith-packard
Copy link
Collaborator Author

Ok, I've removed the extra PICOLIBC_SELECT_MODULE variable and have created PICOLIBC_MODULE at the libc level to control whether to prefer the module to the toolchain for picolibc. PICOLIBC_USE_MODULE is now only used to pass information to the cmake files in the picolibc module and can be removed when the picolibc module is updated to use CONFIG_PICOLIBC and CONFIG_PICOLIBC_MODULE directly. This has the effect of changing the user-visible symbol used to select whether to use the picolibc module in zephyr from PICOLIBC_USE_MODULE to PICOLIBC_MODULE. If that doesn't seem like a good idea, I can update the picolibc module first and then switch PICOLIBC_USE_MODULE to be the user-visible variable again.

@tejlmand
Copy link
Collaborator

Agreed. Help here would be greatly appreciated.

Let me try to give a bit of extra background knowledge on the module.

The ZEPHYR_PICOLIBC_MODULE is a generated setting by the build system to inform rest of Kconfig system whether or not a given module is present.
This means that other parts of Kconfig can add depends on ZEPHYR_PICOLIBC_MODULE, so that if the module is not present (for example removed in a downstream manifest file) then all settings depending on this module are unavailable to user configuration.

PICOLIBC_MODULE -- defined when the picolibc module should be built.

According to my search, this Kconfig was originally not used anywhere in CMake.

PICOLIBC_USE_MODULE -- defined when picolibc is selected and the module should be used.

according to this code

  if(NOT CONFIG_PICOLIBC_USE_MODULE)
    return()
  endif()

Ref: https://github.com/zephyrproject-rtos/picolibc/blob/98e59b70d6ef8eb18f7dccc3d39cecaa0658fa4f/CMakeLists.txt#L52-L54
the PICOLIBC_USE_MODULE setting controls both the building and and use of picolibc as a module.
Hence there seems to be no reason for the PICOLIBC_MODULE setting.

So in original proposal there weren't a need for both, but I will take closer look in the updated proposal, sounds like you have done some cleanup already 👍 .

Copy link
Collaborator

@tejlmand tejlmand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new PICOLIBC_MODULE in lib/libc/Kconfig is now outside of the if PICOLIBC guard.

This means this symbol is always accessible, irrespectively of the PICOLIBC libc selection.
In fact that's already the case today because of the way this module is defined in the Zephyr picolibc module Kconfig, but that's a different story.

I think I understand the purpose you want to achieve, so let me try to it down, and then I can try to create a patch with the described behavior.

Setting: PICOLIBC_SUPPORTED, should be yes when it should be possible to enable picolibc as C library.
This means for pure C development when compiler is gcc based (ie. not ARC mwdt).
Or for C++ development when Zephyr SDK >=0.16 (should be handled by the Zephyr SDK)

Setting: PICOLIBC choice entry, depends on PICOLIBC_SUPPORTED=y.

Setting: PICOLIBC_USE_MODULE config. Only valid if Picolibc is selected a C library.
Requires that Zephyr Picolibc module is available.
Cannot be used together with C++

Will let you know when it's done.

lib/libc/Kconfig Outdated Show resolved Hide resolved
lib/libc/Kconfig Outdated Show resolved Hide resolved
lib/libc/picolibc/Kconfig Outdated Show resolved Hide resolved
lib/libc/Kconfig Outdated Show resolved Hide resolved
@tejlmand
Copy link
Collaborator

Will let you know when it's done.

@keith-packard I hope I got your intentions right.
I have posted a PR here keith-packard#1 that you may look at.

This specifies the additional dependencies and ensures that users can only build picolibc as module when C++ is not used.

When Zephyr SDK 0.16 is ready, then Zephyr SDK itself should be able to inform that it supports picolibc, even if C++ is used, but in that case not allowing Picolibc to be build as a module.
That can be seen here: zephyrproject-rtos/sdk-ng#620

Picolibc itself can then follow later with the removal of PICOLIBC_MODULE symbol as it is not used anywhere.
This can be seen here: zephyrproject-rtos/picolibc#3
instead I kept the PICOLIBC_USE_MODULE as that is the user facing setting, but made it promptless in picolibc repo (read description)

Feel free to give feedback.

@keith-packard
Copy link
Collaborator Author

The new PICOLIBC_MODULE in lib/libc/Kconfig is now outside of the if PICOLIBC guard.

This means this symbol is always accessible, irrespectively of the PICOLIBC libc selection. In fact that's already the case today because of the way this module is defined in the Zephyr picolibc module Kconfig, but that's a different story.

Right, it "has" to be so that it can help drive PICOLIBC_SUPPORTED, which can be used to control the main PICOLIBC value.

Setting: PICOLIBC_SUPPORTED, should be yes when it should be possible to enable picolibc as C library. This means for pure C development when compiler is gcc based (ie. not ARC mwdt). Or for C++ development when Zephyr SDK >=0.16 (should be handled by the Zephyr SDK)

Or any external toolchain -- the Zephyr SDK is somewhat unique in not yet having C++ support for picolibc; crosstool-ng and the arm embedded toolkit already do. So, the only constraint is that we not use the module when C++ is selected.

Setting: PICOLIBC_USE_MODULE config. Only valid if Picolibc is selected a C library. Requires that Zephyr Picolibc module is available. Cannot be used together with C++

Ok, I wasn't aware that the picolibc module itself was optional, although it makes sense.

@nordicjm
Copy link
Collaborator

Ok, I wasn't aware that the picolibc module itself was optional, although it makes sense.

@stephanosio what is the plan with making picolibc the default C library, will it be getting moved into the zephyr repository itself?

@tejlmand
Copy link
Collaborator

Or any external toolchain -- the Zephyr SDK is somewhat unique in not yet having C++ support for picolibc; crosstool-ng and the arm embedded toolkit already do.

Thanks, that was the piece of info I was missing.
Then we should only limit the use to the Zephyr SDK.
Zephyr SDK can then specify in its own Kconfig if it supports C++.
Ref: zephyrproject-rtos/picolibc#3

Ok, I wasn't aware that the picolibc module itself was optional, although it makes sense.

In theory you can remove any project from the Zephyr manifest you like, although you might want to keep some.
If you remove everything you probably end-up to only build hello world for native posix.
But users can remove STM hal if they don't need it, they can remove TF-M if they don't need it, etc.

@tejlmand
Copy link
Collaborator

@keith-packard pushed an update.
Now only the Zephyr SDK is treated specially, cause there we actually have the possibility to pass knowledge from Zephyr SDK to build system on toolchain support.

When using 3rd party toolchains, like gnu ARM emb or crosstool-ng, users themselves are responsible for what is supported in the toolchain version they decide to use.

@keith-packard
Copy link
Collaborator Author

Ok, I wasn't aware that the picolibc module itself was optional, although it makes sense.

@stephanosio what is the plan with making picolibc the default C library, will it be getting moved into the zephyr repository itself?

Picolibc is in the Zephyr SDK and available as a Zephyr module. Both of those are built from the picolibc version in the Zephyr github account, https://github.com/zephyrproject-rtos/picolibc/ At this point, switching to using picolibc by default is a simple matter of adjusting the Kconfig settings.

I've got a pile of PRs queued which are required to resolve test suite issues when using picolibc. Help reviewing those would be greatly appreciated, as that is one of the things blocking this transition.

When using picolibc from the toolchain, we need to use the standard include
paths to make sure the library headers are found, especially for libstdc++.

Add toolchain picolibc to the list of cases for which this is the case.

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

keith-packard commented Jan 18, 2023

Ok, thanks to Torsten Rasmussen, I think I've got this sorted out. The goals of this series are a bit different now that Stephanos Ioannidis has rewritten the C++ library stuff. Now, the end goal is to prevent use of the Picolibc module when the application needs the GNU libstdc++ bits as those can only be provided by the toolchain in combination with a toolchain-provided picolibc. For Zephyr SDK 0.15, that's not quite achievable as I couldn't figure out how to avoid a dependency loop. Once we have SDK 0.16, then you can use the Picolibc module for any C++ applications which aren't usng libstdc++.

This series removes the PICOLIBC_MODULE symbol as that wasn't used anywhere, and replaces a !PICOLIBC_USE_MODULE dependency from GLIBCXX_LIBCPP with a !GLIBCXX_LIBCPP dependency for PICOLIBC_USE_MODULE. This allows applications to select GLIBCXX_LIBCPP and have that automatically select the toolchain Picolibc bits.

libstdc++ is supported with Picolibc only when the toolchain version of
Picolibc is use -- libstdc++ must be built using a specific Picolibc build
and libstdc++ is included with the toolchain.

Ideally, we'd allow the use of the Picolibc module whenever we weren't
using the GNU libstdc++, including when using the minimal libc++. However,
the obvious dependency settings create a loop:

config PICOLIBC
    depends on PICOLIBC_SUPPORTED

config PICOLIBC_SUPPORTED
    depends on !(GLIBCXX_LIBCPP && "$(ZEPHYR_TOOCHAIN_VARIANT" = "zephyr")

config GLIBCXX_LIBCPP
    depends on NEWLIB_LIBC || PICOLIBC

To break this loop, we replace GLIBCXX_LIBCPP in the second block with
CPP:

config PICOLIBC_SUPPORTED
    depends on !(CPP && "$(ZEPHYR_TOOCHAIN_VARIANT" = "zephyr")

This means that picolibc cannot be used with any C++ apps when using the
Zephyr SDK, even when not using the GNU libstdc++. However, Zephyr SDK 0.16
will come with an additional Kconfig file that includes:

config PICOLIBC_SUPPORTED
    def_bool y
    depends on "$(ZEPHYR_TOOLCHAIN_VARIANT)" = "zephyr"

This will override the Kconfig bits included in Zephyr and allow use of the
Picolibc module with C++ code, including using the minimal libc++ bits.

Signed-off-by: Keith Packard <[email protected]>
When the toolchain has picolibc support, run
samples/subsys/cpp/cpp_synchronization and tests/subsys/cpp/libcxx tests
using it.

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

@tejlmand tejlmand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm.

What a clean end result.

thanks for the effort @keith-packard

@keith-packard
Copy link
Collaborator Author

lgtm.

What a clean end result.

thanks for the effort @keith-packard

Thanks for your help, @tejlmand, seems like we ended up in a much simpler place.

@carlescufi carlescufi merged commit 35e017a into zephyrproject-rtos:main Jan 20, 2023
@keith-packard keith-packard deleted the picolibc-c++-fixes branch January 25, 2023 18:23
Comment on lines +18 to +23
# Picolibc with C++ support in Zephyr SDK is handled by Zephyr SDK's own Kconfig.
config PICOLIBC_SUPPORTED
bool
depends on ARC || ARM || ARM64 || MIPS || RISCV
depends on "$(ZEPHYR_TOOLCHAIN_VARIANT)" != "arcmwdt"
depends on !(CPP && "$(ZEPHYR_TOOLCHAIN_VARIANT)" = "zephyr")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the late review.

I think we have tried to work backwards on this and ended up with something quite convoluted -- what we really should be specifying here is when GLIBCXX (aka. libstdc++) is supported, not when PICOLIBC is supported.

GLIBCXX is supported when the toolchain provides the GLIBCXX specifically built for the selected LIBC. This means:

  • When NEWLIB (toolchain-provided) is selected, GLIBCXX is supported if the toolchain provides a GLIBCXX built for NEWLIB.
  • When PICOLIBC (toolchain-provided) is selected, GLIBCXX is supported if the toolchain provides a GLIBCXX built for PICOLIBC.

Since NEWLIB, when available, is currently only toolchain-provided and a C/C++ toolchain that includes NEWLIB almost certainly includes the GLIBCXX built for NEWLIB, we simply specify GLIBCXX_LIBCPP depends on NEWLIB_LIBC; but, in reality, it should really be something like GLIBCXX_LIBCPP depends on NEWLIB_LIBC && TOOLCHAIN_HAS_GLIBCXX_NEWLIB.

Similarly, for PICOLIBC, the condition should essentially be GLIBCXX_LIBCPP depends on PICOLIBC && TOOLCHAIN_HAS_GLIBCXX_PICOLIBC. But, unlike NEWLIB, PICOLIBC can also be non-toolchain-provided, in which case the GLIBCXX built for the PICOLIBC is unavailable, so it needs to be GLIBCXX_LIBCPP depends on PICOLIBC && !PICOLIBC_USE_MODULE && TOOLCHAIN_HAS_GLIBCXX_PICOLIBC.

So, ideally, we should have ended up with something like:

config GLIBCXX_LIBCPP
	bool "GNU C++ Standard Library"
	depends on !NATIVE_APPLICATION
	depends on (NEWLIB_LIBC && TOOLCHAIN_HAS_GLIBCXX_NEWLIB) ||
		   (PICOLIBC && !PICOLIBC_USE_MODULE && TOOLCHAIN_HAS_GLIBCXX_PICOLIBC)

where TOOLCHAIN_HAS_GLIBCXX_NEWLIB and TOOLCHAIN_HAS_GLIBCXX_PICOLIBC are set by the Zephyr SDK CMake package (or automatic toolchain feature detection logic in the future).

Also, C++ library, in its current form, can be considered an extension to the C library and should exist as an upper layer of the C library support; so, the libc choice should dictate which libcpp can be selected, not the other way around.

Since we are near the feature freeze for 3.3, I will leave this alone. I will rework this in the future to make it more logically consistent and straight forward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants