Use picolibc instead of newlib-nano

[keith-packard]

 1. Add picolibc bits to linker scripts. Picolibc uses thread local storage
    for unshared data, and the picolibc startup code relies on carefully
    constructed linker script that allocates one TLS block in RAM for
    applications that don't have explicit TLS support. The picolibc
    startup code also uses different symbols than the tf-m startup code.

 2. Support picolibc stdio. There was already picolibc support code
    present in the library for the ARM LLVM toolchain. The
    conditionals which selected it have been changed to use
    __PICOLIBC__ instead of __clang_major__.

 3. Add _exit stub. Code using assert or abort end up calling _exit
    through the picolibc signal handling code.

 4. Switch to picolibc.specs. This is needed for toolchains which
    don't use picolibc by default, and can also be used without
    trouble in toolchains where picolibc is the default.

[keith-packard]

These are (finally) passing Zephyr tests using both SDK 0.17 and SDK 0.18-alpha4 (although each requires some additional changes which are wending their way upstream). I think they're ready for review and help getting them upstream.

[tomi-font]

How did the linker get confused exactly? Is this really needed?

[keith-packard]

. got set to LOADADDR instead of ADDR when building with binutils 2.43. I'm afraid that after spending several hours chasing this down, I didn't dig into the linker code to figure out precisely why that happened.

[keith-packard]

I've re-added this patch as it is necessary even with SDK 0.17.4.

[tomi-font]

Had a chat with upstream about these (picolibc) changes. Let's keep them only in this fork until they're not needed anymore (TF-M is moving away from C library usage), in the next release or the one following it.
But it would be good to have someone who understands this a bit to review the changes. Don't know who that could be. cc @frkv @d3zd3z @ceolin @dleach02 @Vge0rge

[keith-packard]

Had a chat with upstream about these (picolibc) changes. Let's keep them only in this fork until they're not needed anymore (TF-M is moving away from C library usage), in the next release or the one following it. But it would be good to have someone who understands this a bit to review the changes. Don't know who that could be. cc @frkv @d3zd3z @ceolin @dleach02 @Vge0rge

Sounds good. I'll keep running tests as this series moves forward. This is not buildable using SDK 0.17.2 because of a last-minute oops in that release, but it should be buildable with the upcoming SDK 0.17.3.

[frkv]

I recommend @wearyzen and @adeaarm give it a quick look from TF-M's perspective.

The Clang/LLVM toolchain for TF-M was added with no dependency on libc and/or picolib in the core TF-M code, but you still see references to nano.specs in the GNUARM toolchain

Zephyr, making this choice for GNUARM toolchain (this change), will take any penalty related to support, any security advisories etc... But the choice to go to picolibc is an active choice in the Zephyr community, and in extension it will impact any vendor who makes use of standard c library function in PSA crypto and PSA crypto driver build as well as in bootloaders...

With regards to these commits, I think we should consider them to be marked as commits emitted from zephyr integration, unless they are possible to add upstream in the TF-M project... If they are out-of-tree patches required for Zephyr integration, they would need to be maintained across updated versions of TF-M in our fork

[tomi-font]

With regards to these commits, I think we should consider them to be marked as commits emitted from zephyr integration

Yeah this is something we've agreed on and I think just needs to be implemented (documenting the practice basically). In the meantime I don't think we can require that of anyone yet.

[Anton-TF]

True, TF-M plans to remove libc from all secure binaries (BL1, BL2, tfm_s) for Clang/LLVM and GCC toolchains at least.
As part of this effort, we also intend to clean up and streamline the linker scripts.

Additionally, you might want to consider these recent fixes in the scripts:
https://review.trustedfirmware.org/c/TF-M/trusted-firmware-m/+/40014
https://review.trustedfirmware.org/c/TF-M/trusted-firmware-m/+/40068

[keith-packard]

Sounds like any libc-specific changes will be transient then, which will be nice.

[wearyzen]

While trying to build the non-secure MPS4 board with this PR and Zephyr SDK 0.18.0-alpha4 we see below error:

