Skip to content

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

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Jan 20, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
77599bb
cmake/gcc: Don't use -nostdinc with toolchain picolibc
keith-packard Nov 7, 2022
0fd94bb
libc/picolibc: Clean up Picolibc Kconfig for C++
keith-packard Nov 2, 2022
fe11427
subsys/cpp: Also run C++ tests with picolibc when possible
keith-packard Jan 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions cmake/compiler/gcc/compiler_flags.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,7 @@ set_compiler_property(PROPERTY warning_error_coding_guideline
set_compiler_property(PROPERTY cstd -std=)

if (NOT CONFIG_NEWLIB_LIBC AND
NOT (CONFIG_PICOLIBC AND NOT CONFIG_PICOLIBC_USE_MODULE) AND
NOT COMPILER STREQUAL "xcc" AND
NOT CONFIG_HAS_ESPRESSIF_HAL AND
NOT CONFIG_NATIVE_APPLICATION)
Expand Down
2 changes: 1 addition & 1 deletion lib/cpp/Kconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ config MINIMAL_LIBCPP
config GLIBCXX_LIBCPP
bool "GNU C++ Standard Library"
depends on !NATIVE_APPLICATION
depends on NEWLIB_LIBC || (PICOLIBC && !PICOLIBC_USE_MODULE)
depends on NEWLIB_LIBC || PICOLIBC
help
Build with GNU C++ Standard Library (libstdc++) provided by the GNU
Compiler Collection (GCC)-based toolchain.
Expand Down
3 changes: 3 additions & 0 deletions lib/libc/Kconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,12 @@ config SUPPORT_MINIMAL_LIBC
bool
default y

# 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")
Comment on lines +18 to +23
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.

default y
help
Selected when the target has support for picolibc.
Expand All @@ -42,6 +44,7 @@ config PICOLIBC
select THREAD_LOCAL_STORAGE if ARCH_HAS_THREAD_LOCAL_STORAGE && TOOLCHAIN_SUPPORTS_THREAD_LOCAL_STORAGE
select LIBC_ERRNO if THREAD_LOCAL_STORAGE
depends on !NATIVE_APPLICATION
depends on PICOLIBC_SUPPORTED
help
Build with picolibc library. The picolibc library is built as
a module if PICOLIBC_MODULE is set, otherwise picolibc is
Expand Down
5 changes: 3 additions & 2 deletions lib/libc/picolibc/Kconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,10 @@
if PICOLIBC

config PICOLIBC_USE_MODULE
bool "Use picolibc module"
bool "Picolibc as module"
default y
select PICOLIBC_MODULE
depends on ZEPHYR_PICOLIBC_MODULE
depends on !GLIBCXX_LIBCPP
help
Use picolibc module instead of picolibc included with toolchain

Expand Down
15 changes: 15 additions & 0 deletions samples/cpp/cpp_synchronization/sample.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,3 +13,18 @@ tests:
- "Create semaphore (.*)"
- "main: Hello World!"
- "coop_thread_entry: Hello World!"
sample.cpp.synchronization.picolibc:
filter: CONFIG_PICOLIBC_SUPPORTED
extra_configs:
- CONFIG_PICOLIBC=y
tags: cpp
toolchain_exclude: issm xcc
integration_platforms:
- qemu_x86
harness: console
harness_config:
type: multi_line
regex:
- "Create semaphore (.*)"
- "main: Hello World!"
- "coop_thread_entry: Hello World!"
11 changes: 11 additions & 0 deletions tests/lib/cpp/libcxx/testcase.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,17 @@ tests:
- CONFIG_GLIBCXX_LIBCPP=y
integration_platforms:
- mps2_an385
cpp.libcxx.glibcxx.picolibc:
filter: TOOLCHAIN_HAS_PICOLIBC == 1
toolchain_exclude: xcc
tags: cpp
timeout: 60
extra_configs:
- CONFIG_PICOLIBC=y
- CONFIG_GLIBCXX_LIBCPP=y
- CONFIG_CPP_EXCEPTIONS=y
integration_platforms:
- mps2_an385
cpp.libcxx.arcmwdtlib:
toolchain_allow: arcmwdt
min_flash: 54
Expand Down