[PATCH 6/9] Use enqueue_pending_signal in linux_resume_one_thread

[PATCH 6/9] Use enqueue_pending_signal in linux_resume_one_thread

[Yao Qi]

gdb/gdbserver:

2016-06-29  Yao Qi  <yao.qi@linaro.org>

	* linux-low.c (linux_resume_one_thread): Call
	enqueue_pending_signal.
---
 gdb/gdbserver/linux-low.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index 941b0e8..8061758 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -4917,21 +4917,19 @@ linux_resume_one_thread (struct inferior_list_entry *entry, void *arg)
       /* If we have a new signal, enqueue the signal.  */
       if (lwp->resume->sig != 0)
 	{
-	  struct pending_signals *p_sig = XCNEW (struct pending_signals);
-
-	  p_sig->prev = lwp->pending_signals;
-	  p_sig->signal = lwp->resume->sig;
+	  siginfo_t info, *info_p;
 
 	  /* If this is the same signal we were previously stopped by,
-	     make sure to queue its siginfo.  We can ignore the return
-	     value of ptrace; if it fails, we'll skip
-	     PTRACE_SETSIGINFO.  */
+	     make sure to queue its siginfo.  */
 	  if (WIFSTOPPED (lwp->last_status)
-	      && WSTOPSIG (lwp->last_status) == lwp->resume->sig)
-	    ptrace (PTRACE_GETSIGINFO, lwpid_of (thread), (PTRACE_TYPE_ARG3) 0,
-		    &p_sig->info);
+	      && WSTOPSIG (lwp->last_status) == lwp->resume->sig
+	      && ptrace (PTRACE_GETSIGINFO, lwpid_of (thread),
+			 (PTRACE_TYPE_ARG3) 0, &info) == 0)
+	    info_p = &info;
+	  else
+	    info_p = NULL;
 
-	  lwp->pending_signals = p_sig;
+	  enqueue_pending_signal (lwp, lwp->resume->sig, info_p);
 	}
     }
 
-- 
1.9.1

[PATCH 0/9 V3] Use reinsert breakpoint for vCont;s

[Yao Qi]

Here is the V3 of this patch series, V2 can be found
https://sourceware.org/ml/gdb-patches/2016-06/msg00029.html
some preparatory patches in V2 are already pushed in.  Patches
1 - 5 and 9 are already approved in V2 review.

This patch series is to address the review comments
https://sourceware.org/ml/gdb-patches/2016-06/msg00305.html

> Hmm, seeing that we need to handle installing the breakpoints in
> both places, I wonder about making linux_resume just handle
> setting up the last resume kind and queue signals, and then
> end up calling proceed_all_lwps.  If that works, I suspect it
> would simplify things a good deal.

I do something slightly differently in V3.  In my
"V2 Use reinsert breakpoint for vCont;s", I install reinsert breakpoints
for needed lwps in two places, linux_resume and proceed_all_lwps, which
isn't ideal.

After the chat with Pedro, we don't need to stop all threads when inserting
reinsert breakpoint, so we can move the breakpoint installation further
down to linux_resume_one_thread and proceed_one_lwp.  If
linux_resume_one_thread can use proceed_one_lwp, we can only install
reinsert breakpoints in one place, proceed_one_lwp.  Patch 7 changes
linux_resume_one_thread enqueues signal and calls proceed_one_lwp.
Patch 6 is a refactor one to use existing api enqueue_pending_signal.
With the changes in patch 7, patch 8 is simplified compared with V2.

Regression tested on arm-linux and x86_64-linux.

*** BLURB HERE ***

Yao Qi (9):
  Pass breakpoint type in set_breakpoint_at
  Create sub classes of 'struct breakpoint'
  Refactor clone_all_breakpoints
  Make reinsert_breakpoint thread specific
  Switch current_thread to lwp's thread in
    install_software_single_step_breakpoints
  Use enqueue_pending_signal in linux_resume_one_thread
  Enqueue signal even when resuming threads
  Use reinsert_breakpoint for vCont;s
  Support vCont s and S actions with software single step

 gdb/gdbserver/ChangeLog   |  73 +++++++++++
 gdb/gdbserver/gdbthread.h |   3 +
 gdb/gdbserver/inferiors.c |  14 ++-
 gdb/gdbserver/inferiors.h |   2 +-
 gdb/gdbserver/linux-low.c | 124 +++++++++---------
 gdb/gdbserver/mem-break.c | 313 +++++++++++++++++++++++++++++++++-------------
 gdb/gdbserver/mem-break.h |  45 +++----
 gdb/gdbserver/server.c    |  17 +--
 8 files changed, 410 insertions(+), 181 deletions(-)

-- 
1.9.1

[PATCH 7/9] Enqueue signal even when resuming threads

[Yao Qi]

Nowadays, we only enqueue signal when we leave thread pending in
linux_resume_one_thread.  If lwp->resume->sig isn't zero (GDB wants
to resume with signal), we pass lwp->resume->sig to
linux_resume_one_lwp.

In order to reduce the difference between resuming thread with signal
and proceeding thread with signal, when we resume thread, we can
enqueue signal too, and proceed thread.  The signal will be consumed in
linux_resume_one_lwp_throw from lwp->pending_signals.

gdb/gdbserver:

2016-06-29  Yao Qi  <yao.qi@linaro.org>

	* linux-low.c (proceed_one_lwp): Declare.
	(linux_resume_one_thread): Remove local variable 'step'.
	Lift code enqueue signal.  Call proceed_one_lwp instead of
	linux_resume_one_lwp.
---
 gdb/gdbserver/linux-low.c | 41 ++++++++++++++++++++---------------------
 1 file changed, 20 insertions(+), 21 deletions(-)

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index 8061758..abaf288 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -267,6 +267,7 @@ static int kill_lwp (unsigned long lwpid, int signo);
 static void enqueue_pending_signal (struct lwp_info *lwp, int signal, siginfo_t *info);
 static void complete_ongoing_step_over (void);
 static int linux_low_ptrace_options (int attached);
+static int proceed_one_lwp (struct inferior_list_entry *entry, void *except);
 
 /* When the event-loop is doing a step-over, this points at the thread
    being stepped.  */
@@ -4834,7 +4835,6 @@ linux_resume_one_thread (struct inferior_list_entry *entry, void *arg)
 {
   struct thread_info *thread = (struct thread_info *) entry;
   struct lwp_info *lwp = get_thread_lwp (thread);
-  int step;
   int leave_all_stopped = * (int *) arg;
   int leave_pending;
 
@@ -4901,36 +4901,35 @@ linux_resume_one_thread (struct inferior_list_entry *entry, void *arg)
 		   || lwp->status_pending_p
 		   || leave_all_stopped);
 
+  /* If we have a new signal, enqueue the signal.  */
+  if (lwp->resume->sig != 0)
+    {
+      siginfo_t info, *info_p;
+
+      /* If this is the same signal we were previously stopped by,
+	 make sure to queue its siginfo.  */
+      if (WIFSTOPPED (lwp->last_status)
+	  && WSTOPSIG (lwp->last_status) == lwp->resume->sig
+	  && ptrace (PTRACE_GETSIGINFO, lwpid_of (thread),
+		     (PTRACE_TYPE_ARG3) 0, &info) == 0)
+	info_p = &info;
+      else
+	info_p = NULL;
+
+      enqueue_pending_signal (lwp, lwp->resume->sig, info_p);
+    }
+
   if (!leave_pending)
     {
       if (debug_threads)
 	debug_printf ("resuming LWP %ld\n", lwpid_of (thread));
 
-      step = (lwp->resume->kind == resume_step);
-      linux_resume_one_lwp (lwp, step, lwp->resume->sig, NULL);
+      proceed_one_lwp (entry, NULL);
     }
   else
     {
       if (debug_threads)
 	debug_printf ("leaving LWP %ld stopped\n", lwpid_of (thread));
-
-      /* If we have a new signal, enqueue the signal.  */
-      if (lwp->resume->sig != 0)
-	{
-	  siginfo_t info, *info_p;
-
-	  /* If this is the same signal we were previously stopped by,
-	     make sure to queue its siginfo.  */
-	  if (WIFSTOPPED (lwp->last_status)
-	      && WSTOPSIG (lwp->last_status) == lwp->resume->sig
-	      && ptrace (PTRACE_GETSIGINFO, lwpid_of (thread),
-			 (PTRACE_TYPE_ARG3) 0, &info) == 0)
-	    info_p = &info;
-	  else
-	    info_p = NULL;
-
-	  enqueue_pending_signal (lwp, lwp->resume->sig, info_p);
-	}
     }
 
   thread->last_status.kind = TARGET_WAITKIND_IGNORE;
-- 
1.9.1

[PATCH 2/9] Create sub classes of 'struct breakpoint'

[Yao Qi]

Nowadays, there are three types of breakpoint in GDBserver,

 - gdb breakpoints,
 - reinsert breakpoints, used for software single step,
 - other breakpoints, used for tracepoint,

but we only have one 'struct breakpoint' for all of them.  Some fields
are only useful to one type of breakpoint.  For example, cond_list
and command_list are only used by gdb breakpoints, while handler is
only used by other breakpoints.

This patch changes 'struct breakpoint' to a base class, which has fields
needed by all breakpoint types, also add three sub-classes to
'struct breakpoint' to these three types of breakpoints.

gdb/gdbserver:

2016-06-17  Yao Qi  <yao.qi@linaro.org>

	* mem-break.c (struct breakpoint) <cond_list>: Remove.
	<command_list, handler>: Remove.
	(struct gdb_breakpoint): New.
	(struct other_breakpoint): New.
	(struct reinsert_breakpoint): New.
	(is_gdb_breakpoint): New function.
	(any_persistent_commands): Update command_list if
	is_gdb_breakpoint returns true.
	(set_breakpoint): Create breakpoints according to their types.
	(find_gdb_breakpoint): Return 'struct gdb_breakpoint *'.
	(set_gdb_breakpoint_1): Likewise.
	(set_gdb_breakpoint): Likewise.
	(clear_breakpoint_conditions): Change parameter type to
	'struct gdb_breakpoint *'.
	(clear_breakpoint_commands): Likewise.
	(clear_breakpoint_conditions_and_commands): Likewise.
	(add_condition_to_breakpoint): Likewise.
	(add_breakpoint_condition): Likewise.
	(add_commands_to_breakpoint): Likewise.
	(check_breakpoints): Check other_breakpoint.
	(clone_one_breakpoint): Clone breakpopint according to its type.
	* mem-break.h (struct gdb_breakpoint): Declare.
	(set_gdb_breakpoint): Update declaration.
	(clear_breakpoint_conditions_and_commands): Likewise.
	(add_breakpoint_condition): Likewise.
	(add_breakpoint_commands): Likewise.
	* server.c (process_point_options): Change parameter type to
	'struct gdb_breakpoint *'.
---
 gdb/gdbserver/ChangeLog   |  31 +++++++
 gdb/gdbserver/mem-break.c | 209 +++++++++++++++++++++++++++++++++-------------
 gdb/gdbserver/mem-break.h |  11 +--
 gdb/gdbserver/server.c    |   4 +-
 4 files changed, 188 insertions(+), 67 deletions(-)

diff --git a/gdb/gdbserver/ChangeLog b/gdb/gdbserver/ChangeLog
index 7c24260..a59d4e5 100644
--- a/gdb/gdbserver/ChangeLog
+++ b/gdb/gdbserver/ChangeLog
@@ -47,6 +47,37 @@
 
 2016-06-17  Yao Qi  <yao.qi@linaro.org>
 
+	* mem-break.c (struct breakpoint) <cond_list>: Remove.
+	<command_list, handler>: Remove.
+	(struct gdb_breakpoint): New.
+	(struct other_breakpoint): New.
+	(struct reinsert_breakpoint): New.
+	(is_gdb_breakpoint): New function.
+	(any_persistent_commands): Update command_list if
+	is_gdb_breakpoint returns true.
+	(set_breakpoint): Create breakpoints according to their types.
+	(find_gdb_breakpoint): Return 'struct gdb_breakpoint *'.
+	(set_gdb_breakpoint_1): Likewise.
+	(set_gdb_breakpoint): Likewise.
+	(clear_breakpoint_conditions): Change parameter type to
+	'struct gdb_breakpoint *'.
+	(clear_breakpoint_commands): Likewise.
+	(clear_breakpoint_conditions_and_commands): Likewise.
+	(add_condition_to_breakpoint): Likewise.
+	(add_breakpoint_condition): Likewise.
+	(add_commands_to_breakpoint): Likewise.
+	(check_breakpoints): Check other_breakpoint.
+	(clone_one_breakpoint): Clone breakpopint according to its type.
+	* mem-break.h (struct gdb_breakpoint): Declare.
+	(set_gdb_breakpoint): Update declaration.
+	(clear_breakpoint_conditions_and_commands): Likewise.
+	(add_breakpoint_condition): Likewise.
+	(add_breakpoint_commands): Likewise.
+	* server.c (process_point_options): Change parameter type to
+	'struct gdb_breakpoint *'.
+
+2016-06-17  Yao Qi  <yao.qi@linaro.org>
+
 	* mem-break.c (set_breakpoint_at): Rename it to ...
 	(set_breakpoint_type_at): ... it.
 	(set_breakpoint_at): Call set_breakpoint_type_at.
diff --git a/gdb/gdbserver/mem-break.c b/gdb/gdbserver/mem-break.c
index f0b77f9..3539422 100644
--- a/gdb/gdbserver/mem-break.c
+++ b/gdb/gdbserver/mem-break.c
@@ -173,6 +173,17 @@ struct breakpoint
   /* The breakpoint's type.  */
   enum bkpt_type type;
 
+  /* Link to this breakpoint's raw breakpoint.  This is always
+     non-NULL.  */
+  struct raw_breakpoint *raw;
+};
+
+/* Breakpoint requested by GDB.  */
+
+struct gdb_breakpoint
+{
+  struct breakpoint base;
+
   /* Pointer to the condition list that should be evaluated on
      the target or NULL if the breakpoint is unconditional or
      if GDB doesn't want us to evaluate the conditionals on the
@@ -181,10 +192,13 @@ struct breakpoint
 
   /* Point to the list of commands to run when this is hit.  */
   struct point_command_list *command_list;
+};
 
-  /* Link to this breakpoint's raw breakpoint.  This is always
-     non-NULL.  */
-  struct raw_breakpoint *raw;
+/* Breakpoint used by GDBserver.  */
+
+struct other_breakpoint
+{
+  struct breakpoint base;
 
   /* Function to call when we hit this breakpoint.  If it returns 1,
      the breakpoint shall be deleted; 0 or if this callback is NULL,
@@ -192,6 +206,13 @@ struct breakpoint
   int (*handler) (CORE_ADDR);
 };
 
+/* Reinsert breakpoint.  */
+
+struct reinsert_breakpoint
+{
+  struct breakpoint base;
+};
+
 /* Return the breakpoint size from its kind.  */
 
 static int
@@ -266,6 +287,18 @@ Z_packet_to_raw_bkpt_type (char z_type)
     }
 }
 
+/* Return true if breakpoint TYPE is a GDB breakpoint.  */
+
+static int
+is_gdb_breakpoint (enum bkpt_type type)
+{
+  return (type == gdb_breakpoint_Z0
+	  || type == gdb_breakpoint_Z1
+	  || type == gdb_breakpoint_Z2
+	  || type == gdb_breakpoint_Z3
+	  || type == gdb_breakpoint_Z4);
+}
+
 int
 any_persistent_commands (void)
 {
@@ -275,9 +308,14 @@ any_persistent_commands (void)
 
   for (bp = proc->breakpoints; bp != NULL; bp = bp->next)
     {
-      for (cl = bp->command_list; cl != NULL; cl = cl->next)
-	if (cl->persistence)
-	  return 1;
+      if (is_gdb_breakpoint (bp->type))
+	{
+	  struct gdb_breakpoint *gdb_bp = (struct gdb_breakpoint *) bp;
+
+	  for (cl = gdb_bp->command_list; cl != NULL; cl = cl->next)
+	    if (cl->persistence)
+	      return 1;
+	}
     }
 
   return 0;
@@ -773,11 +811,32 @@ set_breakpoint (enum bkpt_type type, enum raw_bkpt_type raw_type,
       return NULL;
     }
 
-  bp = XCNEW (struct breakpoint);
-  bp->type = type;
+  if (is_gdb_breakpoint (type))
+    {
+      struct gdb_breakpoint *gdb_bp = XCNEW (struct gdb_breakpoint);
+
+      bp = (struct breakpoint *) gdb_bp;
+      gdb_assert (handler == NULL);
+    }
+  else if (type == other_breakpoint)
+    {
+      struct other_breakpoint *other_bp = XCNEW (struct other_breakpoint);
+
+      other_bp->handler = handler;
+      bp = (struct breakpoint *) other_bp;
+    }
+  else if (type == reinsert_breakpoint)
+    {
+      struct reinsert_breakpoint *reinsert_bp
+	= XCNEW (struct reinsert_breakpoint);
 
+      bp = (struct breakpoint *) reinsert_bp;
+    }
+  else
+    gdb_assert_not_reached ("unhandled breakpoint type");
+
+  bp->type = type;
   bp->raw = raw;
-  bp->handler = handler;
 
   bp->next = proc->breakpoints;
   proc->breakpoints = bp;
@@ -924,7 +983,7 @@ delete_breakpoint (struct breakpoint *todel)
    address ADDR and return a pointer to its structure.  If KIND is -1,
    the breakpoint's kind is ignored.  */
 
-static struct breakpoint *
+static struct gdb_breakpoint *
 find_gdb_breakpoint (char z_type, CORE_ADDR addr, int kind)
 {
   struct process_info *proc = current_process ();
@@ -934,7 +993,7 @@ find_gdb_breakpoint (char z_type, CORE_ADDR addr, int kind)
   for (bp = proc->breakpoints; bp != NULL; bp = bp->next)
     if (bp->type == type && bp->raw->pc == addr
 	&& (kind == -1 || bp->raw->kind == kind))
-      return bp;
+      return (gdb_breakpoint *) bp;
 
   return NULL;
 }
@@ -952,10 +1011,10 @@ z_type_supported (char z_type)
    failure returns NULL and sets *ERR to either -1 for error, or 1 if
    Z_TYPE breakpoints are not supported on this target.  */
 
-static struct breakpoint *
+static struct gdb_breakpoint *
 set_gdb_breakpoint_1 (char z_type, CORE_ADDR addr, int kind, int *err)
 {
-  struct breakpoint *bp;
+  struct gdb_breakpoint *bp;
   enum bkpt_type type;
   enum raw_bkpt_type raw_type;
 
@@ -981,12 +1040,12 @@ set_gdb_breakpoint_1 (char z_type, CORE_ADDR addr, int kind, int *err)
 
       if (bp != NULL)
 	{
-	  if (bp->raw->kind != kind)
+	  if (bp->base.raw->kind != kind)
 	    {
 	      /* A different kind than previously seen.  The previous
 		 breakpoint must be gone then.  */
-	      bp->raw->inserted = -1;
-	      delete_breakpoint (bp);
+	      bp->base.raw->inserted = -1;
+	      delete_breakpoint ((struct breakpoint *) bp);
 	      bp = NULL;
 	    }
 	  else if (z_type == Z_PACKET_SW_BP)
@@ -1022,7 +1081,8 @@ set_gdb_breakpoint_1 (char z_type, CORE_ADDR addr, int kind, int *err)
 
   raw_type = Z_packet_to_raw_bkpt_type (z_type);
   type = Z_packet_to_bkpt_type (z_type);
-  return set_breakpoint (type, raw_type, addr, kind, NULL, err);
+  return (struct gdb_breakpoint *) set_breakpoint (type, raw_type, addr,
+						   kind, NULL, err);
 }
 
 static int
@@ -1047,10 +1107,10 @@ check_gdb_bp_preconditions (char z_type, int *err)
 /* See mem-break.h.  This is a wrapper for set_gdb_breakpoint_1 that
    knows to prepare to access memory for Z0 breakpoints.  */
 
-struct breakpoint *
+struct gdb_breakpoint *
 set_gdb_breakpoint (char z_type, CORE_ADDR addr, int kind, int *err)
 {
-  struct breakpoint *bp;
+  struct gdb_breakpoint *bp;
 
   if (!check_gdb_bp_preconditions (z_type, err))
     return NULL;
@@ -1082,7 +1142,7 @@ set_gdb_breakpoint (char z_type, CORE_ADDR addr, int kind, int *err)
 static int
 delete_gdb_breakpoint_1 (char z_type, CORE_ADDR addr, int kind)
 {
-  struct breakpoint *bp;
+  struct gdb_breakpoint *bp;
   int err;
 
   bp = find_gdb_breakpoint (z_type, addr, kind);
@@ -1092,7 +1152,7 @@ delete_gdb_breakpoint_1 (char z_type, CORE_ADDR addr, int kind)
   /* Before deleting the breakpoint, make sure to free its condition
      and command lists.  */
   clear_breakpoint_conditions_and_commands (bp);
-  err = delete_breakpoint (bp);
+  err = delete_breakpoint ((struct breakpoint *) bp);
   if (err != 0)
     return -1;
 
@@ -1132,7 +1192,7 @@ delete_gdb_breakpoint (char z_type, CORE_ADDR addr, int kind)
 /* Clear all conditions associated with a breakpoint.  */
 
 static void
-clear_breakpoint_conditions (struct breakpoint *bp)
+clear_breakpoint_conditions (struct gdb_breakpoint *bp)
 {
   struct point_cond_list *cond;
 
@@ -1157,7 +1217,7 @@ clear_breakpoint_conditions (struct breakpoint *bp)
 /* Clear all commands associated with a breakpoint.  */
 
 static void
-clear_breakpoint_commands (struct breakpoint *bp)
+clear_breakpoint_commands (struct gdb_breakpoint *bp)
 {
   struct point_command_list *cmd;
 
@@ -1180,7 +1240,7 @@ clear_breakpoint_commands (struct breakpoint *bp)
 }
 
 void
-clear_breakpoint_conditions_and_commands (struct breakpoint *bp)
+clear_breakpoint_conditions_and_commands (struct gdb_breakpoint *bp)
 {
   clear_breakpoint_conditions (bp);
   clear_breakpoint_commands (bp);
@@ -1189,7 +1249,7 @@ clear_breakpoint_conditions_and_commands (struct breakpoint *bp)
 /* Add condition CONDITION to GDBserver's breakpoint BP.  */
 
 static void
-add_condition_to_breakpoint (struct breakpoint *bp,
+add_condition_to_breakpoint (struct gdb_breakpoint *bp,
 			     struct agent_expr *condition)
 {
   struct point_cond_list *new_cond;
@@ -1206,7 +1266,7 @@ add_condition_to_breakpoint (struct breakpoint *bp,
 /* Add a target-side condition CONDITION to a breakpoint.  */
 
 int
-add_breakpoint_condition (struct breakpoint *bp, char **condition)
+add_breakpoint_condition (struct gdb_breakpoint *bp, char **condition)
 {
   char *actparm = *condition;
   struct agent_expr *cond;
@@ -1240,7 +1300,7 @@ static int
 gdb_condition_true_at_breakpoint_z_type (char z_type, CORE_ADDR addr)
 {
   /* Fetch registers for the current inferior.  */
-  struct breakpoint *bp = find_gdb_breakpoint (z_type, addr, -1);
+  struct gdb_breakpoint *bp = find_gdb_breakpoint (z_type, addr, -1);
   ULONGEST value = 0;
   struct point_cond_list *cl;
   int err = 0;
@@ -1287,7 +1347,7 @@ gdb_condition_true_at_breakpoint (CORE_ADDR where)
 /* Add commands COMMANDS to GDBserver's breakpoint BP.  */
 
 static void
-add_commands_to_breakpoint (struct breakpoint *bp,
+add_commands_to_breakpoint (struct gdb_breakpoint *bp,
 			    struct agent_expr *commands, int persist)
 {
   struct point_command_list *new_cmd;
@@ -1305,7 +1365,7 @@ add_commands_to_breakpoint (struct breakpoint *bp,
 /* Add a target-side command COMMAND to the breakpoint at ADDR.  */
 
 int
-add_breakpoint_commands (struct breakpoint *bp, char **command,
+add_breakpoint_commands (struct gdb_breakpoint *bp, char **command,
 			 int persist)
 {
   char *actparm = *command;
@@ -1339,7 +1399,7 @@ add_breakpoint_commands (struct breakpoint *bp, char **command,
 static int
 gdb_no_commands_at_breakpoint_z_type (char z_type, CORE_ADDR addr)
 {
-  struct breakpoint *bp = find_gdb_breakpoint (z_type, addr, -1);
+  struct gdb_breakpoint *bp = find_gdb_breakpoint (z_type, addr, -1);
 
   if (bp == NULL)
     return 1;
@@ -1369,7 +1429,7 @@ static int
 run_breakpoint_commands_z_type (char z_type, CORE_ADDR addr)
 {
   /* Fetch registers for the current inferior.  */
-  struct breakpoint *bp = find_gdb_breakpoint (z_type, addr, -1);
+  struct gdb_breakpoint *bp = find_gdb_breakpoint (z_type, addr, -1);
   ULONGEST value = 0;
   struct point_command_list *cl;
   int err = 0;
@@ -1657,14 +1717,20 @@ check_breakpoints (CORE_ADDR stop_pc)
 	      return;
 	    }
 
-	  if (bp->handler != NULL && (*bp->handler) (stop_pc))
+	  if (bp->type == other_breakpoint)
 	    {
-	      *bp_link = bp->next;
+	      struct other_breakpoint *other_bp
+		= (struct other_breakpoint *) bp;
+
+	      if (other_bp->handler != NULL && (*other_bp->handler) (stop_pc))
+		{
+		  *bp_link = bp->next;
 
-	      release_breakpoint (proc, bp);
+		  release_breakpoint (proc, bp);
 
-	      bp = *bp_link;
-	      continue;
+		  bp = *bp_link;
+		  continue;
+		}
 	    }
 	}
 
@@ -2051,12 +2117,6 @@ clone_one_breakpoint (const struct breakpoint *src)
 {
   struct breakpoint *dest;
   struct raw_breakpoint *dest_raw;
-  struct point_cond_list *current_cond;
-  struct point_cond_list *new_cond;
-  struct point_cond_list *cond_tail = NULL;
-  struct point_command_list *current_cmd;
-  struct point_command_list *new_cmd;
-  struct point_command_list *cmd_tail = NULL;
 
   /* Clone the raw breakpoint.  */
   dest_raw = XCNEW (struct raw_breakpoint);
@@ -2068,29 +2128,58 @@ clone_one_breakpoint (const struct breakpoint *src)
   dest_raw->inserted = src->raw->inserted;
 
   /* Clone the high-level breakpoint.  */
-  dest = XCNEW (struct breakpoint);
-  dest->type = src->type;
-  dest->raw = dest_raw;
-  dest->handler = src->handler;
-
-  /* Clone the condition list.  */
-  for (current_cond = src->cond_list; current_cond != NULL;
-       current_cond = current_cond->next)
+  if (is_gdb_breakpoint (src->type))
     {
-      new_cond = XCNEW (struct point_cond_list);
-      new_cond->cond = clone_agent_expr (current_cond->cond);
-      APPEND_TO_LIST (&dest->cond_list, new_cond, cond_tail);
+      struct gdb_breakpoint *gdb_dest = XCNEW (struct gdb_breakpoint);
+      struct point_cond_list *current_cond;
+      struct point_cond_list *new_cond;
+      struct point_cond_list *cond_tail = NULL;
+      struct point_command_list *current_cmd;
+      struct point_command_list *new_cmd;
+      struct point_command_list *cmd_tail = NULL;
+
+      /* Clone the condition list.  */
+      for (current_cond = ((struct gdb_breakpoint *) src)->cond_list;
+	   current_cond != NULL;
+	   current_cond = current_cond->next)
+	{
+	  new_cond = XCNEW (struct point_cond_list);
+	  new_cond->cond = clone_agent_expr (current_cond->cond);
+	  APPEND_TO_LIST (&gdb_dest->cond_list, new_cond, cond_tail);
+	}
+
+      /* Clone the command list.  */
+      for (current_cmd = ((struct gdb_breakpoint *) src)->command_list;
+	   current_cmd != NULL;
+	   current_cmd = current_cmd->next)
+	{
+	  new_cmd = XCNEW (struct point_command_list);
+	  new_cmd->cmd = clone_agent_expr (current_cmd->cmd);
+	  new_cmd->persistence = current_cmd->persistence;
+	  APPEND_TO_LIST (&gdb_dest->command_list, new_cmd, cmd_tail);
+	}
+
+      dest = (struct breakpoint *) gdb_dest;
     }
+  else if (src->type == other_breakpoint)
+    {
+      struct other_breakpoint *other_dest = XCNEW (struct other_breakpoint);
 
-  /* Clone the command list.  */
-  for (current_cmd = src->command_list; current_cmd != NULL;
-       current_cmd = current_cmd->next)
+      other_dest->handler = ((struct other_breakpoint *) src)->handler;
+      dest = (struct breakpoint *) other_dest;
+    }
+  else if (src->type == reinsert_breakpoint)
     {
-      new_cmd = XCNEW (struct point_command_list);
-      new_cmd->cmd = clone_agent_expr (current_cmd->cmd);
-      new_cmd->persistence = current_cmd->persistence;
-      APPEND_TO_LIST (&dest->command_list, new_cmd, cmd_tail);
+      struct reinsert_breakpoint *reinsert_dest
+	= XCNEW (struct reinsert_breakpoint);
+
+      dest = (struct breakpoint *) reinsert_dest;
     }
+  else
+    gdb_assert_not_reached ("unhandled breakpoint type");
+
+  dest->type = src->type;
+  dest->raw = dest_raw;
 
   return dest;
 }
diff --git a/gdb/gdbserver/mem-break.h b/gdb/gdbserver/mem-break.h
index dd5a750..321de12 100644
--- a/gdb/gdbserver/mem-break.h
+++ b/gdb/gdbserver/mem-break.h
@@ -25,6 +25,7 @@
 
 /* Breakpoints are opaque.  */
 struct breakpoint;
+struct gdb_breakpoint;
 struct fast_tracepoint_jump;
 struct raw_breakpoint;
 struct process_info;
@@ -70,8 +71,8 @@ enum target_hw_bp_type raw_bkpt_type_to_target_hw_bp_type
    failure returns NULL and sets *ERR to either -1 for error, or 1 if
    Z_TYPE breakpoints are not supported on this target.  */
 
-struct breakpoint *set_gdb_breakpoint (char z_type, CORE_ADDR addr, int kind,
-				       int *err);
+struct gdb_breakpoint *set_gdb_breakpoint (char z_type, CORE_ADDR addr,
+					   int kind, int *err);
 
 /* Delete a GDB breakpoint of type Z_TYPE and kind KIND previously
    inserted at ADDR with set_gdb_breakpoint_at.  Returns 0 on success,
@@ -107,20 +108,20 @@ int reinsert_breakpoint_inserted_here (CORE_ADDR addr);
 /* Clear all breakpoint conditions and commands associated with a
    breakpoint.  */
 
-void clear_breakpoint_conditions_and_commands (struct breakpoint *bp);
+void clear_breakpoint_conditions_and_commands (struct gdb_breakpoint *bp);
 
 /* Set target-side condition CONDITION to the breakpoint at ADDR.
    Returns false on failure.  On success, advances CONDITION pointer
    past the condition and returns true.  */
 
-int add_breakpoint_condition (struct breakpoint *bp, char **condition);
+int add_breakpoint_condition (struct gdb_breakpoint *bp, char **condition);
 
 /* Set target-side commands COMMANDS to the breakpoint at ADDR.
    Returns false on failure.  On success, advances COMMANDS past the
    commands and returns true.  If PERSIST, the commands should run
    even while GDB is disconnected.  */
 
-int add_breakpoint_commands (struct breakpoint *bp, char **commands,
+int add_breakpoint_commands (struct gdb_breakpoint *bp, char **commands,
 			     int persist);
 
 int any_persistent_commands (void);
diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index 9c50929..18517bc 100644
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -3825,7 +3825,7 @@ main (int argc, char *argv[])
    after the last processed option.  */
 
 static void
-process_point_options (struct breakpoint *bp, char **packet)
+process_point_options (struct gdb_breakpoint *bp, char **packet)
 {
   char *dataptr = *packet;
   int persist;
@@ -4197,7 +4197,7 @@ process_serial_event (void)
 
 	if (insert)
 	  {
-	    struct breakpoint *bp;
+	    struct gdb_breakpoint *bp;
 
 	    bp = set_gdb_breakpoint (type, addr, kind, &res);
 	    if (bp != NULL)
-- 
1.9.1

[PATCH 8/9] Use reinsert_breakpoint for vCont;s

[Yao Qi]

V3: - install breakpoints in proceed_one_lwp, if the thread doesn't have
    reinsert breakpoints installed yet,
    - no longer stop all threads when installing breakpoints,
    - delete reinsert breakpoints when GDBserver wants to report event,

> - Also, if in all-stop mode, if gdb does:
>
> vCont;s:1;c
>
> thus setting thread 1 stepping, and all others continuing,
> and then some other thread but thread 1 hits a breakpoint,
> what is removing the reinsert breakpoint of thread 1?

if thread 1 doesn't hit the reinsert breakpoint, we don't have to
remove them, because GDB will send vCont;s:1 next time, and GDBserver
can only install reinsert breakpoints if they are not installed yet.
if thread hits the reinsert breakpoint, but the event is not reported.
It becomes pending, and GDBserver will delete the reinsert breakpoints
next time when this pending event is reported back to GDB.

V2: fix spaces in changelog entry,
    use maybe_hw_step,
    cancel step-over if signal arrives (!maybe_internal_trap),

This patch is to teach GDBserver using software single step to handle
vCont;s.  Simply speaking, if the thread's resume request is resume_step,
install reinsert breakpoint at the next pcs when GDBserver is about to
resume threads.  These reinsert breakpoints of a thread are removed,
when GDBserver gets an event from that thread and reports this event back
to GDB.

gdb/gdbserver:

2016-06-30  Yao Qi  <yao.qi@linaro.org>

	* linux-low.c (resume_stopped_resumed_lwps): If resume request
	is resume_step, call maybe_hw_step.
	(linux_wait_1): Stop all threads, remove reinsert breakpoints,
	and unstop them.
	(linux_resume_one_lwp_throw): Don't assert the thread has reinsert
	breakpoints or not.
	(proceed_one_lwp): If resume request is resume_step, install
	reinsert breakpoints and call maybe_hw_step.
---
 gdb/gdbserver/linux-low.c | 34 ++++++++++++++++++++++++++--------
 1 file changed, 26 insertions(+), 8 deletions(-)

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index abaf288..c9bb012 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -2563,7 +2563,10 @@ resume_stopped_resumed_lwps (struct inferior_list_entry *entry)
       && !lp->status_pending_p
       && thread->last_status.kind == TARGET_WAITKIND_IGNORE)
     {
-      int step = thread->last_resume_kind == resume_step;
+      int step = 0;
+
+      if (thread->last_resume_kind == resume_step)
+	step = maybe_hw_step (thread);
 
       if (debug_threads)
 	debug_printf ("RSRL: resuming stopped-resumed LWP %s at %s: step=%d\n",
@@ -3622,6 +3625,20 @@ linux_wait_1 (ptid_t ptid,
 
   /* Alright, we're going to report a stop.  */
 
+  /* Remove reinsert breakpoints.  */
+  if (can_software_single_step ()
+      && has_reinsert_breakpoints (current_thread))
+    {
+      /* Stop all threads before removing breakpoints out of memory,
+	 so that threads other than EVENT_CHILD won't hit the
+	 breakpoint in the staled memory.  */
+      stop_all_lwps (0, event_child);
+
+      delete_reinsert_breakpoints (current_thread);
+
+      unstop_all_lwps (0, event_child);
+    }
+
   if (!stabilizing_threads)
     {
       /* In all-stop, stop all threads.  */
@@ -4275,12 +4292,6 @@ linux_resume_one_lwp_throw (struct lwp_info *lwp,
 
       step = maybe_hw_step (thread);
     }
-  else
-    {
-      /* If the thread isn't doing step-over, there shouldn't be any
-	 reinsert breakpoints.  */
-      gdb_assert (!has_reinsert_breakpoints (thread));
-    }
 
   if (fast_tp_collecting == 1)
     {
@@ -5088,7 +5099,14 @@ proceed_one_lwp (struct inferior_list_entry *entry, void *except)
       if (debug_threads)
 	debug_printf ("   stepping LWP %ld, client wants it stepping\n",
 		      lwpid_of (thread));
-      step = 1;
+
+      /* If resume_step is requested by GDB, install reinsert
+	 breakpoints when the thread is about to be actually resumed if
+	 the reinsert breakpoints weren't removed.  */
+      if (can_software_single_step () && !has_reinsert_breakpoints (thread))
+	install_software_single_step_breakpoints (lwp);
+
+      step = maybe_hw_step (thread);
     }
   else if (lwp->bp_reinsert != 0)
     {
-- 
1.9.1

[PATCH 5/9] Switch current_thread to lwp's thread in install_software_single_step_breakpoints

[Yao Qi]

install_software_single_step_breakpoints has parameter lwp, but still
need to switch to current_thread.  In order to simplify its caller,
we do the current_thread save/restore inside install_software_single_step_breakpoints.

gdb/gdbserver:

2016-06-17  Yao Qi  <yao.qi@linaro.org>

	* gdbthread.h (make_cleanup_restore_current_thread): Declare.
	* inferiors.c (do_restore_current_thread_cleanup): New function.
	(make_cleanup_restore_current_thread): Likewise.
	* linux-low.c (install_software_single_step_breakpoints): Call
	make_cleanup_restore_current_thread.  Switch current_thread to
	thread.
---
 gdb/gdbserver/ChangeLog   |  9 +++++++++
 gdb/gdbserver/gdbthread.h |  3 +++
 gdb/gdbserver/inferiors.c | 12 ++++++++++++
 gdb/gdbserver/linux-low.c |  8 ++++++--
 4 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/gdb/gdbserver/ChangeLog b/gdb/gdbserver/ChangeLog
index 07e6743..b043847 100644
--- a/gdb/gdbserver/ChangeLog
+++ b/gdb/gdbserver/ChangeLog
@@ -47,6 +47,15 @@
 
 2016-06-17  Yao Qi  <yao.qi@linaro.org>
 
+	* gdbthread.h (make_cleanup_restore_current_thread): Declare.
+	* inferiors.c (do_restore_current_thread_cleanup): New function.
+	(make_cleanup_restore_current_thread): Likewise.
+	* linux-low.c (install_software_single_step_breakpoints): Call
+	make_cleanup_restore_current_thread.  Switch current_thread to
+	thread.
+
+2016-06-17  Yao Qi  <yao.qi@linaro.org>
+
 	* mem-break.c (struct reinsert_breakpoint) <ptid>: New field.
 	(set_reinsert_breakpoint): New parameter ptid.  Callers updated.
 	(clone_one_breakpoint): Likewise.
diff --git a/gdb/gdbserver/gdbthread.h b/gdb/gdbserver/gdbthread.h
index 4dcb1b7..d2326fb 100644
--- a/gdb/gdbserver/gdbthread.h
+++ b/gdb/gdbserver/gdbthread.h
@@ -87,4 +87,7 @@ struct thread_info *find_any_thread_of_pid (int pid);
 /* Get current thread ID (Linux task ID).  */
 #define current_ptid (current_thread->entry.id)
 
+/* Create a cleanup to restore current_thread.  */
+struct cleanup *make_cleanup_restore_current_thread (void);
+
 #endif /* GDB_THREAD_H */
diff --git a/gdb/gdbserver/inferiors.c b/gdb/gdbserver/inferiors.c
index 1f5149f..7888f3c 100644
--- a/gdb/gdbserver/inferiors.c
+++ b/gdb/gdbserver/inferiors.c
@@ -411,3 +411,15 @@ current_process (void)
   gdb_assert (current_thread != NULL);
   return get_thread_process (current_thread);
 }
+
+static void
+do_restore_current_thread_cleanup (void *arg)
+{
+  current_thread = (struct thread_info *) arg;
+}
+
+struct cleanup *
+make_cleanup_restore_current_thread (void)
+{
+  return make_cleanup (do_restore_current_thread_cleanup, current_thread);
+}
diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index a55a0a5..941b0e8 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -4124,10 +4124,14 @@ install_software_single_step_breakpoints (struct lwp_info *lwp)
 {
   int i;
   CORE_ADDR pc;
-  struct regcache *regcache = get_thread_regcache (current_thread, 1);
+  struct thread_info *thread = get_lwp_thread (lwp);
+  struct regcache *regcache = get_thread_regcache (thread, 1);
   VEC (CORE_ADDR) *next_pcs = NULL;
-  struct cleanup *old_chain = make_cleanup (VEC_cleanup (CORE_ADDR), &next_pcs);
+  struct cleanup *old_chain = make_cleanup_restore_current_thread ();
+
+  make_cleanup (VEC_cleanup (CORE_ADDR), &next_pcs);
 
+  current_thread = thread;
   next_pcs = (*the_low_target.get_next_pcs) (regcache);
 
   for (i = 0; VEC_iterate (CORE_ADDR, next_pcs, i, pc); ++i)
-- 
1.9.1

[PATCH 4/9] Make reinsert_breakpoint thread specific

[Yao Qi]

V3: pass argument "thread" isntead of "ptid"

V2: rewrite commit log to make it easy to read,
    "id" -> "ptid",

This patch makes reinsert_breakpoint thread specific, which means we
insert and remove reinsert_breakpoint breakpoints for a specific
thread.  This motivation of this change is that I'll use
reinsert_breakpoint for vCont;s on software single step target, so that
GDBserver may insert one reinsert_breakpoint for one thread doing
step-over, and insert one reinsert_breakpoint for another thread doing
vCont;s.  After the operation of one thread is finished, GDBserver must
remove reinsert_breakpoint for that thread only.

On the other hand, reinsert_breakpoint is used for step-over nowadays.
GDBserver inserts reinsert_breakpoint, and wait only from the thread
doing step-over.  After the step-over is done, GDBserver removes the
reinsert_breakpoint.  If there is still any threads need step-over, do
the same again until all threads are finished step-over.  In other words,
reinsert_breakpoint is globally thread specific, but in an implicit way.
It is natural to make it explicitly thread specific.

gdb/gdbserver:

2016-06-17  Yao Qi  <yao.qi@linaro.org>

	* mem-break.c (struct reinsert_breakpoint) <ptid>: New field.
	(set_reinsert_breakpoint): New parameter ptid.  Callers updated.
	(clone_one_breakpoint): Likewise.
	(delete_reinsert_breakpoints): Change parameter to thread.
	Callers updated.
	(has_reinsert_breakpoints): Likewise.
	(uninsert_reinsert_breakpoints): Likewise.
	(reinsert_reinsert_breakpoints): Likewise.
	* mem-break.h (set_reinsert_breakpoint): Update declaration.
	(delete_reinsert_breakpoints): Likewise.
	(reinsert_reinsert_breakpoints): Likewise.
	(uninsert_reinsert_breakpoints): Likewise.
	(has_reinsert_breakpoints): Likewise.
---
 gdb/gdbserver/ChangeLog   | 16 +++++++++++
 gdb/gdbserver/linux-low.c | 37 ++++++++----------------
 gdb/gdbserver/mem-break.c | 71 +++++++++++++++++++++++++++++++++++------------
 gdb/gdbserver/mem-break.h | 23 +++++++--------
 4 files changed, 92 insertions(+), 55 deletions(-)

diff --git a/gdb/gdbserver/ChangeLog b/gdb/gdbserver/ChangeLog
index 81c7c74..07e6743 100644
--- a/gdb/gdbserver/ChangeLog
+++ b/gdb/gdbserver/ChangeLog
@@ -47,6 +47,22 @@
 
 2016-06-17  Yao Qi  <yao.qi@linaro.org>
 
+	* mem-break.c (struct reinsert_breakpoint) <ptid>: New field.
+	(set_reinsert_breakpoint): New parameter ptid.  Callers updated.
+	(clone_one_breakpoint): Likewise.
+	(delete_reinsert_breakpoints): Change parameter to thread.
+	Callers updated.
+	(has_reinsert_breakpoints): Likewise.
+	(uninsert_reinsert_breakpoints): Likewise.
+	(reinsert_reinsert_breakpoints): Likewise.
+	* mem-break.h (set_reinsert_breakpoint): Update declaration.
+	(delete_reinsert_breakpoints): Likewise.
+	(reinsert_reinsert_breakpoints): Likewise.
+	(uninsert_reinsert_breakpoints): Likewise.
+	(has_reinsert_breakpoints): Likewise.
+
+2016-06-17  Yao Qi  <yao.qi@linaro.org>
+
 	* inferiors.c (get_thread_process): Make parameter const.
 	* inferiors.h (get_thread_process): Update declaration.
 	* mem-break.c (clone_all_breakpoints): Remove all parameters.
diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index 74dbb9a..a55a0a5 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -548,15 +548,11 @@ handle_extended_wait (struct lwp_info **orig_event_lwp, int wstat)
 	      && can_software_single_step ()
 	      && event == PTRACE_EVENT_VFORK)
 	    {
-	      struct thread_info *saved_thread = current_thread;
-
-	      current_thread = event_thr;
 	      /* If we leave reinsert breakpoints there, child will
 		 hit it, so uninsert reinsert breakpoints from parent
 		 (and child).  Once vfork child is done, reinsert
 		 them back to parent.  */
-	      uninsert_reinsert_breakpoints ();
-	      current_thread = saved_thread;
+	      uninsert_reinsert_breakpoints (event_thr);
 	    }
 
 	  clone_all_breakpoints (child_thr, event_thr);
@@ -591,17 +587,13 @@ handle_extended_wait (struct lwp_info **orig_event_lwp, int wstat)
 	  if (event_lwp->bp_reinsert != 0
 	      && can_software_single_step ())
 	    {
-	      struct thread_info *saved_thread = current_thread;
-
 	      /* The child process is forked and stopped, so it is safe
 		 to access its memory without stopping all other threads
 		 from other processes.  */
-	      current_thread = child_thr;
-	      delete_reinsert_breakpoints ();
-	      current_thread = saved_thread;
+	      delete_reinsert_breakpoints (child_thr);
 
-	      gdb_assert (has_reinsert_breakpoints (parent_proc));
-	      gdb_assert (!has_reinsert_breakpoints (child_proc));
+	      gdb_assert (has_reinsert_breakpoints (event_thr));
+	      gdb_assert (!has_reinsert_breakpoints (child_thr));
 	    }
 
 	  /* Report the event.  */
@@ -655,14 +647,9 @@ handle_extended_wait (struct lwp_info **orig_event_lwp, int wstat)
 
       if (event_lwp->bp_reinsert != 0 && can_software_single_step ())
 	{
-	  struct thread_info *saved_thread = current_thread;
-	  struct process_info *proc = get_thread_process (event_thr);
+	  reinsert_reinsert_breakpoints (event_thr);
 
-	  current_thread = event_thr;
-	  reinsert_reinsert_breakpoints ();
-	  current_thread = saved_thread;
-
-	  gdb_assert (has_reinsert_breakpoints (proc));
+	  gdb_assert (has_reinsert_breakpoints (event_thr));
 	}
 
       /* Report the event.  */
@@ -2554,11 +2541,9 @@ maybe_hw_step (struct thread_info *thread)
     return 1;
   else
     {
-      struct process_info *proc = get_thread_process (thread);
-
       /* GDBserver must insert reinsert breakpoint for software
 	 single step.  */
-      gdb_assert (has_reinsert_breakpoints (proc));
+      gdb_assert (has_reinsert_breakpoints (thread));
       return 0;
     }
 }
@@ -4146,7 +4131,7 @@ install_software_single_step_breakpoints (struct lwp_info *lwp)
   next_pcs = (*the_low_target.get_next_pcs) (regcache);
 
   for (i = 0; VEC_iterate (CORE_ADDR, next_pcs, i, pc); ++i)
-    set_reinsert_breakpoint (pc);
+    set_reinsert_breakpoint (pc, current_ptid);
 
   do_cleanups (old_chain);
 }
@@ -4289,7 +4274,7 @@ linux_resume_one_lwp_throw (struct lwp_info *lwp,
     {
       /* If the thread isn't doing step-over, there shouldn't be any
 	 reinsert breakpoints.  */
-      gdb_assert (!has_reinsert_breakpoints (proc));
+      gdb_assert (!has_reinsert_breakpoints (thread));
     }
 
   if (fast_tp_collecting == 1)
@@ -4782,8 +4767,8 @@ finish_step_over (struct lwp_info *lwp)
 	 threads but LWP stopped while doing that.  */
       if (!can_hardware_single_step ())
 	{
-	  gdb_assert (has_reinsert_breakpoints (current_process ()));
-	  delete_reinsert_breakpoints ();
+	  gdb_assert (has_reinsert_breakpoints (current_thread));
+	  delete_reinsert_breakpoints (current_thread);
 	}
 
       step_over_bkpt = null_ptid;
diff --git a/gdb/gdbserver/mem-break.c b/gdb/gdbserver/mem-break.c
index c14219e..65ca3f9 100644
--- a/gdb/gdbserver/mem-break.c
+++ b/gdb/gdbserver/mem-break.c
@@ -211,6 +211,9 @@ struct other_breakpoint
 struct reinsert_breakpoint
 {
   struct breakpoint base;
+
+  /* Thread the reinsert breakpoint belongs to.  */
+  ptid_t ptid;
 };
 
 /* Return the breakpoint size from its kind.  */
@@ -1476,17 +1479,21 @@ gdb_breakpoint_here (CORE_ADDR where)
 }
 
 void
-set_reinsert_breakpoint (CORE_ADDR stop_at)
+set_reinsert_breakpoint (CORE_ADDR stop_at, ptid_t ptid)
 {
-  struct breakpoint *bp;
+  struct reinsert_breakpoint *bp;
+
+  gdb_assert (ptid_get_pid (current_ptid) == ptid_get_pid (ptid));
 
-  bp = set_breakpoint_type_at (reinsert_breakpoint, stop_at, NULL);
+  bp = (struct reinsert_breakpoint *) set_breakpoint_type_at (reinsert_breakpoint,
+							      stop_at, NULL);
+  bp->ptid = ptid;
 }
 
 void
-delete_reinsert_breakpoints (void)
+delete_reinsert_breakpoints (struct thread_info *thread)
 {
-  struct process_info *proc = current_process ();
+  struct process_info *proc = get_thread_process (thread);
   struct breakpoint *bp, **bp_link;
 
   bp = proc->breakpoints;
@@ -1494,11 +1501,17 @@ delete_reinsert_breakpoints (void)
 
   while (bp)
     {
-      if (bp->type == reinsert_breakpoint)
+      if (bp->type == reinsert_breakpoint
+	  && ptid_equal (((struct reinsert_breakpoint *) bp)->ptid,
+			 ptid_of (thread)))
 	{
+	  struct thread_info *saved_thread = current_thread;
+
+	  current_thread = thread;
 	  *bp_link = bp->next;
 	  release_breakpoint (proc, bp);
 	  bp = *bp_link;
+	  current_thread = saved_thread;
 	}
       else
 	{
@@ -1578,21 +1591,29 @@ uninsert_all_breakpoints (void)
 }
 
 void
-uninsert_reinsert_breakpoints (void)
+uninsert_reinsert_breakpoints (struct thread_info *thread)
 {
-  struct process_info *proc = current_process ();
+  struct process_info *proc = get_thread_process (thread);
   struct breakpoint *bp;
 
   for (bp = proc->breakpoints; bp != NULL; bp = bp->next)
     {
-    if (bp->type == reinsert_breakpoint)
+    if (bp->type == reinsert_breakpoint
+	&& ptid_equal (((struct reinsert_breakpoint *) bp)->ptid,
+		       ptid_of (thread)))
       {
 	gdb_assert (bp->raw->inserted > 0);
 
 	/* Only uninsert the raw breakpoint if it only belongs to a
 	   reinsert breakpoint.  */
 	if (bp->raw->refcount == 1)
-	  uninsert_raw_breakpoint (bp->raw);
+	  {
+	    struct thread_info *saved_thread = current_thread;
+
+	    current_thread = thread;
+	    uninsert_raw_breakpoint (bp->raw);
+	    current_thread = saved_thread;
+	  }
       }
     }
 }
@@ -1642,8 +1663,9 @@ reinsert_breakpoints_at (CORE_ADDR pc)
 }
 
 int
-has_reinsert_breakpoints (struct process_info *proc)
+has_reinsert_breakpoints (struct thread_info *thread)
 {
+  struct process_info *proc = get_thread_process (thread);
   struct breakpoint *bp, **bp_link;
 
   bp = proc->breakpoints;
@@ -1651,7 +1673,9 @@ has_reinsert_breakpoints (struct process_info *proc)
 
   while (bp)
     {
-      if (bp->type == reinsert_breakpoint)
+      if (bp->type == reinsert_breakpoint
+	  && ptid_equal (((struct reinsert_breakpoint *) bp)->ptid,
+			 ptid_of (thread)))
 	return 1;
       else
 	{
@@ -1677,19 +1701,27 @@ reinsert_all_breakpoints (void)
 }
 
 void
-reinsert_reinsert_breakpoints (void)
+reinsert_reinsert_breakpoints (struct thread_info *thread)
 {
-  struct process_info *proc = current_process ();
+  struct process_info *proc = get_thread_process (thread);
   struct breakpoint *bp;
 
   for (bp = proc->breakpoints; bp != NULL; bp = bp->next)
     {
-      if (bp->type == reinsert_breakpoint)
+      if (bp->type == reinsert_breakpoint
+	  && ptid_equal (((struct reinsert_breakpoint *) bp)->ptid,
+			 ptid_of (thread)))
 	{
 	  gdb_assert (bp->raw->inserted > 0);
 
 	  if (bp->raw->refcount == 1)
-	    reinsert_raw_breakpoint (bp->raw);
+	    {
+	      struct thread_info *saved_thread = current_thread;
+
+	      current_thread = thread;
+	      reinsert_raw_breakpoint (bp->raw);
+	      current_thread = saved_thread;
+	    }
 	}
     }
 }
@@ -2113,7 +2145,7 @@ clone_agent_expr (const struct agent_expr *src_ax)
 /* Deep-copy the contents of one breakpoint to another.  */
 
 static struct breakpoint *
-clone_one_breakpoint (const struct breakpoint *src)
+clone_one_breakpoint (const struct breakpoint *src, ptid_t ptid)
 {
   struct breakpoint *dest;
   struct raw_breakpoint *dest_raw;
@@ -2174,6 +2206,9 @@ clone_one_breakpoint (const struct breakpoint *src)
 	= XCNEW (struct reinsert_breakpoint);
 
       dest = (struct breakpoint *) reinsert_dest;
+      /* Since reinsert breakpoint is thread specific, don't copy
+	 thread id from SRC, use ID instead.  */
+      reinsert_dest->ptid = ptid;
     }
   else
     gdb_assert_not_reached ("unhandled breakpoint type");
@@ -2201,7 +2236,7 @@ clone_all_breakpoints (struct thread_info *child_thread,
 
   for (bp = parent_proc->breakpoints; bp != NULL; bp = bp->next)
     {
-      new_bkpt = clone_one_breakpoint (bp);
+      new_bkpt = clone_one_breakpoint (bp, ptid_of (child_thread));
       APPEND_TO_LIST (new_list, new_bkpt, bkpt_tail);
       APPEND_TO_LIST (new_raw_list, new_bkpt->raw, raw_bkpt_tail);
     }
diff --git a/gdb/gdbserver/mem-break.h b/gdb/gdbserver/mem-break.h
index d633003..3322ec5 100644
--- a/gdb/gdbserver/mem-break.h
+++ b/gdb/gdbserver/mem-break.h
@@ -152,31 +152,32 @@ struct breakpoint *set_breakpoint_at (CORE_ADDR where,
 
 int delete_breakpoint (struct breakpoint *bkpt);
 
-/* Set a reinsert breakpoint at STOP_AT.  */
+/* Set a reinsert breakpoint at STOP_AT for thread represented by
+   PTID.  */
 
-void set_reinsert_breakpoint (CORE_ADDR stop_at);
+void set_reinsert_breakpoint (CORE_ADDR stop_at, ptid_t ptid);
 
-/* Delete all reinsert breakpoints.  */
+/* Delete all reinsert breakpoints of THREAD.  */
 
-void delete_reinsert_breakpoints (void);
+void delete_reinsert_breakpoints (struct thread_info *thread);
 
-/* Reinsert all reinsert breakpoints of the current process.  */
+/* Reinsert all reinsert breakpoints of THREAD.  */
 
-void reinsert_reinsert_breakpoints (void);
+void reinsert_reinsert_breakpoints (struct thread_info *thread);
 
-/* Uninsert all reinsert breakpoints of the current process.  This
-   still leaves the reinsert breakpoints in the table.  */
+/* Uninsert all reinsert breakpoints of THREAD.  This still leaves
+   the reinsert breakpoints in the table.  */
 
-void uninsert_reinsert_breakpoints (void);
+void uninsert_reinsert_breakpoints (struct thread_info *thread);
 
 /* Reinsert breakpoints at WHERE (and change their status to
    inserted).  */
 
 void reinsert_breakpoints_at (CORE_ADDR where);
 
-/* Process PROC has reinsert breakpoints or not.  */
+/* The THREAD has reinsert breakpoints or not.  */
 
-int has_reinsert_breakpoints (struct process_info *proc);
+int has_reinsert_breakpoints (struct thread_info *thread);
 
 /* Uninsert breakpoints at WHERE (and change their status to
    uninserted).  This still leaves the breakpoints in the table.  */
-- 
1.9.1

[PATCH 9/9] Support vCont s and S actions with software single step

[Yao Qi]

GDBserver with software single step should be able to claim supporting
vCont s and S actions, so that GDB knows the remote target can do
single step.  It doesn't matter to GDB that the single step in the
remote target is done via hardware or software.

gdb/gdbserver:

2016-06-02  Yao Qi  <yao.qi@linaro.org>

	* server.c (handle_v_requests): Support s and S actions
	if target_supports_software_single_step return true.
---
 gdb/gdbserver/server.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index 18517bc..6d6cb09 100644
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -2958,12 +2958,15 @@ handle_v_requests (char *own_buf, int packet_len, int *new_packet_len)
 	{
 	  strcpy (own_buf, "vCont;c;C;t");
 
-	  if (target_supports_hardware_single_step () || !vCont_supported)
+	  if (target_supports_hardware_single_step ()
+	      || target_supports_software_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.  */
+	      /* If target supports single step either by hardware or by
+		 software, 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");
 	    }
-- 
1.9.1

[PATCH 3/9] Refactor clone_all_breakpoints

[Yao Qi]

V3: Make parent_thread be const, and make get_thread_process parameter
    const too,

V2: pass parent thread instead of parent process to
    clone_all_breakpoints,

This patch is to change the interface of clone_all_breakpoints, from
lists of breakpoints and raw_breakpoints to child thread and parent
thread.  I choose child thread to pass because we need the ptid of
the child thread in the following patch.

gdb/gdbserver:

2016-06-17  Yao Qi  <yao.qi@linaro.org>

	* inferiors.c (get_thread_process): Make parameter const.
	* inferiors.h (get_thread_process): Update declaration.
	* mem-break.c (clone_all_breakpoints): Remove all parameters.
	Add new parameters child_thread and parent_thread.  Callers
	updated.
	* mem-break.h (clone_all_breakpoints): Update declaration.
---
 gdb/gdbserver/ChangeLog   |  9 +++++++++
 gdb/gdbserver/inferiors.c |  2 +-
 gdb/gdbserver/inferiors.h |  2 +-
 gdb/gdbserver/linux-low.c |  4 +---
 gdb/gdbserver/mem-break.c | 15 ++++++++-------
 gdb/gdbserver/mem-break.h |  8 ++++----
 6 files changed, 24 insertions(+), 16 deletions(-)

diff --git a/gdb/gdbserver/ChangeLog b/gdb/gdbserver/ChangeLog
index a59d4e5..81c7c74 100644
--- a/gdb/gdbserver/ChangeLog
+++ b/gdb/gdbserver/ChangeLog
@@ -47,6 +47,15 @@
 
 2016-06-17  Yao Qi  <yao.qi@linaro.org>
 
+	* inferiors.c (get_thread_process): Make parameter const.
+	* inferiors.h (get_thread_process): Update declaration.
+	* mem-break.c (clone_all_breakpoints): Remove all parameters.
+	Add new parameters child_thread and parent_thread.  Callers
+	updated.
+	* mem-break.h (clone_all_breakpoints): Update declaration.
+
+2016-06-17  Yao Qi  <yao.qi@linaro.org>
+
 	* mem-break.c (struct breakpoint) <cond_list>: Remove.
 	<command_list, handler>: Remove.
 	(struct gdb_breakpoint): New.
diff --git a/gdb/gdbserver/inferiors.c b/gdb/gdbserver/inferiors.c
index 4bea4fd..1f5149f 100644
--- a/gdb/gdbserver/inferiors.c
+++ b/gdb/gdbserver/inferiors.c
@@ -399,7 +399,7 @@ have_attached_inferiors_p (void)
 }
 
 struct process_info *
-get_thread_process (struct thread_info *thread)
+get_thread_process (const struct thread_info *thread)
 {
   int pid = ptid_get_pid (thread->entry.id);
   return find_process_pid (pid);
diff --git a/gdb/gdbserver/inferiors.h b/gdb/gdbserver/inferiors.h
index 00dfe60..65ab1c6 100644
--- a/gdb/gdbserver/inferiors.h
+++ b/gdb/gdbserver/inferiors.h
@@ -88,7 +88,7 @@ struct process_info
    no current thread selected.  */
 
 struct process_info *current_process (void);
-struct process_info *get_thread_process (struct thread_info *);
+struct process_info *get_thread_process (const struct thread_info *);
 
 extern struct inferior_list all_processes;
 
diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index 0f4bb87..74dbb9a 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -559,9 +559,7 @@ handle_extended_wait (struct lwp_info **orig_event_lwp, int wstat)
 	      current_thread = saved_thread;
 	    }
 
-	  clone_all_breakpoints (&child_proc->breakpoints,
-				 &child_proc->raw_breakpoints,
-				 parent_proc->breakpoints);
+	  clone_all_breakpoints (child_thr, event_thr);
 
 	  tdesc = XNEW (struct target_desc);
 	  copy_target_description (tdesc, parent_proc->tdesc);
diff --git a/gdb/gdbserver/mem-break.c b/gdb/gdbserver/mem-break.c
index 3539422..c14219e 100644
--- a/gdb/gdbserver/mem-break.c
+++ b/gdb/gdbserver/mem-break.c
@@ -2184,21 +2184,22 @@ clone_one_breakpoint (const struct breakpoint *src)
   return dest;
 }
 
-/* Create a new breakpoint list NEW_LIST that is a copy of the
-   list starting at SRC_LIST.  Create the corresponding new
-   raw_breakpoint list NEW_RAW_LIST as well.  */
+/* See mem-break.h.  */
 
 void
-clone_all_breakpoints (struct breakpoint **new_list,
-		       struct raw_breakpoint **new_raw_list,
-		       const struct breakpoint *src_list)
+clone_all_breakpoints (struct thread_info *child_thread,
+		       const struct thread_info *parent_thread)
 {
   const struct breakpoint *bp;
   struct breakpoint *new_bkpt;
   struct breakpoint *bkpt_tail = NULL;
   struct raw_breakpoint *raw_bkpt_tail = NULL;
+  struct process_info *child_proc = get_thread_process (child_thread);
+  struct process_info *parent_proc = get_thread_process (parent_thread);
+  struct breakpoint **new_list = &child_proc->breakpoints;
+  struct raw_breakpoint **new_raw_list = &child_proc->raw_breakpoints;
 
-  for (bp = src_list; bp != NULL; bp = bp->next)
+  for (bp = parent_proc->breakpoints; bp != NULL; bp = bp->next)
     {
       new_bkpt = clone_one_breakpoint (bp);
       APPEND_TO_LIST (new_list, new_bkpt, bkpt_tail);
diff --git a/gdb/gdbserver/mem-break.h b/gdb/gdbserver/mem-break.h
index 321de12..d633003 100644
--- a/gdb/gdbserver/mem-break.h
+++ b/gdb/gdbserver/mem-break.h
@@ -267,10 +267,10 @@ int insert_memory_breakpoint (struct raw_breakpoint *bp);
 
 int remove_memory_breakpoint (struct raw_breakpoint *bp);
 
-/* Create a new breakpoint list NEW_BKPT_LIST that is a copy of SRC.  */
+/* Create a new breakpoint list in CHILD_THREAD's process that is a
+   copy of breakpoint list in PARENT_THREAD's process.  */
 
-void clone_all_breakpoints (struct breakpoint **new_bkpt_list,
-			    struct raw_breakpoint **new_raw_bkpt_list,
-			    const struct breakpoint *src);
+void clone_all_breakpoints (struct thread_info *child_thread,
+			    const struct thread_info *parent_thread);
 
 #endif /* MEM_BREAK_H */
-- 
1.9.1

[PATCH 1/9] Pass breakpoint type in set_breakpoint_at

[Yao Qi]

Nowadays, set_breakpoint_at creates breakpoint of type
other_breakpoint, but we also use set_breakpoint_at
in set_reinsert_breakpoint to create breakpoint, so that
we have to overwrite the breakpoint type like this,

  bp = set_breakpoint_at (stop_at, NULL);
  bp->type = reinsert_breakpoint;

which looks not very good.  This patch changes set_breakpoint_at
to receive breakpoint type.  Since set_breakpoint_at is
used in many places, I rename it to set_breakpoint_type_at, and wrap
it with set_breakpoint_at, and pass other_breakpoint.  In this way,
we can call set_breakpoint_type_at with reinsert_breakpoint in
set_reinsert_breakpoint too, and code looks cleaner.

gdb/gdbserver:

2016-06-17  Yao Qi  <yao.qi@linaro.org>

	* mem-break.c (set_breakpoint_at): Rename it to ...
	(set_breakpoint_type_at): ... it.
	(set_breakpoint_at): Call set_breakpoint_type_at.
	(set_reinsert_breakpoint): Call set_breakpoint_type_at.
	* mem-break.h (set_breakpoint_at): Update comments.
---
 gdb/gdbserver/ChangeLog   |  8 ++++++++
 gdb/gdbserver/mem-break.c | 20 ++++++++++++++------
 gdb/gdbserver/mem-break.h |  3 ++-
 3 files changed, 24 insertions(+), 7 deletions(-)

diff --git a/gdb/gdbserver/ChangeLog b/gdb/gdbserver/ChangeLog
index 30d0498..7c24260 100644
--- a/gdb/gdbserver/ChangeLog
+++ b/gdb/gdbserver/ChangeLog
@@ -47,6 +47,14 @@
 
 2016-06-17  Yao Qi  <yao.qi@linaro.org>
 
+	* mem-break.c (set_breakpoint_at): Rename it to ...
+	(set_breakpoint_type_at): ... it.
+	(set_breakpoint_at): Call set_breakpoint_type_at.
+	(set_reinsert_breakpoint): Call set_breakpoint_type_at.
+	* mem-break.h (set_breakpoint_at): Update comments.
+
+2016-06-17  Yao Qi  <yao.qi@linaro.org>
+
 	* linux-low.c (handle_extended_wait): Call
 	uninsert_reinsert_breakpoints for the parent process.  Remove
 	reinsert breakpoints from the child process.  Reinsert them to
diff --git a/gdb/gdbserver/mem-break.c b/gdb/gdbserver/mem-break.c
index 5c73326..f0b77f9 100644
--- a/gdb/gdbserver/mem-break.c
+++ b/gdb/gdbserver/mem-break.c
@@ -785,20 +785,29 @@ set_breakpoint (enum bkpt_type type, enum raw_bkpt_type raw_type,
   return bp;
 }
 
-/* See mem-break.h  */
+/* Set breakpoint of TYPE on address WHERE with handler HANDLER.  */
 
-struct breakpoint *
-set_breakpoint_at (CORE_ADDR where, int (*handler) (CORE_ADDR))
+static struct breakpoint *
+set_breakpoint_type_at (enum bkpt_type type, CORE_ADDR where,
+			int (*handler) (CORE_ADDR))
 {
   int err_ignored;
   CORE_ADDR placed_address = where;
   int breakpoint_kind = target_breakpoint_kind_from_pc (&placed_address);
 
-  return set_breakpoint (other_breakpoint, raw_bkpt_type_sw,
+  return set_breakpoint (type, raw_bkpt_type_sw,
 			 placed_address, breakpoint_kind, handler,
 			 &err_ignored);
 }
 
+/* See mem-break.h  */
+
+struct breakpoint *
+set_breakpoint_at (CORE_ADDR where, int (*handler) (CORE_ADDR))
+{
+  return set_breakpoint_type_at (other_breakpoint, where, handler);
+}
+
 
 static int
 delete_raw_breakpoint (struct process_info *proc, struct raw_breakpoint *todel)
@@ -1411,8 +1420,7 @@ set_reinsert_breakpoint (CORE_ADDR stop_at)
 {
   struct breakpoint *bp;
 
-  bp = set_breakpoint_at (stop_at, NULL);
-  bp->type = reinsert_breakpoint;
+  bp = set_breakpoint_type_at (reinsert_breakpoint, stop_at, NULL);
 }
 
 void
diff --git a/gdb/gdbserver/mem-break.h b/gdb/gdbserver/mem-break.h
index 6a06c0c..dd5a750 100644
--- a/gdb/gdbserver/mem-break.h
+++ b/gdb/gdbserver/mem-break.h
@@ -141,7 +141,8 @@ int gdb_breakpoint_here (CORE_ADDR where);
 
 /* Create a new breakpoint at WHERE, and call HANDLER when
    it is hit.  HANDLER should return 1 if the breakpoint
-   should be deleted, 0 otherwise.  */
+   should be deleted, 0 otherwise.  The type of the created
+   breakpoint is other_breakpoint.  */
 
 struct breakpoint *set_breakpoint_at (CORE_ADDR where,
 				      int (*handler) (CORE_ADDR));
-- 
1.9.1

Re: [PATCH 7/9] Enqueue signal even when resuming threads

[Pedro Alves]

On 06/30/2016 03:09 PM, Yao Qi wrote:
> Nowadays, we only enqueue signal when we leave thread pending in
> linux_resume_one_thread.  If lwp->resume->sig isn't zero (GDB wants
> to resume with signal), we pass lwp->resume->sig to
> linux_resume_one_lwp.
> 
> In order to reduce the difference between resuming thread with signal
> and proceeding thread with signal, when we resume thread, we can
> enqueue signal too, and proceed thread.  The signal will be consumed in
> linux_resume_one_lwp_throw from lwp->pending_signals.

This makes one subtle change.  If the thread already
had a pending signal, and we're getting a resume request with
a signal that is a different signal from the one already queued,
then before the patch, we'd tell the kernel to deliver the new
signal first, and then only deliver the pending signal the next time
the thread is resumed.  While after the patch, we'll enqueue the
new signal, and deliver the one that was already pending first.

Can't really say whether the old behavior was necessary.  At least
the new behavior seems to make order or signal delivery consistent
with how the order the signals were made pending in the
first place, so seems better.

This made me notice that this scenario of having more than one
pending signal doesn't seem to be handled perfectly.  We deliver
the first signal, but nothing makes sure to get back control
of the thread immediately in order to deliver the other pending
signal, so it seems the thread may execute a bunch of arbitrary code
until it next stops and is re-resumed, at which point we'll deliver
the other pending signal.  Maybe the simplest would be to
force the thread to immediately stop again, by calling
send_sigstop before resuming it, whenever we have more pending
signals.  Subject for a separate patch, in any case.

Thanks,
Pedro Alves

Re: [PATCH 8/9] Use reinsert_breakpoint for vCont;s

[Pedro Alves]

On 06/30/2016 03:09 PM, Yao Qi wrote:
> V3: - install breakpoints in proceed_one_lwp, if the thread doesn't have
>     reinsert breakpoints installed yet,
>     - no longer stop all threads when installing breakpoints,
>     - delete reinsert breakpoints when GDBserver wants to report event,
> 
>> - Also, if in all-stop mode, if gdb does:
>>
>> vCont;s:1;c
>>
>> thus setting thread 1 stepping, and all others continuing,
>> and then some other thread but thread 1 hits a breakpoint,
>> what is removing the reinsert breakpoint of thread 1?
> 
> if thread 1 doesn't hit the reinsert breakpoint, we don't have to
> remove them, because GDB will send vCont;s:1 next time, and GDBserver

There's no guarantee GDB will send vCont;s:1 next time.
The user may do "continue" instead of "step".

> can only install reinsert breakpoints if they are not installed yet.

The user may even do "return + continue" or "jump", or an infcall,
all of which resume the thread at a different address from the address
the thread last stopped.  So there's no guarantee that the
reinsert breakpoint address makes any sense for the next step request,
or even that the next resume request is a step in the first place.

Basically the previous step request must be completely forgotten after
gdb has seen the thread stop.  In all-stop, gdb "sees "all threads
stopped on each and every event reported to gdb, for any thread.
A stop reply cancels any and all previous resume requests.

> if thread hits the reinsert breakpoint, but the event is not reported.
> It becomes pending, and GDBserver will delete the reinsert breakpoints
> next time when this pending event is reported back to GDB.

I don't follow.  I'm talking about the case where the thread does _not_
hit the reinsert breakpoint.  Instead some other thread hits some unrelated
event.

Thanks,
Pedro Alves

Re: [PATCH 7/9] Enqueue signal even when resuming threads

[Yao Qi]

On Fri, Jul 1, 2016 at 4:06 PM, Pedro Alves <palves@redhat.com> wrote:
> On 06/30/2016 03:09 PM, Yao Qi wrote:
>> Nowadays, we only enqueue signal when we leave thread pending in
>> linux_resume_one_thread.  If lwp->resume->sig isn't zero (GDB wants
>> to resume with signal), we pass lwp->resume->sig to
>> linux_resume_one_lwp.
>>
>> In order to reduce the difference between resuming thread with signal
>> and proceeding thread with signal, when we resume thread, we can
>> enqueue signal too, and proceed thread.  The signal will be consumed in
>> linux_resume_one_lwp_throw from lwp->pending_signals.
>
> This makes one subtle change.  If the thread already
> had a pending signal, and we're getting a resume request with
> a signal that is a different signal from the one already queued,
> then before the patch, we'd tell the kernel to deliver the new
> signal first, and then only deliver the pending signal the next time
> the thread is resumed.  While after the patch, we'll enqueue the
> new signal, and deliver the one that was already pending first.
>
> Can't really say whether the old behavior was necessary.  At least
> the new behavior seems to make order or signal delivery consistent
> with how the order the signals were made pending in the
> first place, so seems better.
>
> This made me notice that this scenario of having more than one
> pending signal doesn't seem to be handled perfectly.  We deliver
> the first signal, but nothing makes sure to get back control
> of the thread immediately in order to deliver the other pending
> signal, so it seems the thread may execute a bunch of arbitrary code
> until it next stops and is re-resumed, at which point we'll deliver
> the other pending signal.  Maybe the simplest would be to
> force the thread to immediately stop again, by calling
> send_sigstop before resuming it, whenever we have more pending
> signals.  Subject for a separate patch, in any case.

You meant "after resuming it" rather than "before resuming it", right?  We have
two pending signals, so we resume the lwp and deliver the first signal.  After
resuming, we need to immediately deliver the second signal, so we call
send_sigstop.

IIUC, this patch is OK as-is, right?

-- 
Yao (齐尧)

Re: [PATCH 7/9] Enqueue signal even when resuming threads

[Pedro Alves]

On 07/01/2016 05:45 PM, Yao Qi wrote:

> You meant "after resuming it" rather than "before resuming it", right?  We have
> two pending signals, so we resume the lwp and deliver the first signal.  After
> resuming, we need to immediately deliver the second signal, so we call
> send_sigstop.

No, I really meant "before resuming it".  We'd queue a SIGSTOP
in the kernel, and then resume the thread with
PTRACE_CONTINUE/STEP + signal.  The idea being that the thread would
continue out of the signal delivery path in the kernel side with
the signal we resume it with, so if there's a signal handler,
it's what the kernel makes the thread execute as soon as it reaches
userspace.  But given we had _also_ queued a SIGSTOP, the thread would
immediately report the SIGSTOP before it had a chance of executing the
first instruction of the handler.  IOW, it'd report the SIGSTOP in
the first instruction of the handler, or where it was already
stopped before, if the signal signal passed to PTRACE_CONTINUE
is SIG_IGN.

Seeing the thread stop for a SIGSTOP that gdbserver had itself
sent, gdbserver would immediately re-resume the thread, this time,
with the other pending signal.  This latter part probably already
works without any change.  See tail end of linux_low_filter_event,
where we have:

  if (WIFSTOPPED (wstat) && WSTOPSIG (wstat) == SIGSTOP
      && child->stop_expected)
    {
      if (debug_threads)
	debug_printf ("Expected stop.\n");
      child->stop_expected = 0;

> IIUC, this patch is OK as-is, right?
> 

Yes.

Thanks,
Pedro Alves

Re: [PATCH 7/9] Enqueue signal even when resuming threads

[Pedro Alves]

On 07/01/2016 05:55 PM, Pedro Alves wrote:
> On 07/01/2016 05:45 PM, Yao Qi wrote:
> 
>> You meant "after resuming it" rather than "before resuming it", right?  We have
>> two pending signals, so we resume the lwp and deliver the first signal.  After
>> resuming, we need to immediately deliver the second signal, so we call
>> send_sigstop.
> 
> No, I really meant "before resuming it".  We'd queue a SIGSTOP
> in the kernel, and then resume the thread with
> PTRACE_CONTINUE/STEP + signal.  The idea being that the thread would
> continue out of the signal delivery path in the kernel side with
> the signal we resume it with, so if there's a signal handler,
> it's what the kernel makes the thread execute as soon as it reaches
> userspace.  But given we had _also_ queued a SIGSTOP, the thread would
> immediately report the SIGSTOP before it had a chance of executing the
> first instruction of the handler.  IOW, it'd report the SIGSTOP in
> the first instruction of the handler, or where it was already
> stopped before, if the signal signal passed to PTRACE_CONTINUE
> is SIG_IGN.

Eh, actually, if this works, looks like I just come up with a way
to step into a signal handler on software single-step targets.

Thanks,
Pedro Alves

> 
> Seeing the thread stop for a SIGSTOP that gdbserver had itself
> sent, gdbserver would immediately re-resume the thread, this time,
> with the other pending signal.  This latter part probably already
> works without any change.  See tail end of linux_low_filter_event,
> where we have:
> 
>   if (WIFSTOPPED (wstat) && WSTOPSIG (wstat) == SIGSTOP
>       && child->stop_expected)
>     {
>       if (debug_threads)
> 	debug_printf ("Expected stop.\n");
>       child->stop_expected = 0;
> 
>> IIUC, this patch is OK as-is, right?
>>
> 
> Yes.
> 
> Thanks,
> Pedro Alves
> 

Re: [PATCH 8/9] Use reinsert_breakpoint for vCont;s

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

>> if thread 1 doesn't hit the reinsert breakpoint, we don't have to
>> remove them, because GDB will send vCont;s:1 next time, and GDBserver
>
> There's no guarantee GDB will send vCont;s:1 next time.
> The user may do "continue" instead of "step".
>
>> can only install reinsert breakpoints if they are not installed yet.
>
> The user may even do "return + continue" or "jump", or an infcall,
> all of which resume the thread at a different address from the address
> the thread last stopped.  So there's no guarantee that the
> reinsert breakpoint address makes any sense for the next step request,
> or even that the next resume request is a step in the first place.
>
> Basically the previous step request must be completely forgotten after
> gdb has seen the thread stop.  In all-stop, gdb "sees "all threads
> stopped on each and every event reported to gdb, for any thread.
> A stop reply cancels any and all previous resume requests.

I add some code to delete all reinsert breakpoints for all threads in
all-stop.  See the patch below,

>
>> if thread hits the reinsert breakpoint, but the event is not reported.
>> It becomes pending, and GDBserver will delete the reinsert breakpoints
>> next time when this pending event is reported back to GDB.
>
> I don't follow.  I'm talking about the case where the thread does _not_
> hit the reinsert breakpoint.  Instead some other thread hits some unrelated
> event.

In your review to V2, your words are about other threads hit a breakpoint, but
doesn't mention "thread 1" (requested for resume_step) hits reinsert
breakpoint or not.  I just list two possible results (not hit vs
hit). You didn't talk about the latter, so we don't have to follow it.

-- 
Yao (齐尧)

From f4aa0593190e7e78831532c03177d8264b7dd1c1 Mon Sep 17 00:00:00 2001
From: Yao Qi <yao.qi@linaro.org>
Date: Thu, 19 May 2016 17:07:49 +0100
Subject: [PATCH] Use reinsert_breakpoint for vCont;s

V4: remove all reinsert breakpoints before sending event back to GDB
    in all-stop mode,

V3: install breakpoints in proceed_one_lwp,
    no longer stop all threads when installing breakpoints,
    delete reinsert breakpoints when GDBserver wants to report event,

V2: fix spaces in changelog entry,
    use maybe_hw_step,
    cancel step-over if signal arrives (!maybe_internal_trap),

This patch is to teach GDBserver using software single step to handle
vCont;s.  Simply speaking, if the thread's resume request is resume_step,
install reinsert breakpoint at the next pcs when GDBserver is about to
resume threads.  These reinsert breakpoints of a thread are removed,
when GDBserver gets an event from that thread and reports it back to
GDB.

gdb/gdbserver:

2016-07-05  Yao Qi  <yao.qi@linaro.org>

	* linux-low.c (resume_stopped_resumed_lwps): If resume request
	is resume_step, call maybe_hw_step.
	(linux_wait_1): Stop all threads, remove reinsert breakpoints,
	and unstop them.
	(linux_resume_one_lwp_throw): Don't assert the thread has reinsert
	breakpoints or not.
	(proceed_one_lwp): If resume request is resume_step, install
	reinsert breakpoints and call maybe_hw_step.

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index abaf288..b579b4d 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -2563,7 +2563,10 @@ resume_stopped_resumed_lwps (struct inferior_list_entry *entry)
       && !lp->status_pending_p
       && thread->last_status.kind == TARGET_WAITKIND_IGNORE)
     {
-      int step = thread->last_resume_kind == resume_step;
+      int step = 0;
+
+      if (thread->last_resume_kind == resume_step)
+	step = maybe_hw_step (thread);
 
       if (debug_threads)
 	debug_printf ("RSRL: resuming stopped-resumed LWP %s at %s: step=%d\n",
@@ -3622,6 +3625,66 @@ linux_wait_1 (ptid_t ptid,
 
   /* Alright, we're going to report a stop.  */
 
+  /* Remove reinsert breakpoints.  */
+  if (can_software_single_step ())
+    {
+      /* Remove reinsert breakpoints or not.  It it is true, stop all
+	 lwps, so that other threads won't hit the breakpoint in the
+	 staled memory.  */
+      int remove_reinsert_breakpoints_p = 0;
+
+      if (non_stop)
+	{
+	  remove_reinsert_breakpoints_p
+	    = has_reinsert_breakpoints (current_thread);
+	}
+      else
+	{
+	  /* In all-stop, a stop reply cancels all previous resume
+	     requests.  Delete all reinsert breakpoints.  */
+	  struct inferior_list_entry *inf, *tmp;
+
+	  ALL_INFERIORS (&all_threads, inf, tmp)
+	    {
+	      struct thread_info *thread = (struct thread_info *) inf;
+
+	      if (has_reinsert_breakpoints (thread))
+		{
+		  remove_reinsert_breakpoints_p = 1;
+		  break;
+		}
+	    }
+	}
+
+      if (remove_reinsert_breakpoints_p)
+	{
+	  /* If we remove reinsert breakpoints from memory, stop all lwps,
+	     so that other threads won't hit the breakpoint in the staled
+	     memory.  */
+	  stop_all_lwps (0, event_child);
+
+	  if (non_stop)
+	    {
+	      gdb_assert (has_reinsert_breakpoints (current_thread));
+	      delete_reinsert_breakpoints (current_thread);
+	    }
+	  else
+	    {
+	      struct inferior_list_entry *inf, *tmp;
+
+	      ALL_INFERIORS (&all_threads, inf, tmp)
+		{
+		  struct thread_info *thread = (struct thread_info *) inf;
+
+		  if (has_reinsert_breakpoints (thread))
+		    delete_reinsert_breakpoints (thread);
+		}
+	    }
+
+	  unstop_all_lwps (0, event_child);
+	}
+    }
+
   if (!stabilizing_threads)
     {
       /* In all-stop, stop all threads.  */
@@ -4275,12 +4338,6 @@ linux_resume_one_lwp_throw (struct lwp_info *lwp,
 
       step = maybe_hw_step (thread);
     }
-  else
-    {
-      /* If the thread isn't doing step-over, there shouldn't be any
-	 reinsert breakpoints.  */
-      gdb_assert (!has_reinsert_breakpoints (thread));
-    }
 
   if (fast_tp_collecting == 1)
     {
@@ -5088,7 +5145,14 @@ proceed_one_lwp (struct inferior_list_entry *entry, void *except)
       if (debug_threads)
 	debug_printf ("   stepping LWP %ld, client wants it stepping\n",
 		      lwpid_of (thread));
-      step = 1;
+
+      /* If resume_step is requested by GDB, install reinsert
+	 breakpoints when the thread is about to be actually resumed if
+	 the reinsert breakpoints weren't removed.  */
+      if (can_software_single_step () && !has_reinsert_breakpoints (thread))
+	install_software_single_step_breakpoints (lwp);
+
+      step = maybe_hw_step (thread);
     }
   else if (lwp->bp_reinsert != 0)
     {

Re: [PATCH 8/9] Use reinsert_breakpoint for vCont;s

[Yao Qi]

Ping?  I want this series go in before the release.

On Tue, Jul 5, 2016 at 9:14 AM, Yao Qi <qiyaoltc@gmail.com> wrote:
> Pedro Alves <palves@redhat.com> writes:
>
>>> if thread 1 doesn't hit the reinsert breakpoint, we don't have to
>>> remove them, because GDB will send vCont;s:1 next time, and GDBserver
>>
>> There's no guarantee GDB will send vCont;s:1 next time.
>> The user may do "continue" instead of "step".
>>
>>> can only install reinsert breakpoints if they are not installed yet.
>>
>> The user may even do "return + continue" or "jump", or an infcall,
>> all of which resume the thread at a different address from the address
>> the thread last stopped.  So there's no guarantee that the
>> reinsert breakpoint address makes any sense for the next step request,
>> or even that the next resume request is a step in the first place.
>>
>> Basically the previous step request must be completely forgotten after
>> gdb has seen the thread stop.  In all-stop, gdb "sees "all threads
>> stopped on each and every event reported to gdb, for any thread.
>> A stop reply cancels any and all previous resume requests.
>
> I add some code to delete all reinsert breakpoints for all threads in
> all-stop.  See the patch below,
>
>>
>>> if thread hits the reinsert breakpoint, but the event is not reported.
>>> It becomes pending, and GDBserver will delete the reinsert breakpoints
>>> next time when this pending event is reported back to GDB.
>>
>> I don't follow.  I'm talking about the case where the thread does _not_
>> hit the reinsert breakpoint.  Instead some other thread hits some unrelated
>> event.
>
> In your review to V2, your words are about other threads hit a breakpoint, but
> doesn't mention "thread 1" (requested for resume_step) hits reinsert
> breakpoint or not.  I just list two possible results (not hit vs
> hit). You didn't talk about the latter, so we don't have to follow it.
>
> --
> Yao (齐尧)
>
> From f4aa0593190e7e78831532c03177d8264b7dd1c1 Mon Sep 17 00:00:00 2001
> From: Yao Qi <yao.qi@linaro.org>
> Date: Thu, 19 May 2016 17:07:49 +0100
> Subject: [PATCH] Use reinsert_breakpoint for vCont;s
>
> V4: remove all reinsert breakpoints before sending event back to GDB
>     in all-stop mode,
>
> V3: install breakpoints in proceed_one_lwp,
>     no longer stop all threads when installing breakpoints,
>     delete reinsert breakpoints when GDBserver wants to report event,
>
> V2: fix spaces in changelog entry,
>     use maybe_hw_step,
>     cancel step-over if signal arrives (!maybe_internal_trap),
>
> This patch is to teach GDBserver using software single step to handle
> vCont;s.  Simply speaking, if the thread's resume request is resume_step,
> install reinsert breakpoint at the next pcs when GDBserver is about to
> resume threads.  These reinsert breakpoints of a thread are removed,
> when GDBserver gets an event from that thread and reports it back to
> GDB.
>
> gdb/gdbserver:
>
> 2016-07-05  Yao Qi  <yao.qi@linaro.org>
>
>         * linux-low.c (resume_stopped_resumed_lwps): If resume request
>         is resume_step, call maybe_hw_step.
>         (linux_wait_1): Stop all threads, remove reinsert breakpoints,
>         and unstop them.
>         (linux_resume_one_lwp_throw): Don't assert the thread has reinsert
>         breakpoints or not.
>         (proceed_one_lwp): If resume request is resume_step, install
>         reinsert breakpoints and call maybe_hw_step.
>
> diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
> index abaf288..b579b4d 100644
> --- a/gdb/gdbserver/linux-low.c
> +++ b/gdb/gdbserver/linux-low.c
> @@ -2563,7 +2563,10 @@ resume_stopped_resumed_lwps (struct inferior_list_entry *entry)
>        && !lp->status_pending_p
>        && thread->last_status.kind == TARGET_WAITKIND_IGNORE)
>      {
> -      int step = thread->last_resume_kind == resume_step;
> +      int step = 0;
> +
> +      if (thread->last_resume_kind == resume_step)
> +       step = maybe_hw_step (thread);
>
>        if (debug_threads)
>         debug_printf ("RSRL: resuming stopped-resumed LWP %s at %s: step=%d\n",
> @@ -3622,6 +3625,66 @@ linux_wait_1 (ptid_t ptid,
>
>    /* Alright, we're going to report a stop.  */
>
> +  /* Remove reinsert breakpoints.  */
> +  if (can_software_single_step ())
> +    {
> +      /* Remove reinsert breakpoints or not.  It it is true, stop all
> +        lwps, so that other threads won't hit the breakpoint in the
> +        staled memory.  */
> +      int remove_reinsert_breakpoints_p = 0;
> +
> +      if (non_stop)
> +       {
> +         remove_reinsert_breakpoints_p
> +           = has_reinsert_breakpoints (current_thread);
> +       }
> +      else
> +       {
> +         /* In all-stop, a stop reply cancels all previous resume
> +            requests.  Delete all reinsert breakpoints.  */
> +         struct inferior_list_entry *inf, *tmp;
> +
> +         ALL_INFERIORS (&all_threads, inf, tmp)
> +           {
> +             struct thread_info *thread = (struct thread_info *) inf;
> +
> +             if (has_reinsert_breakpoints (thread))
> +               {
> +                 remove_reinsert_breakpoints_p = 1;
> +                 break;
> +               }
> +           }
> +       }
> +
> +      if (remove_reinsert_breakpoints_p)
> +       {
> +         /* If we remove reinsert breakpoints from memory, stop all lwps,
> +            so that other threads won't hit the breakpoint in the staled
> +            memory.  */
> +         stop_all_lwps (0, event_child);
> +
> +         if (non_stop)
> +           {
> +             gdb_assert (has_reinsert_breakpoints (current_thread));
> +             delete_reinsert_breakpoints (current_thread);
> +           }
> +         else
> +           {
> +             struct inferior_list_entry *inf, *tmp;
> +
> +             ALL_INFERIORS (&all_threads, inf, tmp)
> +               {
> +                 struct thread_info *thread = (struct thread_info *) inf;
> +
> +                 if (has_reinsert_breakpoints (thread))
> +                   delete_reinsert_breakpoints (thread);
> +               }
> +           }
> +
> +         unstop_all_lwps (0, event_child);
> +       }
> +    }
> +
>    if (!stabilizing_threads)
>      {
>        /* In all-stop, stop all threads.  */
> @@ -4275,12 +4338,6 @@ linux_resume_one_lwp_throw (struct lwp_info *lwp,
>
>        step = maybe_hw_step (thread);
>      }
> -  else
> -    {
> -      /* If the thread isn't doing step-over, there shouldn't be any
> -        reinsert breakpoints.  */
> -      gdb_assert (!has_reinsert_breakpoints (thread));
> -    }
>
>    if (fast_tp_collecting == 1)
>      {
> @@ -5088,7 +5145,14 @@ proceed_one_lwp (struct inferior_list_entry *entry, void *except)
>        if (debug_threads)
>         debug_printf ("   stepping LWP %ld, client wants it stepping\n",
>                       lwpid_of (thread));
> -      step = 1;
> +
> +      /* If resume_step is requested by GDB, install reinsert
> +        breakpoints when the thread is about to be actually resumed if
> +        the reinsert breakpoints weren't removed.  */
> +      if (can_software_single_step () && !has_reinsert_breakpoints (thread))
> +       install_software_single_step_breakpoints (lwp);
> +
> +      step = maybe_hw_step (thread);
>      }
>    else if (lwp->bp_reinsert != 0)
>      {



-- 
Yao (齐尧)

Re: [PATCH 8/9] Use reinsert_breakpoint for vCont;s

[Pedro Alves]

On 07/05/2016 09:14 AM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> writes:
> 
>>> if thread 1 doesn't hit the reinsert breakpoint, we don't have to
>>> remove them, because GDB will send vCont;s:1 next time, and GDBserver
>>
>> There's no guarantee GDB will send vCont;s:1 next time.
>> The user may do "continue" instead of "step".
>>
>>> can only install reinsert breakpoints if they are not installed yet.
>>
>> The user may even do "return + continue" or "jump", or an infcall,
>> all of which resume the thread at a different address from the address
>> the thread last stopped.  So there's no guarantee that the
>> reinsert breakpoint address makes any sense for the next step request,
>> or even that the next resume request is a step in the first place.
>>
>> Basically the previous step request must be completely forgotten after
>> gdb has seen the thread stop.  In all-stop, gdb "sees "all threads
>> stopped on each and every event reported to gdb, for any thread.
>> A stop reply cancels any and all previous resume requests.
> 
> I add some code to delete all reinsert breakpoints for all threads in
> all-stop.  See the patch below,

This version looks good to me.

Thanks,
Pedro Alves

Re: [PATCH 0/9 V3] Use reinsert breakpoint for vCont;s

[Yao Qi]

On Thu, Jun 30, 2016 at 3:09 PM, Yao Qi <qiyaoltc@gmail.com> wrote:
> Here is the V3 of this patch series, V2 can be found
> https://sourceware.org/ml/gdb-patches/2016-06/msg00029.html
> some preparatory patches in V2 are already pushed in.  Patches
> 1 - 5 and 9 are already approved in V2 review.
>
> This patch series is to address the review comments
> https://sourceware.org/ml/gdb-patches/2016-06/msg00305.html
>
>> Hmm, seeing that we need to handle installing the breakpoints in
>> both places, I wonder about making linux_resume just handle
>> setting up the last resume kind and queue signals, and then
>> end up calling proceed_all_lwps.  If that works, I suspect it
>> would simplify things a good deal.
>
> I do something slightly differently in V3.  In my
> "V2 Use reinsert breakpoint for vCont;s", I install reinsert breakpoints
> for needed lwps in two places, linux_resume and proceed_all_lwps, which
> isn't ideal.
>
> After the chat with Pedro, we don't need to stop all threads when inserting
> reinsert breakpoint, so we can move the breakpoint installation further
> down to linux_resume_one_thread and proceed_one_lwp.  If
> linux_resume_one_thread can use proceed_one_lwp, we can only install
> reinsert breakpoints in one place, proceed_one_lwp.  Patch 7 changes
> linux_resume_one_thread enqueues signal and calls proceed_one_lwp.
> Patch 6 is a refactor one to use existing api enqueue_pending_signal.
> With the changes in patch 7, patch 8 is simplified compared with V2.
>
> Regression tested on arm-linux and x86_64-linux.
>

I pushed them in.

-- 
Yao (齐尧)

Re: [PATCH 0/9 V3] Use reinsert breakpoint for vCont;s

[Antoine Tremblay]

Yao Qi writes:

> Here is the V3 of this patch series, V2 can be found
> https://sourceware.org/ml/gdb-patches/2016-06/msg00029.html
> some preparatory patches in V2 are already pushed in.  Patches
> 1 - 5 and 9 are already approved in V2 review.
>
> This patch series is to address the review comments
> https://sourceware.org/ml/gdb-patches/2016-06/msg00305.html
>
>> Hmm, seeing that we need to handle installing the breakpoints in
>> both places, I wonder about making linux_resume just handle
>> setting up the last resume kind and queue signals, and then
>> end up calling proceed_all_lwps.  If that works, I suspect it
>> would simplify things a good deal.
>
> I do something slightly differently in V3.  In my
> "V2 Use reinsert breakpoint for vCont;s", I install reinsert breakpoints
> for needed lwps in two places, linux_resume and proceed_all_lwps, which
> isn't ideal.
>
> After the chat with Pedro, we don't need to stop all threads when inserting
> reinsert breakpoint, so we can move the breakpoint installation further
> down to linux_resume_one_thread and proceed_one_lwp.

I'm following up on random SIGILL/SIGSEGV when using software single stepping/
range stepping with GDBServer on ARM.

And I can't see why we don't need to stop all threads when inserting
reinsert breakpoint.

Since linux_resume will call:

find_inferior (&all_threads, linux_resume_one_thread,
  &leave_all_stopped);

This will start one thread after the other. So for example if thread 3
has a single step breakpoint to install this will start thread 1, then
thread 2 and just modify the program's memory to install reinsert
breakpoints on thread 3 with thread 1 and 2 running.

Thus leading to thread 1 or 2 executing invalid memory, thus the SIGILL
random problems...

Could either of you elaborate on this chat ? (I could not find any
reference in the mail archives...)

Thanks,
Antoine

Re: [PATCH 0/9 V3] Use reinsert breakpoint for vCont;s

[Yao Qi]

On Mon, Nov 14, 2016 at 02:10:32PM -0500, Antoine Tremblay wrote:
> 
> > I do something slightly differently in V3.  In my
> > "V2 Use reinsert breakpoint for vCont;s", I install reinsert breakpoints
> > for needed lwps in two places, linux_resume and proceed_all_lwps, which
> > isn't ideal.
> >
> > After the chat with Pedro, we don't need to stop all threads when inserting
> > reinsert breakpoint, so we can move the breakpoint installation further
> > down to linux_resume_one_thread and proceed_one_lwp.
> 
> I'm following up on random SIGILL/SIGSEGV when using software single stepping/
> range stepping with GDBServer on ARM.
> 
> And I can't see why we don't need to stop all threads when inserting
> reinsert breakpoint.
> 
> Since linux_resume will call:
> 
> find_inferior (&all_threads, linux_resume_one_thread,
>   &leave_all_stopped);
> 
> This will start one thread after the other. So for example if thread 3
> has a single step breakpoint to install this will start thread 1, then
> thread 2 and just modify the program's memory to install reinsert
> breakpoints on thread 3 with thread 1 and 2 running.
> 
> Thus leading to thread 1 or 2 executing invalid memory, thus the SIGILL
> random problems...

Single-step breakpoint is thread specific, so we don't need to stop
other threads when inserting one for a specific thread.  Given the
example above, we insert single-step breakpoint for thread 3 on address
A, if thread 1 goes through address A, but doesn't hit the breakpoint,
IOW, thread 1 still sees the original instruction, that is nothing wrong,
right?  We don't expect thread 1 hits that breakpoint for thread 3 anyway.
If thread 1 hits the breakpoint (IOW, thread 1 sees the breakpoint
instruction), GDBserver just handles that SIGTRAP, and it
has already know that there is a breakpoint on address A.

Thread 1 either sees the original instruction on address A or the
breakpoint instruction.  Unless ptrace read/write 32-bit is not
atomic, IOW, partial ptrace write result is visible to other
threads, I don't see why we get SIGILL here.

Note that we stop all threads when we remove single-step breakpoints
because we want no thread sees single-step breakpoint in memory from
their point of view afterwards.

-- 
Yao (齐尧)

Re: [PATCH 0/9 V3] Use reinsert breakpoint for vCont;s

[Antoine Tremblay]

Yao Qi writes:

> On Mon, Nov 14, 2016 at 02:10:32PM -0500, Antoine Tremblay wrote:
>> 
>> > I do something slightly differently in V3.  In my
>> > "V2 Use reinsert breakpoint for vCont;s", I install reinsert breakpoints
>> > for needed lwps in two places, linux_resume and proceed_all_lwps, which
>> > isn't ideal.
>> >
>> > After the chat with Pedro, we don't need to stop all threads when inserting
>> > reinsert breakpoint, so we can move the breakpoint installation further
>> > down to linux_resume_one_thread and proceed_one_lwp.
>> 
>> I'm following up on random SIGILL/SIGSEGV when using software single stepping/
>> range stepping with GDBServer on ARM.
>> 
>> And I can't see why we don't need to stop all threads when inserting
>> reinsert breakpoint.
>> 
>> Since linux_resume will call:
>> 
>> find_inferior (&all_threads, linux_resume_one_thread,
>>   &leave_all_stopped);
>> 
>> This will start one thread after the other. So for example if thread 3
>> has a single step breakpoint to install this will start thread 1, then
>> thread 2 and just modify the program's memory to install reinsert
>> breakpoints on thread 3 with thread 1 and 2 running.
>> 
>> Thus leading to thread 1 or 2 executing invalid memory, thus the SIGILL
>> random problems...
>
> Single-step breakpoint is thread specific, so we don't need to stop
> other threads when inserting one for a specific thread.  Given the
> example above, we insert single-step breakpoint for thread 3 on address
> A, if thread 1 goes through address A, but doesn't hit the breakpoint,
> IOW, thread 1 still sees the original instruction, that is nothing wrong,
> right?  We don't expect thread 1 hits that breakpoint for thread 3 anyway.
> If thread 1 hits the breakpoint (IOW, thread 1 sees the breakpoint
> instruction), GDBserver just handles that SIGTRAP, and it
> has already know that there is a breakpoint on address A.
>

Yes that would be fine if the instruction write was atomic...

> Thread 1 either sees the original instruction on address A or the
> breakpoint instruction.  Unless ptrace read/write 32-bit is not
> atomic, IOW, partial ptrace write result is visible to other
> threads, I don't see why we get SIGILL here.

I think this is the problem, ptrace read/write doesn't seem to be
atomic, and thread 1 sees some half written memory. (Given that we get
SIGILL/SIGSEGV issues)

Did you have any reference suggesting it was atomic ?

While testing it seems to be atomic for 32bit writes but in thumb mode
with a 16 byte write, it is not.

Given the SIGILL/SIGSEG I get maybe that one is 2 writes of 1 byte ?
I'll have to dig in the ptrace code I guess.

The SIGILL/SIGSEGV do go away if GDBServer stops the threads...

>
> Note that we stop all threads when we remove single-step breakpoints
> because we want no thread sees single-step breakpoint in memory from
> their point of view afterwards.

Yes that's fine.

Re: [PATCH 0/9 V3] Use reinsert breakpoint for vCont;s

[Antoine Tremblay]

Antoine Tremblay writes:

> Yao Qi writes:
>
>> On Mon, Nov 14, 2016 at 02:10:32PM -0500, Antoine Tremblay wrote:
>>> 
>>> > I do something slightly differently in V3.  In my
>>> > "V2 Use reinsert breakpoint for vCont;s", I install reinsert breakpoints
>>> > for needed lwps in two places, linux_resume and proceed_all_lwps, which
>>> > isn't ideal.
>>> >
>>> > After the chat with Pedro, we don't need to stop all threads when inserting
>>> > reinsert breakpoint, so we can move the breakpoint installation further
>>> > down to linux_resume_one_thread and proceed_one_lwp.
>>> 
>>> I'm following up on random SIGILL/SIGSEGV when using software single stepping/
>>> range stepping with GDBServer on ARM.
>>> 
>>> And I can't see why we don't need to stop all threads when inserting
>>> reinsert breakpoint.
>>> 
>>> Since linux_resume will call:
>>> 
>>> find_inferior (&all_threads, linux_resume_one_thread,
>>>   &leave_all_stopped);
>>> 
>>> This will start one thread after the other. So for example if thread 3
>>> has a single step breakpoint to install this will start thread 1, then
>>> thread 2 and just modify the program's memory to install reinsert
>>> breakpoints on thread 3 with thread 1 and 2 running.
>>> 
>>> Thus leading to thread 1 or 2 executing invalid memory, thus the SIGILL
>>> random problems...
>>
>> Single-step breakpoint is thread specific, so we don't need to stop
>> other threads when inserting one for a specific thread.  Given the
>> example above, we insert single-step breakpoint for thread 3 on address
>> A, if thread 1 goes through address A, but doesn't hit the breakpoint,
>> IOW, thread 1 still sees the original instruction, that is nothing wrong,
>> right?  We don't expect thread 1 hits that breakpoint for thread 3 anyway.
>> If thread 1 hits the breakpoint (IOW, thread 1 sees the breakpoint
>> instruction), GDBserver just handles that SIGTRAP, and it
>> has already know that there is a breakpoint on address A.
>>
>
> Yes that would be fine if the instruction write was atomic...
>
>> Thread 1 either sees the original instruction on address A or the
>> breakpoint instruction.  Unless ptrace read/write 32-bit is not
>> atomic, IOW, partial ptrace write result is visible to other
>> threads, I don't see why we get SIGILL here.
>
> I think this is the problem, ptrace read/write doesn't seem to be
> atomic, and thread 1 sees some half written memory. (Given that we get
> SIGILL/SIGSEGV issues)
>
> Did you have any reference suggesting it was atomic ?
>
> While testing it seems to be atomic for 32bit writes but in thumb mode
> with a 16 byte write, it is not.
>
> Given the SIGILL/SIGSEG I get maybe that one is 2 writes of 1 byte ?
> I'll have to dig in the ptrace code I guess.
>
> The SIGILL/SIGSEGV do go away if GDBServer stops the threads...
>

After some digging on the atomicity of replacing instructions I found:

A3.5.4
Concurrent modification and execution of instructions
The ARMv7 architecture limits the set of instructions that can be executed by one thread of execution as they are
being modified by another thread of execution without requiring explicit synchronization.
Except for the instructions identified in this section, the effect of the concurrent modification and execution of an
instruction is UNPREDICTABLE .

I found something similar with kprobes on aarch64 see:

linux/arch/arm64/kernel/insn.c
int __kprobes aarch64_insn_patch_text(void *addrs[], u32 insns[], int cnt)

This checks if the instruction is one that is safe to hot patch, and if
not it basically stops all execution on the machine except for the task
of replacing this instruction.

Given that GDB does not distinguish between the instructions I think
it's safer to stop all the threads before modifiing the instruction.

>>
>> Note that we stop all threads when we remove single-step breakpoints
>> because we want no thread sees single-step breakpoint in memory from
>> their point of view afterwards.
>
> Yes that's fine.

Re: [PATCH 0/9 V3] Use reinsert breakpoint for vCont;s

[Yao Qi]

On Mon, Nov 21, 2016 at 10:34:44AM -0500, Antoine Tremblay wrote:
> > Thread 1 either sees the original instruction on address A or the
> > breakpoint instruction.  Unless ptrace read/write 32-bit is not
> > atomic, IOW, partial ptrace write result is visible to other
> > threads, I don't see why we get SIGILL here.
> 
> I think this is the problem, ptrace read/write doesn't seem to be
> atomic, and thread 1 sees some half written memory. (Given that we get
> SIGILL/SIGSEGV issues)

We need to check in linux-arm-kernel@.

> 
> Did you have any reference suggesting it was atomic ?
> 

No.

> While testing it seems to be atomic for 32bit writes but in thumb mode
> with a 16 byte write, it is not.

I think you meant "16 bit write".  Why is that?

> 
> Given the SIGILL/SIGSEG I get maybe that one is 2 writes of 1 byte ?
> I'll have to dig in the ptrace code I guess.
> 

It is good to get some a clear answer instead of ambiguous speculation.
I think we need to ask in linux-arm-kernel@

-- 
Yao

Re: [PATCH 0/9 V3] Use reinsert breakpoint for vCont;s

[Antoine Tremblay]

Yao Qi writes:

> On Mon, Nov 21, 2016 at 10:34:44AM -0500, Antoine Tremblay wrote:
>> > Thread 1 either sees the original instruction on address A or the
>> > breakpoint instruction.  Unless ptrace read/write 32-bit is not
>> > atomic, IOW, partial ptrace write result is visible to other
>> > threads, I don't see why we get SIGILL here.
>> 
>> I think this is the problem, ptrace read/write doesn't seem to be
>> atomic, and thread 1 sees some half written memory. (Given that we get
>> SIGILL/SIGSEGV issues)
>
> We need to check in linux-arm-kernel@.
>
>> 
>> Did you have any reference suggesting it was atomic ?
>> 
>
> No.
>
>> While testing it seems to be atomic for 32bit writes but in thumb mode
>> with a 16 byte write, it is not.
>
> I think you meant "16 bit write".  Why is that?
>

Yes 16 bit write sorry, because it can write a thumb breakpoint :
0xde01.

>> 
>> Given the SIGILL/SIGSEG I get maybe that one is 2 writes of 1 byte ?
>> I'll have to dig in the ptrace code I guess.
>> 
>
> It is good to get some a clear answer instead of ambiguous speculation.
> I think we need to ask in linux-arm-kernel@

Did you see my follow up email ? :
https://sourceware.org/ml/gdb-patches/2016-11/msg00681.html

Also, I think this will become a moot point in the patch I'm about to
post since:

To install a single step breakpoint on a thread GDBServer needs to make sure
that there is not a breakpoint at the thread's current pc, since it
can't determine what is the next_pc of a breakpoint instruction.

Usually for stepping over it's OK since it's stopped at pc X and it
will install a single-step breakpoint at pc X + next_pc_offset.

So need_step_over returns true and GDBServer starts a step_over process,
which removes all breakpoints, installs a single-step breakpoint on the
nextpc and resumes.

But in this case it is installing single-step breakpoints in threads at
different pcs then the one we're stopped, so the step-over process is
not triggered and it should not be.

So GDBSever does not take care to remove all breakpoints like is the
case in the step-over process.  Because of that it can try to install a
single-step breakpoint where there is already a breakpoint in memory and
thus break get_next_pc and install a breakpoint at an invalid location.

Consider this case:

in non-stop, thread 1-3 are stepping in a loop similar to
non-stop-fair-events test.

 - thread 1 hits its single-step breakpoint at pc A.
 - delete its single-step breakpoint.
 - a check for need_step_over is done, but there's no breakpoint at pc A
 anymore, and nobody is stopped there anyway so it returns false.
 - proceed_one_lwp is called on each thread.

 Now here is the problem:

 thread 1 is at pc A
 thread 2 is at pc B

 B is a branch to A.
 
 thread 1 installs a single-step breakpoint at pc B since it's range stepping.
 thread 2 does not have a single step breakpoint but needs one installed.
 
 - proceed_one_lwp finds that it needs to install a single-step
   breakpoint on thread 2.

 - It calls install_single_step_breakpoints, which calls get_next_pc.

 - get_next_pc reads the current instruction in memory at pc B, but
   since it's a breakpoint, it missinterprets the instruction, you can't
   step over a breakpoint like that anyway, but this is what happens
   now.

   A single-step breakpoint is now inserted at an invalid location.

So my approch in my patch is to fix this by always removing all
breakpoints and fast_tracepoints_jumps, like we do in start_step_over
before calling install_software_single_step.

This makes the breakpoint installation a multiple steps process and thus
can't be atomic.

WDYT ?

Thanks,
Antoine

Re: [PATCH 0/9 V3] Use reinsert breakpoint for vCont;s

[Antoine Tremblay]

Antoine Tremblay writes:

> Yao Qi writes:
>
>> On Mon, Nov 21, 2016 at 10:34:44AM -0500, Antoine Tremblay wrote:
>>> > Thread 1 either sees the original instruction on address A or the
>>> > breakpoint instruction.  Unless ptrace read/write 32-bit is not
>>> > atomic, IOW, partial ptrace write result is visible to other
>>> > threads, I don't see why we get SIGILL here.
>>> 
>>> I think this is the problem, ptrace read/write doesn't seem to be
>>> atomic, and thread 1 sees some half written memory. (Given that we get
>>> SIGILL/SIGSEGV issues)
>>
>> We need to check in linux-arm-kernel@.
>>
>>> 
>>> Did you have any reference suggesting it was atomic ?
>>> 
>>
>> No.
>>
>>> While testing it seems to be atomic for 32bit writes but in thumb mode
>>> with a 16 byte write, it is not.
>>
>> I think you meant "16 bit write".  Why is that?
>>
>
> Yes 16 bit write sorry, because it can write a thumb breakpoint :
> 0xde01.
>
>>> 
>>> Given the SIGILL/SIGSEG I get maybe that one is 2 writes of 1 byte ?
>>> I'll have to dig in the ptrace code I guess.
>>> 
>>
>> It is good to get some a clear answer instead of ambiguous speculation.
>> I think we need to ask in linux-arm-kernel@
>
> Did you see my follow up email ? :
> https://sourceware.org/ml/gdb-patches/2016-11/msg00681.html
>
> Also, I think this will become a moot point in the patch I'm about to
> post since:
>
> To install a single step breakpoint on a thread GDBServer needs to make sure
> that there is not a breakpoint at the thread's current pc, since it
> can't determine what is the next_pc of a breakpoint instruction.
>
> Usually for stepping over it's OK since it's stopped at pc X and it
> will install a single-step breakpoint at pc X + next_pc_offset.
>
> So need_step_over returns true and GDBServer starts a step_over process,
> which removes all breakpoints, installs a single-step breakpoint on the
> nextpc and resumes.
>
> But in this case it is installing single-step breakpoints in threads at
> different pcs then the one we're stopped, so the step-over process is
> not triggered and it should not be.
>
> So GDBSever does not take care to remove all breakpoints like is the
> case in the step-over process.  Because of that it can try to install a
> single-step breakpoint where there is already a breakpoint in memory and
> thus break get_next_pc and install a breakpoint at an invalid location.
>
> Consider this case:
>
> in non-stop, thread 1-3 are stepping in a loop similar to
> non-stop-fair-events test.
>
>  - thread 1 hits its single-step breakpoint at pc A.
>  - delete its single-step breakpoint.
>  - a check for need_step_over is done, but there's no breakpoint at pc A
>  anymore, and nobody is stopped there anyway so it returns false.
>  - proceed_one_lwp is called on each thread.
>
>  Now here is the problem:
>
>  thread 1 is at pc A
>  thread 2 is at pc B
>
>  B is a branch to A.
>  
>  thread 1 installs a single-step breakpoint at pc B since it's range stepping.
>  thread 2 does not have a single step breakpoint but needs one installed.
>  
>  - proceed_one_lwp finds that it needs to install a single-step
>    breakpoint on thread 2.
>
>  - It calls install_single_step_breakpoints, which calls get_next_pc.
>
>  - get_next_pc reads the current instruction in memory at pc B, but
>    since it's a breakpoint, it missinterprets the instruction, you can't
>    step over a breakpoint like that anyway, but this is what happens
>    now.
>
>    A single-step breakpoint is now inserted at an invalid location.
>
> So my approch in my patch is to fix this by always removing all
> breakpoints and fast_tracepoints_jumps, like we do in start_step_over
> before calling install_software_single_step.
>
> This makes the breakpoint installation a multiple steps process and thus
> can't be atomic.
>
> WDYT ?
>
> Thanks,
> Antoine

In fact thinking more about this we may need to remove all breakpoints
at any pc since get_next_pc may read memory in other places then the
current pc to deal with atomic sequences for example or for other
instructions too.

If it reads a breakpoint in memory there it may come-up with an invalid
next pc.

This is a problem with the current step-over logic too.

So we would either need to be able to read past any
breakpoint/fast_tracepoint_jump... anywhere
or uninstall everything before calling get_next_pc.

I'm not sure which one is best at the moment, opinions on this are
welcome.

Thanks,
Antoine

Re: [PATCH 0/9 V3] Use reinsert breakpoint for vCont;s

[Antoine Tremblay]

Antoine Tremblay writes:

> Antoine Tremblay writes:
>
>> Yao Qi writes:
>>
>>> On Mon, Nov 21, 2016 at 10:34:44AM -0500, Antoine Tremblay wrote:
>>>> > Thread 1 either sees the original instruction on address A or the
>>>> > breakpoint instruction.  Unless ptrace read/write 32-bit is not
>>>> > atomic, IOW, partial ptrace write result is visible to other
>>>> > threads, I don't see why we get SIGILL here.
>>>> 
>>>> I think this is the problem, ptrace read/write doesn't seem to be
>>>> atomic, and thread 1 sees some half written memory. (Given that we get
>>>> SIGILL/SIGSEGV issues)
>>>
>>> We need to check in linux-arm-kernel@.
>>>
>>>> 
>>>> Did you have any reference suggesting it was atomic ?
>>>> 
>>>
>>> No.
>>>
>>>> While testing it seems to be atomic for 32bit writes but in thumb mode
>>>> with a 16 byte write, it is not.
>>>
>>> I think you meant "16 bit write".  Why is that?
>>>
>>
>> Yes 16 bit write sorry, because it can write a thumb breakpoint :
>> 0xde01.
>>
>>>> 
>>>> Given the SIGILL/SIGSEG I get maybe that one is 2 writes of 1 byte ?
>>>> I'll have to dig in the ptrace code I guess.
>>>> 
>>>
>>> It is good to get some a clear answer instead of ambiguous speculation.
>>> I think we need to ask in linux-arm-kernel@
>>
>> Did you see my follow up email ? :
>> https://sourceware.org/ml/gdb-patches/2016-11/msg00681.html
>>
>> Also, I think this will become a moot point in the patch I'm about to
>> post since:
>>
>> To install a single step breakpoint on a thread GDBServer needs to make sure
>> that there is not a breakpoint at the thread's current pc, since it
>> can't determine what is the next_pc of a breakpoint instruction.
>>
>> Usually for stepping over it's OK since it's stopped at pc X and it
>> will install a single-step breakpoint at pc X + next_pc_offset.
>>
>> So need_step_over returns true and GDBServer starts a step_over process,
>> which removes all breakpoints, installs a single-step breakpoint on the
>> nextpc and resumes.
>>
>> But in this case it is installing single-step breakpoints in threads at
>> different pcs then the one we're stopped, so the step-over process is
>> not triggered and it should not be.
>>
>> So GDBSever does not take care to remove all breakpoints like is the
>> case in the step-over process.  Because of that it can try to install a
>> single-step breakpoint where there is already a breakpoint in memory and
>> thus break get_next_pc and install a breakpoint at an invalid location.
>>
>> Consider this case:
>>
>> in non-stop, thread 1-3 are stepping in a loop similar to
>> non-stop-fair-events test.
>>
>>  - thread 1 hits its single-step breakpoint at pc A.
>>  - delete its single-step breakpoint.
>>  - a check for need_step_over is done, but there's no breakpoint at pc A
>>  anymore, and nobody is stopped there anyway so it returns false.
>>  - proceed_one_lwp is called on each thread.
>>
>>  Now here is the problem:
>>
>>  thread 1 is at pc A
>>  thread 2 is at pc B
>>
>>  B is a branch to A.
>>  
>>  thread 1 installs a single-step breakpoint at pc B since it's range stepping.
>>  thread 2 does not have a single step breakpoint but needs one installed.
>>  
>>  - proceed_one_lwp finds that it needs to install a single-step
>>    breakpoint on thread 2.
>>
>>  - It calls install_single_step_breakpoints, which calls get_next_pc.
>>
>>  - get_next_pc reads the current instruction in memory at pc B, but
>>    since it's a breakpoint, it missinterprets the instruction, you can't
>>    step over a breakpoint like that anyway, but this is what happens
>>    now.
>>
>>    A single-step breakpoint is now inserted at an invalid location.
>>
>> So my approch in my patch is to fix this by always removing all
>> breakpoints and fast_tracepoints_jumps, like we do in start_step_over
>> before calling install_software_single_step.
>>
>> This makes the breakpoint installation a multiple steps process and thus
>> can't be atomic.
>>
>> WDYT ?
>>
>> Thanks,
>> Antoine
>
> In fact thinking more about this we may need to remove all breakpoints
> at any pc since get_next_pc may read memory in other places then the
> current pc to deal with atomic sequences for example or for other
> instructions too.
>
> If it reads a breakpoint in memory there it may come-up with an invalid
> next pc.
>
> This is a problem with the current step-over logic too.
>
> So we would either need to be able to read past any
> breakpoint/fast_tracepoint_jump... anywhere
> or uninstall everything before calling get_next_pc.
>
> I'm not sure which one is best at the moment, opinions on this are
> welcome.

Sorry for what may seem like a monologue there, but we can't read past
breakpoitns etc all the time since we have know idea of the memory
aligment involved, we don't want to check around a single byte read to
see if it looks like a breakpoint.

So before any call to get_next_pc, we need to remove everthing, I'll
send a patch in that regard.

Thanks,
Antoine

Re: [PATCH 0/9 V3] Use reinsert breakpoint for vCont;s

[Pedro Alves]

I'm behind on this whole discussion, but, the comment below
caught my attention:

On 11/25/2016 01:12 PM, Antoine Tremblay wrote:

> In fact thinking more about this we may need to remove all breakpoints
> at any pc since get_next_pc may read memory in other places then the
> current pc to deal with atomic sequences for example or for other
> instructions too.
> 
> If it reads a breakpoint in memory there it may come-up with an invalid
> next pc.

How can that happen, given gdbserver's memory read routine takes
share to hide breakpoint breakpoints?

I.e., read_inferior_memory -> check_mem_read.

Thanks,
Pedro Alves

Re: [PATCH 0/9 V3] Use reinsert breakpoint for vCont;s

[Antoine Tremblay]

Pedro Alves writes:

> I'm behind on this whole discussion, but, the comment below
> caught my attention:
>
> On 11/25/2016 01:12 PM, Antoine Tremblay wrote:
>
>> In fact thinking more about this we may need to remove all breakpoints
>> at any pc since get_next_pc may read memory in other places then the
>> current pc to deal with atomic sequences for example or for other
>> instructions too.
>> 
>> If it reads a breakpoint in memory there it may come-up with an invalid
>> next pc.
>
> How can that happen, given gdbserver's memory read routine takes
> share to hide breakpoint breakpoints?
>
> I.e., read_inferior_memory -> check_mem_read.
>

Indeed, this is because arm_get_next_pc uses (*the_target->read_memory)
directly.

This should be fixed for read_inferior_memory indeed and it would solve
this issue, sorry I was not so familiar with the check_mem_read hiding
breakpoints/fast tracepoints jumps.

I'm not sure why it uses that directly at the moment, looks like a plain
misstake but I'll dig a bit more.

But it's just what we need this may easily fix that issue in particular!

Thanks,
Antoine

Re: [PATCH 0/9 V3] Use reinsert breakpoint for vCont;s

[Antoine Tremblay]

Antoine Tremblay writes:

> Pedro Alves writes:
>
>> I'm behind on this whole discussion, but, the comment below
>> caught my attention:
>>
>> On 11/25/2016 01:12 PM, Antoine Tremblay wrote:
>>
>>> In fact thinking more about this we may need to remove all breakpoints
>>> at any pc since get_next_pc may read memory in other places then the
>>> current pc to deal with atomic sequences for example or for other
>>> instructions too.
>>> 
>>> If it reads a breakpoint in memory there it may come-up with an invalid
>>> next pc.
>>
>> How can that happen, given gdbserver's memory read routine takes
>> share to hide breakpoint breakpoints?
>>
>> I.e., read_inferior_memory -> check_mem_read.
>>
>
> Indeed, this is because arm_get_next_pc uses (*the_target->read_memory)
> directly.
>
> This should be fixed for read_inferior_memory indeed and it would solve
> this issue, sorry I was not so familiar with the check_mem_read hiding
> breakpoints/fast tracepoints jumps.
>
> I'm not sure why it uses that directly at the moment, looks like a plain
> misstake but I'll dig a bit more.
>
> But it's just what we need this may easily fix that issue in particular!

Turns out this a problem in a few places on arm and on other arches too,
the only place you should call it directly basically is
{arch}_breakpoint_at.

Otherwise call target_read_memory.

I'll send patches for that.

Thanks!
Antoine