Optimise fault handler assembly #12824

kjbracey · 2020-04-17T10:13:15Z

Summary of changes

Fault handler assembly was very simplistic. Optimise for a 112 byte saving.

Also tidy up formatting, and make all 3 toolchain versions more consistent, for ease of diffing and transferring changes between them.

Also streamline access to crash capture region, avoiding use of pointers. Provides a small saving in fault handler even if crash capture is disabled.

Impact of changes

Migration actions required

Documentation

None

Pull request type

[X] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results

[] No Tests required for this change (E.g docs only update)
[X] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers

ciarmcom · 2020-04-17T11:00:36Z

@kjbracey-arm, thank you for your changes.
@ARMmbed/mbed-os-core @ARMmbed/mbed-os-maintainers please review.

0xc0170 · 2020-04-17T12:51:17Z

platform/source/TARGET_CORTEX_M/TOOLCHAIN_IAR/except.S

-        LDR     R1,[R3]
-        BL      mbed_fault_handler
+        MOVS    R1,R7
+        SUBS    R1,#21*4


what do we do here with substract, can you add a comment?

Is this similar what we do on line 99 - ADS to R1 value 48

That's rewinding R7 back to the start of the fault context structure. I'm about to change it to just a reload of the address constant anyway - that's simpler once I've killed an unnecessary pointer indirection.

The other one was skipping over saved floating-point registers. (R1 was the stack pointer at that point, here it's preparing the second parameter for mbed_fault_handler)

0xc0170 · 2020-04-17T12:54:58Z

platform/source/TARGET_CORTEX_M/TOOLCHAIN_IAR/except.S

+        MOVS    R1,R6
+        LSRS    R6,R4,#9                ; Check for if STK was aligned by checking bit-9 in xPSR value
+        BCC     Fault_Handler_Continue1
+        ADDS    R1,#0x4


Also here: can we comment on this addition or its me who does not see it before going into all instructions prior this one?

That's performing the 8-byte stack alignment referenced (or at least hinted at) in the comment above.

kjbracey · 2020-04-20T06:27:35Z

Corrected addressing of stack frame if it's on the Main Stack - needed ADD R6,SP,#16 not MOV R6,SP because we've just pushed R4-R7.

0xc0170 · 2020-04-20T08:02:10Z

CI restarted

0xc0170 · 2020-04-20T09:46:37Z

early note: GCC failed

I found one error at least: [Fatal Error] mbed_crash_data_offsets.h@21,10: platform/TARGET_CORTEX_M/mbed_fault_handler.h: No such file or directory [ERROR] In file included from ./mbed-os/platform/source/TARGET_CORTEX_M/mbed_fault_handler.c:28: ./mbed-os/platform/source/mbed_crash_data_offsets.h:21:10: fatal error: platform/TARGET_CORTEX_M/mbed_fault_handler.h: No such file or directory 21 | #include "platform/TARGET_CORTEX_M/mbed_fault_handler.h" | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ compilation terminated.

mbed-ci · 2020-04-20T10:02:22Z

Test run: FAILED

Summary: 2 of 3 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

jenkins-ci/mbed-os-ci_build-GCC_ARM
jenkins-ci/mbed-os-ci_build-ARM

kjbracey · 2020-04-20T10:22:20Z

My bad - botched rebase of work originally done on a fork.

kjbracey · 2020-04-20T10:36:59Z

Fixed. mbed_fault_handler.h moved into platform/internal from platform/source as mbed_crash_data_offsets.h now needs it.

(Although - shouldn't it be public? Is mbed_get_reboot_fault_context not intended to be a public function?)

0xc0170 · 2020-04-21T14:06:08Z

mbed_get_reboot_fault_context - I would say it should be public, same applied in the design doc:

The below API can be called by application to retrieve the fault context captured in the Crash-Report RAM. The error context is copied into the location pointed by fault_context.

0xc0170 · 2020-04-21T14:06:24Z

Travis is failing still.

Tidy up formatting, and make all 3 toolchain versions more consistent, for ease of diffing and transferring changes between them.

kjbracey · 2020-04-28T10:22:30Z

It's never been implemented for Cortex-A. You'd need a separate except.S, and for the targets to set their vector tables up to send data aborts etc there.

At the minute all Cortex-A targets have their own private exception handlers that just halt the system, as far as I can tell. This change at least gives a structure they could pass to mbed_fault_handler, if it was enabled for Cortex-A. But the existing code wouldn't know how to print it - would need some ifdefs.

Probably could refine it a bit to make the "application" part of the context common (R0-R12,SP,LR,PC,PSR) - the A and M architectures only differ in the "system" side.

0xc0170 · 2020-05-08T11:09:53Z

I started CI meanwhile

mbed-ci · 2020-05-08T14:44:36Z

Test run: SUCCESS

Summary: 6 of 6 test jobs passed
Build number : 3
Build artifacts

ithinuel · 2020-05-28T13:03:20Z

platform/internal/mbed_fault_handler.h


 //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_fault_handler.c) to print the information without using C-lib support.
-void mbed_fault_handler(uint32_t fault_type, const mbed_fault_context_t *mbed_fault_context_in);
+MBED_NORETURN void mbed_fault_handler(uint32_t fault_type, const mbed_fault_context_t *mbed_fault_context_in);


Can we make the fault handlers to be marked as no return only in release mode ?
Or at least provide a dead bx lr only present when not building in release profile ?
Returning from precise faults has multiple times proven extremely useful.
This is nowadays rather cumbersome to achieve in mbed and I'd like to see it become easier rather than marking more functions as no return.

This would take more than undoing this change - the assembler glue currently doesn't support mbed_fault_handler returning, so this was just matching the C annotation to the assembler use. (Get a warning if we do try to return).

Are you thinking more of doing a BX lr directly from the fault handler vector to unwind the stack to overcome backtrace unwinding problems in debuggers?

Yes. I often break on HardFault_Handler and try to find a bx lr somewhere in the flash to then

set $pc = 0x_____ stepi

this is most on the time much easier to debug/understand the context of the fault than hopping for the debugger to sort out the content of a potentially corrupted memory.

Another technique is to poke a "BX lr" onto the stack and execute it - there's a script around the place here people use to do that.

But this is working around a debugger deficiency. There's no fundamental reason in the world why a debugger cannot unwind the stack correctly from the fault vector. (If the BX lr works, the debugger can do the equivalent!) There are practical issues though, like it having to be in a GDB server, not the GDB core.

I spent a while getting this working a lot better in pyOCD's GDB server, at least, but it would need to be done separately for OpenOCD or any other GDB server. If the pyOCD RTOS awareness is on, then you should get a near-perfect display from the fault handler. pyocd/pyOCD#430 would make it basically perfect - showing you the faulting thread up-front without having to select it. I've another PR which would deal with the Thread/Handler stacks separately without RTOS awareness, but that's stale.

Anyway, this has no bearing on mbed_fault_handler - by the time you've reached there, you're past the point where a mere BX lr would take you back to the fault location. For your technique, stick the breakpoint on HardFault_Handler.

adbridge · 2020-06-11T11:33:37Z

@ithinuel Are you happy with this now ?

0xc0170 · 2020-06-17T12:17:11Z

CI started

mbed-ci · 2020-06-17T14:18:36Z

Test run: SUCCESS

Summary: 6 of 6 test jobs passed
Build number : 4
Build artifacts

NB: issues intriduced in ARMmbed#12824

ciarmcom requested review from a team April 17, 2020 11:00

ciarmcom added the needs: review label Apr 17, 2020

kjbracey force-pushed the faultasm branch 3 times, most recently from cad2be9 to dce3712 Compare April 17, 2020 11:45

0xc0170 reviewed Apr 17, 2020

View reviewed changes

kjbracey force-pushed the faultasm branch from dce3712 to 6d18403 Compare April 17, 2020 13:42

mergify bot added needs: work and removed needs: review labels Apr 17, 2020

kjbracey force-pushed the faultasm branch from 6d18403 to 99c20f2 Compare April 20, 2020 06:26

0xc0170 added needs: CI and removed needs: work labels Apr 20, 2020

0xc0170 added needs: work and removed needs: CI labels Apr 20, 2020

kjbracey force-pushed the faultasm branch from 99c20f2 to 73226a2 Compare April 20, 2020 10:35

kjbracey force-pushed the faultasm branch 2 times, most recently from 3ea954f to 12d8915 Compare April 22, 2020 10:11

0xc0170 added needs: review and removed needs: work labels Apr 23, 2020

Reformat except.S

c1641e7

Tidy up formatting, and make all 3 toolchain versions more consistent, for ease of diffing and transferring changes between them.

0xc0170 added needs: review and removed needs: work labels Apr 30, 2020

0xc0170 added needs: CI and removed needs: review labels May 8, 2020

0xc0170 added needs: review and removed needs: CI labels May 13, 2020

ithinuel suggested changes May 28, 2020

View reviewed changes

ithinuel approved these changes May 29, 2020

View reviewed changes

adbridge added the release-type: patch Indentifies a PR as containing just a patch label Jun 11, 2020

0xc0170 approved these changes Jun 17, 2020

View reviewed changes

0xc0170 added needs: CI and removed needs: review labels Jun 17, 2020

mergify bot added ready for merge and removed needs: CI labels Jun 17, 2020

0xc0170 merged commit 62c2431 into ARMmbed:master Jun 18, 2020

mergify bot removed the ready for merge label Jun 18, 2020

0xc0170 mentioned this pull request Jun 22, 2020

Nightly: Export uvision failing with missing context fault handler #13166

Closed

adbridge added release-version: 6.1.0 Release-pending labels Jun 24, 2020

jeromecoutant mentioned this pull request Jun 29, 2020

mbed-os-6.1 can't compile with IAR #13200

Closed

adbridge removed release-type: patch Indentifies a PR as containing just a patch Release-pending labels Jun 30, 2020

jeromecoutant added a commit to jeromecoutant/mbed that referenced this pull request Jul 8, 2020

IAR: compilation issues fix

5c3fca5

NB: issues intriduced in ARMmbed#12824

jeromecoutant mentioned this pull request Jul 8, 2020

IAR: compilation issues fix #13248

Closed

Optimise fault handler assembly #12824

Optimise fault handler assembly #12824

Uh oh!

Conversation

kjbracey commented Apr 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of changes

Impact of changes

Migration actions required

Documentation

Pull request type

Test results

Reviewers

Uh oh!

ciarmcom commented Apr 17, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kjbracey Apr 17, 2020 • 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!

kjbracey Apr 17, 2020 • 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 Apr 20, 2020

Uh oh!

0xc0170 commented Apr 20, 2020

Uh oh!

0xc0170 commented Apr 20, 2020

Uh oh!

mbed-ci commented Apr 20, 2020

Uh oh!

kjbracey commented Apr 20, 2020

Uh oh!

kjbracey commented Apr 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

0xc0170 commented Apr 21, 2020

Uh oh!

0xc0170 commented Apr 21, 2020

Uh oh!

kjbracey commented Apr 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

0xc0170 commented May 8, 2020

Uh oh!

mbed-ci commented May 8, 2020

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 May 29, 2020 • 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!

adbridge commented Jun 11, 2020

Uh oh!

0xc0170 commented Jun 17, 2020

Uh oh!

mbed-ci commented Jun 17, 2020

Uh oh!

Uh oh!

kjbracey commented Apr 17, 2020 •

edited

Loading

kjbracey Apr 17, 2020 •

edited

Loading

kjbracey Apr 17, 2020 •

edited

Loading

kjbracey commented Apr 20, 2020 •

edited

Loading

kjbracey commented Apr 28, 2020 •

edited

Loading

kjbracey May 29, 2020 •

edited

Loading