[PATCH 0/2 V2] Support single step by arch or target

[PATCH 0/2 V2] Support single step by arch or target

[Yao Qi]

This is the V2 of my patch <https://sourceware.org/ml/gdb-patches/2015-07/msg00040.html>
to address Pedro's comment on backward compatibility problem.

Patch 1 is to rename target_ops hook supports_conditional_breakpoints
to supports_hardware_single_step, nothing changes since I post it last
time <https://sourceware.org/ml/gdb-patches/2015-07/msg00035.html>.

Patch 2 is the major one in this series, details can be found in it.

Regression tested on aarch64 with {native, gdbserver} x {32-bit, 64-bit}.

*** BLURB HERE ***

Yao Qi (2):
  [gdbserver] Rename supports_conditional_breakpoints to
    supports_hardware_single_step
  Support single step by arch or target

 gdb/aarch64-linux-nat.c   |  9 ++++++++
 gdb/arm-linux-tdep.c      |  5 ++++
 gdb/doc/gdb.texinfo       |  8 +++++++
 gdb/gdbserver/linux-low.c | 11 +++------
 gdb/gdbserver/lynx-low.c  |  5 +---
 gdb/gdbserver/nto-low.c   |  5 +---
 gdb/gdbserver/server.c    | 35 ++++++++++++++++++++++++----
 gdb/gdbserver/spu-low.c   |  2 +-
 gdb/gdbserver/target.c    |  8 +++++++
 gdb/gdbserver/target.h    | 13 ++++++-----
 gdb/gdbserver/win32-low.c |  5 +---
 gdb/remote.c              | 58 +++++++++++++++++++++++++++++++++++++++--------
 gdb/target-delegates.c    | 31 +++++++++++++++++++++++++
 gdb/target.h              |  9 ++++++++
 14 files changed, 164 insertions(+), 40 deletions(-)

-- 
1.9.1

[PATCH 2/2] Support single step by arch or target

[Yao Qi]

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 native aarch64 linux target, 1 is
returned.  On other targets, -1 is returned.  On the remote target, if
the target supports s and S actions in the vCont? reply, then target
can do single step.  However,  old GDBserver will send s and S in the
reply to vCont?, which will confuse new GDB.  For example, old GDBserver
on arm-linux will send s and S in the reply to vCont?, but it doesn't
support hardware single step.  On the other hand, new GDBserver, on
arm-linux for example, will not send s and S in the reply to vCont?,
but old GDB thinks it doesn't support vCont packet at all.  In order
to address this problem, I add a new qSupported feature vContSupported,
which indicates GDB wants to know the supported actions in the reply
to vCont?, and qSupported response contains vContSupported if the
stub is able tell supported vCont actions in the reply of vCont?.

If the patched GDB talks with patched GDBserver on x86, the RSP traffic
is like this:

 -> $qSupported:...+;vContSupported+
 <- ...+;vContSupported+
 ...
 -> $vCont?
 <- vCont;c;C;t;s;S;r

then, GDB knows the stub can do single step, and may stop using software
single step even the architecture doesn't support hardware single step.

If the patched GDB talks with patched GDBserver on arm, the last vCont?
reply will become:

 <- vCont;c;C;t

GDB thinks the target doesn't support single step, so it will use software
single step.

If the patched GDB talks with unpatched GDBserver, the RSP traffic is like
this:

 -> $qSupported:...+;vContSupported+
 <- ...+
 ...
 -> $vCont?
 <- vCont;c;C;t;s;S;r

although GDBserver returns s and S, GDB still thinks GDBserver may not
support single step because it doesn't support vContSupported.

If the unpatched GDB talks with unpatched GDBserver on x86, the RSP traffic
is like:

 -> $qSupported:...+;
 <- ...+;vContSupported+
 ...
 -> $vCont?
 <- vCont;c;C;t;s;S;r

Since GDB doesn't sent vContSupported in the qSupported feature, GDBserver
sends s and S regardless of the support of hardware single step.

gdb:

2015-09-01  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, S>: New fields.
	(PACKET_vContSupported): New enum.
	(remote_protocol_features): New element for vContSupported.
	(remote_query_supported): Append "vContSupported+".
	(remote_vcont_probe): Remove support_s and support_S, use
	rs->supports_vCont.s and rs->supports_vCont.S instead.  Disable
	vCont packet if c and C actions are not supported.
	(remote_can_do_single_step): New function.
	(init_remote_ops): Install it to to_can_do_single_step.
	(_initialize_remote): Call add_packet_config_cmd.
	* 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/gdbserver:

2015-09-01  Yao Qi  <yao.qi@linaro.org>

	* server.c (vCont_supported): New global variable.
	(handle_query): Set vCont_supported to 1 if "vContSupported+"
	matches.  Append ";vContSupported+" to own_buf.
	(handle_v_requests): Append ";s;S" to own_buf if target supports
	hardware single step or vCont_supported is false.
	(capture_main): Set vCont_supported to zero.

gdb/doc:

2015-09-01  Yao Qi  <yao.qi@linaro.org>

	* gdb.texinfo (General Query Packets): Add vContSupported to
	tables of 'gdbfeatures' and 'stub features' supported in the
	qSupported packet, as well as to the list containing stub
	feature details.
---
 gdb/aarch64-linux-nat.c |  9 ++++++++
 gdb/arm-linux-tdep.c    |  5 +++++
 gdb/doc/gdb.texinfo     |  8 +++++++
 gdb/gdbserver/server.c  | 23 +++++++++++++++++++-
 gdb/remote.c            | 58 +++++++++++++++++++++++++++++++++++++++++--------
 gdb/target-delegates.c  | 31 ++++++++++++++++++++++++++
 gdb/target.h            |  9 ++++++++
 7 files changed, 133 insertions(+), 10 deletions(-)

diff --git a/gdb/aarch64-linux-nat.c b/gdb/aarch64-linux-nat.c
index 9747461..ea52b02 100644
--- a/gdb/aarch64-linux-nat.c
+++ b/gdb/aarch64-linux-nat.c
@@ -810,6 +810,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
@@ -861,6 +869,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;
diff --git a/gdb/arm-linux-tdep.c b/gdb/arm-linux-tdep.c
index b3ad868..bc2cec4 100644
--- a/gdb/arm-linux-tdep.c
+++ b/gdb/arm-linux-tdep.c
@@ -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
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index cd0abad..58d4119 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -36110,6 +36110,10 @@ This feature indicates whether @value{GDBN} supports vfork event
 extensions to the remote protocol.  @value{GDBN} does not use such
 extensions unless the stub also reports that it supports them by
 including @samp{vfork-events+} in its @samp{qSupported} reply.
+
+@item vContSupported
+This feature indicates whether @value{GDBN} wants to know the
+supported actions in the reply to @samp{vCont?} packet.
 @end table
 
 Stubs should ignore any unknown values for
@@ -36578,6 +36582,10 @@ The remote stub reports the @samp{fork} stop reason for fork events.
 The remote stub reports the @samp{vfork} stop reason for vfork events
 and vforkdone events.
 
+@item vContSupported
+The remote stub reports the supported actions in the reply to
+@samp{vCont?} packet.
+
 @end table
 
 @item qSymbol::
diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index 5c0d83d..d614240 100644
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -63,6 +63,11 @@ int non_stop;
 int swbreak_feature;
 int hwbreak_feature;
 
+/* True if the "vContSupported" feature is active.  In that case, GDB
+   wants us to report whether single step is supported in the reply to
+   "vCont?" packet.  */
+static int vCont_supported;
+
 /* Whether we should attempt to disable the operating system's address
    space randomization feature before starting an inferior.  */
 int disable_randomization = 1;
@@ -2111,6 +2116,8 @@ handle_query (char *own_buf, int packet_len, int *new_packet_len_p)
 		  if (target_supports_vfork_events ())
 		    report_vfork_events = 1;
 		}
+	      else if (strcmp (p, "vContSupported+") == 0)
+		vCont_supported = 1;
 	      else
 		target_process_qsupported (p);
 
@@ -2217,6 +2224,8 @@ handle_query (char *own_buf, int packet_len, int *new_packet_len_p)
       if (the_target->pid_to_exec_file != NULL)
 	strcat (own_buf, ";qXfer:exec-file:read+");
 
+      strcat (own_buf, ";vContSupported+");
+
       /* Reinitialize components as needed for the new connection.  */
       hostio_handle_new_gdb_connection ();
       target_handle_new_gdb_connection ();
@@ -2815,7 +2824,18 @@ handle_v_requests (char *own_buf, int packet_len, int *new_packet_len)
 
       if (startswith (own_buf, "vCont?"))
 	{
-	  strcpy (own_buf, "vCont;c;C;s;S;t");
+	  strcpy (own_buf, "vCont;c;C;t");
+
+	  if (target_supports_hardware_single_step () || !vCont_supported)
+	    {
+	      /* If target supports hardware single step, add actions s
+		 and S to the list of supported actions.  On the other
+		 hand, if GDB doesn't request the supported vCont actions
+		 in qSupported packet, add s and S to the list too.  */
+	      own_buf = own_buf + strlen (own_buf);
+	      strcpy (own_buf, ";s;S");
+	    }
+
 	  if (target_supports_range_stepping ())
 	    {
 	      own_buf = own_buf + strlen (own_buf);
@@ -3555,6 +3575,7 @@ captured_main (int argc, char *argv[])
       cont_thread = null_ptid;
       swbreak_feature = 0;
       hwbreak_feature = 0;
+      vCont_supported = 0;
 
       remote_open (port);
 
diff --git a/gdb/remote.c b/gdb/remote.c
index 3afcaf4..5968af6 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -251,6 +251,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.  */
@@ -1401,6 +1407,9 @@ enum {
   /* Support for the Qbtrace-conf:pt:size packet.  */
   PACKET_Qbtrace_conf_pt_size,
 
+  /* Support for query supported vCont actions.  */
+  PACKET_vContSupported,
+
   PACKET_MAX
 };
 
@@ -4280,7 +4289,8 @@ static const struct protocol_feature remote_protocol_features[] = {
   { "vfork-events", PACKET_DISABLE, remote_supported_packet,
     PACKET_vfork_event_feature },
   { "Qbtrace-conf:pt:size", PACKET_DISABLE, remote_supported_packet,
-    PACKET_Qbtrace_conf_pt_size }
+    PACKET_Qbtrace_conf_pt_size },
+  { "vContSupported", PACKET_DISABLE, remote_supported_packet, PACKET_vContSupported }
 };
 
 static char *remote_support_xml;
@@ -4370,6 +4380,9 @@ remote_query_supported (void)
 	    q = remote_query_supported_append (q, "vfork-events+");
 	}
 
+      if (packet_set_cmd_state (PACKET_vContSupported) != AUTO_BOOLEAN_FALSE)
+	q = remote_query_supported_append (q, "vContSupported+");
+
       q = reconcat (q, "qSupported:", q, (char *) NULL);
       putpkt (q);
 
@@ -4945,10 +4958,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;
@@ -4957,9 +4970,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))
@@ -4972,9 +4985,9 @@ remote_vcont_probe (struct remote_state *rs)
 	  p = strchr (p, ';');
 	}
 
-      /* 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 c, and C are not all supported, we can't use vCont.  Clearing
+	 BUF will make packet_ok disable the packet.  */
+      if (!support_c || !support_C)
 	buf[0] = 0;
     }
 
@@ -12503,6 +12516,29 @@ 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)
+{
+  /* We can only tell whether target supports single step or not by
+     supported s and S vCont actions if the stub supports vContSupported
+     feature.  If the stub doesn't support vContSupported feature,
+     we have conservatively to think target doesn't supports single
+     step.  */
+  if (packet_support (PACKET_vContSupported) == PACKET_ENABLE)
+    {
+      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;
+    }
+  else
+    return 0;
+}
+
 static void
 init_remote_ops (void)
 {
@@ -12574,6 +12610,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;
@@ -13272,6 +13309,9 @@ Show the maximum size of the address (in bits) in a memory packet."), NULL,
   add_packet_config_cmd (&remote_protocol_packets[PACKET_Qbtrace_conf_pt_size],
        "Qbtrace-conf:pt:size", "btrace-conf-pt-size", 0);
 
+  add_packet_config_cmd (&remote_protocol_packets[PACKET_vContSupported],
+			 "vContSupported", "verbose-resume-supported", 0);
+
   /* Assert that we've registered "set remote foo-packet" commands
      for all packet configs.  */
   {
diff --git a/gdb/target-delegates.c b/gdb/target-delegates.c
index ddcad94..e134a1c 100644
--- a/gdb/target-delegates.c
+++ b/gdb/target-delegates.c
@@ -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)
 {
@@ -4008,6 +4035,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)
@@ -4276,6 +4305,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;
@@ -4427,6 +4457,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;
diff --git a/gdb/target.h b/gdb/target.h
index 37e4edb..c38407e 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -566,6 +566,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 *)
@@ -1903,6 +1909,9 @@ extern char *target_thread_name (struct thread_info *);
 						      addr, len)
 
 
+#define target_can_do_single_step() \
+  (*current_target.to_can_do_single_step) (&current_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.
-- 
1.9.1

[PATCH 1/2] [gdbserver] Rename supports_conditional_breakpoints to supports_hardware_single_step

[Yao Qi]

In my patch https://sourceware.org/ml/gdb-patches/2015-04/msg01110.html
a new target_ops hook supports_conditional_breakpoints was added to
disable conditional breakpoints if target doesn't have hardware single
step.  This patch is to generalize this hook from
supports_conditional_breakpoints to supports_hardware_single_step,
so that the following patch can use it.

gdb/gdbserver:

2015-09-01  Yao Qi  <yao.qi@linaro.org>

	* linux-low.c (linux_supports_conditional_breakpoints): Rename
	it to ...
	(linux_supports_hardware_single_step): ... New function.
	(linux_target_ops): Update.
	* lynx-low.c (lynx_target_ops): Set field
	supports_hardware_single_step to target_can_do_hardware_single_step.
	* nto-low.c (nto_target_ops): Likewise.
	* spu-low.c (spu_target_ops): Likewise.
	* win32-low.c (win32_target_ops): Likewise.
	* target.c (target_can_do_hardware_single_step): New function.
	* target.h (struct target_ops) <supports_conditional_breakpoints>:
	Remove.  <supports_hardware_single_step>: New field.
	(target_supports_conditional_breakpoints): Remove.
	(target_supports_hardware_single_step): New macro.
	(target_can_do_hardware_single_step): Declare.
	* server.c (handle_query): Use target_supports_hardware_single_step
	instead of target_supports_conditional_breakpoints.
---
 gdb/gdbserver/linux-low.c | 11 +++--------
 gdb/gdbserver/lynx-low.c  |  5 +----
 gdb/gdbserver/nto-low.c   |  5 +----
 gdb/gdbserver/server.c    | 12 +++++++++---
 gdb/gdbserver/spu-low.c   |  2 +-
 gdb/gdbserver/target.c    |  8 ++++++++
 gdb/gdbserver/target.h    | 13 +++++++------
 gdb/gdbserver/win32-low.c |  5 +----
 8 files changed, 31 insertions(+), 30 deletions(-)

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index f4c6029..9ae0522 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -5524,16 +5524,11 @@ linux_supports_stopped_by_hw_breakpoint (void)
   return USE_SIGTRAP_SIGINFO;
 }
 
-/* Implement the supports_conditional_breakpoints target_ops
-   method.  */
+/* Implement the supports_hardware_single_step target_ops method.  */
 
 static int
-linux_supports_conditional_breakpoints (void)
+linux_supports_hardware_single_step (void)
 {
-  /* GDBserver needs to step over the breakpoint if the condition is
-     false.  GDBserver software single step is too simple, so disable
-     conditional breakpoints if the target doesn't have hardware single
-     step.  */
   return can_hardware_single_step ();
 }
 
@@ -6886,7 +6881,7 @@ static struct target_ops linux_target_ops = {
   linux_supports_stopped_by_sw_breakpoint,
   linux_stopped_by_hw_breakpoint,
   linux_supports_stopped_by_hw_breakpoint,
-  linux_supports_conditional_breakpoints,
+  linux_supports_hardware_single_step,
   linux_stopped_by_watchpoint,
   linux_stopped_data_address,
 #if defined(__UCLIBC__) && defined(HAS_NOMMU)	      \
diff --git a/gdb/gdbserver/lynx-low.c b/gdb/gdbserver/lynx-low.c
index 1a187c8..c5eb8fd 100644
--- a/gdb/gdbserver/lynx-low.c
+++ b/gdb/gdbserver/lynx-low.c
@@ -747,10 +747,7 @@ static struct target_ops lynx_target_ops = {
   NULL,  /* supports_stopped_by_sw_breakpoint */
   NULL,  /* stopped_by_hw_breakpoint */
   NULL,  /* supports_stopped_by_hw_breakpoint */
-  /* Although lynx has hardware single step, still disable this
-     feature for lynx, because it is implemented in linux-low.c instead
-     of in generic code.  */
-  NULL,  /* supports_conditional_breakpoints */
+  target_can_do_hardware_single_step,
   NULL,  /* stopped_by_watchpoint */
   NULL,  /* stopped_data_address */
   NULL,  /* read_offsets */
diff --git a/gdb/gdbserver/nto-low.c b/gdb/gdbserver/nto-low.c
index 19f492f..fa216a9 100644
--- a/gdb/gdbserver/nto-low.c
+++ b/gdb/gdbserver/nto-low.c
@@ -950,10 +950,7 @@ static struct target_ops nto_target_ops = {
   NULL, /* supports_stopped_by_sw_breakpoint */
   NULL, /* stopped_by_hw_breakpoint */
   NULL, /* supports_stopped_by_hw_breakpoint */
-  /* Although nto has hardware single step, still disable this
-     feature for not, because it is implemented in linux-low.c instead
-     of in generic code.  */
-  NULL, /* supports_conditional_breakpoints */
+  target_can_do_hardware_single_step,
   nto_stopped_by_watchpoint,
   nto_stopped_data_address,
   NULL, /* nto_read_offsets */
diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index c52cf16..5c0d83d 100644
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -2192,9 +2192,15 @@ handle_query (char *own_buf, int packet_len, int *new_packet_len_p)
 	  strcat (own_buf, ";tracenz+");
 	}
 
-      /* Support target-side breakpoint conditions and commands.  */
-      if (target_supports_conditional_breakpoints ())
-	strcat (own_buf, ";ConditionalBreakpoints+");
+      if (target_supports_hardware_single_step ())
+	{
+	  /* Support target-side breakpoint conditions and commands.
+	     GDBserver needs to step over the breakpoint if the condition
+	     is false.  GDBserver software single step is too simple, so
+	     disable conditional breakpoints if the target doesn't have
+	     hardware single step.  */
+	  strcat (own_buf, ";ConditionalBreakpoints+");
+	}
       strcat (own_buf, ";BreakpointCommands+");
 
       if (target_supports_agent ())
diff --git a/gdb/gdbserver/spu-low.c b/gdb/gdbserver/spu-low.c
index 878ed82..074417a 100644
--- a/gdb/gdbserver/spu-low.c
+++ b/gdb/gdbserver/spu-low.c
@@ -666,7 +666,7 @@ static struct target_ops spu_target_ops = {
   NULL, /* supports_stopped_by_sw_breakpoint */
   NULL, /* stopped_by_hw_breakpoint */
   NULL, /* supports_stopped_by_hw_breakpoint */
-  NULL, /* supports_conditional_breakpoints */
+  NULL, /* supports_hardware_single_step */
   NULL,
   NULL,
   NULL,
diff --git a/gdb/gdbserver/target.c b/gdb/gdbserver/target.c
index 7540f2f..17ff7a6 100644
--- a/gdb/gdbserver/target.c
+++ b/gdb/gdbserver/target.c
@@ -216,3 +216,11 @@ kill_inferior (int pid)
 
   return (*the_target->kill) (pid);
 }
+
+/* Target can do hardware single step.  */
+
+int
+target_can_do_hardware_single_step (void)
+{
+  return 1;
+}
diff --git a/gdb/gdbserver/target.h b/gdb/gdbserver/target.h
index 3e3b80f..7df8df3 100644
--- a/gdb/gdbserver/target.h
+++ b/gdb/gdbserver/target.h
@@ -225,9 +225,8 @@ struct target_ops
      HW breakpoint triggering.  */
   int (*supports_stopped_by_hw_breakpoint) (void);
 
-  /* Returns true if the target can evaluate conditions of
-     breakpoints.  */
-  int (*supports_conditional_breakpoints) (void);
+  /* Returns true if the target can do hardware single step.  */
+  int (*supports_hardware_single_step) (void);
 
   /* Returns 1 if target was stopped due to a watchpoint hit, 0 otherwise.  */
 
@@ -609,9 +608,9 @@ int kill_inferior (int);
   (the_target->supports_stopped_by_hw_breakpoint ? \
    (*the_target->supports_stopped_by_hw_breakpoint) () : 0)
 
-#define target_supports_conditional_breakpoints() \
-  (the_target->supports_conditional_breakpoints ? \
-   (*the_target->supports_conditional_breakpoints) () : 0)
+#define target_supports_hardware_single_step() \
+  (the_target->supports_hardware_single_step ? \
+   (*the_target->supports_hardware_single_step) () : 0)
 
 #define target_stopped_by_hw_breakpoint() \
   (the_target->stopped_by_hw_breakpoint ? \
@@ -649,4 +648,6 @@ int set_desired_thread (int id);
 
 const char *target_pid_to_str (ptid_t);
 
+int target_can_do_hardware_single_step (void);
+
 #endif /* TARGET_H */
diff --git a/gdb/gdbserver/win32-low.c b/gdb/gdbserver/win32-low.c
index 86386ce..212c3c2 100644
--- a/gdb/gdbserver/win32-low.c
+++ b/gdb/gdbserver/win32-low.c
@@ -1810,10 +1810,7 @@ static struct target_ops win32_target_ops = {
   NULL, /* supports_stopped_by_sw_breakpoint */
   NULL, /* stopped_by_hw_breakpoint */
   NULL, /* supports_stopped_by_hw_breakpoint */
-  /* Although win32-i386 has hardware single step, still disable this
-     feature for win32, because it is implemented in linux-low.c instead
-     of in generic code.  */
-  NULL, /* supports_conditional_breakpoints */
+  target_can_do_hardware_single_step,
   win32_stopped_by_watchpoint,
   win32_stopped_data_address,
   NULL, /* read_offsets */
-- 
1.9.1

Re: [PATCH 2/2] Support single step by arch or target

[Antoine Tremblay]

On 09/01/2015 04:41 AM, 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.

Could we name this can_hardware_single_step instead ? Since the target 
may be able to software single step (in gdbserver). And thus it would be 
confusing...It would also be more consistent with the 
supports_hardware_single_step hook ?


> --- a/gdb/arm-linux-tdep.c
> +++ b/gdb/arm-linux-tdep.c
> @@ -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

target_can_do_single_step () should be before 
arm_deal_with_atomic_sequence ...

Re: [PATCH 2/2] Support single step by arch or target

[Yao Qi]

Antoine Tremblay <antoine.tremblay@ericsson.com> writes:

> Could we name this can_hardware_single_step instead ? Since the target
> may be able to software single step (in gdbserver). And thus it would
> be confusing...It would also be more consistent with the
> supports_hardware_single_step hook ?
>

If GDBserver can do software single step, this hook should return true
as well.  Hardware single step and software single step in GDBserver
should make no difference to GDB.

>
>> --- a/gdb/arm-linux-tdep.c
>> +++ b/gdb/arm-linux-tdep.c
>> @@ -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
>
> target_can_do_single_step () should be before
> arm_deal_with_atomic_sequence ...

I am not sure we can do single step (via hardware and linux kernel)
for arm atomic sequence , but we can't do that for aarch64, see
aarch64-tdep.c:aarch64_software_single_step.

-- 
Yao (齐尧)

Re: [PATCH 2/2] Support single step by arch or target

[Eli Zaretskii]

> From: Yao Qi <qiyaoltc@gmail.com>
> Date: Tue,  1 Sep 2015 09:41:55 +0100
> 
> gdb:
> 
> 2015-09-01  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, S>: New fields.
> 	(PACKET_vContSupported): New enum.
> 	(remote_protocol_features): New element for vContSupported.
> 	(remote_query_supported): Append "vContSupported+".
> 	(remote_vcont_probe): Remove support_s and support_S, use
> 	rs->supports_vCont.s and rs->supports_vCont.S instead.  Disable
> 	vCont packet if c and C actions are not supported.
> 	(remote_can_do_single_step): New function.
> 	(init_remote_ops): Install it to to_can_do_single_step.
> 	(_initialize_remote): Call add_packet_config_cmd.
> 	* 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/gdbserver:
> 
> 2015-09-01  Yao Qi  <yao.qi@linaro.org>
> 
> 	* server.c (vCont_supported): New global variable.
> 	(handle_query): Set vCont_supported to 1 if "vContSupported+"
> 	matches.  Append ";vContSupported+" to own_buf.
> 	(handle_v_requests): Append ";s;S" to own_buf if target supports
> 	hardware single step or vCont_supported is false.
> 	(capture_main): Set vCont_supported to zero.
> 
> gdb/doc:
> 
> 2015-09-01  Yao Qi  <yao.qi@linaro.org>
> 
> 	* gdb.texinfo (General Query Packets): Add vContSupported to
> 	tables of 'gdbfeatures' and 'stub features' supported in the
> 	qSupported packet, as well as to the list containing stub
> 	feature details.

OK for the documentation part.

Thanks.

Re: [PATCH 2/2] Support single step by arch or target

[Antoine Tremblay]

On 09/01/2015 10:23 AM, Yao Qi wrote:
> Antoine Tremblay <antoine.tremblay@ericsson.com> writes:
>
>> Could we name this can_hardware_single_step instead ? Since the target
>> may be able to software single step (in gdbserver). And thus it would
>> be confusing...It would also be more consistent with the
>> supports_hardware_single_step hook ?
>>
>
> If GDBserver can do software single step, this hook should return true
> as well.  Hardware single step and software single step in GDBserver
> should make no difference to GDB.
>

OK right indeed,

>>
>>> --- a/gdb/arm-linux-tdep.c
>>> +++ b/gdb/arm-linux-tdep.c
>>> @@ -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
>>
>> target_can_do_single_step () should be before
>> arm_deal_with_atomic_sequence ...
>
> I am not sure we can do single step (via hardware and linux kernel)
> for arm atomic sequence , but we can't do that for aarch64, see
> aarch64-tdep.c:aarch64_software_single_step.
>

I see, thanks for the clarification.

Re: [PATCH 1/2] [gdbserver] Rename supports_conditional_breakpoints to supports_hardware_single_step

[Antoine Tremblay]

On 09/01/2015 04:41 AM, Yao Qi wrote:
> In my patch https://sourceware.org/ml/gdb-patches/2015-04/msg01110.html
> a new target_ops hook supports_conditional_breakpoints was added to
> disable conditional breakpoints if target doesn't have hardware single
> step.  This patch is to generalize this hook from
> supports_conditional_breakpoints to supports_hardware_single_step,
> so that the following patch can use it.
>

Could we generalize this even more to supports_single_step like your 
next patch ?

Since I'm working on software single stepping for ARM, if this patch 
goes in I'll need to implement a supports_software_single_step and 
enable ConditionalBreakpoints for this case too...

Is there a need to know that it's really hardware or just knowing that 
it can single step would be ok ?

Regards,
Antoine

Re: [PATCH 1/2] [gdbserver] Rename supports_conditional_breakpoints to supports_hardware_single_step

[Antoine Tremblay]

On 09/02/2015 03:22 PM, Antoine Tremblay wrote:
>
> On 09/01/2015 04:41 AM, Yao Qi wrote:
>> In my patch https://sourceware.org/ml/gdb-patches/2015-04/msg01110.html
>> a new target_ops hook supports_conditional_breakpoints was added to
>> disable conditional breakpoints if target doesn't have hardware single
>> step.  This patch is to generalize this hook from
>> supports_conditional_breakpoints to supports_hardware_single_step,
>> so that the following patch can use it.
>>
>
> Could we generalize this even more to supports_single_step like your
> next patch ?
>
> Since I'm working on software single stepping for ARM, if this patch
> goes in I'll need to implement a supports_software_single_step and
> enable ConditionalBreakpoints for this case too...
>
> Is there a need to know that it's really hardware or just knowing that
> it can single step would be ok ?
>

Note also that this way would force supports_conditional_breakpoints to 
check for more than can_hardware_single_step and require a target 
function set manually to 1 on targets that we know have a proper 
software or hardware single step like :

static int
linux_supports_conditional_breakpoints (void)
{
   return the_low_target.supports_conditional_breakpoints ();
}

I had initially added that in my set but we could change it for 
the_low_target_.can_single_step () ?

This way targets that have proper software single step can be handled.
Otherwise every software single step implementation is deemed too simple 
to be used...

Would that seem ok ?

Re: [PATCH 1/2] [gdbserver] Rename supports_conditional_breakpoints to supports_hardware_single_step

[Antoine Tremblay]

On 09/03/2015 11:13 AM, Antoine Tremblay wrote:
>
>
> On 09/02/2015 03:22 PM, Antoine Tremblay wrote:
>>
>> On 09/01/2015 04:41 AM, Yao Qi wrote:
>>> In my patch https://sourceware.org/ml/gdb-patches/2015-04/msg01110.html
>>> a new target_ops hook supports_conditional_breakpoints was added to
>>> disable conditional breakpoints if target doesn't have hardware single
>>> step.  This patch is to generalize this hook from
>>> supports_conditional_breakpoints to supports_hardware_single_step,
>>> so that the following patch can use it.
>>>
>>
>> Could we generalize this even more to supports_single_step like your
>> next patch ?
>>
>> Since I'm working on software single stepping for ARM, if this patch
>> goes in I'll need to implement a supports_software_single_step and
>> enable ConditionalBreakpoints for this case too...
>>
>> Is there a need to know that it's really hardware or just knowing that
>> it can single step would be ok ?
>>
>
> Note also that this way would force supports_conditional_breakpoints to
> check for more than can_hardware_single_step and require a target
> function set manually to 1 on targets that we know have a proper
> software or hardware single step like :
>
> static int
> linux_supports_conditional_breakpoints (void)
> {
>    return the_low_target.supports_conditional_breakpoints ();
> }
>
> I had initially added that in my set but we could change it for
> the_low_target_.can_single_step () ?
>
> This way targets that have proper software single step can be handled.
> Otherwise every software single step implementation is deemed too simple
> to be used...
>
> Would that seem ok ?
>
>

After more analysis, I think ConditionalBreakpoints should be enabled 
for software/hardware single step.

But for the vCont s:S I don't think we can assume that gdbserver's 
software single step is better then GDB's, it should be the opposite. 
So it should only be enabled when hardware single step is present so we 
can't do away with supports_hardware_single_step.

So to support all features I suggest we have in GDBServer :

supports_hardware_single_step
supports_software_single_step

Enable ConditionalBreakpoints for supports_hardware_single_step || 
supports_software_single_step

And enable vCont s:s only for supports_hardware_single_step

I can add the supports_software_single_step in my patchset..

Does that sounds right ? So your current patch would be fine..

Re: [PATCH 2/2] Support single step by arch or target

[Antoine Tremblay]

> If the unpatched GDB talks with unpatched GDBserver on x86, the RSP traffic
> is like:
>
>   -> $qSupported:...+;
>   <- ...+;vContSupported+
>   ...
>   -> $vCont?
>   <- vCont;c;C;t;s;S;r
>
Detail but shouldn't that be :

-> $qSupported:...+;
<- ....
...
 > $vCont?
<- vCont;c;C;t;s;S;r

Since vContSupported doesn't exist yet in the unpatched case...

Re: [PATCH 1/2] [gdbserver] Rename supports_conditional_breakpoints to supports_hardware_single_step

[Antoine Tremblay]

On 09/04/2015 10:40 AM, Antoine Tremblay wrote:
>
>
> On 09/03/2015 11:13 AM, Antoine Tremblay wrote:
>>
>>
>> On 09/02/2015 03:22 PM, Antoine Tremblay wrote:
>>>
>>> On 09/01/2015 04:41 AM, Yao Qi wrote:
>>>> In my patch https://sourceware.org/ml/gdb-patches/2015-04/msg01110.html
>>>> a new target_ops hook supports_conditional_breakpoints was added to
>>>> disable conditional breakpoints if target doesn't have hardware single
>>>> step.  This patch is to generalize this hook from
>>>> supports_conditional_breakpoints to supports_hardware_single_step,
>>>> so that the following patch can use it.
>>>>
>>>
>>> Could we generalize this even more to supports_single_step like your
>>> next patch ?
>>>
>>> Since I'm working on software single stepping for ARM, if this patch
>>> goes in I'll need to implement a supports_software_single_step and
>>> enable ConditionalBreakpoints for this case too...
>>>
>>> Is there a need to know that it's really hardware or just knowing that
>>> it can single step would be ok ?
>>>
>>
>> Note also that this way would force supports_conditional_breakpoints to
>> check for more than can_hardware_single_step and require a target
>> function set manually to 1 on targets that we know have a proper
>> software or hardware single step like :
>>
>> static int
>> linux_supports_conditional_breakpoints (void)
>> {
>>    return the_low_target.supports_conditional_breakpoints ();
>> }
>>
>> I had initially added that in my set but we could change it for
>> the_low_target_.can_single_step () ?
>>
>> This way targets that have proper software single step can be handled.
>> Otherwise every software single step implementation is deemed too simple
>> to be used...
>>
>> Would that seem ok ?
>>
>>
>
> After more analysis, I think ConditionalBreakpoints should be enabled
> for software/hardware single step.
>
> But for the vCont s:S I don't think we can assume that gdbserver's
> software single step is better then GDB's, it should be the opposite. So
> it should only be enabled when hardware single step is present so we
> can't do away with supports_hardware_single_step.
>
> So to support all features I suggest we have in GDBServer :
>
> supports_hardware_single_step
> supports_software_single_step
>
> Enable ConditionalBreakpoints for supports_hardware_single_step ||
> supports_software_single_step
>
> And enable vCont s:s only for supports_hardware_single_step
>
> I can add the supports_software_single_step in my patchset..
>
> Does that sounds right ? So your current patch would be fine..
>

Another option would also be to remove the reinsert_addr implementations 
and have can_hardware_single_step still check if that implementation 
exists. Thus removing the need for a manual 
supports_software_single_step ...

If the implementations were disabled for Conditional Breakpoints, I 
don't see why they should be kept ?

I would prefer that option but maybe there's a scenario I"m not seeing?

Re: [PATCH 1/2] [gdbserver] Rename supports_conditional_breakpoints to supports_hardware_single_step

[Yao Qi]

Antoine Tremblay <antoine.tremblay@ericsson.com> writes:

Hi Antoine,

> Could we generalize this even more to supports_single_step like your
> next patch ?
>

I am not sure.

> Since I'm working on software single stepping for ARM, if this patch
> goes in I'll need to implement a supports_software_single_step and
> enable ConditionalBreakpoints for this case too...

Nowadays, GDBserver only support conditional breakpoint for HW single
step target.  Whether GDBserver support conditional breakpoint for SW
single step target is a separate issue, and we can decide this once we
have SW single step in GDBserver.

I believe GDBserver can compute the next instruction of $PC for SW
single step, but GDBserver execution control (target independent part)
will be more complicated if SW single step is involved in.

-- 
Yao (齐尧)

Re: [PATCH 1/2] [gdbserver] Rename supports_conditional_breakpoints to supports_hardware_single_step

[Antoine Tremblay]

On 09/08/2015 10:31 AM, Yao Qi wrote:
> Antoine Tremblay <antoine.tremblay@ericsson.com> writes:
>
> Hi Antoine,
>
>> Could we generalize this even more to supports_single_step like your
>> next patch ?
>>
>
> I am not sure.
>
>> Since I'm working on software single stepping for ARM, if this patch
>> goes in I'll need to implement a supports_software_single_step and
>> enable ConditionalBreakpoints for this case too...
>
> Nowadays, GDBserver only support conditional breakpoint for HW single
> step target.  Whether GDBserver support conditional breakpoint for SW
> single step target is a separate issue, and we can decide this once we
> have SW single step in GDBserver.
>

Alright then, I'll base my patchset based on your patch and most likely 
add a supports_software_single_step, if it's in by the time I post most 
likely later this week...

> I believe GDBserver can compute the next instruction of $PC for SW
> single step, but GDBserver execution control (target independent part)
> will be more complicated if SW single step is involved in.
>

Execution control for software single step is already present in 
GDBServer and working fine as far as I know...?

Re: [PATCH 1/2] [gdbserver] Rename supports_conditional_breakpoints to supports_hardware_single_step

[Antoine Tremblay]

> -  /* Although lynx has hardware single step, still disable this
> -     feature for lynx, because it is implemented in linux-low.c instead
> -     of in generic code.  */
> -  NULL,  /* supports_conditional_breakpoints */
> +  target_can_do_hardware_single_step,
>     NULL,  /* stopped_by_watchpoint */
>     NULL,  /* stopped_data_address */
>     NULL,  /* read_offsets */
...
> +
> +/* Target can do hardware single step.  */
> +
> +int
> +target_can_do_hardware_single_step (void)
> +{
> +  return 1;
> +}

Why would we enable conditional breakpoints on lynx,win32 etc.. since I 
think it's still valid that it's implemented in linux-low ?

Should we not default to return 0 in target_can_do_hardware_single_step ?

Re: [PATCH 2/2] Support single step by arch or target

[Yao Qi]

Antoine Tremblay <antoine.tremblay@ericsson.com> writes:

>> If the unpatched GDB talks with unpatched GDBserver on x86, the RSP traffic
>> is like:
>>
>>   -> $qSupported:...+;
>>   <- ...+;vContSupported+
>>   ...
>>   -> $vCont?
>>   <- vCont;c;C;t;s;S;r
>>
> Detail but shouldn't that be :
>
> -> $qSupported:...+;
> <- ....
> ...
>> $vCont?
> <- vCont;c;C;t;s;S;r
>
> Since vContSupported doesn't exist yet in the unpatched case...

This is about unpatched GDB talks with patched GDBserver on x86.  I'll
fix the commit log accordingly.

-- 
Yao (齐尧)

Re: [PATCH 0/2 V2] Support single step by arch or target

[Yao Qi]

Yao Qi <qiyaoltc@gmail.com> writes:

> This is the V2 of my patch <https://sourceware.org/ml/gdb-patches/2015-07/msg00040.html>
> to address Pedro's comment on backward compatibility problem.
>
> Patch 1 is to rename target_ops hook supports_conditional_breakpoints
> to supports_hardware_single_step, nothing changes since I post it last
> time <https://sourceware.org/ml/gdb-patches/2015-07/msg00035.html>.
>
> Patch 2 is the major one in this series, details can be found in it.
>
> Regression tested on aarch64 with {native, gdbserver} x {32-bit, 64-bit}.

I've pushed them in.

-- 
Yao (齐尧)

Re: [PATCH 2/2] Support single step by arch or target

[Pedro Alves]

On 09/01/2015 09:41 AM, Yao Qi wrote:
> +@item vContSupported
> +This feature indicates whether @value{GDBN} wants to know the
> +supported actions in the reply to @samp{vCont?} packet.
>  @end table
>  

I find this confusing, because vCont? is already supposed
to return the supported vCont actions:

 @item vCont?
 @cindex @samp{vCont?} packet
 Request a list of actions supported by the @samp{vCont} packet.

So ISTM that in the perspective of someone reading the manual without
the context we're discussing here, it's not clear at all what
vContSupported is supposed to mean and why not simply always
return the supported vCont actions in reply to vCont?

Is there an advantage to the vContSupported indirection?  I was
originally thinking we'd make the server report "vCont=c;C;t;s;S;r"
directly in its qSupported reply, and gdb would stop using "vCont?"
going forward (if the target supports that qSupported feature).

Thanks,
Pedro Alves