Unify fault handling and add script to show faults #5847

SenRamakri · 2018-01-12T21:56:27Z

This change set adds support for generating core/register/thread information dump on fault exceptions which helps debugging fault exceptions.
Various fault scenarios has been used to test the core dump generation( See a sample app at https://github.com/SenRamakri/mbed-os-example-fault-handler which generate various exceptions ).

A python script has also been added( crash_log_parser.py ) which can parse the crash dump info and generate more detailed information on the crash using .elf/.map files. See the README.md as part of tools/debug_tools/crash_log_parser for usage of the script.
Also note that assembly files have been kept common for ARMv6M, v7M , v8M cores. Although some of the exceptions defined in assembly files like MemManage/UsageFault/BusFault are not used for some v6M cores, it doesn't cause any issues and helps to keep the code common. Also note that code to check for whether FP context is saved is benign for non-FP cores since bit-4 in LR is defined as SBOP(reads as 1 as we want) for non-FP cores.

SenRamakri · 2018-01-12T22:47:24Z

Triggering a 'morph build' to find the targets which are not defining HardFault_Handler with WEAK attribute.

SenRamakri · 2018-01-12T22:48:09Z

/morph build

mbed-ci · 2018-01-12T23:24:22Z

Build : FAILURE

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

0xc0170 · 2018-01-22T09:52:16Z

Triggering a 'morph build' to find the targets which are not defining HardFault_Handler with WEAK attribute.

Couple of them failed. @SenRamakri Shall we fix those targets ?

0xc0170 · 2018-01-22T09:54:25Z

rtos/TARGET_CORTEX/mbed_rtx_fault_handler.c

+
+    fault_print_str("\n\n-- MbedOS Fault Handler --\n\n",NULL);
+
+    /* Just spin here, we have alrady crashed */


alrady -> already

0xc0170 · 2018-01-22T09:54:35Z

rtos/TARGET_CORTEX/mbed_rtx_fault_handler.c

+    fault_print_str("\n++ MbedOS Fault Handler ++\n\nFaultType: ",NULL);
+
+    switch( fault_type ) {
+      case HARD_FAULT_EXCEPTION: fault_print_str("HardFault",NULL); break;


each statement own line

0xc0170 · 2018-01-22T09:56:01Z

rtos/TARGET_CORTEX/mbed_rtx_fault_handler.h

+#define BUS_FAULT_EXCEPTION        (0x30)
+#define USAGE_FAULT_EXCEPTION      (0x40)
+
+__NO_RETURN void mbed_fault_handler (uint32_t fault_type, void *mbed_fault_context_in, void *osRtxInfoIn);


func documentation missing

I'll add some documentation here, but note that this is not a public API, so its not expected to be called anyone else part from the Fault handlers. So, I'll add some non-doxygen comments.

0xc0170 · 2018-01-22T09:56:52Z

tools/debug_tools/crash_log_parser/crash_log_parser.py

@@ -0,0 +1,266 @@
+#!/usr/bin/env python


license addition here

0xc0170 · 2018-01-22T09:57:17Z

tools/debug_tools/crash_log_parser/crash_log_parser.py

+
+    def file_name_for_function_name(self, funcname):
+        for eachline in self.maplines:
+            #print("%s:%s"%(eachline,funcname))


dead code should be removed in this file

Thanks for the review, and will fix the comments. After some discussion here and getting some feed backs, I'm going to make this a build time enabled feature. We will also make this depend on if TARGET_CORTEX_M is defined so that it doesn't fail on Cortex-A targets.

SenRamakri · 2018-01-26T23:34:12Z

/morph build

mbed-ci · 2018-01-27T00:19:49Z

Build : FAILURE

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

SenRamakri · 2018-01-27T05:33:06Z

/morph build

mbed-ci · 2018-01-27T06:01:51Z

Build : FAILURE

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

SenRamakri · 2018-01-27T20:22:32Z

targets/targets.json

@@ -3575,7 +3575,7 @@
            }
        },
        "inherits": ["Target"],
-        "macros": ["CMSIS_VECTAB_VIRTUAL", "CMSIS_VECTAB_VIRTUAL_HEADER_FILE=\"cmsis_nvic.h\""],


