Skip to content

build system: use thread-safe stdio #21438

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

maribu
Copy link
Member

@maribu maribu commented Apr 25, 2025

Contribution description

build system: introduce bug_% feature category

This introduces a new feature category to declare which bugs are present
in a given build that we cannot fix in RIOT, but need to work around.
These bugs may be silicon bugs, software bugs in ROM, software bugs in
binary blobs needed for a platform, or bugs in the toolchain.

These features behave the same way as arch_% features: Every provided
bug is always used, so that we can inspect FEATURES_USED to check
which bugs need to be worked around.

build system: use thread-safe stdio

This makes use of the new bug modeling to declare all platforms that
can use newlib and have no reentrancy hooks as affected by the
non-thread-safe stdio bug. (Which is every platform but ESP* and AVR,
the former because the reentrancy hooks are provided, the latter because
we do not and never will support newlib on them.)

Building on that, the mpaland-printf package is used when newlib is used
and the bug is present. This way we can rely on the stdio being
thread-safe on every platform and not causing random crashes at
run time.

sys/newlib: drop workaround for stdio

This drops a workaround that initialized newlib's reentrancy structure
on boot to reduce the chances of crashes when using the non-thread-safe
(unless reentrancy hooks are provided) stdio implementation of newlib.

Now that the newlib stdio implementation is only ever used if it is
thread-safe, we no longer need a workaround that reduces the chance
of crashes on concurrent use of stdio.

Testing procedure

Now the package mpaland-printf should be pulled in for all non-ESP boards when newlib is used. E.g.

for board in hifive1b same54-xpro native32 native64 arduino-mega2560 esp32-mh-et-live-minikit esp8266-esp-12x z1 nucleo-l011k4; do
    printf '\n%s\n===================\n' $board
    echo "Features:"
    make BOARD=$board --no-print-directory -C examples/basic/hello-world info-features-used | grep -E 'newlib|libc'
    echo "Packages:"
    make BOARD=$board --no-print-directory -C examples/basic/hello-world info-packages
done
hifive1b
===================
Features:
bug_newlib_broken_stdio
newlib
Packages:
mpaland-printf

same54-xpro
===================
Features:
bug_newlib_broken_stdio
newlib
Packages:
cmsis
mpaland-printf

native32
===================
Features:
Packages:

native64
===================
Features:
Packages:

arduino-mega2560
===================
Features:
Packages:

esp32-mh-et-live-minikit
===================
Features:
newlib
Packages:
esp32_sdk

esp8266-esp-12x
===================
Features:
newlib
Packages:
esp8266_sdk

z1
===================
Features:
bug_newlib_broken_stdio
Packages:
mpaland-printf

nucleo-l011k4
===================
Features:
bug_newlib_broken_stdio
picolibc
Packages:
cmsis

With this, the test in tests/sys/snprintf now should also pass for each and every board, except for ESP32 boards. The reason is that, since ESP* provides the reentrancy hooks, newlib's stdio is thread-safe on those. So we do not pull in the alternative printf. For ESP8266 boards newlib is configured to use the non-nano stdio, which passes the test. For ESP32 newlib-nano is used, which does not implement all format specifiers and, therefore, fails the test.

Issues/PRs references

Depends on and includes:

@github-actions github-actions bot added Platform: MSP Platform: This PR/issue effects MSP-based platforms Platform: ARM Platform: This PR/issue effects ARM-based platforms Area: build system Area: Build system Platform: RISC-V Platform: This PR/issue effects RISC-V-based platforms Area: cpu Area: CPU/MCU ports Area: sys Area: System labels Apr 25, 2025
@crasbe crasbe added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation labels Apr 25, 2025
@riot-ci
Copy link

riot-ci commented Apr 25, 2025

Murdock results

✔️ PASSED

bb76083 sys/newlib: drop workaround for stdio

Success Failures Total Runtime
10319 0 10320 12m:25s

Artifacts

@maribu maribu force-pushed the sys/newlib/fix-stdio branch from 9a524a2 to f058796 Compare April 25, 2025 11:09
@github-actions github-actions bot added the Area: pkg Area: External package ports label Apr 25, 2025
@crasbe crasbe added CI: full build disable CI build filter CI: no fast fail don't abort PR build after first error CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Apr 25, 2025
@crasbe
Copy link
Contributor

crasbe commented Apr 25, 2025

We should probably give this a full build without fast fail, otherwise you'll be chasing your tail for days 😅

@maribu maribu added State: waiting for other PR State: The PR requires another PR to be merged first and removed CI: full build disable CI build filter CI: no fast fail don't abort PR build after first error labels Apr 25, 2025
@maribu
Copy link
Member Author

maribu commented Apr 25, 2025

We should probably give this a full build without fast fail, otherwise you'll be chasing your tail for days 😅

No, actually the memory requirements will go down with this. Unless... both printf variants get linked in into the same app.

See #21439 for a fix of that.

@maribu maribu force-pushed the sys/newlib/fix-stdio branch from f058796 to 31f945b Compare April 25, 2025 11:24
@Enoch247
Copy link
Contributor

