[11/11] New target_ops hook to_can_do_single_step
Commit Message
Nowadays, GDB only knows whether architecture supports hardware single
step or software single step (through gdbarch hook software_single_step),
and for a given instruction or instruction sequence, GDB knows how to
do single step (hardware or software). However, GDB doesn't know whether
the target supports hardware single step. It is possible that the
architecture doesn't support hardware single step, such as arm, but
the target supports, such as simulator. This was discussed in this
thread https://www.sourceware.org/ml/gdb/2009-12/msg00033.html before.
I encounter this problem for aarch64 multi-arch support. When aarch64
debugs arm program, gdbarch is arm, so software single step is still
used. However, the underneath linux kernel does support hardware
single step, so IWBN to use it.
This patch is to add a new target_ops hook to_can_do_single_step, and
only use it in arm_linux_software_single_step to decide whether or not
to use hardware single step. On the remote target, if the target
supports s and S actions in the vCont? reply, then target can do single
step. On the native aarch64 linux target, 1 is returned. On other
targets, -1 is returned.
gdb:
2015-06-30 Yao Qi <yao.qi@linaro.org>
* aarch64-linux-nat.c (aarch64_linux_can_do_single_step): New function.
(_initialize_aarch64_linux_nat): Install it to to_can_do_single_step.
* arm-linux-tdep.c (arm_linux_software_single_step): Return 0
if target_can_do_single_step returns 1.
* remote.c (struct vCont_action_support) <s>: New field.
(remote_vcont_probe): Remove support_s and use
rs->supports_vCont.s instead.
(remote_can_do_single_step): New function.
(init_remote_ops): Install it to to_can_do_single_step.
* target.h (struct target_ops) <to_can_do_single_step>: New field.
(target_can_do_single_step): New macro.
* target-delegates.c: Re-generated.
---
gdb/aarch64-linux-nat.c | 9 +++++++++
gdb/arm-linux-tdep.c | 5 +++++
gdb/remote.c | 33 +++++++++++++++++++++++++++------
gdb/target-delegates.c | 31 +++++++++++++++++++++++++++++++
gdb/target.h | 9 +++++++++
5 files changed, 81 insertions(+), 6 deletions(-)
Comments
On 07/01/2015 02:58 PM, Yao Qi wrote:
> Nowadays, GDB only knows whether architecture supports hardware single
> step or software single step (through gdbarch hook software_single_step),
> and for a given instruction or instruction sequence, GDB knows how to
> do single step (hardware or software). However, GDB doesn't know whether
> the target supports hardware single step. It is possible that the
> architecture doesn't support hardware single step, such as arm, but
> the target supports, such as simulator. This was discussed in this
> thread https://www.sourceware.org/ml/gdb/2009-12/msg00033.html before.
>
> I encounter this problem for aarch64 multi-arch support. When aarch64
> debugs arm program, gdbarch is arm, so software single step is still
> used. However, the underneath linux kernel does support hardware
> single step, so IWBN to use it.
>
> This patch is to add a new target_ops hook to_can_do_single_step, and
> only use it in arm_linux_software_single_step to decide whether or not
> to use hardware single step. On the remote target, if the target
> supports s and S actions in the vCont? reply, then target can do single
> step. On the native aarch64 linux target, 1 is returned. On other
> targets, -1 is returned.
Yeah, I've wanted to do this before too.
But my issue with it is that this breaks gdb/gdbserver compatibility.
Old GDB has:
/* If s, S, c, and C are not all supported, we can't use vCont. Clearing
BUF will make packet_ok disable the packet. */
if (!support_s || !support_S || !support_c || !support_C)
buf[0] = 0;
Which means that new x86-86 gdbserver with old gdb will just
stop using vCont after this change.
And old arm gdbserver will still claim support for vCont;s packets,
which means that new gdb with old gdbserver will be broken.
I think this needs to be addressed somehow.
Thanks,
Pedro Alves
Pedro Alves <palves@redhat.com> writes:
> But my issue with it is that this breaks gdb/gdbserver compatibility.
>
> Old GDB has:
>
> /* If s, S, c, and C are not all supported, we can't use vCont. Clearing
> BUF will make packet_ok disable the packet. */
> if (!support_s || !support_S || !support_c || !support_C)
> buf[0] = 0;
>
> Which means that new x86-86 gdbserver with old gdb will just
> stop using vCont after this change.
I think you meant new arm gdbserver, which doesn't return s and S in the
reply, old gdb will stop using vCont.
>
> And old arm gdbserver will still claim support for vCont;s packets,
> which means that new gdb with old gdbserver will be broken.
That is right. I'll do it in qSupported features.
On 07/02/2015 09:56 AM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> writes:
>
>> But my issue with it is that this breaks gdb/gdbserver compatibility.
>>
>> Old GDB has:
>>
>> /* If s, S, c, and C are not all supported, we can't use vCont. Clearing
>> BUF will make packet_ok disable the packet. */
>> if (!support_s || !support_S || !support_c || !support_C)
>> buf[0] = 0;
>>
>> Which means that new x86-86 gdbserver with old gdb will just
>> stop using vCont after this change.
>
> I think you meant new arm gdbserver, which doesn't return s and S in the
> reply, old gdb will stop using vCont.
Yeah.
>
>>
>> And old arm gdbserver will still claim support for vCont;s packets,
>> which means that new gdb with old gdbserver will be broken.
>
> That is right. I'll do it in qSupported features.
Thanks.
@@ -1607,6 +1607,14 @@ aarch64_linux_watchpoint_addr_within_range (struct target_ops *target,
return start <= addr && start + length - 1 >= addr;
}
+/* Implement the "to_can_do_single_step" target_ops method. */
+
+static int
+aarch64_linux_can_do_single_step (struct target_ops *target)
+{
+ return 1;
+}
+
/* Define AArch64 maintenance commands. */
static void
@@ -1658,6 +1666,7 @@ _initialize_aarch64_linux_nat (void)
t->to_stopped_data_address = aarch64_linux_stopped_data_address;
t->to_watchpoint_addr_within_range =
aarch64_linux_watchpoint_addr_within_range;
+ t->to_can_do_single_step = aarch64_linux_can_do_single_step;
/* Override the GNU/Linux inferior startup hook. */
super_post_startup_inferior = t->to_post_startup_inferior;
@@ -917,6 +917,11 @@ arm_linux_software_single_step (struct frame_info *frame)
if (arm_deal_with_atomic_sequence (frame))
return 1;
+ /* If the target does have hardware single step, GDB doesn't have
+ to bother software single step. */
+ if (target_can_do_single_step () == 1)
+ return 0;
+
next_pc = arm_get_next_pc (frame, get_frame_pc (frame));
/* The Linux kernel offers some user-mode helpers in a high page. We can
@@ -245,6 +245,12 @@ struct vCont_action_support
/* vCont;r */
int r;
+
+ /* vCont;s */
+ int s;
+
+ /* vCont;S */
+ int S;
};
/* Controls whether GDB is willing to use range stepping. */
@@ -4818,10 +4824,10 @@ remote_vcont_probe (struct remote_state *rs)
if (startswith (buf, "vCont"))
{
char *p = &buf[5];
- int support_s, support_S, support_c, support_C;
+ int support_c, support_C;
- support_s = 0;
- support_S = 0;
+ rs->supports_vCont.s = 0;
+ rs->supports_vCont.S = 0;
support_c = 0;
support_C = 0;
rs->supports_vCont.t = 0;
@@ -4830,9 +4836,9 @@ remote_vcont_probe (struct remote_state *rs)
{
p++;
if (*p == 's' && (*(p + 1) == ';' || *(p + 1) == 0))
- support_s = 1;
+ rs->supports_vCont.s = 1;
else if (*p == 'S' && (*(p + 1) == ';' || *(p + 1) == 0))
- support_S = 1;
+ rs->supports_vCont.S = 1;
else if (*p == 'c' && (*(p + 1) == ';' || *(p + 1) == 0))
support_c = 1;
else if (*p == 'C' && (*(p + 1) == ';' || *(p + 1) == 0))
@@ -4847,7 +4853,8 @@ remote_vcont_probe (struct remote_state *rs)
/* If s, S, c, and C are not all supported, we can't use vCont. Clearing
BUF will make packet_ok disable the packet. */
- if (!support_s || !support_S || !support_c || !support_C)
+ if (!rs->supports_vCont.s || !rs->supports_vCont.S
+ || !support_c || !support_C)
buf[0] = 0;
}
@@ -12147,6 +12154,19 @@ remote_pid_to_exec_file (struct target_ops *self, int pid)
return filename;
}
+/* Implement the to_can_do_single_step target_ops method. */
+
+static int
+remote_can_do_single_step (struct target_ops *ops)
+{
+ struct remote_state *rs = get_remote_state ();
+
+ if (packet_support (PACKET_vCont) == PACKET_SUPPORT_UNKNOWN)
+ remote_vcont_probe (rs);
+
+ return rs->supports_vCont.s && rs->supports_vCont.S;
+}
+
static void
init_remote_ops (void)
{
@@ -12216,6 +12236,7 @@ Specify the serial device it is connected to\n\
remote_ops.to_can_async_p = remote_can_async_p;
remote_ops.to_is_async_p = remote_is_async_p;
remote_ops.to_async = remote_async;
+ remote_ops.to_can_do_single_step = remote_can_do_single_step;
remote_ops.to_terminal_inferior = remote_terminal_inferior;
remote_ops.to_terminal_ours = remote_terminal_ours;
remote_ops.to_supports_non_stop = remote_supports_non_stop;
@@ -832,6 +832,33 @@ debug_masked_watch_num_registers (struct target_ops *self, CORE_ADDR arg1, CORE_
return result;
}
+static int
+delegate_can_do_single_step (struct target_ops *self)
+{
+ self = self->beneath;
+ return self->to_can_do_single_step (self);
+}
+
+static int
+tdefault_can_do_single_step (struct target_ops *self)
+{
+ return -1;
+}
+
+static int
+debug_can_do_single_step (struct target_ops *self)
+{
+ int result;
+ fprintf_unfiltered (gdb_stdlog, "-> %s->to_can_do_single_step (...)\n", debug_target.to_shortname);
+ result = debug_target.to_can_do_single_step (&debug_target);
+ fprintf_unfiltered (gdb_stdlog, "<- %s->to_can_do_single_step (", debug_target.to_shortname);
+ target_debug_print_struct_target_ops_p (&debug_target);
+ fputs_unfiltered (") = ", gdb_stdlog);
+ target_debug_print_int (result);
+ fputs_unfiltered ("\n", gdb_stdlog);
+ return result;
+}
+
static void
delegate_terminal_init (struct target_ops *self)
{
@@ -3935,6 +3962,8 @@ install_delegators (struct target_ops *ops)
ops->to_can_accel_watchpoint_condition = delegate_can_accel_watchpoint_condition;
if (ops->to_masked_watch_num_registers == NULL)
ops->to_masked_watch_num_registers = delegate_masked_watch_num_registers;
+ if (ops->to_can_do_single_step == NULL)
+ ops->to_can_do_single_step = delegate_can_do_single_step;
if (ops->to_terminal_init == NULL)
ops->to_terminal_init = delegate_terminal_init;
if (ops->to_terminal_inferior == NULL)
@@ -4197,6 +4226,7 @@ install_dummy_methods (struct target_ops *ops)
ops->to_region_ok_for_hw_watchpoint = default_region_ok_for_hw_watchpoint;
ops->to_can_accel_watchpoint_condition = tdefault_can_accel_watchpoint_condition;
ops->to_masked_watch_num_registers = tdefault_masked_watch_num_registers;
+ ops->to_can_do_single_step = tdefault_can_do_single_step;
ops->to_terminal_init = tdefault_terminal_init;
ops->to_terminal_inferior = tdefault_terminal_inferior;
ops->to_terminal_ours_for_output = tdefault_terminal_ours_for_output;
@@ -4345,6 +4375,7 @@ init_debug_target (struct target_ops *ops)
ops->to_region_ok_for_hw_watchpoint = debug_region_ok_for_hw_watchpoint;
ops->to_can_accel_watchpoint_condition = debug_can_accel_watchpoint_condition;
ops->to_masked_watch_num_registers = debug_masked_watch_num_registers;
+ ops->to_can_do_single_step = debug_can_do_single_step;
ops->to_terminal_init = debug_terminal_init;
ops->to_terminal_inferior = debug_terminal_inferior;
ops->to_terminal_ours_for_output = debug_terminal_ours_for_output;
@@ -557,6 +557,12 @@ struct target_ops
int (*to_masked_watch_num_registers) (struct target_ops *,
CORE_ADDR, CORE_ADDR)
TARGET_DEFAULT_RETURN (-1);
+
+ /* Return 1 for sure target can do single step. Return -1 for
+ unknown. Return 0 for target can't do. */
+ int (*to_can_do_single_step) (struct target_ops *)
+ TARGET_DEFAULT_RETURN (-1);
+
void (*to_terminal_init) (struct target_ops *)
TARGET_DEFAULT_IGNORE ();
void (*to_terminal_inferior) (struct target_ops *)
@@ -1861,6 +1867,9 @@ extern char *target_thread_name (struct thread_info *);
addr, len)
+#define target_can_do_single_step() \
+ (*current_target.to_can_do_single_step) (¤t_target)
+
/* Set/clear a hardware watchpoint starting at ADDR, for LEN bytes.
TYPE is 0 for write, 1 for read, and 2 for read/write accesses.
COND is the expression for its condition, or NULL if there's none.