armv8m: make the debugger handle better faults through ICSR #2187
Conversation
dragonmux
left a comment
dragonmux
left a comment
There was a problem hiding this comment.
Got a few notes on going through this PR - with them fixed though we'll be happy to merge this.
| #define CORTEXM_ICSR_VEC_PENDING(x) (((x) >> 12) & 0x1ff) | ||
| #define CORTEXM_ICSR_VEC_ACTIVE(x) (((x) >> 0) & 0x1ff) |
There was a problem hiding this comment.
Please suffix the numbers here with U so the bit shifting and masking is done unsigned.
| #define CORTEXM_DWT_MASK(i) (CORTEXM_DWT_BASE + 0x024U + (0x10U * (i))) | ||
| #define CORTEXM_DWT_FUNC(i) (CORTEXM_DWT_BASE + 0x028U + (0x10U * (i))) | ||
|
|
||
| /* Arm V8 External Debug Fault Status Register */ |
| #define CORTEXM_XPSR_THUMB (1U << 24U) | ||
| #define CORTEXM_XPSR_EXCEPTION_MASK 0x0000001fU | ||
|
|
||
| /* ICSR for ArmV8m, the exception are the same as IPSR */ |
| if ((target->target_options & CORTEXM_TOPT_FLAVOUR_V8M)) { | ||
| const uint32_t icsr = target_mem32_read32(target, CORTEXM_ICSR); | ||
| const uint32_t pending = CORTEXM_ICSR_VEC_PENDING(icsr); | ||
| if (pending != 0 && pending < 8) // catch all faults |
There was a problem hiding this comment.
Given pending is unsigned, perhaps (pending > 0U && pending < 8U)? - also please let clang-format run here, the { should be on this line (or, well.. more the point.. aren't necessary with this if block at all).
Perhaps re-express this as fault_state = pending > 0U && pending < 8U;?
| { | ||
| fault_state = true; | ||
| } | ||
| } else { |
There was a problem hiding this comment.
Please drop the else block braces, they're not necessary and not part of the code base code style.
| fault_state = true; | ||
| } | ||
| } else { | ||
| fault_state = !!(dfsr & CORTEXM_DFSR_VCATCH); |
There was a problem hiding this comment.
Rather than using !! which is inobvious, please use != 0U on the end of the expression for the bool conversion - it won't change codegen, but it does improve code clarity by better expression your intent, which isn't to double negate the value, but rather to check the value is not 0.
|
I'll update the MR this weekend |
2 participants
Detailed description
The ArmV8m does not handle faults the same way. The two main differences are :
As a result the current code does not handle fault very well.
Put a breakpoint on a fault (for example asm("udf #0") and try to "si" on it.
Gdb will appear to get stuck there because the debugger will see the state BEFORE the fault is actually executed and as a result not detect the fault.
The MR adds a separate path that , for ArmV8M, it checks if a fault is pending on ICSR
The "si" command on a fault now works and you proceed on to the handler.
I tested it on a RP2350 and on another CortexM33 successfully.
I'm not convinced all cases are covered as some parts are configurable, but at least it's working better.
Not sure either if the new two new registers should be renamed with V8M in the name or something.
Your checklist for this pull request
Closing issues
N/A