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

[PoC, RFC] Don't use <stdio.h> in RIOT #13710

Closed
wants to merge 15 commits into from
Closed

Conversation

maribu
Copy link
Member

@maribu maribu commented Mar 25, 2020

Disclaimer

This PR contains a bunch of commits that would be better split into separate PR. But this PR is not indented to be merged nor to be reviewed in-depth, but rather to demonstrate the feasibility of an idea. If the general idea is accepted to be worth pursuing, this PR will be split up into multiple PRs to allow efficient reviewing.

BEWARE: This is almost completely untested.

Contribution description

This PR replaces the use of <stdio.h> within some portions of RIOT with sys/fmt:

  • Internal RIOT stuff uses the print_...() family of functions directly
  • DEBUG() and LOG_DEBUG()/LOG_INFO()/... cannot simply be replaced, as the API is closely coupled to printf()
    • A printf_nano() function is added as module fmt_stdio. This function implements a small subset of the printf() format specifiers using the print_... functions.
    • This printf_nano() should be enough for our debugging and logging requirements and is used by default for DEBUG() and LOG_...() by default

Why?

Code Size

With master:

make BOARD=bluepill BUILD_IN_DOCKER=1 -C examples/default/
[...]
   text	   data	    bss	    dec	    hex	filename
  18292	    132	   2672	  21096	   5268	/data/riotbuild/riotbase/examples/default/bin/bluepill/default.elf

With this PR:

make BOARD=bluepill BUILD_IN_DOCKER=1 -C examples/default/
[...]
   text	   data	    bss	    dec	    hex	filename
  13924	    128	   2652	  16704	   4140	/data/riotbuild/riotbase/examples/default/bin/bluepill/default.elf

Note: There is likely still some room for further improvement. This PR is just a quickly written proof of concept.

Stack Requirements

The newlibs implementation of <stdio.h> uses significant amounts of stack.

  • This resulted in a stack check added to DEBUG() to prevent stack overflows. With a lightweight alternative like the printf_nano() provided here should often without having to increase the stack size of driver threads or at least with increasing the stack only a little
  • In crash handler for ARM Cortex M boards printf() is used, which results in significant stack allocation for that handler. Using print_...() provided by fmt.h should result reducing that stack. (BUT: This has not been done in this PR, as the actual stack requirements need to be evaluated with care, as different compilers might produce binaries with different stack requirements.)

Huge Complexity of the API in <stdio.h>

  • There have been numerous bugs in various stdio implementations. Most of those are due to the inherent complexity.
  • There have been even more bugs in applications using <stdio.h>
  • There are various aspects that are either unspecified or implementation defined behavior, opening up a lot of pitfalls to fall in

MISRA C Compliance

Testing procedure

Doesn't apply (yet). Detailed testing descriptions will be provided for the individual PRs split out of this, if the general approach is agreed upon.

Issues/PRs references

- Added functions to fill columns with arbitrary chars
- Added functions to fill on the right side (left-aligned columns)
- Added functions to print with leading zeros
- Reduces ROM requirements by ~7K for newlib (unless printf() is used by other
  stuff)
- Works fine for threads with smalls stacks
- Reduces ROM requirements (when stdio is not pulled in by other stuff) by
  about 7K for newlib
- Works fine even for threads with tiny stacks
@maribu maribu added Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR State: demonstrator State: This PR (loosely) demonstrates a concept and is not necessarily meant to be merged. labels Mar 25, 2020
@benpicco
Copy link
Contributor

I like printf_nano (reminds me of tinyprintf) - but can't we just use that instead of manually calling print_u32_dec() etc?
I'd imagine the impact to be minor (might even be smaller due to less function calls)

@maribu
Copy link
Member Author

maribu commented Mar 25, 2020

I like printf_nano (reminds me of tinyprintf) - but can't we just use that instead of manually calling print_u32_dec() etc?

To some degree that would be possible. But it does not support stuff like %-30s. Thus, printing formatted output like tables for the ps shell command can not be done with it. So at least advanced users are better of using "fmt.h" directly.

Btw: We could also use something like LINKFLAGS += -Wl,-wrap=printf and PROVIDE(__wrap_printf,printf_nano) to use it without touching C code.

@benpicco
Copy link
Contributor

I'd trade these 'luxury' printf features for smaller code any time 😉
How much are those used anyway?

@kaspar030
Copy link
Contributor

I have a printff implementation using fmt as backend, which is also a lot smaller and uses a lot less stack. In fact, I got a branch that adds a homegrown replacement C library. But comparing it's codesize with pico libc made me put it on hold. picolibs has a much smaller stdio implementation.

We should aim at switching to that first, then evaluate libc replacements against that.

@maribu
Copy link
Member Author

maribu commented Mar 25, 2020

We should aim at switching to that first, then evaluate libc replacements against that.

Please note that the idea proposed here is "getting rid of <stdio.h> as much as possible(*)", rather than "re-implementing (parts of) the standard C lib". And the reasons for doing so are not only reducing ROM/RAM requirements, but also that <stdio.h> simply provides only poorly designed APIs. (See reasons above.)

(*) That is everywhere in sys, core, drivers and cpu except for DEBUG() and LOG_...().

That said: If picolib's printf() indeed has better RAM/ROM efficiency, there would be no reason to provide printf_nano() for that C library; maybe except as a static inline function that just calls printf(). (In fact the AVR libc also has pretty efficient printf() implementation, so there might be no reason to have it there as well.)

@kaspar030
Copy link
Contributor

In fact the AVR libc also has pretty efficient printf() implementation, so there might be no reason to have it there as well.

I think picolibc is actually using printf from AVR libc...

@kaspar030
Copy link
Contributor

Please note that the idea proposed here is "getting rid of <stdio.h> as much as possible(*)", rather than "re-implementing (parts of) the standard C lib".

Ok. ;)

@maribu
Copy link
Member Author

maribu commented Mar 25, 2020

Hmm, maybe this idea would be something for discussion in the next VMA. I think it is somewhat radical - especially in the context that RIOT wants to provide a POSIX like look and feel with a high degree of POSIX compatibility. But during a discussion with potential users outside of the academic sphere it became obvious that full MISRA C compliance is simply not optional for many actors in the industry. And enabling a boarder user base might be just the argument required to sell such radical idea.

(Note: I don't want to indicate that enabling users to not use <stdio.h> in production code is the only thing missing for full MISRA C compliance. But the remaining things will be IMO much less controversial than this.)

@kfessel
Copy link
Contributor

kfessel commented Oct 28, 2020

many uses of the DEBUG would be simplified to a string and therefor not require a fmt parser, if it just adds a line number and file name by default.
like:
putc(FILE);
putc(LINE);
putc(errormessage);

this could further improve the situation if a second makro is defined that not prints the error message which would remove it from the binary.

@benpicco
Copy link
Contributor

With picolibc I don't know if there is much to gain still from using fmt.
In fact, I think using fmt makes it worse as a lot a apps will still use printf - then you end up with both fmt and printf in the binary.

@kfessel
Copy link
Contributor

kfessel commented Oct 28, 2020

printf might need a larger stack. Even if not the stack of a thread is enlarged indirectly by making it able to debug print using the newlibc printf.

@chrysn
Copy link
Member

chrysn commented May 10, 2021

This sounds like a worthy change, but also a big effort to do system-wide.

Is there a subset of this that we could get usable earlier and migrate where it matters most?

Point in case: #16448 (comment) runs into stack overflows in DEVHELP mode in the panic handler, and can easily be moved to puts-style prints rather than printf ones -- but starts them through LOG_ERROR. Would it make sense, for example, to string-only log functions for these calling sites, while retaining printf (with all its stack usage) for the general case / while waiting for the full solution?

@maribu
Copy link
Member Author

maribu commented May 10, 2021

Is there a subset of this that we could get usable earlier and migrate where it matters most?

Certainly. All those panic handlers would be good candidates.

But I think we need first come to the consensus that we really do not want to ban use of stdio from RIOT except for debugging.

I think there is a consensus that newlib's stdio is performing exceptionally poor in terms of both stack usage and ROM requirements (even the nano variant) - which is quite surprising considering its lack of features (e.g. no %lld / %llu). But there are two alternative solutions to that:

  1. Just use picolibc. (I personally believe picolibc replacing newlib as the de-facto standard for MCUs in a few years. Just the sane build system compared to the nightmare of newlib is reason enough, IMO.)
  2. Use -Wl.-wrap=printf and provide a decent printf implementation. E.g. tinyprintf or the printf of avrlibc / picolibc are readily available and could easily be hooked up.
    • In this PR I even hacked together a simple printf replacement based on fmt that performs way better than newlib's nano printf. If and only if fmt is used directly anyway, this is likely the most ROM efficient version. If fmt is only used by that printf, I believe it to be larger than tinyprintf and picolib's printf.

@chrysn
Copy link
Member

chrysn commented May 10, 2021 via email

@maribu
Copy link
Member Author

maribu commented May 10, 2021

And when was this ever about banning stdio in full?

Check the PR title and description ;-)

@chrysn
Copy link
Member

chrysn commented May 10, 2021

[PoC, RFC] Don't use <stdio.h> in RIOT

we need first come to the consensus that we really do not want to ban use of stdio from RIOT except for debugging.

(Emphasis mine.) I understood this combination to indicate a "The PoC shows we could go completely stdio-free but it's more an exercise to find the pain points" -- or was there just a stray "not" in #13710 (comment) ?

@MrKevinWeiss MrKevinWeiss added this to the Release 2021.07 milestone Jun 21, 2021
@MrKevinWeiss MrKevinWeiss removed this from the Release 2021.07 milestone Jul 15, 2021
@maribu maribu closed this Jun 2, 2022
@maribu maribu deleted the no-stdio branch June 2, 2022 19:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR State: demonstrator State: This PR (loosely) demonstrates a concept and is not necessarily meant to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants