armv8m: make the debugger handle better faults through ICSR#2187
armv8m: make the debugger handle better faults through ICSR#2187mean00 wants to merge 2 commits intoblackmagic-debug:mainfrom
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.
src/target/cortexm.c
Outdated
| 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;?
src/target/cortexm.c
Outdated
| { | ||
| 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.
|
I'll update the MR this weekend |
|
I think i got all the remarks as a separate commit for easy review |
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