Skip to content

More mbed_error refinements #8441

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

Merged
merged 6 commits into from
Oct 23, 2018
Merged

More mbed_error refinements #8441

merged 6 commits into from
Oct 23, 2018

Conversation

kjbracey
Copy link
Contributor

@kjbracey kjbracey commented Oct 16, 2018

Description

This is based on and contains #8255 and #8076

New changes:

  • Add mbed_error_puts to allow unlimited-length prints. Use it for error message and filename.
    • Switching to puts avoids undefined behaviour if there was a % in an error message.
  • Always print full filename if available in print_error_report, so we get filename from MBED_ASSERT after Change behaviour of mbed_asert to use mbed_error instead of mbed_die #8255, regardless of the setting of platform.error-capture-filename-enabled.
  • Print thread names.
  • Correct full thread info print - wasn't showing ready threads, and was totally broken by type mismatch hidden by cast.
  • Simplify read of current stack pointer - makes it work on Cortex-A
  • Remove redundant memset

Pull request type

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

@cmonr
Copy link
Contributor

cmonr commented Oct 16, 2018

Fyi:

[Build K66F IAR] [Error] mbed_board.c@94,0: [Pe118]: a void function may not return a value
[Build K66F IAR] [ERROR]
[Build K66F IAR] return mbed_error_vprintf(format, arg);
[Build K66F IAR] ^
[Build K66F IAR] "/builds/ws/mbed-os-ci_cloud-client-test/mbed-cloud-client-example/mbed-os/platform/mbed_board.c",94 Error[Pe118]: a void function may not return a value
[Build K66F IAR]
[Build K66F IAR] [mbed] ERROR: "/usr/bin/python" returned error code 1.
[Build K66F IAR] [mbed] ERROR: Command "/usr/bin/python -u /builds/ws/mbed-os-ci_cloud-client-test/mbed-cloud-client-example/mbed-os/tools/make.py -t IAR -m K66F --source . --build ./BUILD/K66F/IAR" in "/builds/ws/mbed-os-ci_cloud-client-test/mbed-cloud-client-example"
[Build K66F IAR] ---
[Build K66F IAR] [mbed] Auto-installing missing Python modules...

@kjbracey kjbracey force-pushed the error_puts branch 2 times, most recently from 3c0baf2 to 124bb51 Compare October 17, 2018 09:56
@0xc0170 0xc0170 requested a review from a team October 17, 2018 23:42
@cmonr
Copy link
Contributor

cmonr commented Oct 19, 2018

@mbed-os-core Whenever y'all are available, this became unblocked when #8076 came in.

Copy link

@deepikabhavnani deepikabhavnani left a comment

Choose a reason for hiding this comment

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

Looks good to me

@kjbracey
Copy link
Contributor Author

Puzzled that GitHub is still showing the first 5 commits from #8076. Expected them to quietly vanish once merged, which they definitely have been. Guess I'll rebase just to clear up view.

This is potentially useful for printing long strings such as filenames
from assert messages, avoiding the buffer limit inherent in
mbed_error_printf.
If we want zero-fill, strncpy does it anyway.
Casts were covering up a type mismatch.

Print the "ready" list, and remove the explicit print of the idle thread
(it should be in the ready list).
As part of this, don't show empty current thread info in non-RTOS
build.
Don't extract filename from the stored error - print it directly.

Use "mbed_error_puts" for both error message and filename to avoid
buffer length limits.

Switch to puts also fixes the potential problem of an error message
containing a '%' upsetting the formatter - it should have been
mbed_printf_error("%s", error_msg) in the first place.
@cmonr
Copy link
Contributor

cmonr commented Oct 22, 2018

/morph build

@mbed-ci
Copy link

mbed-ci commented Oct 22, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci
Copy link

mbed-ci commented Oct 22, 2018

@mbed-ci
Copy link

mbed-ci commented Oct 23, 2018

@cmonr
Copy link
Contributor

cmonr commented Oct 23, 2018

@SenRamakri Ok with this PR?

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.

Just a question.

@@ -29,34 +29,18 @@
#include <stdio.h>
#endif

//Helper macro to get the current SP
#define GET_CURRENT_SP(sp) \
Copy link
Contributor

Choose a reason for hiding this comment

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

Huh. Why is this macro no longer needed?

Choose a reason for hiding this comment

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

SP pointer is fetched using the address of local variable, hence not needed

Copy link
Contributor Author

@kjbracey kjbracey Oct 23, 2018

Choose a reason for hiding this comment

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

It was a very complicated way of getting the current stack pointer.

Although, on reflection, I now think the current stack pointer is not what you wanted anyway. It was being used as part of a "show current thread" line, where the current SP is being shown next to the stack size and limit for the current thread.

In a fault handler, both the complex and simple version will give you MSP, which is not going to be anywhere near the thread stack. You really wanted the PSP (or SP_usr for Cortex-A).

I'll probably adjust it again in a further patch.

(It's no big deal, as if it is a fault, you'll have the PSP visible in the preceding register dump.)

@cmonr cmonr merged commit 29e9619 into ARMmbed:master Oct 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants