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 1.8.5 #62882

Merged
merged 8 commits into from
Nov 20, 2023
10 changes: 10 additions & 0 deletions doc/releases/migration-guide-3.5.rst
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,16 @@ C Library
compiler will now warn about declarations of `main` which don't conform to
the Zephyr required type -- ``int main(void)``.

* Picolibc has four different printf/scanf variants supported in Zephyr,
'double', 'long long', 'integer', and 'minimal. 'double' offers a
complete printf implementation with exact floating point in decimal and
hexidecimal formats, full integer support including long long, C99
integer size specifiers (j, z, t) and POSIX positional arguments. 'long
long' mode removes float support, 'integer' removes long long support
while 'minimal' mode also removes support for format modifiers and
positional arguments. Building the library as a module allows finer
control over the feature set provided at each level.

* Picolibc's default floating point input/output code is larger than the
minimal C library version (this is necessary to conform with the C
language "round trip" requirements for these operations). If you use
Expand Down
1 change: 0 additions & 1 deletion lib/libc/Kconfig
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ config REQUIRES_FULL_LIBC

config REQUIRES_FLOAT_PRINTF
bool "Requires floating point support in printf"
select PICOLIBC_IO_FLOAT if PICOLIBC
select CBPRINTF_FP_SUPPORT if MINIMAL_LIBC
select NEWLIB_LIBC_FLOAT_PRINTF if NEWLIB_LIBC
help
Expand Down
6 changes: 6 additions & 0 deletions lib/libc/picolibc/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,12 @@ if(NOT CONFIG_PICOLIBC_USE_MODULE)
if(CONFIG_PICOLIBC_IO_FLOAT)
zephyr_compile_definitions(PICOLIBC_DOUBLE_PRINTF_SCANF)
zephyr_link_libraries(-DPICOLIBC_DOUBLE_PRINTF_SCANF)
elseif(CONFIG_PICOLIBC_IO_MINIMAL)
zephyr_compile_definitions(PICOLIBC_MINIMAL_PRINTF_SCANF)
zephyr_link_libraries(-DPICOLIBC_MINIMAL_PRINTF_SCANF)
elseif(CONFIG_PICOLIBC_IO_LONG_LONG)
zephyr_compile_definitions(PICOLIBC_LONG_LONG_PRINTF_SCANF)
zephyr_link_libraries(-DPICOLIBC_LONG_LONG_PRINTF_SCANF)
else()
zephyr_compile_definitions(PICOLIBC_INTEGER_PRINTF_SCANF)
zephyr_link_libraries(-DPICOLIBC_INTEGER_PRINTF_SCANF)
Expand Down
75 changes: 63 additions & 12 deletions lib/libc/picolibc/Kconfig
Original file line number Diff line number Diff line change
Expand Up @@ -31,16 +31,42 @@ config PICOLIBC_HEAP_SIZE
If set to -2, then the value of COMMON_LIBC_MALLOC_ARENA_SIZE
will be used.

config PICOLIBC_IO_LONG_LONG
bool "support for long long in integer-only printf/scanf"
choice PICOLIBC_IO_LEVEL
prompt "Picolibc printf/scanf level"
default PICOLIBC_IO_MINIMAL if CBPRINTF_NANO
default PICOLIBC_IO_LONG_LONG if CBPRINTF_FULL_INTEGRAL
default PICOLIBC_IO_INTEGER
help
Includes support for long long in integer-only printf/scanf. long long
types are always supported in the floating-point versions.
Selects the level of printf and scanf support

config PICOLIBC_IO_FLOAT
bool "support for floating point values in printf/scanf"
bool "full support for integer/floating point values in printf/scanf"
help
Include floating point support in printf/scanf functions.
Include full floating point and integer support in printf/scanf
functions.

config PICOLIBC_IO_LONG_LONG
bool "full support for integer values, including long long, in printf/scanf"
depends on !REQUIRES_FLOAT_PRINTF && (!CBPRINTF_FP_SUPPORT || CBPRINTF_NANO)
help
Includes full integer with long long, but no floating
point in printf/scanf.

