foss-for-synopsys-dwc-arc-processors/openocd — pull request #3 by anthony-kolesov (2013-08-21 11:50:02+00:00)
Title: ARC: Examine whether execution halted due to software breakpoint

This patch adds arc_dbg_examine_debug_reason function, which determines whether execution has stopped due to software breakpoint (instruction brk_s).

This commit fixes invalid behaviour of GDB client due to malicious behaviour of GDB server in OpenOCD. It was observed that several GDB features doesn't work as expected. Among them:

* command "next" over function call was stepping over instruction instead of over source line
* temporary breakpoints were not removed
* conditional breakpoints were hit unconditionally

This was caused by OpenOCD gdb_server returning T00-packet in certain conditions. Number after T means a signal that caused target application to halt and there is no signal 0, so in theory T00-packet should never be returned and indicates an error in gdb_server. GDB client instead of emiting any warning was trying its best to recover from situation and in some situation this wasn't making any difference in observed behaviour and in rest cases behaviour was not the one expected, but still not breaking things catastrophically. It was found that T00-packet was returned as a response to c-packet, single step s-packet was getting T05-packet as a response, where 5 means SIGTRAP - software breakpoint. c-packet also should get T05-packet in normal cases, so GDB client was a victim of invalid packet from gdb_server. Note that here GDB commands doesn't directly correspond to GDB RSP packets. For a particular test case of next over function GDB was emitting a seuqence of four packets: s, c, s, s; but because "c" was getting an unexpected response, GDB was confused and didn't emitted subsequent s-packets, thus execution was stopped inside current source line instead of next line.

Different result for continue and single step are explained by usage of different functions for these tasks in OpenOCD. Those functions (arc_dbg_step and arc_db_resume) should set a value to field target->debug_reason, so that later gdb_server will use this value to determine signal value for packet T. arc_dbg_step was setting target->debug_reason to DBG_REASON_SINGLESTEP, which was later converted to SIGTRAP by gdb_server. arc_dbg_result on the other hand was setting target->debug_reason to DBG_REASON_NOTHALTED, comment in gdb_server code mentiones that this should never happen when returning packet T, but still it was just returning zero value without any warnings/errors. Simpliest solution would be to set target->debug_reason to DBG_REASON_BREAKPOINT instead of DBG_REASON_NOTHALED right inside of arc_dbg_resume, but I suspect that this would be too good to be true, and as I think (I'm not sure) it can't cover all possible reasons to halt execution started by arc_dbg_resume.

After looking at solutions implemented by other targets (mips_m4k, arms) I've added a function arc_dbg_examine_debug_reason and a call of this function in arc_dbg_debug_entry. If target->debug_reason is not already set to meaningfull value (DBGRQ or SINGLESTEP), then it will attempt to determine the reason of stop. Actually it checks only for software breakpoints written in memory in form of brk_s instructions. If current instruction (the one pointed by PC value) is brk_s, than this is a software breakpoint and target->debug_reason is set to respective value otherwise target->debug_reason is not changed. Later support of hardware breakpoints can be added to this function. This function relies on the fact that software breakpoints are written to and out of memory by commands from GDB client which are executed only after arc_dbg_debug_entry.

After addition of arc_dbg_examine_debug_reason function target->debug_reason is set to DBG_REASON_BREAKPOINT after resume and then halt on breakpoint, which fixes behaviour for "normal" cases. I'm not sure this would work with some "special" cases when execution is stopped by anything that is not brk_s instruction (signals, HW breakpoints, etc). But I believe that those "special" cases wouldn't work either before, so this commit shouldn't break any existing functionality.

Signed-off-by: Anton Kolesov