[01/10] Fix breakpoint size when stepping over a permanent breakpoint in GDBServer.
Commit Message
When manually stepping over a permanent breakpoint on ARM we need to fetch the
right breakpoint size based on the current instruction set used.
Since this is not encoded in the stop_pc, the instruction mode needs to be
fetched from the CPSR register.
This is done by introducing a new target operation called :
breakpoint_kind_from_current_state.
For other targets that do not need this, breakpoint_kind_from_pc is used.
No regressions, tested on ubuntu 14.04 ARMv7 and x86.
With gdbserver-{native,extended} / { -marm -mthumb }
gdb/gdbserver/ChangeLog:
* linux-arm-low.c (arm_is_thumb_mode): New function.
(arm_breakpoint_at): Use arm_is_thumb_mode.
(arm_breakpoint_kind_from_current_state): New function.
(struct linux_target_ops) <breakpoint_kind_from_current_state>:
Initialize.
* linux-low.c (linux_wait_1): Call breakpoint_kind_from_current_state.
(linux_breakpoint_kind_from_current_state): New function.
(struct target_ops <breakpoint_kind_from_current_state>: Initialize.
* linux-low.h (struct linux_target_ops)
<breakpoint_kind_from_current_state>: New field.
* target.h (struct target_ops): Likewise.
(target_breakpoint_kind_from_current_state): New macro.
---
gdb/gdbserver/linux-arm-low.c | 38 +++++++++++++++++++++++++++++++++++++-
gdb/gdbserver/linux-low.c | 18 ++++++++++++++++--
gdb/gdbserver/linux-low.h | 3 +++
gdb/gdbserver/target.h | 11 +++++++++++
4 files changed, 67 insertions(+), 3 deletions(-)
Comments
Antoine Tremblay <antoine.tremblay@ericsson.com> writes:
> static int
> -arm_breakpoint_at (CORE_ADDR where)
> +arm_is_thumb_mode (void)
> {
Please add comments to arm_is_thumb_mode.
> diff --git a/gdb/gdbserver/target.h b/gdb/gdbserver/target.h
> index 769c876..f0fb9d7 100644
> --- a/gdb/gdbserver/target.h
> +++ b/gdb/gdbserver/target.h
> @@ -451,6 +451,12 @@ struct target_ops
> specific meaning like the Z0 kind parameter.
> SIZE is set to the software breakpoint's length in memory. */
> const gdb_byte *(*sw_breakpoint_from_kind) (int kind, int *size);
> +
> + /* Return the breakpoint kind for this target based on the current state
Maybe "current machine state" or "current processor state" is better
than "current state"?
> + (e.g. the current instruction mode on ARM) and the PC. The PCPTR is
Two spaces before "The PCPTR".
> + adjusted to the real memory location in case a flag (e.g., the Thumb
> + bit on ARM) was present in the PC. */
"is present in the PC." ?
Otherwise, looks good to me.
On 11/03/2015 11:21 AM, Yao Qi wrote:
> Antoine Tremblay <antoine.tremblay@ericsson.com> writes:
>
>> static int
>> -arm_breakpoint_at (CORE_ADDR where)
>> +arm_is_thumb_mode (void)
>> {
>
> Please add comments to arm_is_thumb_mode.
Fixed as : /* Returns 1 if the current instruction set is thumb, 0
otherwise. */
>
>> diff --git a/gdb/gdbserver/target.h b/gdb/gdbserver/target.h
>> index 769c876..f0fb9d7 100644
>> --- a/gdb/gdbserver/target.h
>> +++ b/gdb/gdbserver/target.h
>> @@ -451,6 +451,12 @@ struct target_ops
>> specific meaning like the Z0 kind parameter.
>> SIZE is set to the software breakpoint's length in memory. */
>> const gdb_byte *(*sw_breakpoint_from_kind) (int kind, int *size);
>> +
>> + /* Return the breakpoint kind for this target based on the current state
>
> Maybe "current machine state" or "current processor state" is better
> than "current state"?
Agreed. set as "current processor state"
>
>> + (e.g. the current instruction mode on ARM) and the PC. The PCPTR is
>
> Two spaces before "The PCPTR".
Fixed.
>
>> + adjusted to the real memory location in case a flag (e.g., the Thumb
>> + bit on ARM) was present in the PC. */
>
> "is present in the PC." ?
>
Fixed, thanks.
@@ -265,7 +265,7 @@ static const unsigned short thumb2_breakpoint[] = { 0xf7f0, 0xa000 };
#define thumb2_breakpoint_len 4
static int
-arm_breakpoint_at (CORE_ADDR where)
+arm_is_thumb_mode (void)
{
struct regcache *regcache = get_thread_regcache (current_thread, 1);
unsigned long cpsr;
@@ -273,6 +273,17 @@ arm_breakpoint_at (CORE_ADDR where)
collect_register_by_name (regcache, "cpsr", &cpsr);
if (cpsr & 0x20)
+ return 1;
+ else
+ return 0;
+}
+
+/* Returns 1 if there is a software breakpoint at location. */
+
+static int
+arm_breakpoint_at (CORE_ADDR where)
+{
+ if (arm_is_thumb_mode ())
{
/* Thumb mode. */
unsigned short insn;
@@ -993,6 +1004,23 @@ arm_sw_breakpoint_from_kind (int kind , int *size)
return NULL;
}
+/* Implementation of the linux_target_ops method
+ "breakpoint_kind_from_current_state". */
+
+static int
+arm_breakpoint_kind_from_current_state (CORE_ADDR *pcptr)
+{
+ if (arm_is_thumb_mode ())
+ {
+ *pcptr = MAKE_THUMB_ADDR (*pcptr);
+ return arm_breakpoint_kind_from_pc (pcptr);
+ }
+ else
+ {
+ return arm_breakpoint_kind_from_pc (pcptr);
+ }
+}
+
struct linux_target_ops the_low_target = {
arm_arch_setup,
arm_regs_info,
@@ -1018,6 +1046,14 @@ struct linux_target_ops the_low_target = {
arm_new_thread,
arm_new_fork,
arm_prepare_to_resume,
+ NULL, /* process_qsupported */
+ NULL, /* supports_tracepoints */
+ NULL, /* get_thread_area */
+ NULL, /* install_fast_tracepoint_jump_pad */
+ NULL, /* emit_ops */
+ NULL, /* get_min_fast_tracepoint_insn_len */
+ NULL, /* supports_range_stepping */
+ arm_breakpoint_kind_from_current_state
};
void
@@ -3006,7 +3006,8 @@ linux_wait_1 (ptid_t ptid,
int breakpoint_kind = 0;
CORE_ADDR stop_pc = event_child->stop_pc;
- breakpoint_kind = the_target->breakpoint_kind_from_pc (&stop_pc);
+ breakpoint_kind =
+ the_target->breakpoint_kind_from_current_state (&stop_pc);
the_target->sw_breakpoint_from_kind (breakpoint_kind, &increment_pc);
if (debug_threads)
@@ -6948,6 +6949,18 @@ linux_sw_breakpoint_from_kind (int kind, int *size)
return (*the_low_target.sw_breakpoint_from_kind) (kind, size);
}
+/* Implementation of the target_ops method
+ "breakpoint_kind_from_current_state". */
+
+static int
+linux_breakpoint_kind_from_current_state (CORE_ADDR *pcptr)
+{
+ if (the_low_target.breakpoint_kind_from_current_state != NULL)
+ return (*the_low_target.breakpoint_kind_from_current_state) (pcptr);
+ else
+ return linux_breakpoint_kind_from_pc (pcptr);
+}
+
static struct target_ops linux_target_ops = {
linux_create_inferior,
linux_arch_setup,
@@ -7043,7 +7056,8 @@ static struct target_ops linux_target_ops = {
linux_mntns_unlink,
linux_mntns_readlink,
linux_breakpoint_kind_from_pc,
- linux_sw_breakpoint_from_kind
+ linux_sw_breakpoint_from_kind,
+ linux_breakpoint_kind_from_current_state
};
static void
@@ -233,6 +233,9 @@ struct linux_target_ops
/* Returns true if the low target supports range stepping. */
int (*supports_range_stepping) (void);
+
+ /* See target.h. */
+ int (*breakpoint_kind_from_current_state) (CORE_ADDR *pcptr);
};
extern struct linux_target_ops the_low_target;
@@ -451,6 +451,12 @@ struct target_ops
specific meaning like the Z0 kind parameter.
SIZE is set to the software breakpoint's length in memory. */
const gdb_byte *(*sw_breakpoint_from_kind) (int kind, int *size);
+
+ /* Return the breakpoint kind for this target based on the current state
+ (e.g. the current instruction mode on ARM) and the PC. The PCPTR is
+ adjusted to the real memory location in case a flag (e.g., the Thumb
+ bit on ARM) was present in the PC. */
+ int (*breakpoint_kind_from_current_state) (CORE_ADDR *pcptr);
};
extern struct target_ops *the_target;
@@ -638,6 +644,11 @@ int kill_inferior (int);
? (*the_target->breakpoint_kind_from_pc) (pcptr) \
: default_breakpoint_kind_from_pc (pcptr))
+#define target_breakpoint_kind_from_current_state(pcptr) \
+ (the_target->breakpoint_kind_from_current_state \
+ ? (*the_target->breakpoint_kind_from_current_state) (pcptr) \
+ : target_breakpoint_kind_from_pc (pcptr))
+
/* Start non-stop mode, returns 0 on success, -1 on failure. */
int start_non_stop (int nonstop);