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

Do not print error reports in release builds #7404

Merged
merged 2 commits into from Jul 11, 2018

Conversation

Projects
None yet
6 participants
@SenRamakri
Contributor

SenRamakri commented Jul 2, 2018

Description

This avoids printing error reports in release builds.

Pull request type

[x] Fix
[ ] Refactor
[ ] New target
[ ] Feature
[ ] Breaking change
@cmonr

This comment has been minimized.

Contributor

cmonr commented Jul 3, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Jul 3, 2018

Build : SUCCESS

Build number : 2501
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/7404/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@0xc0170 0xc0170 requested a review from ARMmbed/mbed-os-core Jul 3, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jul 3, 2018

Fixes #7387

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jul 3, 2018

/morph export-build

@mbed-ci

This comment has been minimized.

@@ -78,7 +78,9 @@ WEAK void error(const char *format, ...)
//Call handle_error/print_error_report permanently setting error_in_progress flag
handle_error(MBED_ERROR_UNKNOWN, 0, NULL, 0);
#ifndef NDEBUG
print_error_report(&last_error_ctx, "Fatal Run-time error");

This comment has been minimized.

@deepikabhavnani

deepikabhavnani Jul 3, 2018

Contributor

Shouldn't we guard everything in error function with NDEBUG? Should handle_error be called in case of NDEBUG?

This comment has been minimized.

@SenRamakri

SenRamakri Jul 3, 2018

Contributor

We should not guard everything. We still need handle_error to be called because we need to record if its first/last error which is always tracked and we will report it externally(using devicehealth apis for example) when required.

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Jul 5, 2018

Contributor

But you could guard everything in print_error_report() with NDEBUG or have a dummy print_error_report() macro - neater than having multiple callers do ifdefs.

You should also watch out for "unused static function" warnings, which I think you'll get from the current version on some compilers.

@cmonr cmonr added needs: work and removed needs: CI labels Jul 3, 2018

@SenRamakri SenRamakri dismissed stale reviews from 0xc0170 and cmonr via 61458d5 Jul 5, 2018

@SenRamakri SenRamakri force-pushed the SenRamakri:sen_FixErrorReportLinkForRelease branch from 6d6db5e to 61458d5 Jul 5, 2018

@cmonr cmonr dismissed their stale review Jul 5, 2018

Changes still needed

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jul 5, 2018

@deepikabhavnani Are changes still needed after the current updated commits?

#define ERROR_REPORT print_error_report
#else
#define ERROR_REPORT
#endif

This comment has been minimized.

@deepikabhavnani

deepikabhavnani Jul 5, 2018

Contributor

Shall we not add an additional macro and instead just update the code as below:

#ifdef NDEBUG
#define print_error_report
#endif

This comment has been minimized.

@SenRamakri

SenRamakri Jul 5, 2018

Contributor

That would be confusing and may affect the readability to have actual function name to be nulled out. The macro bound to NDEBUG or not name clearly indicates the issue being addressed. Also its not a public macro, should not be a concern.

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Jul 6, 2018

Contributor

Would kind of expect this version to generate a lot of warnings for release, as it expands to just

(&last_error_ctx, error_msg);

Which is legal, but a compiler is going to rightly think looks weird.

It's more conventional to define a function-like macro:

#ifdef NDEBUG
#define print_error_report(ctx, error_msg) ((void) 0)
#endif

or

#ifndef NDEBUG
#define ERROR_REPORT(ctx, error_msg) print_error_report(ctx, error_msg)
#else
#define ERROR_REPORT(ctx, error_msg) ((void) 0)
#endif

This comment has been minimized.

@SenRamakri

SenRamakri Jul 6, 2018

Contributor

@kjbracey-arm - Ok, I can change it to include args. And I would go with second version, I have found the first version to be confusing and affects readability.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jul 5, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Jul 5, 2018

Build : SUCCESS

Build number : 2540
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/7404/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@SenRamakri SenRamakri force-pushed the SenRamakri:sen_FixErrorReportLinkForRelease branch from 61458d5 to b6179d2 Jul 6, 2018

@cmonr cmonr added needs: review and removed needs: work labels Jul 6, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jul 6, 2018

@kjbracey-arm @deepikabhavnani Mind re-reviewing?

@0xc0170

0xc0170 approved these changes Jul 9, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jul 10, 2018

/morph build

@0xc0170 0xc0170 added needs: CI and removed needs: review labels Jul 10, 2018

@mbed-ci

This comment has been minimized.

mbed-ci commented Jul 10, 2018

Build : SUCCESS

Build number : 2558
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/7404/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

@mbed-ci

This comment has been minimized.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jul 11, 2018

/morph export-build

@mbed-ci

This comment has been minimized.

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Jul 11, 2018

@cmonr cmonr merged commit 6130744 into ARMmbed:master Jul 11, 2018

14 checks passed

AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-mbed2-build build completed
Details
ci-morph-test test completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
travis-ci/astyle Passed, 791 files
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 9387 cycles (-169 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 9960B (-0.04%)
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details

pan- pushed a commit to pan-/mbed that referenced this pull request Aug 22, 2018

Merge pull request ARMmbed#7404 from SenRamakri/sen_FixErrorReportLin…
…kForRelease

Do not print error reports in release builds
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment