Skip to content

Conversation

@kaspar030
Copy link
Contributor

Contribution description

#17706 introduced printing of stack metrics. But, it printed them in the exiting thread, causing stack overflows due to printf(), for very small stacks.

This PR fixes the issue two-fold:

  1. print_str() is used, if fmt is linked in.
  2. the actual stack size is checked, and nothing is printed if the stack is too small

Testing procedure

Issues/PRs references

@kaspar030 kaspar030 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: run tests If set, CI server will run tests on hardware for the labeled PR labels Mar 31, 2022
@github-actions github-actions bot added Area: sys Area: System Area: tests Area: tests and testing framework labels Mar 31, 2022
Copy link
Contributor

@fjmolinas fjmolinas left a comment

Choose a reason for hiding this comment

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

ACK

@benpicco
Copy link
Contributor

Instead of making the code more cluttered with fmt why not just

USEMODULE += picolibc

?

@kaspar030
Copy link
Contributor Author

Instead of making the code more cluttered with fmt why not just

USEMODULE += picolibc

?

Because it changes the test.

(And, picolibc printf() still uses way more stack than print_str().)

Copy link
Member

@maribu maribu left a comment

Choose a reason for hiding this comment

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

See inline. I wonder if we should just let it depend on fmt, though.

Somewhat unrelated: I always wanted to start using fmt in the panic handlers (at least for newlib). Because the very moment the panic handler uses printf I get an "ISR stack overflown", but that was caused by the panic handler's use of printf. If that would get upstream, there would be fmt anyway whenever newlib is used.

@kaspar030
Copy link
Contributor Author

See inline. I wonder if we should just let it depend on fmt, though.

I thought about it, but it does add >200 bytes to ROM is fmt is not used otherwise. Which would in turn kick tests over the limit.

@fjmolinas
Copy link
Contributor

I thought about it, but it does add >200 bytes to ROM is fmt is not used otherwise. Which would in turn kick tests over the limit.

+1 I think since this is a utility it's good to make it just use what is available, and not have it change the footprint of an application outside of the code size added by the function itself.

@kaspar030 kaspar030 force-pushed the print_stack_usage_fmt branch from 84f6298 to 480ed47 Compare March 31, 2022 12:48
@maribu maribu enabled auto-merge March 31, 2022 13:02
@maribu maribu merged commit 3977ff6 into RIOT-OS:master Mar 31, 2022
@kaspar030 kaspar030 deleted the print_stack_usage_fmt branch April 1, 2022 07:22
@OlegHahm OlegHahm added this to the Release 2022.04 milestone Apr 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: sys Area: System Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: run tests If set, CI server will run tests on hardware for the labeled PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants