Change behaviour of mbed_asert to use mbed_error instead of mbed_die #8255

MateuszMaz · 2018-09-26T10:04:13Z

Description

@jamesbeyond @0xc0170 @mprse
Task: IOTCORE-378
This PR changes behaviour of mbed_assert to use mbed_error, instead of mbed_die that goes around new error reporting mechanism.

The output of following example when flag MBED_CONF_PLATFORM_ERROR_FILENAME_CAPTURE_ENABLED = 1:

#include "mbed.h"

int main()
{
    MBED_ASSERT(0 > 1);
    return 0;
}

is:

++ MbedOS Error Info ++
Error Status: 0x80FF0101 Code: 257 Module: 255
Error Message: Mbed assertation failed 0>1
Location: 0x2081
File:.\main.cpp+6
Error Value: 0x0
Current Thread: Id: 0x20001EAC Entry: 0x2BF7 StackSize: 0x1000 StackMem: 0x20001EF0 SP: 0x20002E18
For more info, visit: https://armmbed.github.io/mbedos-error/?error=0x80FF0101
-- MbedOS Error Info --

I have decided to use error status, : MBED_ERROR_INVALID_ARGUMENT and im not really sure if it is correct, please note that.
I use mbed_error instead of macros MBED_ERROR or MBED_ERROR1, to get file name and line where assertion failed and not where MBED_ERROR where used.

Pull request type

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

deepikabhavnani · 2018-09-26T14:55:51Z

CC @SenRamakri

jamesbeyond

Hi @SenRamakri,
Could you confirm is MBED_ERROR_INVALID_ARGUMENT the correct error status to be used in this case?
If not, could you suggest one?

Everything else looks OK to me.

SenRamakri · 2018-10-04T17:39:04Z

@MateuszMaz - Please define a new error code for "assertion failed" in mbed_error.h as below -
MBED_DEFINE_SYSTEM_ERROR(ASSERTION_FAILED, 68), /* 324 Assertion Failed */
Make sure you add to the end of the enum. Everything else looks ok to me.

MateuszMaz · 2018-10-04T18:49:28Z

The output now is:

++ MbedOS Error Info ++
Error Status: 0x80FF0144 Code: 324 Module: 255
Error Message: Mbed assertation failed 0 > 1
Location: 0x8000C21
Error Value: 0x0
Current Thread: Id: 0x2000187C Entry: 0x8001743 StackSize: 0x1000 StackMem: 0x200018C0 SP: 0x200027F8
For more info, visit: https://armmbed.github.io/mbedos-error/?error=0x80FF0144
-- MbedOS Error Info --

kjbracey · 2018-10-05T14:12:17Z

platform/mbed_assert.c

+
+    const char error_description[] = "Mbed assertation failed ";
+    unsigned error_message_length = strlen(error_description) + strlen(expr) + 1;
+    char error_message[error_message_length];


Does anyone know if variable-length arrays are totally safe in IRQ-disabled context? Possibly are, but a bit wary about potential heap fallback rather than using the stack.

Even then, using the stack is possibly hairy if big - would it be possible to reuse the static buffer that mbed_error_printf uses?

Or do we even need a buffer? Could mbed_error be persuaded to print something about assertion failure itself from the error code, so we just pass expr?

@kjbracey-arm - I think that make sense, lets have mbed_error.c handle creating the message in print_error_report() function. We already do that for few special error codes. That way mbed_assert_internal() can just pass the expr.
Also print_error_report() is wrapped with right flags.

@MateuszMaz - Can you please make these changes to just pass the expr into mbed_error and have print_error_report() function cook the message(Eg:- "Assertion failed", no need to prepend that with Mbed) for assertion fails.

kjbracey · 2018-10-05T14:21:22Z

platform/mbed_error.h

@@ -780,7 +780,8 @@ typedef enum _mbed_error_code {
    MBED_DEFINE_SYSTEM_ERROR(BLE_NO_FRAME_INITIALIZED, 65),             /* 321      BLE No frame initialized */
    MBED_DEFINE_SYSTEM_ERROR(BLE_BACKEND_CREATION_FAILED, 66),          /* 322      BLE Backend creation failed */
    MBED_DEFINE_SYSTEM_ERROR(BLE_BACKEND_NOT_INITIALIZED, 67),          /* 323      BLE Backend not initialized */
-
+    MBED_DEFINE_SYSTEM_ERROR(ASSERTATION_FAILED, 68),                   /* 324      Assertation Failed */


"Assertation" is barely a word - could you correct it to "assertion" before baking it into API?

( I've already got a PR pending which corrects the text: #8076 . Been sitting unreviewed for over 3 weeks though. )

…ed_error

kjbracey

Can you update the GitHub message to show the final output?

kjbracey · 2018-10-08T08:23:46Z

platform/mbed_error.c

@@ -349,6 +349,10 @@ static void print_error_report(mbed_error_ctx *ctx, const char *error_msg)
            mbed_error_printf("MessageQueue: 0x%X, ", ctx->error_value);
            break;

+        case MBED_ERROR_CODE_ASSERTION_FAILED:
+            mbed_error_printf("Assertion failed, ", ctx->error_value);


You want to have : here not ,, so the output is Asssertion failed: expression.

The comma was to separate the error_value, which has been left in - drop it.

kjbracey · 2018-10-08T09:42:35Z

This should resolve #6850

cmonr · 2018-10-08T18:57:22Z

This should resolve #6850

Making sure I understand, this PR needs to come in before #8076 (comment), correct?

kjbracey · 2018-10-09T07:54:52Z

platform/mbed_error.c

@@ -349,6 +349,10 @@ static void print_error_report(mbed_error_ctx *ctx, const char *error_msg)
            mbed_error_printf("MessageQueue: 0x%X, ", ctx->error_value);
            break;

+        case MBED_ERROR_CODE_ASSERTION_FAILED:
+            mbed_error_printf("Assertion failed: ", ctx->error_value);


This still has the stray ctx->error_value.

(You should ideally be getting a warning about that, but unfortunately there's no format checking directives on mbed_error_printf. I am intending to add some to all the local printf-type functions).

kjbracey · 2018-10-09T10:41:13Z

Making sure I understand, this PR needs to come in before #8076 (comment), correct?

It should now be independent - I've revised #8076 to avoid collision.

cmonr · 2018-10-09T18:28:03Z

platform/mbed_assert.c


 void mbed_assert_internal(const char *expr, const char *file, int line)
 {
    core_util_critical_section_enter();
-    mbed_error_printf("mbed assertation failed: %s, file: %s, line %d \n", expr, file, line);
-    mbed_die();


Why does this function no longer need mbed_die()?

@deepikabhavnani @SenRamakri

Nvm, looked over the PR again and it looks ok.

mbed_die is called but after error processing, just like any other error, next addition to removal is mbed_error()

mbed-os/platform/mbed_error.c

Line 200 in e6ae3d2

WEAK mbed_error_status_t mbed_error(mbed_error_status_t error_status, const char *error_msg, unsigned int error_value, const char *filename, int line_number)

cmonr · 2018-10-09T18:33:56Z

platform/mbed_assert.c


 void mbed_assert_internal(const char *expr, const char *file, int line)
 {
    core_util_critical_section_enter();
-    mbed_error_printf("mbed assertation failed: %s, file: %s, line %d \n", expr, file, line);
-    mbed_die();


Nvm, looked over the PR again and it looks ok.

NirSonnenschein · 2018-10-14T08:10:31Z

/morph build

kjbracey · 2018-10-15T11:33:11Z

It's just come to my attention that there is a significant functional change here - this is going to only output filenames if platform.error-filename-capture-enabled is on, and even then truncate to platform.max-error-filename-lin. These default to off and 16.

I don't think that's a reason to block this PR - I think the PR is sound. The problem is in the way mbed_error works - it should be accepting and printing filenames and line numbers even if it isn't "storing" them. I think we'll need to follow that up after #8255 and #8076.

The filename conditions for "assert" should not be inherently the same as the filename conditions for "error" or "warning", as assert otherwise only has a not-fully-identifying expression, whereas "error" and "warning" can be expected to have identifying text, making the filename redundant.

cmonr · 2018-10-15T19:57:39Z

@SenRamakri Fyi ^^^

/morph build

mbed-ci · 2018-10-15T21:22:38Z

Build : SUCCESS

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

Triggering tests

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

mbed-ci · 2018-10-15T22:48:39Z

Exporter Build : SUCCESS

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

mbed-ci · 2018-10-16T00:13:56Z

Test : SUCCESS

Build number : 3163
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/8255/3163

kjbracey · 2018-10-16T12:09:32Z

Created #8441 to make sure full filename is always shown by MBED_ASSERT, plus more fixes.

0xc0170 · 2018-10-16T14:07:47Z

/morph mbed2-build

cmonr · 2018-10-16T15:08:12Z

/morph mbed2-build

cmonr · 2018-10-16T16:06:14Z

Bringing this in, but it makes me a but uneasy that I can't rerun the unittest due to it's age (build 140).

Changed behaviour of mbed_asert to use mbed_error instead of mbed_die.

c3e7ddb

0xc0170 added the needs: review label Sep 26, 2018

0xc0170 requested review from jamesbeyond, mprse and a team September 26, 2018 10:33

0xc0170 requested a review from SenRamakri October 1, 2018 10:52

jamesbeyond reviewed Oct 1, 2018

View reviewed changes

0xc0170 changed the title ~~Changed behaviour of mbed_asert to use mbed_error instead of mbed_die.~~ Change behaviour of mbed_asert to use mbed_error instead of mbed_die Oct 4, 2018

Add ASSERTATION_FAILED error code

a2f3c59

jamesbeyond approved these changes Oct 5, 2018

View reviewed changes

0xc0170 approved these changes Oct 5, 2018

View reviewed changes

kjbracey reviewed Oct 5, 2018

View reviewed changes

MateuszMaz added 2 commits October 5, 2018 21:15

Add case MBED_ERROR_CODE_ASSERTION_FAILED, fix names, pass expr to mb…

845655b

…ed_error

Remove empty line

63d3a32

kjbracey reviewed Oct 8, 2018

View reviewed changes

0xc0170 added needs: work and removed needs: review labels Oct 8, 2018

kjbracey mentioned this pull request Oct 8, 2018

Error output improvements #8076

Merged

Fix message string

cf43e1c

cmonr added needs: review and removed needs: work labels Oct 8, 2018

kjbracey reviewed Oct 9, 2018

View reviewed changes

Remove stray error_value

e6ae3d2

cmonr reviewed Oct 9, 2018

View reviewed changes

cmonr approved these changes Oct 9, 2018

View reviewed changes

cmonr added the release-version: 5.11.0-rc1 label Oct 9, 2018

kjbracey approved these changes Oct 10, 2018

View reviewed changes

cmonr added needs: CI and removed needs: review labels Oct 10, 2018

SenRamakri approved these changes Oct 15, 2018

View reviewed changes

kjbracey mentioned this pull request Oct 16, 2018

More mbed_error refinements #8441

Merged

cmonr added ready for merge and removed needs: CI labels Oct 16, 2018

cmonr merged commit ce7e61e into ARMmbed:master Oct 16, 2018

cmonr removed the ready for merge label Oct 16, 2018

mattbrown015 mentioned this pull request Oct 22, 2018

MBED_ASSERT No Longer Calls mbed_die #8496

Closed

kjbracey mentioned this pull request Oct 25, 2018

Error path tightening: use MBED_NORETURN; add+use core_util_atomic_flag #8328

Merged

Change behaviour of mbed_asert to use mbed_error instead of mbed_die #8255

Change behaviour of mbed_asert to use mbed_error instead of mbed_die #8255

Uh oh!

Conversation

MateuszMaz commented Sep 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Pull request type

Uh oh!

deepikabhavnani commented Sep 26, 2018

Uh oh!

jamesbeyond left a comment

Choose a reason for hiding this comment

Uh oh!

SenRamakri commented Oct 4, 2018

Uh oh!

MateuszMaz commented Oct 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kjbracey left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kjbracey commented Oct 8, 2018

Uh oh!

cmonr commented Oct 8, 2018

Uh oh!

kjbracey Oct 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kjbracey commented Oct 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deepikabhavnani Oct 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NirSonnenschein commented Oct 14, 2018

Uh oh!

kjbracey commented Oct 15, 2018

Uh oh!

cmonr commented Oct 15, 2018

Uh oh!

mbed-ci commented Oct 15, 2018

Triggering tests

Uh oh!

mbed-ci commented Oct 15, 2018

Uh oh!

mbed-ci commented Oct 16, 2018

Uh oh!

kjbracey commented Oct 16, 2018

Uh oh!

0xc0170 commented Oct 16, 2018

Uh oh!

cmonr commented Oct 16, 2018

Uh oh!

cmonr commented Oct 16, 2018

Uh oh!

Uh oh!

MateuszMaz commented Sep 26, 2018 •

edited

Loading

kjbracey Oct 9, 2018 •

edited

Loading

deepikabhavnani Oct 9, 2018 •

edited

Loading