Skip to content

Disable printf in crash reporting for release builds #9164

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Dec 21, 2018
Merged

Disable printf in crash reporting for release builds #9164

merged 1 commit into from
Dec 21, 2018

Conversation

SenRamakri
Copy link
Contributor

Description

The current implementation of crash reporting uses printf() to print some debug messages to terminal during boot up. This is pulling in printf() dependencies which is potentially causing ~7k increase in ROM size for GCC ARM. This change wraps it for debug builds only so release builds doesn't increase in size for applications not using printf() otherwise. Interestingly other toolchains(IAR, ARMC) is not impacted by this.

Pull request type

[x] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

@SenRamakri
Copy link
Contributor Author

@0xc0170 @cmonr - I strongly think we should back port this into 5.11 branch, because applications not using printf() may get impacted by this for GCC ARM.

cmonr
cmonr approved these changes Dec 20, 2018
View reviewed changes
Copy link
Contributor

@cmonr cmonr left a comment

Choose a reason for hiding this comment

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

LGTM

@cmonr
Copy link
Contributor

cmonr commented Dec 20, 2018

Out of curiosity, how was this error/issue discovered? Is there something we can reference as being fixed? If the resulting compiled files change significantly with this (for the better), can the before/after results be shared?

@cmonr
Copy link
Contributor

cmonr commented Dec 20, 2018

#9164 (comment)

Woop, never mind. Saw the email.

@kjbracey
Copy link
Contributor

Isn't mbed_error full of prints, so they're in the image anyway?

Ah, no, it goes via vsnprintf, and then uses the special "exception-safe" print. So what this is pulling in is stdout, I guess?

You may as well do the same thing here, even though you don't need the exception-safe - use mbed_error_printf, rather than making it for debug specifically? Make it consistent so that you get this output on the same conditions that the crash prints are. Which I think would be using mbed_error_printf inside an NDEBUG test.

@kjbracey
Copy link
Contributor

Ah, the title of the PR confused me. That test isn't checking for "debug builds only". NDEBUG is set for develop and debug builds, and off only for release builds.

kjbracey
kjbracey reviewed Dec 20, 2018
View reviewed changes
Copy link
Contributor

@kjbracey kjbracey left a comment

Choose a reason for hiding this comment

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

Title needs correction - crash reporting remains enabled for develop builds. Suggest saying you're disabling it for release builds.

platform/mbed_error.c
@@ -211,7 +211,9 @@ mbed_error_status_t mbed_error_initialize(void)
if ((report_error_ctx->crc_error_ctx == crc_val) && (report_error_ctx->is_error_processed == 0)) {
is_reboot_error_valid = true;
//Report the error info
#ifndef NDEBUG
Copy link
Contributor

Choose a reason for hiding this comment

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

It would make sense for this to use mbed_error_printf, so you're definitely not pulling in anything extra beyond the existing error prints.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

These are the only 2 printf in this file,

@0xc0170 0xc0170 changed the title Enable printf in crash reporting for debug builds only Disable printf in crash reporting for release builds Dec 20, 2018
@0xc0170
Copy link
Contributor

0xc0170 commented Dec 20, 2018

I updated the title as requested

@kjbracey
Copy link
Contributor

I updated the title as requested

Commit message still needs the same change.

@SenRamakri SenRamakri force-pushed the sen_MemFixCrashReportPrint branch from 97bea7f to daa8b0e Compare December 20, 2018 19:47
@SenRamakri
Copy link
Contributor Author

@kjbracey-arm @cmonr @0xc0170 - Commit message has been amended.

@cmonr
Copy link
Contributor

cmonr commented Dec 20, 2018

@SenRamakri Do you have any thoughts on #9164 (comment)?

@SenRamakri
Copy link
Contributor Author

@cmonr - We are not able to use mbed_error_printf() that early in boot due to how IAR retargeting layer is implemented. Please see my previous comment here as well - #8702 (comment)
Thats why we have to printf() in those cases, and I also tried to minimize the use of printfs - just have 2 calls as of now.

@cmonr
Copy link
Contributor

cmonr commented Dec 21, 2018

Thanks @SenRamakri for the link to the past explanation.

@kjbracey-arm thoughts? Looks like the printfs might be the best we can do for now.

@0xc0170
Copy link
Contributor

0xc0170 commented Dec 21, 2018

CI started (restarted also Travis job)

@mbed-ci
Copy link

mbed-ci commented Dec 21, 2018

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 1
Build artifacts

@cmonr cmonr merged commit 5ad20d4 into ARMmbed:master Dec 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bulislaw bulislaw bulislaw approved these changes

@cmonr cmonr cmonr approved these changes

@0xc0170 0xc0170 0xc0170 approved these changes

@kjbracey kjbracey kjbracey approved these changes

Assignees
No one assigned
Labels
release-version: 5.11.1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@SenRamakri @cmonr @kjbracey @0xc0170 @mbed-ci @bulislaw