Skip to content

PicoLibc support as alternative to Newlib(-nano)#12305

Closed
bergzand wants to merge 29 commits intoRIOT-OS:masterfrom
bergzand:pr/libc/picolibc_initial
Closed

PicoLibc support as alternative to Newlib(-nano)#12305
bergzand wants to merge 29 commits intoRIOT-OS:masterfrom
bergzand:pr/libc/picolibc_initial

Conversation

@bergzand
Copy link
Member

@bergzand bergzand commented Sep 26, 2019

Contribution description

This PR adds support to use PicoLibc as libc implementation on RIOT. PicoLibc at the moment uses Newlib as a basis with the stdio implementation from avr-libc strapped on. For the installation of PicoLibc I'd like to refer to the build guide. For RIOT the default build settings should work fine.

Main advantage of PicoLibc is that it shaves approximately 3KB from the flash footprint of a binary. In case of the hello-world example on a samr21-xpro:

Total Total(LTO=1) libc libc(LTO=1)
Newlib-nano: 8.5KB 7.8KB 4.1KB 4.1KB
PicoLibc: 5.6KB 4.9KB 1.3KB 1.3KB

Testing procedure

Enable with:

make PICOLIBC=1

such as:

make -C examples/gnrc_networking BOARD=samr21-xpro PICOLIBC=1

For now only the Cortex-m and RISC-V MCUs are supported.

Issues/PRs references

None

Todo:

  • implement syscalls related to VFS
  • Look at Thread-Local Storage integration
  • Test printf_float

@bergzand bergzand added State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet Type: new feature The issue requests / The PR implemements a new feature for RIOT Area: sys Area: System labels Sep 26, 2019
@bergzand bergzand requested a review from kaspar030 September 26, 2019 07:48
@bergzand
Copy link
Member Author

Some benchmarks on a samr21-xpro:

Newlib-Nano PicoLibc
bench_msg_pingpong 56336 56736
bench_thread_flags_pingpong 61854 62743
bench_thread_yield_pingpong 86328 87589
bench_mutex_pingpong 61854 62336
bench_sched_nop 183902 185322

bench_runtime_coreapis

Picolibc:

Runtime of Selected Core API functions

                 nop loop:    104173us  ---   0.104us per call  ---    9599416 calls per sec

             mutex_init():         7us  ---   0.000us per call  ---  1123222089 calls per sec
        mutex lock/unlock:   2375007us  ---   2.375us per call  ---     421051 calls per sec

       thread_flags_set():   1458340us  ---   1.458us per call  ---     685711 calls per sec
     thread_flags_clear():   1541674us  ---   1.541us per call  ---     648645 calls per sec
thread flags set/wait any:   4333340us  ---   4.333us per call  ---     230768 calls per sec
thread flags set/wait all:   3791674us  ---   3.791us per call  ---     263735 calls per sec
thread flags set/wait one:   4354173us  ---   4.354us per call  ---     229664 calls per sec

        msg_try_receive():   2250007us  ---   2.250us per call  ---     444443 calls per sec
              msg_avail():    520840us  ---   0.520us per call  ---    1919975 calls per sec

Newlib-Nano:

Runtime of Selected Core API functions

                 nop loop:    104173us  ---   0.104us per call  ---    9599416 calls per sec

             mutex_init():         7us  ---   0.000us per call  ---  1123222089 calls per sec
        mutex lock/unlock:   2520840us  ---   2.520us per call  ---     396693 calls per sec

       thread_flags_set():   1520840us  ---   1.520us per call  ---     657531 calls per sec
     thread_flags_clear():   1562507us  ---   1.562us per call  ---     639997 calls per sec
thread flags set/wait any:   4437507us  ---   4.437us per call  ---     225351 calls per sec
thread flags set/wait all:   3895840us  ---   3.895us per call  ---     256684 calls per sec
thread flags set/wait one:   4479174us  ---   4.479us per call  ---     223255 calls per sec

        msg_try_receive():   2312507us  ---   2.312us per call  ---     432431 calls per sec
              msg_avail():    520840us  ---   0.520us per call  ---    1919975 calls per sec

@benpicco
Copy link
Contributor

benpicco commented Feb 4, 2020

Would it be possible to include picolibc in https://github.com/RIOT-OS/toolchains ?

@kaspar030
Copy link
Contributor

Would it be possible to include picolibc in https://github.com/RIOT-OS/toolchains ?

Definitely, if that needs special toolchain support!

@benpicco
Copy link
Contributor

benpicco commented Jul 1, 2020

Looks like we'll also need thread-local storage - zephyrproject-rtos/zephyr#26545

@keith-packard
Copy link
Contributor

Looks like we'll also need thread-local storage - zephyrproject-rtos/zephyr#26545

You can build picolibc without TLS, but that means all libc calls with persistent state will share that across threads, including errno. Note that picolibc can only support TLS if the C compiler it was built with supports TLS, so make sure TLS is enabled in your compiler build (it used to be disabled in a bunch of toolchain releases for ARM).

@keith-packard
Copy link
Contributor

Looks like we'll also need thread-local storage - zephyrproject-rtos/zephyr#26545

I've got something working here:

https://github.com/keith-packard/RIOT/tree/picolibc_initial

This is based off this PR with some additional fixes added.

@bergzand
Copy link
Member Author

bergzand commented Jul 3, 2020

Hi @keith-packard

Those commits are exactly what we need here. Do you mind if I pull them into this PR, of course keeping your commit authorship for those?

@keith-packard
Copy link
Contributor

Hi @keith-packard

Those commits are exactly what we need here. Do you mind if I pull them into this PR, of course keeping your commit authorship for those?

That would be awesome.

@bergzand
Copy link
Member Author

bergzand commented Jul 6, 2020

Now includes the commits from @keith-packard

@benpicco
Copy link
Contributor

benpicco commented Jul 6, 2020

Now this needs a rebase.

@bergzand bergzand force-pushed the pr/libc/picolibc_initial branch from a5c765b to 77d6c83 Compare July 6, 2020 12:15
@bergzand
Copy link
Member Author

bergzand commented Jul 6, 2020

Now this needs a rebase.

Rebased and updated

Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

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

I gave it a quick test on samr21-xpro and for the gnrc_networking it sheds ~7k off .text compared to newlib 😃

Since it requires some adaption to the platform code, this could be modeled as a FEATURE until all platforms have support for it.

picolibc_syscalls_default/syscalls.c and newlib_syscalls_default/syscalls.c can probably share some code, e.g. no need to implement _sbrk_r twice.

This would also give us the VFS integration.

return 1;
}
FILE picolibc_stdio =
FDEV_SETUP_STREAM(picolibc_put, picolibc_get, NULL, _FDEV_SETUP_RW);
Copy link
Contributor

Choose a reason for hiding this comment

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

Will there be a flush function in case of CDC ACM stdio?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, what's the API for that? I didn't see one in the newlib code.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I reviewed the CDC ACM implementation and that doesn't have any buffering internally; each call to stdio_write causes a separate USB transaction. So, the current code will 'work', although output will be slow. I've added a buffer for output, and also a pile of additional picolibc bits at

https://github.com/keith-packard/RIOT/tree/pr/libc/picolibc_initial

Copy link
Contributor

Choose a reason for hiding this comment

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

Want to push that pile of additional picolibc bits again? :D
Or do you want to pull them @bergzand ?

I kind of missed this comment here, I see you hooked up VFS too now.
For the buffering, I wonder if CDC ACM should handle this, but that can be changed later.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've rebased my changes on top of @bergzand's current branch and pushed them to my branch again.

https://github.com/keith-packard/RIOT/tree/pr/libc/picolibc_initial

I also added a config option so that by default the integer-only printf/scanf code is use, but the full float code can be selected.

Copy link
Contributor

Choose a reason for hiding this comment

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

As for the CDC ACM buffering, it would be nice to include that in the underlying implementation, but that would require exposing a flush function that all users called as needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've rebased my changes on top of @bergzand's current branch and pushed them to my branch again.

And pulled them in.

Copy link
Contributor

Choose a reason for hiding this comment

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

I note that you left out the stdout buffering code. That makes USB performance pretty poor; it might be nice to add that back in and plan on removing it if/when the underlying ACM code adds buffering. I'm not really sure it should though -- you'd have to modify all users of that code to know about the buffering so that they could flush when needed.

Copy link
Contributor

@aabadie aabadie left a comment

Choose a reason for hiding this comment

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

Some small comments regarding the build system.

The integration in the build system can certainly be improved. Using the FEATURES is appealing but that is not meant for that normally (it related to features are provided by the hardware). One could argue that cpp support is using this mechanism though.
For the moment, we should keep it simple.

@bergzand bergzand changed the title [WIP]: PicoLibc support as alternative to Newlib(-nano) PicoLibc support as alternative to Newlib(-nano) Jul 7, 2020
@bergzand bergzand removed the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Jul 7, 2020
@bergzand
Copy link
Member Author

bergzand commented Jul 7, 2020

Compiling and testing on the nrf52dk these are resulting failures:

failures
ERROR:nrf52dk:Tests failed: 58                                                           
Failures during compilation:                                                             
- [examples/dtls-echo](examples/dtls-echo/compilation.failed)                            
- [examples/dtls-sock](examples/dtls-sock/compilation.failed)                            
- [examples/dtls-wolfssl](examples/dtls-wolfssl/compilation.failed)                      
- [examples/filesystem](examples/filesystem/compilation.failed)                          
- [examples/javascript](examples/javascript/compilation.failed)                          
- [examples/lua_REPL](examples/lua_REPL/compilation.failed)                              
- [examples/lua_basic](examples/lua_basic/compilation.failed)                            
- [examples/micropython](examples/micropython/compilation.failed)                                                                                                                  
- [examples/riot_and_cpp](examples/riot_and_cpp/compilation.failed)                      
- [tests/cpp11_condition_variable](tests/cpp11_condition_variable/compilation.failed)                                                                                              
- [tests/cpp11_mutex](tests/cpp11_mutex/compilation.failed)                                                                                                                        
- [tests/cpp11_thread](tests/cpp11_thread/compilation.failed)                            
- [tests/cpp_ctors](tests/cpp_ctors/compilation.failed)                                                                                                                            
- [tests/cpp_exclude](tests/cpp_exclude/compilation.failed)                                                                                                                        
- [tests/cpp_ext](tests/cpp_ext/compilation.failed)                                      
- [tests/mcuboot](tests/mcuboot/compilation.failed)                                      
- [tests/pkg_emlearn](tests/pkg_emlearn/compilation.failed)                                                                                                                        
- [tests/pkg_flatbuffers](tests/pkg_flatbuffers/compilation.failed)                      
- [tests/pkg_libcose](tests/pkg_libcose/compilation.failed)                              
- [tests/pkg_nanopb](tests/pkg_nanopb/compilation.failed)                                
- [tests/pkg_relic](tests/pkg_relic/compilation.failed)                                  
- [tests/pkg_tensorflow-lite](tests/pkg_tensorflow-lite/compilation.failed)              
- [tests/pkg_tinycbor](tests/pkg_tinycbor/compilation.failed)                            
- [tests/pkg_tinydtls_sock_async](tests/pkg_tinydtls_sock_async/compilation.failed)      
- [tests/pkg_ubasic](tests/pkg_ubasic/compilation.failed)                                
- [tests/pkg_utensor](tests/pkg_utensor/compilation.failed)                              
- [tests/pkg_wolfssl](tests/pkg_wolfssl/compilation.failed)                              
                                                                                         
Failures during test:                                                                                                                                                              
- [examples/suit_update](examples/suit_update/test.failed)             
- [tests/emcute](tests/emcute/test.failed)                                                                                                                                         
- [tests/gnrc_dhcpv6_client](tests/gnrc_dhcpv6_client/test.failed)      
- [tests/gnrc_dhcpv6_client_6lbr](tests/gnrc_dhcpv6_client_6lbr/test.failed)             
- [tests/gnrc_ipv6_ext](tests/gnrc_ipv6_ext/test.failed)                                                                                                                           
- [tests/gnrc_ipv6_ext_frag](tests/gnrc_ipv6_ext_frag/test.failed)                                                                                                                 
- [tests/gnrc_ipv6_ext_opt](tests/gnrc_ipv6_ext_opt/test.failed)                                                                                                                   
- [tests/gnrc_ipv6_nib_dns](tests/gnrc_ipv6_nib_dns/test.failed)                                                                                                                   
- [tests/gnrc_rpl_srh](tests/gnrc_rpl_srh/test.failed)                                   
- [tests/gnrc_sock_dns](tests/gnrc_sock_dns/test.failed)                                 
- [tests/gnrc_tcp](tests/gnrc_tcp/test.failed)                                           
- [tests/malloc](tests/malloc/test.failed)                                               
- [tests/nordic_softdevice](tests/nordic_softdevice/test.failed)                         
- [tests/periph_timer_short_relative_set](tests/periph_timer_short_relative_set/test.failed)                                                                                       
- [tests/pkg_fatfs_vfs](tests/pkg_fatfs_vfs/test.failed)           
- [tests/pkg_hacl](tests/pkg_hacl/test.failed)                                           
- [tests/pkg_libfixmath_unittests](tests/pkg_libfixmath_unittests/test.failed)
- [tests/pkg_littlefs](tests/pkg_littlefs/test.failed)                                   
- [tests/pkg_littlefs2](tests/pkg_littlefs2/test.failed)           
- [tests/pkg_semtech-loramac](tests/pkg_semtech-loramac/test.failed)       
- [tests/pkg_tiny-asn1](tests/pkg_tiny-asn1/test.failed)                                 
- [tests/pthread](tests/pthread/test.failed)                 
- [tests/pthread_barrier](tests/pthread_barrier/test.failed)                             
- [tests/pthread_cleanup](tests/pthread_cleanup/test.failed)
- [tests/pthread_cooperation](tests/pthread_cooperation/test.failed)                     
- [tests/pthread_flood](tests/pthread_flood/test.failed)                                 
- [tests/pthread_tls](tests/pthread_tls/test.failed)                      
- [tests/riotboot](tests/riotboot/test.failed)                                           
- [tests/shell](tests/shell/test.failed)                                                 
- [tests/struct_tm_utility](tests/struct_tm_utility/test.failed)                         
- [tests/xtimer_now_irq](tests/xtimer_now_irq/test.failed)       

Most of the compile-time failures are related to C++ issues, ::vsscanf being undeclared is the first issue I found so far.

@bergzand bergzand force-pushed the pr/libc/picolibc_initial branch from 02926c8 to ad2620f Compare July 29, 2020 09:10
@bergzand
Copy link
Member Author

pls rebase!

Rebased!

@bergzand
Copy link
Member Author

With GCC there is still an issue with CPP code:

koen@morgen ~/dev/RIOT-picolibc $ PICOLIBC=1 make -C tests/cpp_ext -j BOARD=samr21-xpro
make: Entering directory '/home/koen/dev/RIOT-picolibc/tests/cpp_ext'
Building application "tests_cpp_ext" for "samr21-xpro" with MCU "samd21".

"make" -C /home/koen/dev/RIOT-picolibc/boards/samr21-xpro
"make" -C /home/koen/dev/RIOT-picolibc/core
"make" -C /home/koen/dev/RIOT-picolibc/cpu/samd21
"make" -C /home/koen/dev/RIOT-picolibc/drivers
"make" -C /home/koen/dev/RIOT-picolibc/sys
"make" -C /home/koen/dev/RIOT-picolibc/tests/cpp_ext/module
"make" -C /home/koen/dev/RIOT-picolibc/drivers/periph_common
"make" -C /home/koen/dev/RIOT-picolibc/sys/auto_init
"make" -C /home/koen/dev/RIOT-picolibc/sys/isrpipe
"make" -C /home/koen/dev/RIOT-picolibc/sys/picolibc_syscalls_default
"make" -C /home/koen/dev/RIOT-picolibc/sys/pm_layered
"make" -C /home/koen/dev/RIOT-picolibc/sys/stdio_uart
"make" -C /home/koen/dev/RIOT-picolibc/sys/test_utils/interactive_sync
"make" -C /home/koen/dev/RIOT-picolibc/cpu/cortexm_common
"make" -C /home/koen/dev/RIOT-picolibc/cpu/sam0_common
"make" -C /home/koen/dev/RIOT-picolibc/cpu/samd21/periph
"make" -C /home/koen/dev/RIOT-picolibc/sys/tsrb
"make" -C /home/koen/dev/RIOT-picolibc/cpu/sam0_common/periph
"make" -C /home/koen/dev/RIOT-picolibc/cpu/cortexm_common/periph
/opt/gcc-arm-none-eabi-9-2019-q4-major/bin/../lib/gcc/arm-none-eabi/9.2.1/../../../../arm-none-eabi/bin/ld: /opt/gcc-arm-none-eabi-9-2019-q4-major/bin/../lib/gcc/arm-none-eabi/9.2.1/../../../../arm-none-eabi/lib/thumb/v6-m/nofp/libstdc++.a(vterminate.o): in function `__gnu_cxx::__verbose_terminate_handler()':
vterminate.cc:(.text._ZN9__gnu_cxx27__verbose_terminate_handlerEv+0xfc): undefined reference to `_impure_ptr'
collect2: error: ld returned 1 exit status
make: *** [/home/koen/dev/RIOT-picolibc/tests/cpp_ext/../../Makefile.include:574: /home/koen/dev/RIOT-picolibc/tests/cpp_ext/bin/samr21-xpro/tests_cpp_ext.elf] Error 1
make: Leaving directory '/home/koen/dev/RIOT-picolibc/tests/cpp_ext'