config PICOLIBC_IO_INTEGER
bool "full support for integer values, other than long long, in printf/scanf"
depends on !REQUIRES_FLOAT_PRINTF && ((!CBPRINTF_FP_SUPPORT && !CBPRINTF_FULL_INTEGRAL) || CBPRINTF_NANO)
help
Include full integer other than long long, but no floating point
in printf/scanf.

config PICOLIBC_IO_MINIMAL
bool "limited support for integer values in printf/scanf"
depends on !REQUIRES_FLOAT_PRINTF && CBPRINTF_NANO
help
Include limited integer and no floating point support in
printf/scanf.

endchoice

if PICOLIBC_USE_MODULE

Expand Down Expand Up @@ -88,21 +114,31 @@ config PICOLIBC_FAST_STRCMP
This provides a faster strcmp version even when libc is
built in space-optimized mode

config PICOLIBC_ASSERT_VERBOSE
bool "assert provides verbose information"
help
The usual picolibc assert helper, __assert_func, takes file, line,
function and expression information to make the presented message
more helpful. These all require storage in the image. Unselecting
this option eliminates all of that information, making the results
less helpful but also making them consume less memory.

config PICOLIBC_IO_C99_FORMATS
bool "support C99 format additions in printf/scanf"
default y
help
Includes support for hex floats (in floating-point version) and j, z,
t size modifiers.
Includes C99 printf and scanf support for j, z, t size
modifiers. C99 support is always included in the floating-point
variant

config PICOLIBC_IO_POS_ARGS
bool "Support POSIX positional args (e.g. %$1d) in printf/scanf"
default y
depends on !PICOLIBC_IO_FLOAT
depends on !PICOLIBC_IO_MINIMAL
help
Includes support for positional args (e.g. $1) in integer-only printf
and scanf. Positional args are always supported in the floating-point
versions.
Includes support for positional args (e.g. $1) in integer-only
printf and scanf. Positional args are always supported in the
floating-point variant.

config PICOLIBC_IO_FLOAT_EXACT
bool "support for exact float/string conversion"
Expand All @@ -112,6 +148,21 @@ config PICOLIBC_IO_FLOAT_EXACT
This ensures that printf values with enough digits can be
fed to scanf and generate exactly the same binary value.

config PICOLIBC_IO_SMALL_ULTOA
bool "avoid soft division in printf"
default y
help
Replaces division and modulus by 10 with shifts and adds when
doing binary to decimal conversion in printf for 64-bit
values on 32-bit targets.

config PICOLIBC_IO_MINIMAL_LONG_LONG
bool "Include long long support in minimal printf"
depends on PICOLIBC_IO_MINIMAL
default y if CBPRINTF_FULL_INTEGRAL
help
Include long long support in the minimal picolibc printf code

config PICOLIBC_LOCALE_INFO
bool "support locales in libc functions"
help
Expand Down
21 changes: 21 additions & 0 deletions lib/libc/picolibc/libc-hooks.c
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,27 @@ void __retarget_lock_release(_LOCK_T lock)

#endif /* CONFIG_MULTITHREADING */

#ifdef CONFIG_PICOLIBC_ASSERT_VERBOSE

FUNC_NORETURN void __assert_func(const char *file, int line,
const char *function, const char *expression)
{
__ASSERT(0, "assertion \"%s\" failed: file \"%s\", line %d%s%s\n",
expression, file, line,
function ? ", function: " : "", function ? function : "");
CODE_UNREACHABLE;
}

#else

FUNC_NORETURN void __assert_no_args(void)
{
__ASSERT_NO_MSG(0);
CODE_UNREACHABLE;
}

#endif

/* This function gets called if static buffer overflow detection is enabled on
* stdlib side (Picolibc here), in case such an overflow is detected. Picolibc
* provides an implementation not suitable for us, so we override it here.
Expand Down
3 changes: 1 addition & 2 deletions lib/os/Kconfig.cbprintf
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ choice CBPRINTF_INTEGRAL_CONV
# 01: 0% / 0 B (01 / 00)
config CBPRINTF_FULL_INTEGRAL
bool "Convert the full range of integer values"
select PICOLIBC_IO_LONG_LONG if PICOLIBC
select PICOLIBC_IO_MINIMAL_LONG_LONG if PICOLIBC_IO_MINIMAL && PICOLIBC_USE_MODULE
help
Build cbprintf with buffers sized to support converting the full
range of all integral and pointer values.
Expand Down Expand Up @@ -67,7 +67,6 @@ config CBPRINTF_FP_SUPPORT
bool "Floating point formatting in cbprintf"
default y if FPU
depends on CBPRINTF_COMPLETE
select PICOLIBC_IO_FLOAT if PICOLIBC
help
Build the cbprintf utility function with support for floating
point format specifiers. Selecting this increases stack size
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@
* Add another fake portion of FLASH to simulate a secondary or external FLASH
* that we can do XIP from.
*/
#define EXTFLASH_ADDR 0x5000
#define EXTFLASH_ADDR 0x7000
#define EXTFLASH_SIZE (CONFIG_FLASH_SIZE * 1K - EXTFLASH_ADDR)

#endif
Expand Down
2 changes: 1 addition & 1 deletion subsys/net/lib/lwm2m/lwm2m_observation.c
Original file line number Diff line number Diff line change
Expand Up @@ -529,7 +529,7 @@ struct observe_node *engine_observe_node_discover(sys_slist_t *observe_node_list
const uint8_t *token, uint8_t tkl)
{
struct observe_node *obs;
int obs_list_size, path_list_size;
int obs_list_size, path_list_size = 0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this change?
On what compiler did you see the error/warning code?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

running twister using SDK version 0.16.3-rc1 on my local machine. I'm afraid I've lost the precise tree at this point. Looking at the code, I wasn't surprised by the compiler generating a spurious warning here as the assignment and use are both protected inside 'if' clauses. I'd be fine removing this patch, but as I couldn't get a clean twister run locally without it, I included it here assuming it would also be needed for the CI run against the PR.


if (lwm2m_path_list) {
path_list_size = engine_path_list_size(lwm2m_path_list);
Expand Down
71 changes: 65 additions & 6 deletions tests/kernel/common/src/printk.c
Original file line number Diff line number Diff line change
Expand Up @@ -17,39 +17,96 @@ int (*_old_char_out)(int);

#if defined(CONFIG_PICOLIBC)

#define ZEPHYR_PICOLIBC_VERSION (__PICOLIBC__ * 10000 + \
Copy link
Member

Choose a reason for hiding this comment

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

I would be ok filtering-out individual checks as long as he minimal config isn't the default.

if (IS_ENABLED(CONFIG_PICOLIBC_IO_MINIMAL)) {
  ztest_test_skip();
}

After all, users would explicitly be choosing to use a format that is trimmed-down, maybe non-standard, discards things, etc.

Copy link
Member

Choose a reason for hiding this comment

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

Not blocking though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So just skip tests in that case instead of verifying that they generate the expected result? I'm not sure that's as useful for this test where we're kinda walking through C library configurations and making sure they work as intended?

Copy link
Member

Choose a reason for hiding this comment

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

Output is more or less an unexpected result with minimal io config anyway.

I guess the question is how specific do we want to be about the deviation. Fine to be overly specific as well, but just saying you have the option to be less specific too, which means PicoLibC would not be contractually bound to Zephyr to ensure minimal io formatting is a certain way. I.e. if there ever need to be modifications to picolibc's minimal io formatter, to save a few byte here or there, there is more flexibility.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, that's a fair point. I think I'll leave this patch and burn that bridge when we get to it.

__PICOLIBC_MINOR__ * 100 + \
__PICOLIBC_PATCHLEVEL__)

#ifdef CONFIG_PICOLIBC_IO_MINIMAL
/*
* If picolibc is >= 1.8.4, then minimal printf is available. Otherwise,
* we're going to get the floating point version when the minimal one is
* selected.
*/
#if ZEPHYR_PICOLIBC_VERSION >= 10804
#define HAS_PICOLIBC_IO_MINIMAL
#else
#define HAS_PICOLIBC_IO_FLOAT
#endif
#endif

#ifdef CONFIG_PICOLIBC_IO_LONG_LONG
/*
* If picolibc is >= 1.8.5, then long long printf is available. Otherwise,
* we're going to get the floating point version when the long long one is
* selected.
*/
#if ZEPHYR_PICOLIBC_VERSION >= 10805
#define HAS_PICOLIBC_IO_LONG_LONG
#else
#define HAS_PICOLIBC_IO_FLOAT
#endif
#endif

#ifdef CONFIG_PICOLIBC_IO_FLOAT
#define HAS_PICOLIBC_IO_FLOAT
#endif

/*
* Picolibc long long support is present if the picolibc _WANT_IO_LONG_LONG
* symbol is defined or if the Zephyr configuration has enabled floating
* point support. Note that CONFIG_PICOLIBC_IO_LONG_LONG is only useful
* when using the picolibc module as it cannot affect picolibc included
* with the toolchain
* Picolibc long long support is present if Zephyr configuration has
* enabled long long or floating point support.
*/

char expected_32[] = "22 113 10000 32768 40000 22\n"
"p 112 -10000 -32768 -40000 -22\n"
#if defined(HAS_PICOLIBC_IO_MINIMAL)
"0x1 0x1 0x1 0x1 0x1\n"
"0x1 0x1 0x1 0x1\n"
"42 42 42 42\n"
"-42 -42 -42 -42\n"
"42 42 42 42\n"
"42 42 42 42\n"
"25542abcdef 42\n"
#if defined(_WANT_MINIMAL_IO_LONG_LONG)
"68719476735 -1 18446744073709551615 ffffffffffffffff\n"
#else
"-1 -1 4294967295 ffffffff\n"
#endif
#else
"0x1 0x01 0x0001 0x00000001 0x0000000000000001\n"
"0x1 0x 1 0x 1 0x 1\n"
"42 42 0042 00000042\n"
"-42 -42 -042 -0000042\n"
"42 42 42 42\n"
"42 42 0042 00000042\n"
"255 42 abcdef 42\n"
#if defined(_WANT_IO_LONG_LONG) || defined(CONFIG_PICOLIBC_IO_FLOAT)
#if defined(HAS_PICOLIBC_IO_LONG_LONG) || defined(HAS_PICOLIBC_IO_FLOAT)
"68719476735 -1 18446744073709551615 ffffffffffffffff\n"
#else
"-1 -1 4294967295 ffffffff\n"
#endif
#endif
"0xcafebabe 0xbeef 0x2a\n"
;

char expected_64[] = "22 113 10000 32768 40000 22\n"
"p 112 -10000 -32768 -40000 -22\n"
#if defined(HAS_PICOLIBC_IO_MINIMAL)
"0x1 0x1 0x1 0x1 0x1\n"
"0x1 0x1 0x1 0x1\n"
"42 42 42 42\n"
"-42 -42 -42 -42\n"
"42 42 42 42\n"
"42 42 42 42\n"
"25542abcdef 42\n"
#else
"0x1 0x01 0x0001 0x00000001 0x0000000000000001\n"
"0x1 0x 1 0x 1 0x 1\n"
"42 42 0042 00000042\n"
"-42 -42 -042 -0000042\n"
"42 42 42 42\n"
"42 42 0042 00000042\n"
"255 42 abcdef 42\n"
#endif
"68719476735 -1 18446744073709551615 ffffffffffffffff\n"
"0xcafebabe 0xbeef 0x2a\n"
;
Expand Down Expand Up @@ -161,6 +218,8 @@ ZTEST(printk, test_printk)
printk("0x%x %p %-2p\n", hex, ptr, (char *)42);

pk_console[pos] = '\0';
__printk_hook_install(_old_char_out);
printk("expected '%s'\n", expected);
zassert_true((strcmp(pk_console, expected) == 0), "printk failed");

(void)memset(pk_console, 0, sizeof(pk_console));
Expand Down
2 changes: 1 addition & 1 deletion west.yml
Original file line number Diff line number Diff line change
Expand Up @@ -310,7 +310,7 @@ manifest:
- debug
- name: picolibc
path: modules/lib/picolibc
revision: d07c38ff051386f8e09a143ea0a6c1d6d66dd1d8
revision: 1a5c603b9f8e228f9459bdafedad15ea28efc700
- name: segger
revision: 9d0191285956cef43daf411edc2f1a7788346def
path: modules/debug/segger
Expand Down