Skip to content

kconfig: review proposal on PR#53338 #1

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

Closed
wants to merge 7 commits into from
Closed

kconfig: review proposal on PR#53338 #1

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
2240cc5
cmake/gcc: Don't use -nostdinc with toolchain picolibc
keith-packard Nov 7, 2022
04870f5
toolchain/zephyr: Add TOOLCHAIN_HAS_PICOLIBC for sdk >= 0.16
keith-packard Jan 6, 2023
17cbb1b
libc/picolibc: Use libc-level PICOLIBC_MODULE to control picolibc mode
keith-packard Nov 2, 2022
4c4632e
libc/picolibc: Allow toolchain picolibc use with C++
keith-packard Nov 7, 2022
3cb0c74
subsys/cpp: Use toolchain picolibc for C++ tests
keith-packard Jan 6, 2023
eb84c8c
subsys/cpp: Also run C++ tests with picolibc when possible
keith-packard Jan 6, 2023
9cba2fd
kconfig: review proposal on PR#53338
tejlmand Jan 13, 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
2 changes: 1 addition & 1 deletion CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -695,7 +695,7 @@ add_custom_command(
# Make sure Picolibc is built before the rest of the system; there's no explicit
# reference to any of the files as they're all picked up by various compiler
# settings
if(CONFIG_PICOLIBC_USE_MODULE)
if(CONFIG_PICOLIBC AND CONFIG_PICOLIBC_MODULE)
set(picolibc_dependency PicolibcBuild)
endif()

Expand Down
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_MODULE) AND
NOT COMPILER STREQUAL "xcc" AND
NOT CONFIG_HAS_ESPRESSIF_HAL AND
NOT CONFIG_NATIVE_APPLICATION)
Expand Down
4 changes: 4 additions & 0 deletions cmake/toolchain/zephyr/generic.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,4 +4,8 @@ include(${ZEPHYR_SDK_INSTALL_DIR}/cmake/zephyr/generic.cmake)

set(TOOLCHAIN_KCONFIG_DIR ${ZEPHYR_SDK_INSTALL_DIR}/cmake/zephyr)

if(SDK_VERSION VERSION_GREATER_EQUAL 0.16)
set(TOOLCHAIN_HAS_PICOLIBC ON CACHE BOOL "True if toolchain supports picolibc")
endif()

message(STATUS "Found toolchain: zephyr ${SDK_VERSION} (${ZEPHYR_SDK_INSTALL_DIR})")
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 @@ -19,6 +19,8 @@ config PICOLIBC_SUPPORTED
bool
depends on ARC || ARM || ARM64 || MIPS || RISCV
depends on "$(ZEPHYR_TOOLCHAIN_VARIANT)" != "arcmwdt"
# Picolibc with C++ support in Zephyr SDK is handled by Zephyr SDK's own Kconfig.
depends on !(CPLUSPLUS && "$(ZEPHYR_TOOLCHAIN_VARIANT)" = "zephyr")
Copy link
Owner

Choose a reason for hiding this comment

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

This definitely feels wrong -- PICOLIBC_SUPPORTED is true for everything except building with C++ and using picolibc from the module source. Every toolchain other than zephyr 0.15 and earlier may include picolibc, and may (or may not) include C++ support. I think we want to make the user responsible for determining whether the toolchain has the necessary bits in that case?

Copy link
Author

@tejlmand tejlmand Jan 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for your feedback @keith-packard, highly appreciated so that we can get things right.

Especially because Kconfig and good dependencies can be tricky.

I tried to give the reason here:
zephyrproject-rtos#53338 (review)

But let's take one step back, so that we are clear.

  1. Picolibc, as built-in or as module, shall only be selectable if a toolchain supports picolibc.
    Support in this case can be supporting picolibc built as a module or if the toolchain has built-in support.
    1. arcmwdt does not support picolibc. Not built-in, nor as a as a module.
      (My understanding, please correct me if arcmwdt does support using picolibc as a module)
    2. Zephyr SDK 0.15 supports picolibc for C development
    3. Zephyr SDK >=0.16 supports picolibc for C and C++ development.
    4. Other 3rd party toolchains (crosstool-ng, arm embedded, etc) may support picolibc for C and C++ development.
      This depends on actual toolchain build configuration and / or version.
      Zephyr don't have toolchain testing in place for this kind of testing, so it's users responsibility to ensure picolibc is supported when using such toolchain. Thus Kconfig should always provide the possibility to select picolibc when such a toolchain is used.
  2. If picolibc is supported, then user can decide if picolibc should be build as a module, or using the built-in version from the toolchain.
    For 3rd party toolchains, it's the users responsibility to know if toolchain supports built-in version, as described in 1.iv.

This gives us the PICOLIBC choice entry in Kconfig under LIBC_IMPLEMENTATION choice menu.

To minimize number of tests and dependencies in PICOLIBC entry we create the helper symbol PICOLIBC_SUPPORTED
This allows a simple:

config PICOLIBC
	bool "Picolibc library"
	depends on PICOLIBC_SUPPORTED

The PICOLIBC_SUPPORTED then defines if PICOLIBC should be available for user control.
Combining above, then this gives PICOLIBC_SUPPORTED=y in following cases:
(3rd party toolchain && !arcmwdt) || (zephyr-sdk==0.15 && !C++) || (zephyr-sdk>=0.16)

As a toolchain which is not arcmwdt and not Zephyr must be a 3rd party toolchain, then the above can be reduced to:
(!arcmwdt && !zephyr-sdk) || (zephyr-sdk && !C++)

Because ZEPHYR_TOOLCHAIN_VARIANT cannot be arcmwdt and Zephyr at the same time, the above can be written as:
(!arcmwdt) && !(zephyr-sdk && C++)

The last part:
|| (zephyr-sdk>=0.16)
is what is included when Zephyr SDK 0.16 is included, cause then the Kconfig from there is sourced, and you'll have the following in Kconfig:

config PICOLIBC_SUPPORTED # From lib/libc/Kconfig
	bool
	depends on "$(ZEPHYR_TOOLCHAIN_VARIANT)" != "arcmwdt"
    depends on !(CPLUSPLUS && "$(ZEPHYR_TOOLCHAIN_VARIANT)" = "zephyr")
	
config PICOLIBC_SUPPORTED # From zephyr-sdk-0.16/cmake/zephyr/Kconfig
    depends on "$(ZEPHYR_TOOLCHAIN_VARIANT)" = "zephyr")

Two depends on in same setting translates into &&.
depends on in two configs of identical name translates into ||.

An Zephyr-sdk 0.15 does not include the zephyr-sdk-0.16/cmake/zephyr/Kconfig, so therefore only the first part remains.

All of this only covers picolibc itself.

Whether to use picolibc as a module is then dependent on picolibc being used.
For example a user selecting NEWLIB should not be able to select PICOLIBC_USE_MODULE, and picolibc as a module should not be selectable if C++ is enabled (AFAIU).
And ofcourse it also depends on the Zephyr picolibc module being present in the workspace (west manifest)

Bullet 2. above therfore gives:

if PICOLIBC

config PICOLIBC_USE_MODULE
	bool "Picolibc as module"
	depends on ZEPHYR_PICOLIBC_MODULE
	depends on !CPLUSPLUS

No dependency loops in this description.
Of course both PICOLIBC when using a certain Zephyr SDK version depends on !C++ and so does the use of PICOLIBC_USE_MODULE, but that is not a loop.
A dependency loop in Kconfig forms if one of them suddenly starts to select CPLUSPLUS.

Of course if any assumptions in 1. and 2. are wrong, then the implementation is wrong.

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
Copy link
Owner

Choose a reason for hiding this comment

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

I'd left this out as external toolchains may not indicate picolibc support? How do we detect newlib support for those?

Copy link
Author

Choose a reason for hiding this comment

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