We already enable other features like MBED_HEAP_STATS_ENABLED, MBED_STACK_STATS_ENABLED, MBED_TRAP_ERRORS_ENABLED on this platform for testing. So disabling mbed-os hardfault handler feature as this target is limited on ram/rom.

SenRamakri · 2018-01-27T20:23:20Z

targets/targets.json

@@ -3613,7 +3613,7 @@
        "inherits": ["Target"],
        "detect_code": ["4600"],
        "extra_labels": ["Realtek", "AMEBA", "RTL8195A"],
-        "macros": ["__RTL8195A__","CONFIG_PLATFORM_8195A","CONFIG_MBED_ENABLED","PLATFORM_CMSIS_RTOS"],


REALTEK_AMEBA provides their own implementation of HardFault handler as binary. So, disabling Mbed-OS fault handling.

Thanks for the details, they should be part of the commit message to have them available not only on github

MBED_FAULT_HANDLER_DISABLED vs CONFIG_MBED_ENABLED (enabled or disabled might be confusing, so one of these should be fixed?). Can we have this fault handler enabled by default, and only removed if not used (first comes to my mind using Target from targets to define it , or another way to do this) ?

@0xc0170 - Yes, Currently this is enabled by default and is disabled only if you specify MBED_FAULT_HANDLER_DISABLED flag. targets.json can be used to disable this on specific targets if not required. I think that matches your expectations?

@SenRamakri Could you make this configurable through the configuration system? Currently it's a macro, and we would like to eliminate macros going forward.

{ "name": "rtos", "config": { "present": 1, "detailed-fault-handler": { "help": "Configures the detail of the fault handler", "value": true } }, "target_overrides" : { "K64F": { "detailed-fault-handler": false } } }

@0xc0170 - Looks like we cannot use Json based config system for this as the Config defines are not visible in .S files for IAR toolchain. @theotherjimmy and I tried to work through this but couldn't really make it work for IAR due to some differences in how IAR preprocessing works. So, reverting back to old mechanism, unfortunately.

SenRamakri · 2018-01-27T20:23:58Z

tools/debug_tools/crash_log_parser/crash_log_parser.py

@@ -0,0 +1,278 @@
+#!/usr/bin/env python
+"""


License added

SenRamakri · 2018-01-28T04:31:22Z

/morph build

mbed-ci · 2018-01-28T05:00:48Z

Build : SUCCESS

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

Triggering tests

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

mbed-ci · 2018-01-28T06:09:38Z

Test : SUCCESS

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

mbed-ci · 2018-01-28T06:31:14Z

Exporter Build : SUCCESS

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

cmonr · 2018-01-29T16:31:18Z

/morph uvisor-test

cmonr · 2018-01-29T16:31:54Z

Any comments @sg- @c1728p9 @studavekar?

mbed-ci · 2018-01-29T16:51:02Z

@SenRamakri, thank you for your changes. You should get some feedback from the following people shortly: @bulislaw @ARMmbed/team-st-mcd @c1728p9 @pan- @mbed-os-maintainers @theotherjimmy @scartmell-arm @ARMmbed/team-onsemi please review.

theotherjimmy

Please let me know if you want help with any of this!

theotherjimmy · 2018-01-29T17:12:29Z

tools/debug_tools/crash_log_parser/crash_log_parser.py

+
+from __future__ import print_function
+from os import path
+import re, bisect


Could you split this import into two lines to match the style throughout the rest of the tools?

theotherjimmy · 2018-01-29T17:12:50Z

tools/debug_tools/crash_log_parser/crash_log_parser.py

+NM_EXEC = "arm-none-eabi-nm"
+OPT = "-nlC"
+ptn = re.compile("([0-9a-f]*) ([Tt]) ([^\t\n]*)(?:\t(.*):([0-9]*))?")
+fnt = None


This is reassigned on the next line. Please delete this line.

theotherjimmy · 2018-01-29T17:13:20Z

tools/debug_tools/crash_log_parser/crash_log_parser.py

+#arm-none-eabi-nm -nl <elf file>
+NM_EXEC = "arm-none-eabi-nm"
+OPT = "-nlC"
+ptn = re.compile("([0-9a-f]*) ([Tt]) ([^\t\n]*)(?:\t(.*):([0-9]*))?")


Could this be uppercase, and start with an underscore. That would indicate that it's a private global variable.

theotherjimmy · 2018-01-29T17:13:34Z

tools/debug_tools/crash_log_parser/crash_log_parser.py

+import sys
+
+#arm-none-eabi-nm -nl <elf file>
+NM_EXEC = "arm-none-eabi-nm"


So we don't support ARM or IAR here, do we?

theotherjimmy · 2018-01-29T17:14:35Z

tools/debug_tools/crash_log_parser/crash_log_parser.py

+        else:    
+            return toks[-1]
+
+def parseHFSR(hfsr):


This function both parses and prints. Maybe a different name is in order.

theotherjimmy · 2018-01-29T17:45:55Z

tools/debug_tools/crash_log_parser/crash_log_parser.py

+
+    p = args.elfpath
+    m = args.mappath
+    i = args.input


Please don't use one letter variable names. They're hard to follow.

theotherjimmy · 2018-01-29T17:46:28Z

tools/debug_tools/crash_log_parser/crash_log_parser.py

+
+    # specify arguments
+    parser.add_argument('-i','--input', metavar='<path to input file with crash log output>', type=str,
+                       help='path to input file')      


What is an input file? Please give the format for this file type.

theotherjimmy · 2018-01-29T17:47:07Z

tools/debug_tools/crash_log_parser/crash_log_parser.py

+    parser.add_argument('-e','--elfpath', metavar='<module or elf file path>', type=str,
+                       help='path to elf file')             
+    parser.add_argument('-m','--mappath', metavar='<map file path>', type=str,
+                       help='path to map file')                                    


The three prior arguments are all required. Help the user by adding required=True to each of the add_argument calls.

Further they're all files, so please use FileType as the type instead of str, which is the defualt.

theotherjimmy · 2018-01-29T17:49:16Z

tools/debug_tools/crash_log_parser/crash_log_parser.py

+    if not path.exists(i):
+        print("Input crash log path %s does not exist"%(str(i)))
+        parser.print_usage()
+        sys.exit(1) 


These prior 4 checks will all be eliminated by the use of FileType as described above.

theotherjimmy · 2018-01-29T17:49:52Z

tools/debug_tools/crash_log_parser/crash_log_parser.py

+    print("Inputs:")        
+    print("\tCrash Log: %s"%(i))        
+    print("\tElf Path: %s"%(p))        
+    print("\tMap Path: %s"%(m))        


This is wonderful for debugging the script, but useless to the user. Please remove these prints.

theotherjimmy · 2018-01-29T17:51:09Z

@SenRamakri Your title seems to have slipped off the maximum length. Let me get that for you.

mbed-ci · 2018-01-31T17:07:33Z

Test : SUCCESS

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

mbed-ci · 2018-01-31T17:39:29Z

Exporter Build : SUCCESS

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

theotherjimmy

One last nit

theotherjimmy · 2018-01-31T23:23:48Z

tools/debug_tools/crash_log_parser/crash_log_parser.py

+        return funcname
+
+def print_HFSR_info(hfsr):
+    if hfsr != 0:            


Could you drop this line please?

cmonr · 2018-02-01T00:32:50Z

@theotherjimmy Does this latest commit mean you're review is now all good?

theotherjimmy

I would like these fixed, but I'm not going to block this on 2 small style nits.

theotherjimmy · 2018-02-01T16:21:36Z

tools/debug_tools/crash_log_parser/crash_log_parser.py

+    else:        
+        print("\t\tProcessor Arch: ARM-V7M or above")
+
+    print("\t\tProcessor Variant: %X"%((int(cpuid,16) & 0xFFF0 ) >> 4))    


Could you add a space on each side of the %?

theotherjimmy · 2018-02-01T16:22:12Z

tools/debug_tools/crash_log_parser/crash_log_parser.py

+
+
+
+


Could you reduce the number of extra lines at the end of the file to 1?

studavekar · 2018-02-09T19:42:56Z

Thanks @SenRamakri for this feature looks good!

crash_log_parser.py utility is really helpful to quickly decode the crash 👍

0xc0170 · 2018-02-12T16:12:59Z

/morph build

mbed-ci · 2018-02-12T16:41:31Z

Build : FAILURE

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

cmonr · 2018-02-12T17:37:50Z

Build failure is due to a new device being introduced since the last rebase. Rebasing the PR should fix the issue.

…ions

…eview/other fixes

cmonr · 2018-02-12T18:14:23Z

/morph build

mbed-ci · 2018-02-12T18:40:15Z

Build : SUCCESS

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

Triggering tests

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

mbed-ci · 2018-02-12T20:36:00Z

Exporter Build : SUCCESS

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

mbed-ci · 2018-02-12T21:49:30Z

Test : SUCCESS

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

SenRamakri requested review from sg-, 0xc0170, studavekar and c1728p9 January 12, 2018 22:00

0xc0170 added the needs: work label Jan 15, 2018

0xc0170 reviewed Jan 22, 2018

View reviewed changes

SenRamakri force-pushed the sen_FaultHandler branch from 4480b9d to 9529da0 Compare January 26, 2018 23:33

SenRamakri force-pushed the sen_FaultHandler branch from 211cd35 to f5bdab9 Compare January 27, 2018 20:18

SenRamakri commented Jan 27, 2018

View reviewed changes

tools/debug_tools/crash_log_parser/crash_log_parser.py

@@ -0,0 +1,278 @@

#!/usr/bin/env python

"""

Copy link

Contributor Author

SenRamakri Jan 27, 2018

Choose a reason for hiding this comment

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

License added

mbed-ci added the needs: review label Jan 29, 2018

theotherjimmy suggested changes Jan 29, 2018

View reviewed changes

theotherjimmy changed the title ~~Support for generating core/register/thread-info dump on fault except…~~ Unify fault handling and add script to show faults Jan 29, 2018

SenRamakri force-pushed the sen_FaultHandler branch from 91ccce7 to 857ee13 Compare January 31, 2018 17:05

theotherjimmy suggested changes Jan 31, 2018

View reviewed changes

SenRamakri force-pushed the sen_FaultHandler branch from 8ebbcd4 to bc43459 Compare January 31, 2018 23:40

cmonr added needs: review and removed needs: work labels Feb 1, 2018

theotherjimmy approved these changes Feb 1, 2018

View reviewed changes

studavekar approved these changes Feb 9, 2018

View reviewed changes

0xc0170 approved these changes Feb 12, 2018

View reviewed changes

cmonr added needs: work and removed needs: review labels Feb 12, 2018

SenRamakri added 5 commits February 12, 2018 11:50

Support for generating core/register/thread-info dump on fault except…

29348d8

…ions

Fixes for targets with invalid HardFault_Handler implementation and r…

96d900c

…eview/other fixes

crash_log_parser review comment fixes

8967b52

Fix code style issues and update documentation

3340a02

Fix MTB_MTS_XDOT build breakage

19ad4e2

SenRamakri force-pushed the sen_FaultHandler branch from bc43459 to 19ad4e2 Compare February 12, 2018 18:06

cmonr merged commit 84f42f6 into ARMmbed:master Feb 12, 2018

cmonr removed the needs: work label Feb 12, 2018

cmonr added the release-version: 5.8.0-rc1 label Feb 28, 2018


		fault_print_str("\n\n-- MbedOS Fault Handler --\n\n",NULL);

		/* Just spin here, we have alrady crashed */

Unify fault handling and add script to show faults #5847

Unify fault handling and add script to show faults #5847

Uh oh!

Conversation

SenRamakri commented Jan 12, 2018

Uh oh!

SenRamakri commented Jan 12, 2018

Uh oh!

SenRamakri commented Jan 12, 2018

Uh oh!

mbed-ci commented Jan 12, 2018

Uh oh!

0xc0170 commented Jan 22, 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!

SenRamakri Jan 22, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SenRamakri commented Jan 26, 2018

Uh oh!

mbed-ci commented Jan 27, 2018

Uh oh!

SenRamakri commented Jan 27, 2018

Uh oh!

mbed-ci commented Jan 27, 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SenRamakri commented Jan 28, 2018

Uh oh!

mbed-ci commented Jan 28, 2018

Triggering tests

Uh oh!

mbed-ci commented Jan 28, 2018

Uh oh!

mbed-ci commented Jan 28, 2018

Uh oh!

cmonr commented Jan 29, 2018

Uh oh!

cmonr commented Jan 29, 2018

Uh oh!

mbed-ci commented Jan 29, 2018

Uh oh!

theotherjimmy left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SenRamakri Jan 22, 2018 •

edited

Loading