-
Notifications
You must be signed in to change notification settings - Fork 8.2k
Provide support for Picolibc #26545
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
Provide support for Picolibc #26545
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks cool. I have a couple of drive-by build system questions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since GNURISCVEMB_TOOLCHAIN_PATH is new, why bother adding environment variable support for setting its value in an already deprecated state? A plain CMake variable should do, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll see if I can figure this out; I just followed the existing gnuarmemb path for this work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More a question than a request for change, since I recall some historical back and forth around what configuration variables fall back to the environment if not set as ordinary CMake variables. I defer to @tejlmand as build system maintainer for what the right thing to do is here.
|
Welcome to Zephyr! Glad to see that a regular newlib contributor is helping out with the project :) |
|
@galak I suppose we could potentially integrate this into the Zephyr SDK/toolchains by adding @keith-packard Do you have any plans to add something like |
I think you should be able to use the existing
The PR already depends on the picolibc.specs file which is shipped and installed with picolibc itself, so there's no need to include an additional specs file in zephyr. If there are additional zephyr-specific compiler configuration bits needed, we could create a separate .specs file that referenced the existing picolibc one. The advantage of using the installed picolibc.specs file is that it has the picolibc install paths configured so that you don't need any library or include paths for the toolchain. |
Been meaning to drop by for a while now. Thanks for the warm welcome. |
5a7a6d9 to
71b417a
Compare
1d404a8 to
2697348
Compare
2697348 to
d70a059
Compare
d70a059 to
721bca5
Compare
721bca5 to
90ef364
Compare
|
I've updated this patch to sit on top of current master and require THREAD_LOCAL_STORAGE for architectures which support that. The changes are now far smaller as they don't include the TLS code. |
Neither of these libraries can support the built-in Zephyr errno code, so make sure it gets disabled when either are selected Signed-off-by: Keith Packard <[email protected]>
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-m0 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);
}
text data bss dec hex
16908 428 4917 22253 56ed minimal
15460 464 4924 20848 5170 picolibc integer-only
20272 464 4924 25660 643c picolibc full version
20862 548 4936 26346 66ea newlib-nano integer-only
50542 916 4936 56394 dc4a newlib-nano full version
48170 2924 4984 56078 db0e newlib
Signed-off-by: Keith Packard <[email protected]>
---
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.
Signed-off-by: Keith Packard <[email protected]>
90ef364 to
5413f80
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updated work.
Looks good, only a couple of small suggestions.
| if(CONFIG_PICOLIBC) | ||
| set(LIBC_PARTITIONS -l libc.a z_libc_partition) | ||
| endif() | ||
| if(CONFIG_NEWLIB_LIBC) | ||
| set(NEWLIB_PART -l libc.a z_libc_partition) | ||
| set(LIBC_PARTITIONS -l libc.a z_libc_partition) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about:
if(CONFIG_PICOLIBC OR CONFIG_NEWLIB_LIBC)
set(LIBC_PARTITIONS -l libc.a z_libc_partition)
endif()
| if(CONFIG_PICOLIBC) | ||
| add_subdirectory(picolibc) | ||
| else() | ||
| if(CONFIG_NEWLIB_LIBC) | ||
| add_subdirectory(newlib) | ||
| else() | ||
| add_subdirectory(minimal) | ||
| endif() | ||
| endif() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make an elseif() in this case.
| if(CONFIG_PICOLIBC) | |
| add_subdirectory(picolibc) | |
| else() | |
| if(CONFIG_NEWLIB_LIBC) | |
| add_subdirectory(newlib) | |
| else() | |
| add_subdirectory(minimal) | |
| endif() | |
| endif() | |
| if(CONFIG_PICOLIBC) | |
| add_subdirectory(picolibc) | |
| elseif(CONFIG_NEWLIB_LIBC) | |
| add_subdirectory(newlib) | |
| else() | |
| add_subdirectory(minimal) | |
| endif() |
| } | ||
|
|
||
| static int | ||
| z_putc(char c, FILE *file) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why a newline here ?
| } | ||
|
|
||
| static int | ||
| z_getc(FILE *file) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why a newline here ?
| zephyr_link_libraries( | ||
| m | ||
| c | ||
| gcc # Lib C depends on libgcc. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No changes requested, but with the fact that Zephyr is working towards more toolchains, especially LLVM, and thus also the support of compiler-rt runtime library, I was wondering if picolib could be used in that case.
Also this makes me wonder if the use of picolib should be restricted, based on toolchain in use.
But that would probably need to go into a bigger work at later time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
picolibc built with llvm still requires libgcc as that has all of the helper functions required by the underlying ABI, such as the __aeabi functions for ARM. -lgcc would be automatically inserted into the link command by the compiler front-end (either clang or gcc) if zephyr wasn't using -nostdlib. zephyr could probably remove that flag and simply let the compiler front end insert both -lc and -lgcc.
|
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. |
Picolibc is a libc implementation designed for and tested on small embedded systems.
Features:
Porting to additional architectures requires initializing the per-CPU thread pointer in the context
switch code. On ARM, this requires a toolchain with TLS support (GCC in Debian is built correctly).
A sample program linked for cortex-m0 shows the following memory usage: