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

Error handling configuration updates and Optimization for exception handling #7214

Merged
merged 3 commits into from Jun 25, 2018

Conversation

Projects
None yet
5 participants
@SenRamakri
Contributor

SenRamakri commented Jun 13, 2018

Description

This change is targeted towards optimizing Error handling and exception handling to save memory.
The PR contains following changes:

  1. Updates to configuration flags used in error handling implementation
  2. Make Error history tracking switched off by default and enabled by using the config flag MBED_CONF_PLATFORM_ERROR_HIST_ENABLED.
  3. Update exception handling code to remove dedicated fault safe printfs and to use mbed_error_printf.
    Dedicated safe printfs were originally developed to print data over serial with minimal resources. But this adds more code space, so we are switching to use mbed_error_printf. Although it uses more resources, but it saves on rom usage.
  4. Another change is to remove printing info on all threads always when generating exception report. This will be turned off by default and can be turned on using config flag MBED_CONF_PLATFORM_ERROR_ALL_THREADS_INFO if required.
    These optimizations should save around 800 bytes on Rom.

Pull request type

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

@SenRamakri SenRamakri requested review from cmonr, geky, 0xc0170 and studavekar Jun 13, 2018

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Jun 15, 2018

Looking at this, I'm a teeny bit nervous some of the prints are getting a bit close to the 128-byte buffer limit in mbed_error_vfprintf. I've seen people hit that problem in mbed_assert, which can certainly blow the limit and fail to print the full filename + line as a result. (Doubly bad in Greentea, which will actually show none of the assert due to no line terminator.)

So mbed_assert_internal does need its mbed_error_printf broken up - not sure if this does, but watch out for anywhere you're printing a filename in %s. That can be rather long.

@@ -53,6 +53,6 @@ typedef struct {

//This is a handler function called from Fault handler to print the error information out.
//This runs in fault context and uses special functions(defined in mbed_rtx_fault_handler.c) to print the information without using C-lib support.
__NO_RETURN void mbed_fault_handler (uint32_t fault_type, void *mbed_fault_context_in, void *osRtxInfoIn);
void mbed_fault_handler (uint32_t fault_type, void *mbed_fault_context_in, void *osRtxInfoIn);

This comment has been minimized.

@0xc0170

0xc0170 Jun 15, 2018

Member

why are we removing no_return here?

This comment has been minimized.

@SenRamakri

SenRamakri Jun 18, 2018

Contributor

@0xc0170 - With the new error handling implementation fault handler will call into error handler which will handle halting the system and thus no longer required here.

mbed_error_printf("\nLocation: 0x%X", ctx->error_address);

#if MBED_CONF_PLATFORM_ERROR_FILENAME_CAPTURE_ENABLED && !defined(NDEBUG)
if((NULL != ctx->error_filename[0]) && (ctx->error_line_number != 0)) {

This comment has been minimized.

@0xc0170

0xc0170 Jun 15, 2018

Member

if ( space . Look at astyle failures for this changes in Travis

}

void print_context_info(void)
MBED_NOINLINE void print_context_info(void)

This comment has been minimized.

@0xc0170

0xc0170 Jun 15, 2018

Member

inlining was causing some problems for this function?

This comment has been minimized.

@SenRamakri

SenRamakri Jun 18, 2018

Contributor

No specific problems with inlining. But sometimes debugging and also identifying the space required for individual functions becomes hard. So I removed inline as I was working on some optimizations.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jun 15, 2018

The PR contains following changes:

That info should be part of the commit messages here

@SenRamakri

This comment has been minimized.

Contributor

SenRamakri commented Jun 18, 2018

@0xc0170 - You mentioned my description should be part of commit messages? Are you meaning I should include the entire description?

@SenRamakri

This comment has been minimized.

Contributor

SenRamakri commented Jun 18, 2018

@kjbracey-arm - Good point about the mbed_error_printf/%s. Currently we are using %s for printing filename. I'll change the implementation to limit the filename length to 64 chars max, by default its 16. That should take care of this as we have one printf which prints the filename+linenum but nothing else.

@SenRamakri SenRamakri force-pushed the SenRamakri:sen_ErrorOptimAndConfig branch from b31e28e to 4a92075 Jun 18, 2018

//We have to limit this to 64 bytes since we use mbed_error_printf for error reporting
//and mbed_error_vfprintf uses 128bytes internal buffer which may not be sufficient for anything
//longer that 64 bytes with the current implementation.
#error "Unsupported error filename buffer length detected, max supported length is 64 chars. Please change MBED_CONF_PLATFORM_MAX_ERROR_FILENAME_LEN or max-error-filename-len in configuration."

This comment has been minimized.

@SenRamakri

SenRamakri Jun 18, 2018

Contributor

@kjbracey-arm - Please see this change to limit the filename buffer to 64 chars. This should take care of 128 chars buffer limitation in mbed_error_printf implementation.

@SenRamakri SenRamakri force-pushed the SenRamakri:sen_ErrorOptimAndConfig branch 2 times, most recently from 6057ccd to 28be10c Jun 18, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jun 19, 2018

@0xc0170 - You mentioned my description should be part of commit messages? Are you meaning I should include the entire description?

Yes, I dont see a reason why not.

This needs a rebase now

@SenRamakri SenRamakri force-pushed the SenRamakri:sen_ErrorOptimAndConfig branch 2 times, most recently from 1736154 to dc66deb Jun 19, 2018

@SenRamakri

This comment has been minimized.

Contributor

SenRamakri commented Jun 20, 2018

@0xc0170 - Commit message has been updated.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jun 20, 2018

The last commit was amended. What I had in mind was reworked all 3 commits to provide details, this needs rebase interactive and edit each one of them . The last commit does not make much sense , or am I misreading the changes vs commit msg there (the last commit contains style changes that should be separate commit - this is making it hard to review).

SenRamakri added some commits Jun 11, 2018

Updates to configuration flags used in error handling implementation.
The new configuration make Error history tracking switched off by default and enabled by using the config flag MBED_CONF_PLATFORM_ERROR_HIST_ENABLED.
Config flag MBED_CONF_PLATFORM_ERROR_ALL_THREADS_INFO enables printing info of all threads. This will be turned off by default.
Update exception handling code to remove dedicated fault safe printfs…
… and use mbed_error_printf to optimize memory usage.

Dedicated safe printfs were originally developed to print data over serial with minimal resources. But this adds more code space, so we are switching to use mbed_error_printf.

@SenRamakri SenRamakri force-pushed the SenRamakri:sen_ErrorOptimAndConfig branch from dc66deb to baa44eb Jun 20, 2018

@SenRamakri

This comment has been minimized.

Contributor

SenRamakri commented Jun 20, 2018

@0xc0170 - Commit messages have been split/re-worked.

@0xc0170 0xc0170 requested a review from ARMmbed/mbed-os-core Jun 21, 2018

@cmonr

cmonr approved these changes Jun 21, 2018

LGTM

@cmonr cmonr removed the needs: review label Jun 21, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jun 21, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Jun 21, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jun 21, 2018

Looks like the K64F example mbed-os-example-error-handling failed on all compilers, but looks like a simple fix.

@cmonr cmonr added needs: work and removed needs: CI labels Jun 21, 2018

@SenRamakri

This comment has been minimized.

Contributor

SenRamakri commented Jun 21, 2018

@cmonr - Thanks Cruz, I'll look into the failure and push a fix.

@SenRamakri

This comment has been minimized.

Contributor

SenRamakri commented Jun 24, 2018

@cmonr - I have pushed a fix to the example app(on master branch).

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jun 25, 2018

/morph build

@cmonr cmonr added needs: CI and removed needs: work labels Jun 25, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jun 25, 2018

@SenRamakri Thansk for the quick direct commit to the example, but I'm thinking that once an example becomes public, fixes and the such should come into the example as PRs instead of direct commits.

@ARMmbed/mbed-os-maintainers Thoughts? I'm not sure ownership/process is clear/public for internal examples that become public.

@mbed-ci

This comment has been minimized.

mbed-ci commented Jun 25, 2018

Build : SUCCESS

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

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.

@cmonr cmonr added ready for merge and removed needs: CI labels Jun 25, 2018

@cmonr cmonr merged commit ea47342 into ARMmbed:master Jun 25, 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, 919 files
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 10077 cycles (+1181 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 9964B (+0.00%)
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment