Pr/libc/picolibc by keith-packard · Pull Request #14827 · RIOT-OS/RIOT

keith-packard · 2020-08-21T23:39:37Z

This is the same code as on my pr/libc/picolibc-initial branch referenced in PR #12305 but with the patches squashed together to look a little cleaner.

benpicco

Travis spotted two instances where tabs were used instead of spaces 😉

I tested this with examples/filesystem on weact-f411ce (so CDC ACM is used for stdio) and it works quite well.

Thank you for the contribution and creating picolibc 😃

sys/picolibc_syscalls_default/syscalls.c

keith-packard · 2020-08-23T00:21:45Z

Travis spotted two instances where tabs were used instead of spaces wink

Fixed! Would it be reasonable to add an 'EditorConfig' file to the source tree?

benpicco · 2020-08-23T11:16:29Z

Would it be reasonable to add an 'EditorConfig' file to the source tree?

Sounds like a good idea, I didn't know about that standard.

benpicco

CI is happy now and the new code looks good, does pretty much what the newlibc code did for the most part.

With this and LTO we can get RIOT smaller than ever :)

benpicco · 2020-08-23T11:36:06Z

makefiles/libc/picolibc.mk

+  LINKFLAGS += -Wl,--defsym=__heap_end=_eheap
+  LINKFLAGS += -Wl,--defsym=__heap_start=_sheap


This will only work for one continuous region of memory.
Can we simply implement _sbrk_r ourself like we did for newlib to allow for multiple non-continuous memory regions?

(tests/malloc still works with this, but only the first memory region is used)

Yes, you can. I've modified the smaller malloc implementation so that it should work with dis-continuous sbrk regions, but I have not tested that. The larger malloc implementation, and both versions in newlib have bugs in this area...

With this and LTO we can get RIOT smaller than ever :)

Would be fun to LTO the whole C library and see what that managed to do.

aabadie · 2020-08-23T18:39:45Z

sys/Makefile.include

  endif
 endif

+ifeq (,$(filter printf_float,$(USEMODULE)))


Both blocks could be factorized in a single one I think:

Suggested change

ifeq (,$(filter printf_float,$(USEMODULE)))

ifeq (,$(filter printf_float scanf_float,$(USEMODULE)))

Since the CFLAGS is the same for both printf and scanf.

This code wasn't working because there was code in makefiles/libc/picolibc.mk that was overriding it. I've removed the code from sys/Makefile.include and fixed the code in makefiles/libc/picolibc.mk to check USEMODULE instead of using a separate configuration variable.

aabadie · 2020-08-23T18:41:48Z

Did you try on RISC-V ? Last time I tried #12305 on the sifive hifive1b it was not working. I can give this one another try tomorrow.

