-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Enable -Wstack-usage on device builds #3683
Enable -Wstack-usage on device builds #3683
Conversation
#### Problem RAM on small devices is limited, so excesssive stack should not be used without good reason. Stack allocation is also not easily checked at run time so overflow is likely to cause crashes. #### Summary of Changes - Add a `-Wstack-usage` compiler flag on embedded builds. (For this purpose, an ‘embedded build’ is any that is not using a whitelisted non-embedded OS, so new platforms will have this enabled by default.) In this PR, the stack limit is set high enough that _only_ dynamically unbounded stack usage triggers it. The intent is to lower the limit in the future so that any unusually large stack requires whitelisting with justification in review. - Replace most uses of dynamically unbounded stack. - Filed separate issues for two remaining uses of dynamic stack: - project-chip#3662 emberAfPrintBuffer() - project-chip#3663 payloadBase41RepresentationWithTLV() fixes project-chip#3505 Use "-Wstack-usage" for device builds
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing this!
Size increase report for "esp32-example-build" from 99aaed7
Full report output
|
@@ -23,6 +23,7 @@ | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It makes me a bit sad to have to fix ManualSetupPayloadGenerator.cpp
and QRCodeSetupPayloadGenerator.cpp
for stack usage. That would make the code a bit less readable and harder to maintain.
For the rest of CHIP that definitively makes sense.
For those files I think the initial intent was that the code that creates the manual/QR code was not designed to be shipped on devices. We mostly have used that as an easy way to create a QRCode on the M5Stack while developing. I wonder if instead of fixing those files, it is time to just get those files out of the devices (not in this PR though).
Thoughts ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no objection to leaving it. The main goal of this PR is to make sure it doesn't happen by accident or get overlooked in new code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Problem
RAM on small devices is limited, so excesssive stack should not be used
without good reason. Stack allocation is also not easily checked at run
time so overflow is likely to cause crashes.
Summary of Changes
Add a
-Wstack-usage
compiler flag on embedded builds. (For thispurpose, an ‘embedded build’ is any that is not using a whitelisted
non-embedded OS, so new platforms will have this enabled by default.)
In this PR, the stack limit is set high enough that only dynamically
unbounded stack usage triggers it. The intent is to lower the limit
in the future so that any unusually large stack requires whitelisting
with justification in review.
Replace most uses of dynamically unbounded stack.
Filed separate issues for two remaining uses of dynamic stack:
fixes #3505 Use "-Wstack-usage" for device builds