In Kconfig ? we don't.
But we should definately improve on this, and if we have a picolibc helper symbol then I believe we should use it.
Alternatively, if it is not actively used, then remove it and place full responsibility at the end-user (like it's done for newlib).
My preference is to improve things, and when changing picolibc handling, then that's a good place to start.

We do have the TOOLCHAIN_HAS_NEWLIB, but that is only used for twister test filtering.
It's set here:
https://github.com/zephyrproject-rtos/zephyr/blob/ef203fd8aeb71758a106c4416aff5effba42c8e2/cmake/toolchain/xtools/generic.cmake#L36
https://github.com/zephyrproject-rtos/zephyr/blob/ef203fd8aeb71758a106c4416aff5effba42c8e2/cmake/toolchain/gnuarmemb/generic.cmake#L21

but was intended for twister filtering, as seen in zephyrproject-rtos#12393
would probably be good to cleanup this part and make it a bit more consistent.

Copy link
Author

Choose a reason for hiding this comment

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

created: zephyrproject-rtos#53856

help
Build with picolibc library. The picolibc library is built as
a module if PICOLIBC_MODULE is set, otherwise picolibc is
Expand Down
2 changes: 1 addition & 1 deletion lib/libc/picolibc/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ zephyr_library_sources(libc-hooks.c)
# used by the network stack
zephyr_compile_definitions(__LINUX_ERRNO_EXTENSIONS__)

if(NOT CONFIG_PICOLIBC_USE_MODULE)
if(CONFIG_PICOLIBC AND NOT CONFIG_PICOLIBC_MODULE)

# Use picolibc provided with the toolchain

Expand Down
7 changes: 4 additions & 3 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
Copy link
Owner

Choose a reason for hiding this comment

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

And here's the dependency loop if we want to base PICOLIBC_SUPPORTED on whether the user is trying to use the picolibc module with a project using C++ -- PICOLIBC should depend on PICOLIBC_SUPPORTED, but PICOLIBC_SUPPORTED needs to depend on !CPLUSPLUS || !PICOLIBC_USE_MODULE.

Copy link
Author

Choose a reason for hiding this comment

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

no, PICOLIBC_SUPPORTED only needs to depend on !(zephyr==0.15 && c++).
as you mentioned, other toolchain may likely have support for picolibc, even when C++ is used.

The fact that PICOLIBC_USE_MODULE depends on !C++ and PICOLIBC does not form a loop in Kconfig as it is in same straight forward chain of dependencies.

See also other comment.

Copy link
Owner

Choose a reason for hiding this comment

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

ah, I was just thinking about this incorrectly -- I was thinking that PICOLIBC_SUPPORTED would depend on the user not selecting the module when C++ was in use. However, you've flipped that over and made the PICOLIBC_USE_MODULE depend on not using C++, which makes a lot more sense.

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

Expand Down Expand Up @@ -148,6 +149,6 @@ config PICOLIBC_GLOBAL_ERRNO
which can be used to avoid TLS variable usage by the library if
necessary.

endif # PICOLIBC_USE_MODULE
endif # PICOLIBC_MODULE

endif # PICOLIBC
15 changes: 15 additions & 0 deletions samples/subsys/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: (TOOLCHAIN_HAS_PICOLIBC == 1) and 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!"
2 changes: 1 addition & 1 deletion tests/subsys/cpp/cxx/testcase.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,6 @@ tests:
- CONFIG_NEWLIB_LIBC_NANO=y
cpp.main.picolibc:
tags: picolibc
filter: CONFIG_PICOLIBC_SUPPORTED
filter: (TOOLCHAIN_HAS_PICOLIBC == 1) and CONFIG_PICOLIBC_SUPPORTED
extra_configs:
- CONFIG_PICOLIBC=y
9 changes: 9 additions & 0 deletions tests/subsys/cpp/libcxx/testcase.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,15 @@ tests:
- CONFIG_NEWLIB_LIBC_NANO=y
integration_platforms:
- mps2_an385
cpp.libcxx.picolibc:
filter: (TOOLCHAIN_HAS_PICOLIBC == 1) and CONFIG_PICOLIBC_SUPPORTED
toolchain_exclude: xcc
tags: cpp
timeout: 60
extra_configs:
- CONFIG_PICOLIBC=y
integration_platforms:
- mps2_an385
cpp.libcxx.arcmwdtlib:
toolchain_allow: arcmwdt
min_flash: 54
Expand Down