picolibc: Use thread_getpid for getpid() in picolibc_syscalls_default Instead of directly accessing the sched_active_pid variable (which isn't defined in this context), use the existing wrapper function to get that value. Signed-off-by: Keith Packard <keithp@keithp.com> --- v2: Squash a couple of fixes in * fixup! picolibc: Use thread_getpid for getpid() in picolibc_syscalls_default * squashme: Add `times` to picolibc syscalls * Add __noreturn__ attribute to _exit * Add VFS syscall wrappers. This provides the POSIX api used by picolibc stdio

keith-packard · 2020-08-23T19:46:58Z

Did you try on RISC-V ? Last time I tried #12305 on the sifive hifive1b it was not working. I can give this one another try tomorrow.

Oops, somehow I missed a chunk of the riscv-v linker script. I've fixed that, run 'hello-world' on my hifive1-revb and pushed an updated PR.

I've also added a call to stdio_init from cpu/fe310/cpu.c in the PICOLIBC case so that the uart gets initialized. I've tested 'hello-world' again and it still works (the previous test had hacked up hello-world to call stdio_init manually).

Support for picolibc as alternative libc implementation is added with this commit. For now only cortex-m CPU's are supported. Enable via PICOLIBC=1 --- v2: squash fixes in v3: Remove picolibc integer printf/scanf stuff from sys/Makefile.include, it gets set in makefiles/libc/picolibc.mk fixup for dependency

Picolibc makes atexit state per-thread instead of global, so we can't register destructors with atexit in a non-thread context as we won't have any TLS space initialized. Signed-off-by: Keith Packard <keithp@keithp.com>

aabadie · 2020-08-24T06:39:13Z

somehow I missed a chunk of the riscv-v linker script. I've fixed that, run 'hello-world' on my hifive1-revb and pushed an updated PR.

I confirm this is now working, with 3.6kB less ROM used (from 8.6kB without picolibc to 5kB).

benpicco · 2020-08-24T10:02:24Z

Looks like CI is complaining about the commit message picolibc: Set USE_MODULE += printf_float to enable float printf/scanf code [v2] being longer than 72 characters.
The rest are just warnings produced by existing code…

cpu/fe310/cpu.c

Disable the newlib-nano stubs code when picolibc is in use Signed-off-by: Keith Packard <keithp@keithp.com> --- v2: Squash fixes in v3: call stdio_init in _PICOLIBC_ mode to initialize uart v3: Remove call to stdio_init from nanostubs_init, always call from cpu_init.

Allocate and initialize a thread-local block for each thread at the top of the stack. Set the tls base when switching to a new thread. Add tdata/tbss linker instructions to cortex_m and risc-v scripts. Signed-off-by: Keith Packard <keithp@keithp.com> --- v2: Squash fixes v3: Replace tabs with spaces v4: Add tbss to fe310 linker script

--- v2: Squash fixes in

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> ---- v2: Use USEMODULE=printf_float instead of separate parameter

Add buffering so that USB back-ends get some batching for stdout Signed-off-by: Keith Packard <keithp@keithp.com> ---- v2: Replace tabs with spaces.

keith-packard · 2020-08-24T15:28:53Z

Looks like CI is complaining about the commit message picolibc: Set USE_MODULE += printf_float to enable float printf/scanf code [v2] being longer than 72 characters.

Oops. Adding [v2] went over the limit. Fixed now.

benpicco · 2020-08-24T16:07:30Z

And Murdock is happy - thanks a lot!

bergzand · 2020-08-24T17:14:56Z

Thanks @keith-packard!

benpicco · 2020-08-24T17:42:44Z

Hm sorry for the sloppy testing, when I now compile an application that uses write (e.g. examples/gnrc_networking) I will get

/usr/lib/gcc/arm-none-eabi/9.2.1/../../../arm-none-eabi/bin/ld: /home/benpicco/dev/RIOT/examples/gnrc_networking/bin/samr21-xpro/fmt/fmt.o: in function `print':
/home/benpicco/dev/RIOT/sys/fmt/fmt.c:511: undefined reference to `write'
collect2: error: ld returned 1 exit status
make: *** [/home/benpicco/dev/RIOT/examples/gnrc_networking/../../Makefile.include:587: /home/benpicco/dev/RIOT/examples/gnrc_networking/bin/samr21-xpro/gnrc_networking.elf] Error 1
make BOARD=samr21-xpro PICOLIBC=1 -j  43,76s user 4,20s system 323% cpu 14,828 total

This worked yesterday, but on a different machine.
Here I just installed the latest picolibc-arm-none-eabi Debian package.

When I check out an old version of the pr/libc/picolibc_initial branch I have locally, I can link it though.

keith-packard · 2020-08-24T17:59:16Z

Hm sorry for the sloppy testing, when I now compile an application that uses write (e.g. examples/gnrc_networking) I will get
/usr/lib/gcc/arm-none-eabi/9.2.1/../../../arm-none-eabi/bin/ld: /home/benpicco/dev/RIOT/examples/gnrc_networking/bin/samr21-xpro/fmt/fmt.o: in function `print':
/home/benpicco/dev/RIOT/sys/fmt/fmt.c:511: undefined reference to `write'
collect2: error: ld returned 1 exit status
make: *** [/home/benpicco/dev/RIOT/examples/gnrc_networking/../../Makefile.include:587: /home/benpicco/dev/RIOT/examples/gnrc_networking/bin/samr21-xpro/gnrc_networking.elf] Error 1
make BOARD=samr21-xpro PICOLIBC=1 -j  43,76s user 4,20s system 323% cpu 14,828 total
This worked yesterday, but on a different machine.
Here I just installed the latest picolibc-arm-none-eabi Debian package.

When I check out an old version of the pr/libc/picolibc_initial branch I have locally, I can link it though.

picolibc doesn't provide or require this API for stdin/stdout/stderr, so I didn't leave it in the merge request. If you add the VFS layer, you'll get it, and it will rely on VFS operations. stdin/stdout/stderr will not be connected through that by default though; they'll still be connected to the console.

Does the VFS layer reserve FDs 0-2 to avoid stepping on the usual FDs for stdin/stdout/stderr? If so, changing the VFS implementation for picolibc to redirect those through the console would make things work like newlib?

benpicco · 2020-08-24T18:06:06Z

picolibc doesn't provide or require this API for stdin/stdout/stderr

fmt.c already has a work-around in place for this, I just had to enable it for picolibc
Which raises two questions:

how did this work before?
why not also use fwrite with newlib?

Does the VFS layer reserve FDs 0-2 to avoid stepping on the usual FDs for stdin/stdout/stderr?

No and it doesn't seem to consider fds to be anything other than indices to open files.
Which also raises the question how fmt.c would work with with the vfs module.

keith-packard · 2020-08-24T19:32:35Z

picolibc doesn't provide or require this API for stdin/stdout/stderr

fmt.c already has a work-around in place for this, I just had to enable it for picolibc
Which raises two questions:
* how did this work before?

I didn't test anything that used fmt. I think it would be reasonable to consider deprecating this code in favor of just using printf though; picolibc's printf is reasonably small, even if you want 'exact' floating point conversions ('exact' means you can print and then re-scan a string and get the same bits back).

* why not also use `fwrite` with newlib?

If the underlying system offers a POSIX api, then using it directly will avoid linking in a bunch of stdio code, which saves space and is probably more efficient. For picolibc and avrlibc (which share stdio origins btw), the obvious fix seems like a good idea.

No and it doesn't seem to consider fds to be anything other than indices to open files.
Which also raises the question how fmt.c would work with with the vfs module.

So the newlib POSIX apis may be conflicting with the VFS code? Would probably be good to sort that all out.

keith-packard requested review from DipSwitch, MichelRottleuthner, aabadie, fjmolinas, haukepetersen, kaspar030, leandrolanzieri, maribu, miri64, smlng and thomaseichinger as code owners August 21, 2020 23:39

keith-packard mentioned this pull request Aug 21, 2020

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

Closed

3 tasks

benpicco added Area: sys Area: System Type: new feature The issue requests / The PR implemements a new feature for RIOT labels Aug 21, 2020

benpicco requested a review from bergzand August 21, 2020 23:42

benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Aug 21, 2020

benpicco reviewed Aug 22, 2020

View reviewed changes

sys/picolibc_syscalls_default/syscalls.c Outdated Show resolved Hide resolved

sys/picolibc_syscalls_default/syscalls.c Outdated Show resolved Hide resolved

keith-packard force-pushed the pr/libc/picolibc branch from 4279290 to 76aae2b Compare August 23, 2020 00:20

benpicco approved these changes Aug 23, 2020

View reviewed changes

benpicco reviewed Aug 23, 2020

View reviewed changes

benpicco mentioned this pull request Aug 23, 2020

cpu/lpc23xx: enable support for picolibc #14830

Merged

aabadie reviewed Aug 23, 2020

View reviewed changes

keith-packard force-pushed the pr/libc/picolibc branch from 76aae2b to faac559 Compare August 23, 2020 19:45

bergzand and others added 2 commits August 23, 2020 13:12

cpu/fe310: Don't register __libc_fini_array with atexit

76f6362

Picolibc makes atexit state per-thread instead of global, so we can't register destructors with atexit in a non-thread context as we won't have any TLS space initialized. Signed-off-by: Keith Packard <keithp@keithp.com>

keith-packard force-pushed the pr/libc/picolibc branch 3 times, most recently from 729714d to 64b04c3 Compare August 23, 2020 20:48

benpicco reviewed Aug 24, 2020

View reviewed changes

cpu/fe310/cpu.c Show resolved Hide resolved

keith-packard and others added 8 commits August 24, 2020 08:24

posix/select: Add picolibc to cyclic dependency resolution

26c1920

picolibc: glue sbrk to heap start/end [v2]

79d733a

--- v2: Squash fixes in

docker: Add picolibc build flag

c507927

picolibc: Use most NEWLIB code with picolibc

e215261

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>

picolibc: Add stdout buffering [v2]

fda68c4

Add buffering so that USB back-ends get some batching for stdout Signed-off-by: Keith Packard <keithp@keithp.com> ---- v2: Replace tabs with spaces.

keith-packard force-pushed the pr/libc/picolibc branch from 64b04c3 to fda68c4 Compare August 24, 2020 15:27

benpicco merged commit f3e1032 into RIOT-OS:master Aug 24, 2020

benpicco mentioned this pull request Aug 24, 2020

sys/fmt: use fwrite with picolibc #14841

Closed

		LINKFLAGS += -Wl,--defsym=__heap_end=_eheap
		LINKFLAGS += -Wl,--defsym=__heap_start=_sheap

	ifeq (,$(filter printf_float,$(USEMODULE)))
	ifeq (,$(filter printf_float scanf_float,$(USEMODULE)))

Conversation

keith-packard commented Aug 21, 2020

Uh oh!

benpicco left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

keith-packard commented Aug 23, 2020

Uh oh!

benpicco commented Aug 23, 2020

Uh oh!

benpicco left a comment

Choose a reason for hiding this comment

Uh oh!

benpicco Aug 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keith-packard Aug 23, 2020

Choose a reason for hiding this comment

Uh oh!

aabadie Aug 23, 2020

Choose a reason for hiding this comment

Uh oh!

keith-packard Aug 23, 2020

Choose a reason for hiding this comment

Uh oh!

aabadie commented Aug 23, 2020

Uh oh!

keith-packard commented Aug 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aabadie commented Aug 24, 2020

Uh oh!

benpicco commented Aug 24, 2020

Uh oh!

Uh oh!

keith-packard commented Aug 24, 2020

Uh oh!

benpicco commented Aug 24, 2020

Uh oh!

bergzand commented Aug 24, 2020

Uh oh!

benpicco commented Aug 24, 2020

Uh oh!

keith-packard commented Aug 24, 2020

Uh oh!

benpicco commented Aug 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

keith-packard commented Aug 24, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

benpicco Aug 23, 2020 •

edited

Loading

keith-packard commented Aug 23, 2020 •

edited

Loading

benpicco commented Aug 24, 2020 •

edited

Loading