@keith-packard
Copy link
Contributor

With GCC there is still an issue with CPP code:

vterminate.cc:(.text._ZN9__gnu_cxx27__verbose_terminate_handlerEv+0xfc): undefined reference to `_impure_ptr'

That's because your libstdc++ is built against newlib instead of picolibc and ends up referencing symbols found in newlib but not in picolibc. I've spent a bit of time adjusting the libstdc++ code that comes with gcc to make it work with picolibc, but that will still require building it correctly and using the right library. The whole libstdc++ issue is a mess -- it should not be included with GCC as that creates circular build dependencies.

@benpicco
Copy link
Contributor

So when using picolib, we could still provide the cpp feature, but not the libstdcpp one. (#14503)
I'd say that's good enough.

@keith-packard
Copy link
Contributor

So when using picolib, we could still provide the cpp feature, but not the libstdcpp one. (#14503)
I'd say that's good enough.

Sounds about right. I've got patches for GCC which build libstdc++ against picolibc; perhaps I should start building the Debian embedded toolchains to use those, which would enable libstdc++ with picolibc.

Signal that this function will never return

Signed-off-by: Keith Packard <keithp@keithp.com>
This provides the POSIX api used by picolibc stdio

Signed-off-by: Keith Packard <keithp@keithp.com>
In most places, picolibc and newlib are the same, so use
the existing newlib code when compiling with picolibc.

Signed-off-by: Keith Packard <keithp@keithp.com>
This makes RIOT use the integer-only printf/scanf code by default and
includes a new make parameter to select the full floating point
version. This saves about 6kB of text space when building hello-world
for the microbit board.

Signed-off-by: Keith Packard <keithp@keithp.com>
Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

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

Now with the last changes this looks like a pretty simple addition!
As for the buffering, I think we can do that as a separate PR.
Right now no applications will call flush as it's a no-op.

The sooner we can get this in, the sooner we will be able to iron out such kinks.

Some numbers for examples/gnrc_networking:

master

   text	   data	    bss	    dec	    hex	filename
  97352	    184	  19236	 116772	  1c824	/home/benpicco/dev/RIOT/examples/gnrc_networking/bin/samr21-xpro/gnrc_networking.elf

PICOLIBC=1

   text	   data	    bss	    dec	    hex	filename
  90064	    464	  19228	 109756	  1acbc	/home/benpicco/dev/RIOT/examples/gnrc_networking/bin/samr21-xpro/gnrc_networking.elf

PICOLIBC=1 LTO=1

   text	   data	    bss	    dec	    hex	filename
  83864	    464	  19224	 103552	  19480	/home/benpicco/dev/RIOT/examples/gnrc_networking/bin/samr21-xpro/gnrc_networking.elf

Needs a rebase btw 😉

* @return 0 on success
* @return -1 on error, @c errno set to a constant from errno.h to indicate the error
*/
int fcntl (int fd, int cmd, int arg)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
int fcntl (int fd, int cmd, int arg)
int fcntl(int fd, int cmd, int arg)

} > rom
__tls_size = __tbss_end - __tdata_start;
__tbss_size = __tls_size - __tdata_size;

Copy link
Contributor

Choose a reason for hiding this comment

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

lpc23xx.ld will need that block too, but I can add in in a follow-up PR if you don't have the hardware to test.

Copy link
Contributor

Choose a reason for hiding this comment

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

lpc23xx.ld will need that block too, but I can add in in a follow-up PR if you don't have the hardware to test.

Sounds like a plan -- I've tested the series I've posted, you can create another PR with the lpc23xx.ld fixes after testing those :-)

@keith-packard
Copy link
Contributor

keith-packard commented Aug 21, 2020

I've rebased and then also squashed these patches so that the series looks a little cleaner. I've filed #14827 with that branch.

@keith-packard keith-packard mentioned this pull request Aug 21, 2020
@bergzand
Copy link
Member Author

Closing this one

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: sys Area: System Type: new feature The issue requests / The PR implemements a new feature for RIOT

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants