kconfig: review proposal on PR#53338 #1

tejlmand · 2023-01-13T14:10:02Z

Review proposal for PR#53338.
Changes can be moved to correct commits or this commit message can be re-written and changes used as is.

Signed-off-by: Torsten Rasmussen [email protected]

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]>

Zephyr SDK version 0.16 will include picolibc support; add a variable so that tests may check for it. Signed-off-by: Keith Packard <[email protected]>

C++ is supported with picolibc only when the toolchain version of picolibc is use -- libstdc++ must be built with picolibc support and libstdc++ is included with the toolchain. That creates a circular dependency if the control of whether to use the module is placed under the picolibc configuration block (as PICOLIBC_USE_MODULE is). Make an explicit PICOLIBC_MODULE variable at the libc level to be the user-visible control for this feature and make code depending on whether the picolibc module should be used depend on both PICOLIBC and PICOLIBC_MODULE. Change PICOLIBC_USE_MODULE to be a hidden configuration variable that the picolibc module cmake files can use to determine whether to include the picolibc module sources in the zephyr application build. This symbol can be removed when the picolibc module is updated to use PICOLIBC AND PICOLIBC_MODULE instead Signed-off-by: Keith Packard <[email protected]>

The Zephyr SDK includes C++ support for picolibc when using picolibc packaged with the SDK. Signed-off-by: Keith Packard <[email protected]>

The picolibc module is not supported for C++, so make sure the toolchain has C++ support and then use it for the tests. 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]>

Review proposal for PR#53338. Changes can be moved to correct commits or this commit message can be re-written and changes used as is. Signed-off-by: Torsten Rasmussen <[email protected]>

keith-packard · 2023-01-16T18:18:33Z

lib/libc/Kconfig

@@ -19,7 +19,8 @@ config PICOLIBC_SUPPORTED
 	bool
 	depends on ARC || ARM || ARM64 || MIPS || RISCV
 	depends on "$(ZEPHYR_TOOLCHAIN_VARIANT)" != "arcmwdt"
-	depends on !CPLUSPLUS || !PICOLIBC_MODULE
+        # Picolibc with C++ support in Zephyr SDK is handled by Zephyr SDK's own Kconfig.
+        depends on !(CPLUSPLUS && "$(ZEPHYR_TOOLCHAIN_VARIANT)" = "zephyr")


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?

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.

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.

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)

Zephyr SDK 0.15 supports picolibc for C development

Zephyr SDK >=0.16 supports picolibc for C and C++ development.

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.

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.

keith-packard · 2023-01-16T18:20:03Z

lib/libc/Kconfig

@@ -43,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


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

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.

created: zephyrproject-rtos#53856

keith-packard · 2023-01-16T18:22:07Z

lib/libc/picolibc/Kconfig

@@ -3,6 +3,14 @@

 if PICOLIBC

+config PICOLIBC_USE_MODULE


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.

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.

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.

keith-packard

I can't see how we can avoid having a global symbol which selects whether the user wants to use the picolibc module or the toolchain picolibc bits. With that, along with the PICOLIBC symbol, we can create a separate hidden symbol which determines whether to actually build the picolibc module. Alternatively, we can simply use those two symbols together everywhere. I think the confusion from my first patch was partially the introduction of yet another user-visible symbol?

tejlmand · 2023-01-17T09:50:13Z

global symbol which selects whether the user wants to use the picolibc module or the toolchain picolibc bits.

my understanding is that first user decides whether to use picolibc or not.
And if using picolibc and !C++, then the possibility of deciding built-in vs. module is available.

keith-packard · 2023-01-17T23:26:05Z

my understanding is that first user decides whether to use picolibc or not. And if using picolibc and !C++, then the possibility of deciding built-in vs. module is available.

Yes, I think this will work. I'm looking forward to the goal of using picolibc by default, which presumably means adding default PICOLIBC if PICOLIBC_SUPPORTED to the choice LIBC_IMPLEMENTATION block. With the way you've restructured things here, I think that will work. I can experiment with it.

tejlmand · 2023-01-18T08:19:19Z

I'm looking forward to the goal of using picolibc by default

Me too.

tejlmand · 2023-01-19T12:25:03Z

closing, as this has now served its purpose.

Previously the sample was using some headers that aren't available to the host, now we can add a `Makefile.host` to compile the example on a POSIX OS like Linux: ``` # Go to the sample dir cd ${ZEPHYR_BASE}/samples/posix/uname # Compile the sample make -f Makefile.host # Run the binary ./build/uname sysname[65]: Linux nodename[65]: LAPTOP-YC release[65]: 5.10.16.3-microsoft-standard-WSL2 version[65]: #1 SMP Fri Apr 2 22:23:49 UTC 2021 machine[65]: x86_64 ``` Signed-off-by: Yong Cong Sin <[email protected]>

hci_packet_complete(buf, buf_size) should check whether buf_size is enough. For instance, hci_packet_complete can receive buf with buf_size 1, leading to the buffer overflow in cmd->param_len, which is buf[3]. This can happen when rx_thread() receives two frames in 512 bytes and the first frame size is 511. Then, rx_thread() will call hci_packet_complete() with 1. ==5==ERROR: AddressSanitizer: global-buffer-overflow on address 0x000000ad81c2 at pc 0x0000005279b3 bp 0x7fffe74f5b70 sp 0x7fffe74f5b68 READ of size 2 at 0x000000ad81c2 thread T6 #0 0x5279b2 (/root/zephyr.exe+0x5279b2) #1 0x4d697d (/root/zephyr.exe+0x4d697d) #2 0x7ffff60e5daa (/lib/x86_64-linux-gnu/libc.so.6+0x89daa) (BuildId: 2e01923fea4ad9f7fa50fe24e0f3385a45a6cd1c) 0x000000ad81c2 is located 2 bytes to the right of global variable 'rx_thread.frame' defined in 'zephyr/drivers/bluetooth/hci/userchan.c' (0xad7fc0) of size 512 SUMMARY: AddressSanitizer: global-buffer-overflow (/root/zephyr.exe+0x5279b2) Thread T6 created by T2 here: #0 0x48c17c (/root/zephyr.exe+0x48c17c) #1 0x530192 (/root/zephyr.exe+0x530192) #2 0x4dcc22 (/root/zephyr.exe+0x4dcc22) Thread T2 created by T1 here: #0 0x48c17c (/root/zephyr.exe+0x48c17c) #1 0x530192 (/root/zephyr.exe+0x530192) #2 0x4dcc22 (/root/zephyr.exe+0x4dcc22) Thread T1 created by T0 here: #0 0x48c17c (/root/zephyr.exe+0x48c17c) #1 0x52f36c (/root/zephyr.exe+0x52f36c) #2 0x5371dc (/root/zephyr.exe+0x5371dc) zephyrproject-rtos#3 0x5312a6 (/root/zephyr.exe+0x5312a6) zephyrproject-rtos#4 0x52ed7b (/root/zephyr.exe+0x52ed7b) zephyrproject-rtos#5 0x52eddd (/root/zephyr.exe+0x52eddd) zephyrproject-rtos#6 0x7ffff6083c89 (/lib/x86_64-linux-gnu/libc.so.6+0x27c89) (BuildId: 2e01923fea4ad9f7fa50fe24e0f3385a45a6cd1c) ==5==ABORTING Signed-off-by: Sungwoo Kim <[email protected]>

With introduction of Raw modes, nRF70 driver now advertises get_c onfig OP, but doesn't implement all types. This causes problems two-fold with checksum calculations: 1. The "config" isn't uninitialized, so, every call returns differnet values. So, for UDP header checksum would be done and pkt->chksumdone would be set. But for IPv4 header checksum might be skipped. 2. Even if we initialize to zero, then network stack gets all zeros and calculates checksum by itself rendering offload moot. There is another problem in #1, as there is only single flag for pkt for all checksum, nRF70 driver sees this and tells UMAC to skip checksum for the entire packet. The design isn't coherent, and should be converted to communicate per-type checksum status (some are filled by network stack and some HW). But as nRF70 support all checksum offloads, advertise all types for both RX and TX. Upstream PR #: 80882 Signed-off-by: Chaitanya Tata <[email protected]>

Current code does not build on Cortex-M0, seems like it does not like subs: Error: instruction not supported in Thumb16 mode -- `subs r3,#1' Adding a unified assembler language declaration in the snippet seems to fix the problem, also add an M0+ board so this is tested in CI. Signed-off-by: Fabio Baltieri <[email protected]>

keith-packard added 6 commits January 12, 2023 11:28

toolchain/zephyr: Add TOOLCHAIN_HAS_PICOLIBC for sdk >= 0.16

04870f5

Zephyr SDK version 0.16 will include picolibc support; add a variable so that tests may check for it. Signed-off-by: Keith Packard <[email protected]>

libc/picolibc: Allow toolchain picolibc use with C++

4c4632e

The Zephyr SDK includes C++ support for picolibc when using picolibc packaged with the SDK. Signed-off-by: Keith Packard <[email protected]>

subsys/cpp: Use toolchain picolibc for C++ tests

3cb0c74

The picolibc module is not supported for C++, so make sure the toolchain has C++ support and then use it for the tests. Signed-off-by: Keith Packard <[email protected]>

subsys/cpp: Also run C++ tests with picolibc when possible

eb84c8c

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]>

tejlmand mentioned this pull request Jan 13, 2023

Allow use of SDK version of picolibc with C++ zephyrproject-rtos/zephyr#53338

Merged

kconfig: review proposal on PR#53338

9cba2fd

Review proposal for PR#53338. Changes can be moved to correct commits or this commit message can be re-written and changes used as is. Signed-off-by: Torsten Rasmussen <[email protected]>

tejlmand force-pushed the picolibc_cplusplus branch from e33c259 to 9cba2fd Compare January 16, 2023 10:45

keith-packard reviewed Jan 16, 2023

View reviewed changes

keith-packard force-pushed the picolibc-c++-fixes branch 2 times, most recently from 55d2805 to fe11427 Compare January 18, 2023 21:12

tejlmand closed this Jan 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

kconfig: review proposal on PR#53338 #1

kconfig: review proposal on PR#53338 #1

Uh oh!

tejlmand commented Jan 13, 2023

Uh oh!

keith-packard Jan 16, 2023

Uh oh!

tejlmand Jan 17, 2023 •

edited

Loading

Uh oh!

keith-packard Jan 16, 2023

Uh oh!

tejlmand Jan 17, 2023

Uh oh!

tejlmand Jan 17, 2023

Uh oh!

keith-packard Jan 16, 2023

Uh oh!

tejlmand Jan 17, 2023

Uh oh!

keith-packard Jan 17, 2023

Uh oh!

keith-packard left a comment

Uh oh!

tejlmand commented Jan 17, 2023

Uh oh!

keith-packard commented Jan 17, 2023

Uh oh!

tejlmand commented Jan 18, 2023

Uh oh!

tejlmand commented Jan 19, 2023

Uh oh!

Uh oh!

kconfig: review proposal on PR#53338 #1

kconfig: review proposal on PR#53338 #1

Uh oh!

Conversation

tejlmand commented Jan 13, 2023

Uh oh!

keith-packard Jan 16, 2023

Choose a reason for hiding this comment

Uh oh!

tejlmand Jan 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keith-packard Jan 16, 2023

Choose a reason for hiding this comment

Uh oh!

tejlmand Jan 17, 2023

Choose a reason for hiding this comment

Uh oh!

tejlmand Jan 17, 2023

Choose a reason for hiding this comment

Uh oh!

keith-packard Jan 16, 2023

Choose a reason for hiding this comment

Uh oh!

tejlmand Jan 17, 2023

Choose a reason for hiding this comment

Uh oh!

keith-packard Jan 17, 2023

Choose a reason for hiding this comment

Uh oh!

keith-packard left a comment

Choose a reason for hiding this comment

Uh oh!

tejlmand commented Jan 17, 2023

Uh oh!

keith-packard commented Jan 17, 2023

Uh oh!

tejlmand commented Jan 18, 2023

Uh oh!

tejlmand commented Jan 19, 2023

Uh oh!

Uh oh!

tejlmand Jan 17, 2023 •

edited

Loading