west build -p -b mps4/corstone320/fvp/ns samples/hello_world/ -t run
...
/home/zephyr-sdk-0.18.0-alpha4/gnu/arm-zephyr-eabi/bin/../lib/gcc/arm-zephyr-eabi/14.3.0/../../../../arm-zephyr-eabi/bin/ld: (.text._start+0x30): undefined reference to `__data_source'
/home/zephyr-sdk-0.18.0-alpha4/gnu/arm-zephyr-eabi/bin/../lib/gcc/arm-zephyr-eabi/14.3.0/../../../../arm-zephyr-eabi/bin/ld: (.text._start+0x34): undefined reference to `__data_start'
/home/zephyr-sdk-0.18.0-alpha4/gnu/arm-zephyr-eabi/bin/../lib/gcc/arm-zephyr-eabi/14.3.0/../../../../arm-zephyr-eabi/bin/ld: (.text._start+0x38): undefined reference to `__bss_size'
/home/zephyr-sdk-0.18.0-alpha4/gnu/arm-zephyr-eabi/bin/../lib/gcc/arm-zephyr-eabi/14.3.0/../../../../arm-zephyr-eabi/bin/ld: (.text._start+0x3c): undefined reference to `__bss_start'
/home/zephyr-sdk-0.18.0-alpha4/gnu/arm-zephyr-eabi/bin/../lib/gcc/arm-zephyr-eabi/14.3.0/../../../../arm-zephyr-eabi/bin/ld: (.text._start+0x40): undefined reference to `__tls_base'
/home/zephyr-sdk-0.18.0-alpha4/gnu/arm-zephyr-eabi/bin/../lib/gcc/arm-zephyr-eabi/14.3.0/../../../../arm-zephyr-eabi/bin/ld: /home/zephyr-sdk-0.18.0-alpha4/gnu/arm-zephyr-eabi/bin/../lib/gcc/arm-zephyr-eabi/14.3.0/../../../../arm-zephyr-eabi/lib/thumb/v8-m.main/nofp/space/libc.a(libc_picolib_machine_arm_set_tls.c.o): in function `_set_tls':
set_tls.c:(.text._set_tls+0xc): undefined reference to `__arm32_tls_tcb_offset'

Referring to the changes in this PR, I tried below patch and it seems to fix the issue:

git diff
diff --git a/bl1/bl1_2/bl1_dummy_rotpk.prv b/bl1/bl1_2/bl1_dummy_rotpk.prv
index bf011cd4d..b6bf9b600 100644
Binary files a/bl1/bl1_2/bl1_dummy_rotpk.prv and b/bl1/bl1_2/bl1_dummy_rotpk.prv differ
diff --git a/platform/ext/target/arm/mps4/common/device/source/gcc/mps4_corstone3xx_bl1_1.ld b/platform/ext/target/arm/mps4/common/device/source/gcc/mps4_corstone3xx_bl1_1.ld
index 44f04021a..7fad80960 100644
--- a/platform/ext/target/arm/mps4/common/device/source/gcc/mps4_corstone3xx_bl1_1.ld
+++ b/platform/ext/target/arm/mps4/common/device/source/gcc/mps4_corstone3xx_bl1_1.ld
@@ -160,6 +160,25 @@ SECTIONS
     } > RAM

     bss_size = __bss_end__ - __bss_start__;
+    /* --- add after the .bss section --- */
+    PROVIDE( __bss_start = __bss_start__ );
+    PROVIDE( __bss_end   = __bss_end__ );
+    PROVIDE( __bss_size  = __bss_end - __bss_start );
+
+    PROVIDE( __data_start  = ADDR(.data) );
+    PROVIDE( __data_end    = __data_end__ );
+    PROVIDE( __data_source = LOADADDR(.data) );
+    PROVIDE( __data_size   = __data_end - __data_start );
+
+    /* BL1_1 doesn’t use TLS, but picolibc’s _set_tls needs these present */
+    PROVIDE( __tls_base = 0 );
+    PROVIDE( __tls_align = 8 );
+    PROVIDE( __arm32_tls_tcb_offset = 8 );
+
+    /* --- add after the .heap section (where __HeapBase/Limit are set) --- */
+    PROVIDE( __heap_start = __HeapBase );
+    PROVIDE( __heap_end   = __HeapLimit );
+

 #if defined (__ARM_FEATURE_CMSE) && (__ARM_FEATURE_CMSE == 3U)
     .msp_stack (NOLOAD) : ALIGN(32)
diff --git a/platform/ext/target/arm/mps4/common/device/source/gcc/mps4_corstone3xx_bl1_2.ld b/platform/ext/target/arm/mps4/common/device/source/gcc/mps4_corstone3xx_bl1_2.ld
index 83aaf144f..9151ff3a0 100644
--- a/platform/ext/target/arm/mps4/common/device/source/gcc/mps4_corstone3xx_bl1_2.ld
+++ b/platform/ext/target/arm/mps4/common/device/source/gcc/mps4_corstone3xx_bl1_2.ld
@@ -162,6 +162,25 @@ SECTIONS

     bss_size = __bss_end__ - __bss_start__;

+    /* --- add after the .bss section --- */
+    PROVIDE( __bss_start = __bss_start__ );
+    PROVIDE( __bss_end   = __bss_end__ );
+    PROVIDE( __bss_size  = __bss_end - __bss_start );
+
+    PROVIDE( __data_start  = ADDR(.data) );
+    PROVIDE( __data_end    = __data_end__ );
+    PROVIDE( __data_source = LOADADDR(.data) );
+    PROVIDE( __data_size   = __data_end - __data_start );
+
+    /* BL1_1 doesn’t use TLS, but picolibc’s _set_tls needs these present */
+    PROVIDE( __tls_base = 0 );
+    PROVIDE( __tls_align = 8 );
+    PROVIDE( __arm32_tls_tcb_offset = 8 );
+
+    /* --- add after the .heap section (where __HeapBase/Limit are set) --- */
+    PROVIDE( __heap_start = __HeapBase );
+    PROVIDE( __heap_end   = __HeapLimit );
+
 #if defined (__ARM_FEATURE_CMSE) && (__ARM_FEATURE_CMSE == 3U)
     .msp_stack (NOLOAD) : ALIGN(32)
     {
(END)

I am not a linker script expert but if above change looks good then please let me know if you would like to include it in this PR.
Thanks!

[Vge0rge]

@keith-packard I attached here the linker for when building the TF-M regression tests for nRF54L15.

nrf54l_tfm_s.txt

The symbols which I confirmed that they are not initialized correctly are:

• fs_cfg_its
• fs_cfg_ps
• nv_increment_status

There are probably more, I just found these until now. But these are normal global static variables which are placed in .data as expected. So maybe it is indeed crt related and not TLS after all?

EDIT I did some more digging and it seems that the problem is not the linker script at all. The problem is simpler, the function picolibc_startup is never called for me. So the __copy_table_t which is initialized in this function and sets the values of the global initialized objects is never actually copied.

I confirmed that by manually implementing the function inside the ResetHandler and then I could run and pass the TF-M regression tests.

This was introduced with this PR because the cmsis header changes the __PROGRAM_START define based on the picolibc:
https://github.com/zephyrproject-rtos/CMSIS_6/blob/30a859f44ef8ab4dc8f84b03ed586fd16ccf9d74/CMSIS/Core/Include/m-profile/cmsis_gcc_m.h#L33

So before this PR the define was pointing to the cmsis_start in the same file which copies the table but now that it is not used the global data are not initialized.

EDIT2 I am wondering if this is because the TF-M main function belongs to a separate target (tfm_spm) compared to the picolib_startup source file. So since there is no main function in the target that the picolib_startup belongs to the constructor attribute does nothing?

I did more checks with 2 Nordic devices, I run the TF-M tests and some TF-M applications and all work fine as long as I include the logic of the picolib_startup in the reset handler. So Keith please give a proposal on how to resolve the issue and we can move forward with this from Nordics side.

[keith-packard]

I've pushed an update which omits the linker script changes as those aren't necessary unless something needs thread local storage (which doesn't appear to be true in the current code).

[Vge0rge]

@keith-packard I cannot build with it anymore because of the same issues as here:
#134 (comment)

But as I said before the problem at least for Nordic devices is not the linker script. The problem is that the "constructor" attribute doesn't seem to work as expected for me. Maybe @etienne-lms can also try in ST devices if the issue that I described exists there as well and maybe then we can try to overcome this issue here and keep your previous revision of the linker files which seems to work?

[keith-packard]

@keith-packard I cannot build with it anymore because of the same issues as here: #134 (comment)

But as I said before the problem at least for Nordic devices is not the linker script. The problem is that the "constructor" attribute doesn't seem to work as expected for me. Maybe @etienne-lms can also try in ST devices if the issue that I described exists there as well and maybe then we can try to overcome this issue here and keep your previous revision of the linker files which seems to work?

As far as I can tell, __PROGRAM_START should point at the picolibc startup code, _start, which resides in crt0.o. That's built in a slightly indirect manner, but the implementation ends up in __start. That code calls __libc_init_array which uses the constructor array generated by the linker from __init_array_start to __init_array_end. The linker map you provided shows that array being initialized with a symbol from the provided picolibc.c, which should be the constructor in that file.

If you can debug the initialization sequence of this firmware, you should be able to verify that picolibc's _start is called, and that should call __libc_init_array, which should call picolibc_startup to initialize all of the data sections.

In any case, I've re-added the linker script changes, this time as a separate commit so that they can be tested in isolation. That patch now contains changes for the mps4 targets which should resolve the issue referenced above. There are a lot more linker scripts that could be modified; I'm unclear as to which of those should get included in this series as I can only test those used by Zephyr CI.

[keith-packard]

Ok, I did a bunch more digging today and discovered that the existing patch didn't track picolibc 1.8.10 changes in initialization code. That version changed the symbols used as the bounds for the constructor array; when undefined, the array is assumed to be empty (!) so no constructors would run. So sorry that I didn't dig into this in more detail earlier.

In any case, I've radically restructured the patch so that the TLS linker script changes are not necessary as those are difficult to get correct without testing. The only linker script changes in the current series add aliases for existing symbols that match what picolibc expects for the constructor list and the heap bounds.

picolibc.c now defines the picolibc-specific memory initialization symbols so that if it isn't included the link will fail, providing insurance that the initialization constructor will be included.

That involved adding picolibc.c to a pile of target_sources calls, but each addition was a place which would have failed to work before.

I've left the series as a sequence of changes on top of the old bits in case anyone wants to see how things have changed. Once we're happy, I'd propose restructuring the series to allow for bisecting; performing the link script and CMakeLists.txt changes first, then adding picolibc.c and finally updating the toolchain files to enable picolibc.

[keith-packard]

Oh, as a bonus feature, I've also updated the llvm linker scripts -- those will also need the new constructor bounds symbols once ARM updates that toolchain to use picolibc 1.8.10.

[keith-packard]

and pushed another small set of changes that just stops using picolibc's crt0 entirely; we don't need any CPU initialization it might do, we just need memory set up and constructors run.

[Vge0rge]

Hey @keith-packard, thanks a lot for the detailed investigation! I tried the new version of this PR and I still could not boot TF-M in Nordic devices. I did some debugging and I can now confirm that the code to copy tables is now executed form picolib as you described. The issue that I found now is that the linker script does not include the TFM_BSS section in the zero table, so I added this patch:

diff --git a/platform/ext/common/gcc/tfm_common_s.ld.template b/platform/ext/common/gcc/tfm_common_s.ld.template
index 8fd95bdea..e623e621a 100644
--- a/platform/ext/common/gcc/tfm_common_s.ld.template
+++ b/platform/ext/common/gcc/tfm_common_s.ld.template
@@ -275,6 +275,9 @@ SECTIONS

         LONG (ADDR(.TFM_APP_ROT_LINKER_BSS))
         LONG (SIZEOF(.TFM_APP_ROT_LINKER_BSS) / 4)
+
+        LONG (ADDR(.TFM_BSS))
+        LONG (SIZEOF(.TFM_BSS) / 4)
 #if defined(CONFIG_TFM_PARTITION_META)
         LONG (ADDR(.TFM_SP_META_PTR))
         LONG (SIZEOF(.TFM_SP_META_PTR) / 4)

And this allowed me to boot and run the tests, do you think that you can include this patch in your commits?

[keith-packard]

Hey @keith-packard, thanks a lot for the detailed investigation! I tried the new version of this PR and I still could not boot TF-M in Nordic devices. I did some debugging and I can now confirm that the code to copy tables is now executed form picolib as you described. The issue that I found now is that the linker script does not include the TFM_BSS section in the zero table, so I added this patch:

That's good to know; I had assumed that all of the initialization required was included in those tables. It would be easy enough to add an explicit initialization of the BSS segment separate from the table in case other linker scripts are also missing this element, but your change avoids the extra code that would involve.

And this allowed me to boot and run the tests, do you think that you can include this patch in your commits?

I'll certainly fix it; I'll go investigate and see if it seems like it will be easier to just add explicit code so that we don't end up with picolibc-specific additions to the image (the other linker script changes only add symbols if needed and don't add anything to the image).

[keith-packard]

I'll certainly fix it; I'll go investigate and see if it seems like it will be easier to just add explicit code so that we don't end up with picolibc-specific additions to the image (the other linker script changes only add symbols if needed and don't add anything to the image).

I found that tfm_isolation_s.ld.template was also missing this section. I reviewed all of the other linker scripts and they all include the default bss segment in their zero tables. I also reviewed them to make sure they all include the default data segment in their copy tables. That indicates to me that the two templates were just buggy; relying on some startup code to initialize that separately from the table while not relying on that startup code to initialize the default data segment. I've pushed that patch on top of the existing picolibc branch. When it all looks good, I'll go rework the series so that it won't break bisecting and make a bit more sense for future readers.

[Vge0rge]

Thanks again for the fix @keith-packard . I can now confirm that this patch works for Nordic devices :)

Feel free to rework the history and let me know when you are done so that I can approve this.

Small suggestion, it would be nice in my opinion to include tags like the one used in mbedtls fork of Zephyr (https://github.com/zephyrproject-rtos/mbedtls/blob/zephyr/README.md) when you do the rewrite. I am not gonna block the PR for this since this is not an official rule in this repo yet but I am just suggesting it.

Edit: I ended up approving so that I don't NACK you anymore. If you rewrite it will anyway request my review again.

[keith-packard]

Thanks again for the fix @keith-packard . I can now confirm that this patch works for Nordic devices :)

Feel free to rework the history and let me know when you are done so that I can approve this.

Just pushed a rebase -i that cleans up the sequence a bit and should work through bisect, but the result is exactly the same as what you've already tested (at least according to git diff?)

Small suggestion, it would be nice in my opinion to include tags like the one used in mbedtls fork of Zephyr (https://github.com/zephyrproject-rtos/mbedtls/blob/zephyr/README.md) when you do the rewrite. I am not gonna block the PR for this since this is not an official rule in this repo yet but I am just suggesting it.

Sure, happy to tag these with [zep noup] as we're expecting upstream to drop libc entirely, making this whole sequence unnecessary.

Edit: I ended up approving so that I don't NACK you anymore. If you rewrite it will anyway request my review again.

Thanks. Really appreciate your help getting this over the line. I'm looking forward to having SDK 1.0 fully supported by Zephyr 4.3, even if it won't be the "official" toolchain.

[tomi-font]

Can you please add details to the commit message of at least the first commit to briefly explain why this is not submitted upstream and what is expected from upstream related to that? for tracking purposes

[Vge0rge]

D

Thanks again for the fix @keith-packard . I can now confirm that this patch works for Nordic devices :)
Feel free to rework the history and let me know when you are done so that I can approve this.

Just pushed a rebase -i that cleans up the sequence a bit and should work through bisect, but the result is exactly the same as what you've already tested (at least according to git diff?)

Small suggestion, it would be nice in my opinion to include tags like the one used in mbedtls fork of Zephyr (https://github.com/zephyrproject-rtos/mbedtls/blob/zephyr/README.md) when you do the rewrite. I am not gonna block the PR for this since this is not an official rule in this repo yet but I am just suggesting it.

Sure, happy to tag these with [zep noup] as we're expecting upstream to drop libc entirely, making this whole sequence unnecessary.

Edit: I ended up approving so that I don't NACK you anymore. If you rewrite it will anyway request my review again.

Thanks. Really appreciate your help getting this over the line. I'm looking forward to having SDK 1.0 fully supported by Zephyr 4.3, even if it won't be the "official" toolchain.

Diff looks good to me as well :) Thanks for adding the tags and for the support as well. Glad to see that moving forward!

[JarmouniA]

Cc @etienne-lms @ahmadstm

[etienne-lms]

I've successfully tested on the few supported STM32 platforms.

For info, while a couple of hundred bytes are saved in flash and RAM when using picolib, I also saw that BL2 footprint was increased by more than 2kByte of flash when console traces are enabled. Observed (from build tests) on various platforms, not only the STM32 ones.

[etienne-lms]

Prefer ASCII char only? (replace © with (C))

[keith-packard]

(C) is meaningless. I'll just delete the ©

[keith-packard]

For info, while a couple of hundred bytes are saved in flash and RAM when using picolib, I also saw that BL2 footprint was increased by more than 2kByte of flash when console traces are enabled. Observed (from build tests) on various platforms, not only the STM32 ones.

What are console traces and how are they enabled? There's nothing obvious in the source tree that I could find.

If this ends up enabling 'assert' macros, then that can cause a pretty significant jump in flash usage as the default picolibc assert behavior includes __FILE__, __LINE__ and also the asserting expression as a string. Zephyr SDK 1.0 changes that default to remove that information, so asserts no longer cause significant increases in flash usage.

I just reviewed the picolibc code and there's no easy way to disable the verbose assert behavior once enabled. If you want to do this, you'd need to do:

/* make sure the library defaults are loaded */
#include <stdlib.h>
/* unset any default for __ASSERT_VERBOSE */
#undef __ASSERT_VERBOSE
/* get the quiet version of assert defined */
#include <assert.h>