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

Unify fault handling and add script to show faults #5847

Merged
merged 5 commits into from Feb 12, 2018

Conversation

Projects
None yet
6 participants
@SenRamakri
Contributor

SenRamakri commented Jan 12, 2018

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 SenRamakri requested review from sg-, 0xc0170, studavekar and c1728p9 Jan 12, 2018

@SenRamakri

This comment has been minimized.

Contributor

SenRamakri commented Jan 12, 2018

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

@SenRamakri

This comment has been minimized.

Contributor

SenRamakri commented Jan 12, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Jan 12, 2018

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

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jan 22, 2018

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 ?

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

This comment has been minimized.

@0xc0170

0xc0170 Jan 22, 2018

Member

alrady -> already

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

This comment has been minimized.

@0xc0170

0xc0170 Jan 22, 2018

Member

each statement own line

#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);

This comment has been minimized.

@0xc0170

0xc0170 Jan 22, 2018

Member

func documentation missing

This comment has been minimized.

@SenRamakri

SenRamakri Jan 22, 2018

Contributor

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.

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

This comment has been minimized.

@0xc0170

0xc0170 Jan 22, 2018

Member

license addition here

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

This comment has been minimized.

@0xc0170

0xc0170 Jan 22, 2018

Member

dead code should be removed in this file

This comment has been minimized.

@SenRamakri

SenRamakri Jan 22, 2018

Contributor

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 SenRamakri force-pushed the SenRamakri:sen_FaultHandler branch from 4480b9d to 9529da0 Jan 26, 2018

@SenRamakri

This comment has been minimized.

Contributor

SenRamakri commented Jan 26, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Jan 27, 2018

@SenRamakri

This comment has been minimized.

Contributor

SenRamakri commented Jan 27, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Jan 27, 2018

@SenRamakri SenRamakri force-pushed the SenRamakri:sen_FaultHandler branch from 211cd35 to f5bdab9 Jan 27, 2018

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

This comment has been minimized.

@SenRamakri

SenRamakri Jan 27, 2018

Contributor

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.

@@ -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"],

This comment has been minimized.

@SenRamakri

SenRamakri Jan 27, 2018

Contributor

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

This comment has been minimized.

@0xc0170

0xc0170 Jan 29, 2018

Member

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) ?

This comment has been minimized.

@SenRamakri

SenRamakri Jan 29, 2018

Contributor

@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?

This comment has been minimized.

@theotherjimmy

theotherjimmy Jan 29, 2018

Contributor

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

This comment has been minimized.

@theotherjimmy

theotherjimmy Jan 30, 2018

Contributor
{                                                                                                        
    "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                                                              
        }                                                                                                
    }                                                                                                    
}

This comment has been minimized.

@SenRamakri

SenRamakri Jan 30, 2018

Contributor

@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.

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

This comment has been minimized.

@SenRamakri

SenRamakri Jan 27, 2018

Contributor

License added

@SenRamakri

This comment has been minimized.

Contributor

SenRamakri commented Jan 28, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Jan 28, 2018

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

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jan 29, 2018

/morph uvisor-test

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jan 29, 2018

Any comments @sg- @c1728p9 @studavekar?

@mbed-ci

This comment has been minimized.

mbed-ci commented Jan 29, 2018

@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!

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

This comment has been minimized.

@theotherjimmy

theotherjimmy Jan 29, 2018

Contributor

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

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

This comment has been minimized.

@theotherjimmy

theotherjimmy Jan 29, 2018

Contributor

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

#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]*))?")

This comment has been minimized.

@theotherjimmy

theotherjimmy Jan 29, 2018

Contributor

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

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

This comment has been minimized.

@theotherjimmy

theotherjimmy Jan 29, 2018

Contributor

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

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

This comment has been minimized.

@theotherjimmy

theotherjimmy Jan 29, 2018

Contributor

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

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

This comment has been minimized.

@theotherjimmy

theotherjimmy Jan 29, 2018

Contributor

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

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

This comment has been minimized.

@theotherjimmy

theotherjimmy Jan 29, 2018

Contributor

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

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')

This comment has been minimized.

@theotherjimmy

theotherjimmy Jan 29, 2018

Contributor

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.

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

This comment has been minimized.

@theotherjimmy

theotherjimmy Jan 29, 2018

Contributor

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

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

This comment has been minimized.

@theotherjimmy

theotherjimmy Jan 29, 2018

Contributor

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

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jan 29, 2018

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

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

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@theotherjimmy

One last nit

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

This comment has been minimized.

@theotherjimmy

theotherjimmy Jan 31, 2018

Contributor

Could you drop this line please?

@SenRamakri SenRamakri force-pushed the SenRamakri:sen_FaultHandler branch from 8ebbcd4 to bc43459 Jan 31, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Feb 1, 2018

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

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

@theotherjimmy

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

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

This comment has been minimized.

@theotherjimmy

theotherjimmy Feb 1, 2018

Contributor

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

This comment has been minimized.

@theotherjimmy

theotherjimmy Feb 1, 2018

Contributor

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

@studavekar

This comment has been minimized.

Collaborator

studavekar commented Feb 9, 2018

Thanks @SenRamakri for this feature looks good!

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

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Feb 12, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Feb 12, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Feb 12, 2018

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

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

@SenRamakri SenRamakri force-pushed the SenRamakri:sen_FaultHandler branch from bc43459 to 19ad4e2 Feb 12, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Feb 12, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Feb 12, 2018

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

This comment has been minimized.

@mbed-ci

This comment has been minimized.

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

17 checks passed

AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-exporter 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
mbed-ci-generic Build finished.
Details
travis-ci/docs/ Local docs testing has passed
Details
travis-ci/events/ Local events testing has passed
Details
travis-ci/littlefs/ Local littlefs testing has passed
Details
travis-ci/mbed2-ATMEL/ Local mbed2-ATMEL testing has passed
Details
travis-ci/mbed2-MAXIM/ Local mbed2-MAXIM testing has passed
Details
travis-ci/mbed2-NORDIC/ Local mbed2-NORDIC testing has passed
Details
travis-ci/mbed2-NUVOTON/ Local mbed2-NUVOTON testing has passed
Details
travis-ci/mbed2-NXP/ Local mbed2-NXP testing has passed
Details
travis-ci/mbed2-SILICON_LABS/ Local mbed2-SILICON_LABS testing has passed
Details
travis-ci/mbed2-STM/ Local mbed2-STM testing has passed
Details

@cmonr cmonr removed the needs: work label Feb 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment