Skip to content
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

Picolibc optimizations #39564

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 3 additions & 0 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -917,6 +917,9 @@ if(CONFIG_USERSPACE)
if(CONFIG_NEWLIB_LIBC_NANO)
set(NEWLIB_PART -l libc_nano.a z_libc_partition -l libm_nano.a z_libc_partition)
endif()
if(CONFIG_PICOLIBC)
set(NEWLIB_PART -l libc.a z_libc_partition)
endif()

add_custom_command(
OUTPUT ${APP_SMEM_UNALIGNED_LD} ${APP_SMEM_PINNED_UNALIGNED_LD}
Expand Down
2 changes: 1 addition & 1 deletion arch/Kconfig
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ config ARM
# FIXME: current state of the code for all ARM requires this, but
# is really only necessary for Cortex-M with ARM MPU!
select GEN_PRIV_STACKS
select ARCH_HAS_THREAD_LOCAL_STORAGE if CPU_AARCH32_CORTEX_R || CPU_CORTEX_M
select ARCH_HAS_THREAD_LOCAL_STORAGE if CPU_AARCH32_CORTEX_R || CPU_CORTEX_M || CPU_AARCH32_CORTEX_A
help
ARM architecture

Expand Down
1 change: 1 addition & 0 deletions arch/arm/core/aarch32/cortex_a_r/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -15,3 +15,4 @@ zephyr_library_sources(
)

zephyr_library_sources_ifdef(CONFIG_USERSPACE thread.c)
zephyr_library_sources_ifdef(CONFIG_THREAD_LOCAL_STORAGE __aeabi_read_tp.S)
20 changes: 20 additions & 0 deletions arch/arm/core/aarch32/cortex_a_r/__aeabi_read_tp.S
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
/*
* Copyright (c) 2020 Intel Corporation
Copy link
Collaborator

Choose a reason for hiding this comment

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

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 ?

*
* SPDX-License-Identifier: Apache-2.0
*/

#include <toolchain.h>

_ASM_FILE_PROLOGUE

GTEXT(__aeabi_read_tp)

SECTION_FUNC(text, __aeabi_read_tp)
/*
* Load TLS base address, which is stored in the TPIDRURO register, also
* known as the "Process ID" register. Refer to the code in z_arm_pendsv
* to see where this register is set.
*/
mrc 15, 0, r0, c13, c0, 3
bx lr
Comment on lines +19 to +20
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

2 changes: 1 addition & 1 deletion arch/arm/core/aarch32/swap_helper.S
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,7 @@ out_fp_endif:
adds r4, r2, r4
ldr r0, [r4]

#if defined(CONFIG_CPU_AARCH32_CORTEX_R)
#if defined(CONFIG_CPU_AARCH32_CORTEX_R) || defined(CONFIG_CPU_AARCH32_CORTEX_A)
/* Store TLS pointer in the "Process ID" register.
* This register is used as a base pointer to all
* thread variables with offsets added by toolchain.
Expand Down
1 change: 1 addition & 0 deletions cmake/toolchain/gnuarmemb/generic.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -28,5 +28,6 @@ set(SYSROOT_TARGET arm-none-eabi)
set(CROSS_COMPILE ${TOOLCHAIN_HOME}/bin/${CROSS_COMPILE_TARGET}-)
set(SYSROOT_DIR ${TOOLCHAIN_HOME}/${SYSROOT_TARGET})
set(TOOLCHAIN_HAS_NEWLIB ON CACHE BOOL "True if toolchain supports newlib")
set(TOOLCHAIN_HAS_PICOLIBC ON CACHE BOOL "True if toolchain supports picolibc")

message(STATUS "Found toolchain: gnuarmemb (${GNUARMEMB_TOOLCHAIN_PATH})")
1 change: 1 addition & 0 deletions cmake/toolchain/xtools/generic.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ set(SYSROOT_TARGET ${CROSS_COMPILE_TARGET})
set(CROSS_COMPILE ${some_toolchain_root}/${CROSS_COMPILE_TARGET}/bin/${CROSS_COMPILE_TARGET}-)
set(SYSROOT_DIR ${some_toolchain_root}/${SYSROOT_TARGET}/${SYSROOT_TARGET})
set(TOOLCHAIN_HAS_NEWLIB ON CACHE BOOL "True if toolchain supports newlib")
set(TOOLCHAIN_HAS_PICOLIBC ON CACHE BOOL "True if toolchain supports picolibc")

unset(some_toolchain_root)
unset(some_toolchain)
Expand Down
20 changes: 17 additions & 3 deletions include/sys/libc-hooks.h
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
* that need to call into the kernel as system calls
*/

#if defined(CONFIG_NEWLIB_LIBC) || defined(CONFIG_ARCMWDT_LIBC)
#if defined(CONFIG_NEWLIB_LIBC) || defined(CONFIG_ARCMWDT_LIBC) || defined(CONFIG_PICOLIBC)

/* syscall generation ignores preprocessor, ensure this is defined to ensure
* we don't have compile errors
Expand Down Expand Up @@ -56,7 +56,21 @@ extern struct k_mem_partition z_malloc_partition;
*/
#define Z_MALLOC_PARTITION_EXISTS 1
#endif /* CONFIG_MINIMAL_LIBC_MALLOC_ARENA_SIZE > 0 */
#endif /* CONFIG_MINIMAL_LIBC */
#elif defined(CONFIG_PICOLIBC)
/* If we are using picolibc, the heap arena is in one of two areas:
* - If we have an MPU that requires power of two alignment, the heap bounds
* must be specified in Kconfig via CONFIG_PICOLIBC_ALIGNED_HEAP_SIZE.
* - Otherwise, the heap arena on most arches starts at a suitably
* aligned base addreess after the `_end` linker symbol, through to the end
* of system RAM.
*/
#if (!defined(CONFIG_MPU_REQUIRES_POWER_OF_TWO_ALIGNMENT) || \
(defined(CONFIG_MPU_REQUIRES_POWER_OF_TWO_ALIGNMENT) && \
CONFIG_PICOLIBC_ALIGNED_HEAP_SIZE))
#define Z_MALLOC_PARTITION_EXISTS 1
extern struct k_mem_partition z_malloc_partition;
#endif
#endif /* CONFIG_PICOLIBC */

#ifdef Z_MALLOC_PARTITION_EXISTS
/* Memory partition containing the libc malloc arena. Configuration controls
Expand All @@ -66,7 +80,7 @@ extern struct k_mem_partition z_malloc_partition;
#endif

#if defined(CONFIG_NEWLIB_LIBC) || defined(CONFIG_STACK_CANARIES) || \
defined(CONFIG_NEED_LIBC_MEM_PARTITION)
defined(CONFIG_PICOLIBC) || defined(CONFIG_NEED_LIBC_MEM_PARTITION)
/* - All newlib globals will be placed into z_libc_partition.
* - Minimal C library globals, if any, will be placed into
* z_libc_partition.
Expand Down
11 changes: 11 additions & 0 deletions include/sys/printk.h
Original file line number Diff line number Diff line change
Expand Up @@ -60,11 +60,22 @@ static inline __printf_like(1, 0) void vprintk(const char *fmt, va_list ap)
}
#endif

#ifdef _PICOLIBC__

#include <stdio.h>

#define snprintk(...) snprintf(__VA_ARGS__)
#define vsnprintk(str, size, fmt, ap) vsnprintf(str, size, fmt, ap)

#else

extern __printf_like(3, 4) int snprintk(char *str, size_t size,
const char *fmt, ...);
extern __printf_like(3, 0) int vsnprintk(char *str, size_t size,
const char *fmt, va_list ap);

#endif

#ifdef __cplusplus
}
#endif
Expand Down
8 changes: 8 additions & 0 deletions kernel/Kconfig
Original file line number Diff line number Diff line change
Expand Up @@ -202,9 +202,17 @@ config THREAD_USERSPACE_LOCAL_DATA
depends on USERSPACE
default y if ERRNO && !ERRNO_IN_TLS

config LIBC_ERRNO
bool
help
Use external libc errno, not the internal one. This makes sure that
ERRNO is not set for libc configurations that provide their own
implementation.

config ERRNO
bool "Errno support"
default y
depends on !LIBC_ERRNO
help
Enable per-thread errno in the kernel. Application and library code must
include errno.h provided by the C library (libc) to use the errno
Expand Down
3 changes: 3 additions & 0 deletions kernel/errno.c
Original file line number Diff line number Diff line change
Expand Up @@ -44,10 +44,13 @@ static inline int *z_vrfy_z_errno(void)
#include <syscalls/z_errno_mrsh.c>

#else
#ifndef CONFIG_LIBC_ERRNO
int *z_impl_z_errno(void)
{
return &_current->errno_var;
}
#endif /* CONFIG_PICOLIBC */

#endif /* CONFIG_USERSPACE */

#endif /* CONFIG_ERRNO_IN_TLS */
Expand Down
2 changes: 2 additions & 0 deletions lib/libc/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -8,4 +8,6 @@ elseif(CONFIG_MINIMAL_LIBC)
add_subdirectory(minimal)
elseif(CONFIG_ARMCLANG_STD_LIBC)
add_subdirectory(armstdc)
elseif(CONFIG_PICOLIBC)
add_subdirectory(picolibc)
endif()
33 changes: 33 additions & 0 deletions lib/libc/Kconfig
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,15 @@ config MINIMAL_LIBC
help
Build with minimal C library.

config PICOLIBC
bool "Picolibc library"
depends on !NATIVE_APPLICATION
select THREAD_LOCAL_STORAGE if ARCH_HAS_THREAD_LOCAL_STORAGE
Copy link
Collaborator

Choose a reason for hiding this comment

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

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:

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

select LIBC_ERRNO
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

help
Build with picolibc library. The picolibc library is expected to be
part of the SDK in this case.

config NEWLIB_LIBC
bool "Newlib C library"
depends on !NATIVE_APPLICATION
Expand All @@ -53,6 +62,30 @@ endchoice # LIBC_IMPLEMENTATION
config HAS_NEWLIB_LIBC_NANO
bool

if PICOLIBC

config PICOLIBC_INTEGER_PRINTF
bool "Build with picolibc integer-only printf"
help
Build with floating point printf disabled. This will reduce the size
of the image.

config PICOLIBC_ALIGNED_HEAP_SIZE
int "Picolibc aligned heap size (bytes)"
depends on MPU_REQUIRES_POWER_OF_TWO_ALIGNMENT
depends on USERSPACE
default 0
help
If user mode is enabled, and MPU hardware has requirements that
regions be sized to a power of two and aligned to their size,
and user mode threads need to access this heap, then this is necessary
to properly define an MPU region for the heap.

If this is left at 0, then remaining system RAM will be used for this
area and it may not be possible to program it as an MPU region.

endif # PICOLIBC

if NEWLIB_LIBC

config NEWLIB_LIBC_NANO
Expand Down
41 changes: 41 additions & 0 deletions lib/libc/picolibc/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
# SPDX-License-Identifier: Apache-2.0

zephyr_library()
zephyr_library_sources(libc-hooks.c)

# Zephyr normally uses -ffreestanding, which with current GNU toolchains
# means that the flag macros used by picolibc <inttypes.h> to signal
# support for PRI.64 macros are not present. To make them available we
# need to hook into the include path before the system files and
# explicitly include the picolibc header that provides those macros.
zephyr_include_directories(include)

# define __LINUX_ERRNO_EXTENSIONS__ so we get errno defines like -ESHUTDOWN
# used by the network stack
zephyr_compile_definitions(__LINUX_ERRNO_EXTENSIONS__)

zephyr_link_libraries(
m
c
gcc # Lib C depends on libgcc.
)

# The -T/dev/null avoids pulling in picolibc.ld
zephyr_link_libraries(
--specs=picolibc.specs
-T/dev/null
)

zephyr_compile_options(
--specs=picolibc.specs
-D_GNU_SOURCE
)

if(CONFIG_PICOLIBC_INTEGER_PRINTF)
zephyr_compile_options(
-DPICOLIBC_INTEGER_PRINTF_SCANF
)
zephyr_link_libraries(
-DPICOLIBC_INTEGER_PRINTF_SCANF
)
endif()
Loading