Picolibc optimizations #39564

keith-packard · 2021-10-20T06:31:47Z

Here are two optimizations made possible by the design of picolibc stdio which funnels all I/O through per-character functions. These build on the picolibc pull request; I'm posting them to give a fuller sense of what enabling picolibc will do.

Replace printk with wrapper around picolibc vfprintf
Replace cbprintf with code that uses picolibc vfprintf and sends each character to the callback function.

To test the effect of these substitutions, I built the 'philosophers' sample for qemu_cortex_m3 using picolibc.

                   FLASH    SRAM
before int         19768    9568
after int          18432    9584
before float       21720    9568
after float        23140    9584

The floating point version is larger for picolibc because it does exact conversions using the Ryū algorithm, instead of the approximate conversions used in the cbprintf code. If you're enabling floating point printf, presumably you want correct answers though...

Please only place comments about the commits related to replacing printk and cbvprintf on this PR. Comments about the basic picolibc support should be made in #39563

peter-mitsis · 2021-10-20T14:42:48Z

lib/os/CMakeLists.txt

@@ -26,6 +25,10 @@ zephyr_sources(
 multi_heap.c
 )

+if(NOT CONFIG_PICOLIBC)


It is my understanding that this could be simplified to ...

zephyr_sources_ifndef(CONFIG_PICOLIBC printk.c)

Oh, good point. However, the next patch adds more stuff here which (I think) is best done with the if(), so adding that here makes the next patch a bit cleaner?

tejlmand · 2021-10-21T08:05:42Z

lib/libc/CMakeLists.txt

+if(CONFIG_PICOLIBC)
+ add_subdirectory(picolibc)
+else()


Why is if(CONFIG_PICOLIBC) outside the if...elseif...elseif structure.

Why not have CONFIG_PICOLIBC as an elseif(CONFIG_PICOLIBC) in that list ?

Thanks. (this is actually related to the picolibc PR #39563)

tejlmand · 2021-10-21T08:10:54Z

lib/os/CMakeLists.txt

+ zephyr_sources_ifdef(CONFIG_CBPRINTF_COMPLETE cbprintf_complete.c)
+ zephyr_sources_ifdef(CONFIG_CBPRINTF_NANO cbprintf_nano.c)


So CBPRINTF_COMPLETE and CBPRINTF_NANO is now dependent on !PICOLIB ?

I don't see this dependency reflected in Kconfig.

I hadn't thought of that. I've pushed an update. Thanks!

carlescufi · 2021-12-20T17:37:52Z

@tejlmand please take another look

keith-packard · 2021-12-20T23:11:18Z

I've updated this along with the picolibc update.

tejlmand

@keith-packard looking much better.

Mostly some minor nits, but one thing I noticed is that running CMake for qemu cortex m3 for hello world and enabling Picolib fails.
This failure relates to the comment I made regarding THREAD_LOCAL_STORAGE:

$ cmake -GNinja -DBOARD=qemu_cortex_m3 -Bbuild samples/hello_world/ -DCONFIG_PICOLIBC=y
....

warning: THREAD_LOCAL_STORAGE (defined at kernel/Kconfig:903) has direct dependencies ARCH_HAS_THREAD_LOCAL_STORAGE && TOOLCHAIN_SUPPORTS_THREAD_LOCAL_STORAGE with value n, but is currently being y-selected by the following symbols:
 - PICOLIBC (defined at lib/libc/Kconfig:32), with value y, direct dependencies !NATIVE_APPLICATION && <choice LIBC_IMPLEMENTATION> (value: y), and select condition ARCH_HAS_THREAD_LOCAL_STORAGE && !NATIVE_APPLICATION && <choice LIBC_IMPLEMENTATION> (value: y)

error: Aborting due to Kconfig warnings

tejlmand · 2021-12-21T11:30:17Z

arch/arm/core/aarch32/cortex_a_r/__aeabi_read_tp.S

@@ -0,0 +1,17 @@
+/*
+ * Copyright (c) 2020 Intel Corporation


Got curios because of this old copyright like.
I assume it's actually this file: https://github.com/zephyrproject-rtos/zephyr/blob/main/arch/arm/core/aarch32/cortex_m/__aeabi_read_tp.S that has been copied and modified, correct ?

tejlmand · 2021-12-21T11:31:37Z

arch/arm/core/aarch32/cortex_a_r/__aeabi_read_tp.S

+ mrc 15, 0, r0, c13, c0, 3
+ bx lr


Could you please add a comment on what this is doing.

tejlmand · 2021-12-21T12:01:30Z

lib/libc/Kconfig

+config PICOLIBC
+ bool "Picolibc library"
+ depends on !NATIVE_APPLICATION
+ select THREAD_LOCAL_STORAGE if ARCH_HAS_THREAD_LOCAL_STORAGE


A small question trying to open a chat to see if this is right path.

Initially it appears wrong to me that the libc Kconfig should know such details of the Kernel config.

We have a TOOLCHAIN_SUPPORTS_THREAD_LOCAL_STORAGE here:

zephyr/kernel/Kconfig

Line 888 in 20b3d18

config TOOLCHAIN_SUPPORTS_THREAD_LOCAL_STORAGE

so maybe we should ensure that this value defaults to y when a toolchain with picolib is enabled.

Extra question, will using PICOLIBC break if TREAD_LOCAL_STORAGE is disabled ?
Reason i'm asking is because that will indicate whether a select or default y if XYZ is the best way to go.

The only support picolibc has for thread local variables, including 'errno', is by using the underlying native TLS support. If the toolchain doesn't have TLS support, then all thread local variables in picolibc are simple globals.

As a result, by default, picolibc uses TLS whenever the toolchain has TLS support. What Zephyr needs to do is enable TLS whenever picolibc was built to use it.

We could check whether picolibc was built to use TLS by looking at the picolibc.h header file to see if PICOLIBC_TLS is defined, but I'm not sure how to do that with cmake. Right now, the checks I've added assume that picolibc was built in the default mode and uses TLS iff the toolchain has TLS support.

I'd appreciate whatever help you can offer to clean up the cmake bits here; I'm very unsure of how to use cmake in complicated projects like this.

fwiw, this review series relates to changes proposed in #39563

tejlmand · 2021-12-21T12:05:09Z

lib/libc/Kconfig

+ bool "Picolibc library"
+ depends on !NATIVE_APPLICATION
+ select THREAD_LOCAL_STORAGE if ARCH_HAS_THREAD_LOCAL_STORAGE
+ select LIBC_ERRNO


here you are selecting a Kconfig setting from Kernel, but the name of this setting is LIBC_ERRNO, so maybe it should live in this Kconfig.

For example like:

config LIBC_ERRNO bool help The libc library provides a errno implementation.

Are you saying that LIBC_ERRNO should move to lib/libc/Kconfig? Happy to move it there, of course.

tejlmand · 2021-12-21T12:40:28Z

lib/os/Kconfig.cbprintf

@@ -3,6 +3,7 @@

 choice CBPRINTF_IMPLEMENTATION
 prompt "Capabilities of cbprintf implementation"
+ depends on !PICOLIBC


This should not be on the choice, but on the config items.

Reason, if someone elsewhere (downstream) want to extend CBPRINTF_IMPLEMENTATION, they can do so by writing:

choice CBPRINTF_IMPLEMENTATION prompt "Downstream cbprintf implementation" config DOWNSTREAM_CBPRINTF bool "my impl" endchoice

in such a case CBPRINTF_COMPLETE will suddenly reappear, which is clearly wrong.

Suggested change

depends on !PICOLIBC

I don't understand what change you're suggesting be made here.

fwiw, these comments relate to changes in this PR, not #39563

tejlmand · 2021-12-21T12:41:15Z

lib/os/Kconfig.cbprintf

@@ -3,6 +3,7 @@

 choice CBPRINTF_IMPLEMENTATION
 prompt "Capabilities of cbprintf implementation"
+ depends on !PICOLIBC
 default CBPRINTF_COMPLETE

 config CBPRINTF_COMPLETE


This should hold the depends on !PICOLIBC and ditto for the CBPRINTF_NANO, but cannot place a comment there.

tejlmand · 2021-12-21T12:47:37Z

lib/os/Kconfig.cbprintf

@@ -26,6 +27,7 @@ endchoice # CBPRINTF_IMPLEMENTATION

 choice CBPRINTF_INTEGRAL_CONV
 prompt "Control range of convertible integer values"
+ depends on !PICOLIBC


same comment regarding a depends on the choice vs. individual configs.

tejlmand · 2021-12-21T13:35:49Z

General note.

@keith-packard could you please in the comment of this PR here: #39564 (comment) make it very clear on which commits in this PR that should not be commented in this PR.

github-actions · 2022-03-06T00:29:24Z

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

V7-A also supports TPIDRURO, so go ahead and use that for TLS, enabling thread local storage for the other ARM architectures. Add __aeabi_read_tp function in case code was compiled to use that. Signed-off-by: Keith Packard <keithp@keithp.com>

Picolibc is a fork of newlib designed and tested on embedded systems. It offers a smaller memory footprint (both ROM and RAM), and native TLS support, which uses the Zephyr TLS support. By default, the full printf version is included in the executable, which includes exact floating point and long long input and output. A configuration option has been added to switch to the integer-only version (which also omits long long support). Here are some size comparisons using qemu-cortex-m3 and this application (parameters passed to printf to avoid GCC optimizing it into puts): void main(void) { printf("Hello World! %s %d\n", CONFIG_BOARD, 12); } FLASH SRAM minimal 8696 3952 picolibc int 7600 3960 picolibc float 12304 3960 newlib-nano int 11696 4128 newlib-nano float 30516 4496 newlib 34800 6112 --- v2: Include picolibc-tls.ld v3: Document usage in guides/c_library.rst and getting_started/toolchain_other_x_compilers.rst v4: Lost the lib/libc/picolibc directory somehow! v5: Add PICOLIBC_ALIGNED_HEAP_SIZE configuration option. Delete PICOLIBC_SEMIHOST option support code v6: Don't allocate static RAM for TLS values; TLS values only need to be allocated for each thread. v7: Use arm coprocessor for TLS pointer storage where supported for compatibility with the -mtp=cp15 compiler option (or when the target cpu type selects this option) Add a bunch of tests Round TLS segment up to stack alignment so that overall stack remains correctly aligned Add aarch64 support Rebase to upstream head v8: Share NEWLIB, NEWLIB_NANO and PICOLIBC library configuration variables in a single LIBC_PARTITIONS variable instead of having separate PICOLIBC_PART and NEWLIB_PART variables. v9: Update docs to reference pending sdk-ng support for picolibc v10: Support memory protection by creating a partition for picolibc shared data and any pre-defined picolibc heap. v11: Fix formatting in arch/arm/core/aarch64/switch.S v12: Remove TLS support from this patch now that TLS is upstream Require THREAD_LOCAL_STORAGE when using PICOLIBC for architectures that support it. v13: Merge errno changes as they're only needed for picolibc. Adapt cmake changes suggested by Torsten Tejlmand Rasmussen v14: Update to picolibc 1.7 and newer (new stdin/stdout/stderr ABI) v15: Respond to comments from dcpleung: * switch kernel/errno to use CONFIG_LIBC_ERRNO instead of CONFIG_PICOLIBC * Add comment to test/lib/sprintf as to why the %n test was disabled for picolibc. Signed-off-by: Keith Packard <keithp@keithp.com>

Picolibc printf is smaller than the built-in printk while offering a full POSIX implementation. Signed-off-by: Keith Packard <keithp@keithp.com>

Picolibc already provides the functionality offered by cbprintf, so there's no reason to use the larger and less functional version included in zephyr. Signed-off-by: Keith Packard <keithp@keithp.com> --- v2: Add Kconfig changes to make relevant cbprintf options depend on !PICOLIBC

keith-packard · 2022-03-22T23:07:27Z

I've created a follow-on PR which builds picolibc as a module instead of depending on it existing in the SDK; would be nice to know if that seems like the right direction. #44096

keith-packard · 2022-04-28T16:22:18Z

This PR has been replaced by #44096 which allows use of either the picolibc module or an external picolibc provided with the toolchain.

keith-packard requested review from andyross, carlescufi, dcpleung, galak, MaureenHelm, nashif, pfalcon and tejlmand as code owners October 20, 2021 06:31

github-actions bot added area: API Changes to public APIs area: Build System area: C Library C Standard Library area: Kernel area: Tests Issues related to a particular existing or missing test labels Oct 20, 2021

zephyrbot added area: Base OS Base OS Library (lib/os) area: POSIX POSIX API Library labels Oct 20, 2021

zephyrbot requested review from ceolin, enjiamai, KangJianX, peter-mitsis and stephanosio October 20, 2021 11:54

zephyrbot assigned tejlmand Oct 20, 2021

peter-mitsis reviewed Oct 20, 2021

View reviewed changes

keith-packard mentioned this pull request Oct 20, 2021

lib/libc: Add picolibc support (aarch32, aarch64 and RISC-V) [v14] #39563

Closed

keith-packard force-pushed the picolibc-optimize branch from c6c13d5 to 1337d70 Compare October 20, 2021 18:36

tejlmand requested changes Oct 21, 2021

View reviewed changes

keith-packard force-pushed the picolibc-optimize branch from 1337d70 to 5c54fce Compare October 21, 2021 16:03

keith-packard force-pushed the picolibc-optimize branch from 5c54fce to a6bac00 Compare December 20, 2021 23:07

keith-packard requested review from bbolen and ioannisg as code owners December 20, 2021 23:07

github-actions bot added the area: ARM ARM (32-bit) Architecture label Dec 20, 2021

tejlmand requested changes Dec 21, 2021

View reviewed changes

keith-packard force-pushed the picolibc-optimize branch from a6bac00 to 9379a85 Compare December 21, 2021 18:26

carlescufi requested review from tejlmand and peter-mitsis December 22, 2021 14:13

keith-packard force-pushed the picolibc-optimize branch from 9379a85 to 08a57ba Compare January 4, 2022 20:54

github-actions bot added the Stale label Mar 6, 2022

github-actions bot closed this Mar 20, 2022

carlescufi reopened this Mar 20, 2022

keith-packard added 4 commits March 21, 2022 19:46

arch/arm: Use TPIDRURO on cortex-a too

7fe9b3f

V7-A also supports TPIDRURO, so go ahead and use that for TLS, enabling thread local storage for the other ARM architectures. Add __aeabi_read_tp function in case code was compiled to use that. Signed-off-by: Keith Packard <keithp@keithp.com>

lib/os: Use picolibc printf as a replacement for printk

16ea72e

Picolibc printf is smaller than the built-in printk while offering a full POSIX implementation. Signed-off-by: Keith Packard <keithp@keithp.com>

keith-packard force-pushed the picolibc-optimize branch from 08a57ba to a0d98b3 Compare March 22, 2022 02:48

github-actions bot removed the Stale label Mar 23, 2022

keith-packard closed this Apr 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Picolibc optimizations #39564

Picolibc optimizations #39564

keith-packard commented Oct 20, 2021 •

edited

Loading

peter-mitsis Oct 20, 2021

keith-packard Oct 20, 2021

tejlmand Oct 21, 2021

keith-packard Oct 21, 2021

tejlmand Oct 21, 2021

keith-packard Oct 21, 2021

carlescufi commented Dec 20, 2021

keith-packard commented Dec 20, 2021

tejlmand left a comment

tejlmand Dec 21, 2021

tejlmand Dec 21, 2021

tejlmand Dec 21, 2021

keith-packard Dec 21, 2021

keith-packard Dec 30, 2021

tejlmand Dec 21, 2021

keith-packard Dec 21, 2021

tejlmand Dec 21, 2021

keith-packard Dec 21, 2021

keith-packard Dec 30, 2021

tejlmand Dec 21, 2021

tejlmand Dec 21, 2021

tejlmand commented Dec 21, 2021

github-actions bot commented Mar 6, 2022

keith-packard commented Mar 22, 2022

keith-packard commented Apr 28, 2022

		zephyr_sources_ifdef(CONFIG_CBPRINTF_COMPLETE cbprintf_complete.c)
		zephyr_sources_ifdef(CONFIG_CBPRINTF_NANO cbprintf_nano.c)

Picolibc optimizations #39564

Picolibc optimizations #39564

Conversation

keith-packard commented Oct 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlescufi commented Dec 20, 2021

keith-packard commented Dec 20, 2021

tejlmand left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tejlmand commented Dec 21, 2021

github-actions bot commented Mar 6, 2022

keith-packard commented Mar 22, 2022

keith-packard commented Apr 28, 2022

keith-packard commented Oct 20, 2021 •

edited

Loading