sys/fmt: use fflush(); stdio_write() instead of fwrite()
#19250
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kaspar030
added
CI: ready for build
benpicco
reviewed
Feb 6, 2023
benpicco
approved these changes
Feb 6, 2023
kaspar030
force-pushed
the
fmt_fwrite_to_fflush
branch
2 times, most recently
from
9e06684 to
bfcf2e5
Compare
|
I think the failed test is just a fluke, what do you think? |
Unfortunately not. Somehow the if condition caused printf to be used, which fails with the small stacks in those tests. I've changed |
kaspar030
force-pushed
the
fmt_fwrite_to_fflush
branch
2 times, most recently
from
7e8be4c to
2fb98a9
Compare
|
I've also added a bit more commentary. |
kaspar030
force-pushed
the
fmt_fwrite_to_fflush
branch
from
ba8a886 to
c77b104
Compare
|
bors merge |
|
Build succeeded: |
|
Thanks for the review! |
Teufelchen1
reviewed
Feb 12, 2024
Comment on lines
+510
to
+514
| /* native gets special treatment as native's stdio code is ... special. | ||
| * And when not building for RIOT, there's no `stdio_write()`. | ||
| * In those cases, just defer to `printf()`. | ||
| */ | ||
| #if IS_USED(MODULE_STDIO_NATIVE) || !defined(RIOT_VERSION) |
There was a problem hiding this comment.
Could you explain to me why this is needed? I fail to understand the reason on my own. E.g. if I remove the guarded code, the unit test still pass for native. What am I misunderstanding?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Contribution description
Currently, fmt's
print()is using libc'sfwrite()in order to mitigate bothlibc and fmt output clashing wrt. libc stdio buffers. That causes
print()touse quite a bit more stack, we had to remove the lower stack case in
print_stack_usage_metrics().This PR changes
print()to usefflush();stdio_write()instead, bypassinga lot of libc's FILE machinery.
A quick test on nrf52840dk shows that
print_stack_usage_metrics()on anotherwise empty thread uses 124b stack without the
fflush(), 160b with thefflush()(this PR's state). So another commit re-adds theMIN_SIZEfmtcase.
Testing procedure
Issues/PRs references