in one of your commit messages and in the description of this PR you have every misspelled.

This way we can rely on the stdio being thread-safe on evyer platform and not causing random crashes at run time.

@maribu maribu force-pushed the sys/newlib/fix-stdio branch 2 times, most recently from 473d6ca to 933acf3 Compare April 27, 2025 07:03
@github-actions github-actions bot removed the Area: pkg Area: External package ports label Apr 27, 2025
@maribu maribu removed the State: waiting for other PR State: The PR requires another PR to be merged first label Apr 27, 2025
@maribu maribu enabled auto-merge April 27, 2025 07:03
@maribu maribu added this pull request to the merge queue Apr 27, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 27, 2025
@maribu
Copy link
Member Author

maribu commented Apr 27, 2025

For MSP430 both .text and .data slightly increase with this. This is not expected and needs to be investigated first.

@maribu
Copy link
Member Author

maribu commented Apr 27, 2025

Ah, 64 bit math is expensive on MSP430. Disabling support for long long and .data stays the same and .text goes down when switching to mpaland-printf.

I was hoping that mpaland-printf would be smaller for all platforms while supporting all features. But maybe support for printing long long should be a module to not cause regressions for MSP430. It could still be a default module on 32 bit and 64 bit platforms, were support for long long is super cheap.

@crasbe
Copy link
Contributor

crasbe commented Apr 27, 2025

Btw: you can use the built-in function to compare Build Sizes: https://doc.riot-os.org/advanced-build-system-tricks.html#comparing-build-sizes

Someone (👀👀) recently fixed it :D

@crasbe
Copy link
Contributor

crasbe commented Apr 27, 2025

I didn't want to install the RISC-V and ESP32 toolchains as well, so this is just a smaller set.

cbuec@W11nMate:~/RIOTstuff/riot-mpaland/RIOT/examples/basic/hello-world$ OLDBIN=master NEWBIN=mpaland make info-buildsizes-diff
using BOARD="native64" as "native" on a 64-bit system
text    data    bss     dec     BOARD/BINDIRBASE

22      0       0       22      arduino-mega2560
5532    118     904     6554    master
5554    118     904     6576    mpaland

24      0       0       24      native64
27566   1080    59128   87774   master
27590   1080    59128   87798   mpaland

20      0       0       20      nucleo-l011k4
5996    24      1772    7792    master
6016    24      1772    7812    mpaland

-3188   -108    -8      -3304   same54-xpro
12632   116     2308    15056   master
9444    8       2300    11752   mpaland

@maribu
Copy link
Member Author

maribu commented Apr 27, 2025

I think there is missing a RIOT_CI_BUILD=1,as the longer version (which includes the branch name unless it is master) is probably the cause for the size diff on AVR. (avrlibc's stdio is thread safe naturally and better optimized for 8 bit platforms than mpaland-printf, so sticking with the native printf there makes sense and is done here.) Same with the nucleo-l011k, which only supports picolibc. And on picolibc the native printf is also excellent.

@crasbe
Copy link
Contributor

crasbe commented Apr 27, 2025

Yes, with RIOT_CI_BUILD=1 the sizes are identical for the other boards.

@maribu
Copy link
Member Author

maribu commented Apr 27, 2025

#21445 now makes 64 bit printing support optional. It makes that support opt-in for MSP430 (due it being expensive there), and opt-out for 32-bit systems (due to it being cheap there).

maribu and others added 3 commits April 28, 2025 07:46
This introduces a new feature category to declare which bugs are present
in a given build that we cannot fix in RIOT, but need to work around.
These bugs may be silicon bugs, software bugs in ROM, software bugs in
binary blobs needed for a platform, or bugs in the toolchain.

These features behave the same way as `arch_%` features: Every provided
bug is always used, so that we can inspect `FEATURES_USED` to check
which bugs need to be worked around.

Co-authored-by: crasbe <[email protected]>
This makes use of the new bug modeling to declare all platforms that
can use newlib and have no reentrancy hooks as affected by the
non-thread-safe stdio bug. (Which is every platform but ESP* and AVR,
the former because the reentrancy hooks are provided, the latter because
we do not and never will support newlib on them.)

Building on that, the mpaland-printf package is used when newlib is
used and the bug is present. This way we can rely on the stdio being
thread-safe on every platform and not causing random crashes at run
time.
This drops a workaround that initialized newlib's reentrancy structure
on boot to reduce the chances of crashes when using the non-thread-safe
(unless reentrancy hooks are provided) stdio implementation of newlib.

Now that the newlib stdio implementation is only ever used if it is
thread-safe, we no longer need a workaround that reduces the chance
of crashes on concurrent use of stdio.
@maribu maribu force-pushed the sys/newlib/fix-stdio branch from 933acf3 to bb76083 Compare April 28, 2025 05:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: build system Area: Build system Area: cpu Area: CPU/MCU ports Area: sys Area: System CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ARM Platform: This PR/issue effects ARM-based platforms Platform: MSP Platform: This PR/issue effects MSP-based platforms Platform: RISC-V Platform: This PR/issue effects RISC-V-based platforms Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants