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

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

[Yao Qi]

Nowadays, reinsert breakpoint is used in GDBserver to step over a
breakpoint.  I want to use it to handle vCont;s too.  The idea is that
when GDBserver receives resume_step request from GDB, use the software
single step logic, insert reinsert_breakpoint on the next pcs, and
resume the thread.  This means we have multiple reinsert_breakpoints
for different threads for either step-over or vCont;s.  Patch #6 makes
reinsert breakpoints thread specific, patch #7 does the major thing,
and patch #8 claims supporting vCont;s with software single step.

Patch #1 and #2 fixes the existing bugs.  Patch #3, #4, and #5 refactor
the code.

The whole series are tested on arm-linux, aarch64-linux and
x86_64-linux.

*** BLURB HERE ***

Yao Qi (8):
  Switch to current thread before finish_step_over
  Delete reinsert breakpoints from forked child
  Pass breakpoint type in set_breakpoint_at
  Create sub classes of 'struct breakpoint'
  Refactor clone_all_breakpoints
  Make reinsert_breakpoint thread specific
  Use reinsert_breakpoint for vCont;s
  [GDBserver] Support vCont s and S actions with software single step

 gdb/gdbserver/linux-low.c | 180 ++++++++++++++++++++++++++--
 gdb/gdbserver/mem-break.c | 293 +++++++++++++++++++++++++++++++++-------------
 gdb/gdbserver/mem-break.h |  35 +++---
 gdb/gdbserver/server.c    |  17 +--
 4 files changed, 412 insertions(+), 113 deletions(-)

-- 
1.9.1

[PATCH 6/8] Make reinsert_breakpoint thread specific

[Yao Qi]

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 step-over, and the
other one for 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,
which is an operation to one thread in one go.  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-05-20  Yao Qi  <yao.qi@linaro.org>

	* mem-break.c (struct reinsert_breakpoint) <id>: New field.
	(set_reinsert_breakpoint): New parameter id.  Callers updated.
	(delete_reinsert_breakpoints): Likewise.
	(has_reinsert_breakpoints): Change parameter to thread.  Callers
	updated.
	(clone_one_breakpoint): Likewise.
---
 gdb/gdbserver/linux-low.c | 16 ++++++++--------
 gdb/gdbserver/mem-break.c | 34 +++++++++++++++++++++++++---------
 gdb/gdbserver/mem-break.h | 13 +++++++------
 3 files changed, 40 insertions(+), 23 deletions(-)

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index 539d787..d5f7097 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -578,11 +578,11 @@ handle_extended_wait (struct lwp_info **orig_event_lwp, int wstat)
 		 to access its memory without stopping all other threads
 		 from other processes.  */
 	      current_thread = child_thr;
-	      delete_reinsert_breakpoints ();
+	      delete_reinsert_breakpoints (ptid);
 	      current_thread = saved_thread;
 
-	      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.  */
@@ -4097,7 +4097,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);
 }
@@ -4238,7 +4238,7 @@ linux_resume_one_lwp_throw (struct lwp_info *lwp,
       else
 	{
 	  gdb_assert (step == 0);
-	  gdb_assert (has_reinsert_breakpoints (proc));
+	  gdb_assert (has_reinsert_breakpoints (thread));
 	}
     }
 
@@ -4728,8 +4728,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 (get_lwp_thread (lwp)));
+	  delete_reinsert_breakpoints (ptid_of_lwp (lwp));
 	}
 
       step_over_bkpt = null_ptid;
@@ -5067,7 +5067,7 @@ proceed_one_lwp (struct inferior_list_entry *entry, void *except)
       else
 	{
 	  /* GDBserver must insert reinsert breakpoint for step-over.  */
-	  gdb_assert (has_reinsert_breakpoints (get_thread_process (thread)));
+	  gdb_assert (has_reinsert_breakpoints (thread));
 	}
     }
 
diff --git a/gdb/gdbserver/mem-break.c b/gdb/gdbserver/mem-break.c
index 4e45c0e..5d699c3 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 id;
 };
 
 /* Return the breakpoint size from its kind.  */
@@ -1476,25 +1479,32 @@ gdb_breakpoint_here (CORE_ADDR where)
 }
 
 void
-set_reinsert_breakpoint (CORE_ADDR stop_at)
+set_reinsert_breakpoint (CORE_ADDR stop_at, ptid_t id)
 {
-  struct breakpoint *bp;
+  struct reinsert_breakpoint *bp;
 
-  bp = set_breakpoint_at_1 (reinsert_breakpoint, stop_at, NULL);
+  gdb_assert (ptid_get_pid (current_ptid) == ptid_get_pid (id));
+
+  bp = (struct reinsert_breakpoint *) set_breakpoint_at_1 (reinsert_breakpoint,
+							   stop_at, NULL);
+  bp->id = id;
 }
 
 void
-delete_reinsert_breakpoints (void)
+delete_reinsert_breakpoints (ptid_t id)
 {
   struct process_info *proc = current_process ();
   struct breakpoint *bp, **bp_link;
 
+  gdb_assert (ptid_get_pid (current_ptid) == ptid_get_pid (id));
+
   bp = proc->breakpoints;
   bp_link = &proc->breakpoints;
 
   while (bp)
     {
-      if (bp->type == reinsert_breakpoint)
+      if (bp->type == reinsert_breakpoint
+	  && ptid_equal (((struct reinsert_breakpoint *) bp)->id, id))
 	{
 	  *bp_link = bp->next;
 	  release_breakpoint (proc, bp);
@@ -1622,8 +1632,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;
@@ -1631,7 +1642,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)->id,
+			 ptid_of (thread)))
 	return 1;
       else
 	{
@@ -2070,7 +2083,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 id)
 {
   struct breakpoint *dest;
   struct raw_breakpoint *dest_raw;
@@ -2131,6 +2144,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->id = id;
     }
   else
     gdb_assert_not_reached ("unhandled breakpoint type");
@@ -2157,7 +2173,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 757399a..38d5911 100644
--- a/gdb/gdbserver/mem-break.h
+++ b/gdb/gdbserver/mem-break.h
@@ -152,22 +152,23 @@ 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
+   ID.  */
 
-void set_reinsert_breakpoint (CORE_ADDR stop_at);
+void set_reinsert_breakpoint (CORE_ADDR stop_at, ptid_t id);
 
-/* Delete all reinsert breakpoints.  */
+/* Delete all reinsert breakpoints of thread represented by ID.  */
 
-void delete_reinsert_breakpoints (void);
+void delete_reinsert_breakpoints (ptid_t id);
 
 /* 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 3/8] 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_at_1, and wrap
it with set_breakpoint_at, and pass other_breakpoint.  In this way,
we can call set_breakpoint_at_1 with reinsert_breakpoint in
set_reinsert_breakpoint too, and code looks cleaner.

gdb/gdbserver:

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

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

diff --git a/gdb/gdbserver/mem-break.c b/gdb/gdbserver/mem-break.c
index 3313459..3e3be8b 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_at_1 (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_at_1 (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_at_1 (reinsert_breakpoint, stop_at, NULL);
 }
 
 void
diff --git a/gdb/gdbserver/mem-break.h b/gdb/gdbserver/mem-break.h
index b84dc1e..f6c9631 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

[PATCH 1/8] Switch to current thread before finish_step_over

[Yao Qi]

This patch adds some sanity check that reinsert breakpoints must be
there when doing step-over on software single step target.  The check
triggers an assert when running forking-threads-plus-breakpoint.exp
on arm-linux target,

 gdb/gdbserver/linux-low.c:4714: A problem internal to GDBserver has been detected.^M
 int finish_step_over(lwp_info*): Assertion `has_reinsert_breakpoints ()' failed.

the error happens when GDBserver has already resumed a thread of
process A for step-over (and wait for it hitting reinsert breakpoint),
but receives detach request for process B from GDB, which is shown in
the backtrace below,

 (gdb) bt
 #2  0x000228aa in finish_step_over (lwp=0x12bbd98) at /home/yao/SourceCode/gnu/gdb/git/gdb/gdbserver/linux-low.c:4703
 #3  0x00025a50 in finish_step_over (lwp=0x12bbd98) at /home/yao/SourceCode/gnu/gdb/git/gdb/gdbserver/linux-low.c:4749
 #4  complete_ongoing_step_over () at /home/yao/SourceCode/gnu/gdb/git/gdb/gdbserver/linux-low.c:4760
 #5  linux_detach (pid=25228) at /home/yao/SourceCode/gnu/gdb/git/gdb/gdbserver/linux-low.c:1503
 #6  0x00012bae in process_serial_event () at /home/yao/SourceCode/gnu/gdb/git/gdb/gdbserver/server.c:3974
 #7  handle_serial_event (err=<optimized out>, client_data=<optimized out>) at /home/yao/SourceCode/gnu/gdb/git/gdb/gdbserver/server.c:4347
 #8  0x00016d68 in handle_file_event (event_file_desc=<optimized out>) at /home/yao/SourceCode/gnu/gdb/git/gdb/gdbserver/event-loop.c:429
 #9  0x000173ea in process_event () at /home/yao/SourceCode/gnu/gdb/git/gdb/gdbserver/event-loop.c:184
 #10 start_event_loop () at /home/yao/SourceCode/gnu/gdb/git/gdb/gdbserver/event-loop.c:547
 #11 0x0000aa2c in captured_main (argv=<optimized out>, argc=<optimized out>) at /home/yao/SourceCode/gnu/gdb/git/gdb/gdbserver/server.c:3719
 #12 main (argc=<optimized out>, argv=<optimized out>) at /home/yao/SourceCode/gnu/gdb/git/gdb/gdbserver/server.c:3804

the sanity check tries to find the reinsert breakpoint from process B,
but nothing is found.  It is wrong, we need to search in process A,
since we started step-over of a thread of process A.

 (gdb) p lwp->thread->entry.id
 $3 = {pid = 25120, lwp = 25131, tid = 0}
 (gdb) p current_thread->entry.id
 $4 = {pid = 25228, lwp = 25228, tid = 0}

This patch switched current_thread to the step we are doing step-over
in complete_ongoing_step_over.

gdb/gdbserver:

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

	* linux-low.c (linux_resume_one_lwp_throw): Call
	has_reinsert_breakpoints.
	(finish_step_over): Likewise.
	(proceed_one_lwp): Likewise.
	* mem-break.c (has_reinsert_breakpoints): New function.
	* mem-break.h (has_reinsert_breakpoints): Declare.
---
 gdb/gdbserver/linux-low.c | 30 +++++++++++++++++++++++++-----
 gdb/gdbserver/mem-break.c | 22 ++++++++++++++++++++++
 gdb/gdbserver/mem-break.h |  4 ++++
 3 files changed, 51 insertions(+), 5 deletions(-)

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index 81134b0..8e8f710 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -4218,6 +4218,11 @@ linux_resume_one_lwp_throw (struct lwp_info *lwp,
 
 	  step = 1;
 	}
+      else
+	{
+	  gdb_assert (step == 0);
+	  gdb_assert (has_reinsert_breakpoints (proc));
+	}
     }
 
   if (fast_tp_collecting == 1)
@@ -4705,7 +4710,10 @@ finish_step_over (struct lwp_info *lwp)
 	 because we were stepping over a breakpoint, and we hold all
 	 threads but LWP stopped while doing that.  */
       if (!can_hardware_single_step ())
-	delete_reinsert_breakpoints ();
+	{
+	  gdb_assert (has_reinsert_breakpoints (current_process ()));
+	  delete_reinsert_breakpoints ();
+	}
 
       step_over_bkpt = null_ptid;
       return 1;
@@ -4741,7 +4749,13 @@ complete_ongoing_step_over (void)
 
       lwp = find_lwp_pid (step_over_bkpt);
       if (lwp != NULL)
-	finish_step_over (lwp);
+	{
+	  struct thread_info *saved_thread = current_thread;
+
+	  current_thread = get_lwp_thread (lwp);
+	  finish_step_over (lwp);
+	  current_thread = saved_thread;
+	}
       step_over_bkpt = null_ptid;
       unsuspend_all_lwps (lwp);
     }
@@ -5017,6 +5031,7 @@ proceed_one_lwp (struct inferior_list_entry *entry, void *except)
       send_sigstop (lwp);
     }
 
+  step = 0;
   if (thread->last_resume_kind == resume_step)
     {
       if (debug_threads)
@@ -5029,10 +5044,15 @@ proceed_one_lwp (struct inferior_list_entry *entry, void *except)
       if (debug_threads)
 	debug_printf ("   stepping LWP %ld, reinsert set\n",
 		      lwpid_of (thread));
-      step = 1;
+
+      if (can_hardware_single_step ())
+	step = 1;
+      else
+	{
+	  /* GDBserver must insert reinsert breakpoint for step-over.  */
+	  gdb_assert (has_reinsert_breakpoints (get_thread_process (thread)));
+	}
     }
-  else
-    step = 0;
 
   linux_resume_one_lwp (lwp, step, 0, NULL);
   return 0;
diff --git a/gdb/gdbserver/mem-break.c b/gdb/gdbserver/mem-break.c
index 363d7ca..3313459 100644
--- a/gdb/gdbserver/mem-break.c
+++ b/gdb/gdbserver/mem-break.c
@@ -1553,6 +1553,28 @@ reinsert_breakpoints_at (CORE_ADDR pc)
     }
 }
 
+int
+has_reinsert_breakpoints (struct process_info *proc)
+{
+  struct breakpoint *bp, **bp_link;
+
+  bp = proc->breakpoints;
+  bp_link = &proc->breakpoints;
+
+  while (bp)
+    {
+      if (bp->type == reinsert_breakpoint)
+	return 1;
+      else
+	{
+	  bp_link = &bp->next;
+	  bp = *bp_link;
+	}
+    }
+
+  return 0;
+}
+
 void
 reinsert_all_breakpoints (void)
 {
diff --git a/gdb/gdbserver/mem-break.h b/gdb/gdbserver/mem-break.h
index 4d9a76c..b84dc1e 100644
--- a/gdb/gdbserver/mem-break.h
+++ b/gdb/gdbserver/mem-break.h
@@ -163,6 +163,10 @@ void delete_reinsert_breakpoints (void);
 
 void reinsert_breakpoints_at (CORE_ADDR where);
 
+/* Process PROC has reinsert breakpoints or not.  */
+
+int has_reinsert_breakpoints (struct process_info *proc);
+
 /* Uninsert breakpoints at WHERE (and change their status to
    uninserted).  This still leaves the breakpoints in the table.  */
 
-- 
1.9.1

[PATCH 2/8] Delete reinsert breakpoints from forked child

[Yao Qi]

When a thread is stepping over a syscall instruction with software
single step, GDBserver inserts reinsert breakpoints at the next pcs.
If the syscall call is fork, the forked child has reinsert breakpoint
in its space, and GDBserver clones parent's breakpoint list to child's.
When GDBserver resumes the child, it hits the reinsert breakpoint.  Since
it is a GDBserver internal breakpoint, GDBserver will do step-over again,
and remove this reinsert breakpoint on step-over is finished, so the
reinsert breakpoint left in the child process doesn't make any trouble.
However, GDBserver still need to remove the reinsert breakpoints from
the child, in order to avoid the unnecessary breakpoint hit and
step-over.

gdb/gdbserver:

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

	* linux-low.c (handle_extended_wait): If the parent is doing
	step-over, remove the reinsert breakpoints from the child.
---
 gdb/gdbserver/linux-low.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index 8e8f710..a63cc7a 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -568,6 +568,25 @@ handle_extended_wait (struct lwp_info **orig_event_lwp, int wstat)
 	  event_lwp->status_pending_p = 1;
 	  event_lwp->status_pending = wstat;
 
+	  /* If the parent thread is doing step-over with reinsert
+	     breakpoints, the reinsert breakpoints are still in child's
+	     process space and cloned to its breakpoint list from the
+	     parent's.  Remove them.  */
+	  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;
+
+	      gdb_assert (has_reinsert_breakpoints (parent_proc));
+	      gdb_assert (!has_reinsert_breakpoints (child_proc));
+	    }
+
 	  /* Report the event.  */
 	  return 0;
 	}
-- 
1.9.1

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

[Yao Qi]

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.  Note that GDBserver may
or may not report this event back to GDB.

gdb/gdbserver:

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

	* linux-low.c (resume_stopped_resumed_lwps): If resume request
	is resume_step, either set step to 1 or assert that reinsert
	breakpoints are installed.
	(linux_wait_1): Stop all threads, remove reinsert breakpoints,
	and unstop them.
	(linux_resume_one_thread): If resume request is resume_step,
	either set step to 1 or assert that reinsert breakpoints are
	installed.
	(linux_resume): Install reinsert breakpoints if the thread is
	requested to resume_step.
	(proceed_one_lwp): If resume request is resume_step,
        either set step to 1 or assert that reinsert breakpoints are
        installed.
	(proceed_all_lwps): Install reinsert breakpoints if the thread is
        requested to resume_step.
---
 gdb/gdbserver/linux-low.c | 125 ++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 121 insertions(+), 4 deletions(-)

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index d5f7097..22cf51a 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -2526,7 +2526,18 @@ 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)
+	{
+	  if (can_hardware_single_step ())
+	    step = 1;
+	  else
+	    {
+	      /* GDBserver must insert reinsert breakpoint for vCont;s.  */
+	      gdb_assert (has_reinsert_breakpoints (thread));
+	    }
+	}
 
       if (debug_threads)
 	debug_printf ("RSRL: resuming stopped-resumed LWP %s at %s: step=%d\n",
@@ -3477,6 +3488,18 @@ linux_wait_1 (ptid_t ptid,
       return ignore_event (ourstatus);
     }
 
+  /* Remove reinsert breakpoints for resume_step.  */
+  if (can_software_single_step ()
+      && current_thread->last_resume_kind == resume_step
+      && has_reinsert_breakpoints (current_thread))
+    {
+      stop_all_lwps (1, event_child);
+
+      delete_reinsert_breakpoints (current_ptid);
+
+      unstop_all_lwps (1, event_child);
+    }
+
   /* Note that all addresses are always "out of the step range" when
      there's no range to begin with.  */
   in_step_range = lwp_in_step_range (event_child);
@@ -4796,7 +4819,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;
 
@@ -4865,10 +4887,22 @@ linux_resume_one_thread (struct inferior_list_entry *entry, void *arg)
 
   if (!leave_pending)
     {
+      int step = 0;
+
       if (debug_threads)
 	debug_printf ("resuming LWP %ld\n", lwpid_of (thread));
 
-      step = (lwp->resume->kind == resume_step);
+      if (lwp->resume->kind == resume_step)
+	{
+	  if (can_hardware_single_step ())
+	    step = 1;
+	  else
+	    {
+	      /* GDBserver must insert reinsert breakpoint for vCont;s.  */
+	      gdb_assert (has_reinsert_breakpoints (thread));
+	    }
+	}
+
       linux_resume_one_lwp (lwp, step, lwp->resume->sig, NULL);
     }
   else
@@ -4909,6 +4943,7 @@ linux_resume (struct thread_resume *resume_info, size_t n)
   struct thread_info *need_step_over = NULL;
   int any_pending;
   int leave_all_stopped;
+  int resume_step_is_handled = 0;
 
   if (debug_threads)
     {
@@ -4952,12 +4987,57 @@ linux_resume (struct thread_resume *resume_info, size_t n)
 	debug_printf ("Resuming, no pending status or step over needed\n");
     }
 
+  /* Before we resume the threads, if resume_step is requested by GDB,
+     stop all threads and install reinsert breakpoints.  */
+  if (!leave_all_stopped && can_software_single_step ())
+    {
+      struct inferior_list_entry *inf, *tmp;
+
+      if (debug_threads)
+	debug_printf ("Handle resume_step.\n");
+
+      ALL_INFERIORS (&all_threads, inf, tmp)
+	{
+	  struct thread_info *thread = (struct thread_info *) inf;
+	  struct lwp_info *lwp = get_thread_lwp (thread);
+
+	  if (lwp->resume != NULL && lwp->resume->kind == resume_step)
+	    {
+	      struct thread_info *saved_thread;
+
+	      if (!resume_step_is_handled)
+		{
+		  stop_all_lwps (0, NULL);
+
+		  if (debug_threads)
+		    debug_printf ("Done stopping all threads.\n");
+
+		  resume_step_is_handled = 1;
+		}
+
+	      saved_thread = current_thread;
+	      current_thread = thread;
+	      install_software_single_step_breakpoints (lwp);
+	      current_thread = saved_thread;
+
+	      if (debug_threads)
+		debug_printf ("Insert breakpoint for resume_step LWP %ld\n",
+			      lwpid_of (thread));
+	    }
+	}
+
+      if (debug_threads)
+	debug_printf ("Handle resume_step.  Done\n");
+    }
+
   /* Even if we're leaving threads stopped, queue all signals we'd
      otherwise deliver.  */
   find_inferior (&all_threads, linux_resume_one_thread, &leave_all_stopped);
 
   if (need_step_over)
     start_step_over (get_thread_lwp (need_step_over));
+  else if (resume_step_is_handled)
+    unstop_all_lwps (0, NULL);
 
   if (debug_threads)
     {
@@ -5054,7 +5134,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 (can_hardware_single_step ())
+	step = 1;
+      else
+	{
+	  /* GDBserver must insert reinsert breakpoint for vCont;s.  */
+	  gdb_assert (has_reinsert_breakpoints (thread));
+	}
     }
   else if (lwp->bp_reinsert != 0)
     {
@@ -5124,6 +5211,36 @@ proceed_all_lwps (void)
   if (debug_threads)
     debug_printf ("Proceeding, no step-over needed\n");
 
+  /* Re-install the reinsert breakpoints on software single step target
+     if the client wants it step.  */
+  if (can_software_single_step ())
+    {
+      struct inferior_list_entry *inf, *tmp;
+
+      ALL_INFERIORS (&all_threads, inf, tmp)
+	{
+	  struct thread_info *thread = (struct thread_info *) inf;
+
+	  if (thread->last_resume_kind == resume_step)
+	    {
+	      struct lwp_info *lwp = get_thread_lwp (thread);
+	      struct thread_info *saved_thread;
+
+	      saved_thread = current_thread;
+	      current_thread = thread;
+
+	      if (!has_reinsert_breakpoints (thread))
+		install_software_single_step_breakpoints (lwp);
+
+	      current_thread = saved_thread;
+
+	      if (debug_threads)
+		debug_printf ("Insert breakpoint for resume_step LWP %ld\n",
+			      lwpid_of (thread));
+	    }
+	}
+    }
+
   find_inferior (&all_threads, proceed_one_lwp, NULL);
 }
 
-- 
1.9.1

[PATCH 4/8] 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-05-20  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/mem-break.c | 209 +++++++++++++++++++++++++++++++++-------------
 gdb/gdbserver/mem-break.h |  11 +--
 gdb/gdbserver/server.c    |   4 +-
 3 files changed, 157 insertions(+), 67 deletions(-)

diff --git a/gdb/gdbserver/mem-break.c b/gdb/gdbserver/mem-break.c
index 3e3be8b..1fe1d20 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;
@@ -1619,14 +1679,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;
+		}
 	    }
 	}
 
@@ -2008,12 +2074,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);
@@ -2025,29 +2085,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 f6c9631..280e4f8 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/8] [GDBserver] 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-05-20  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 5/8] Refactor clone_all_breakpoints

[Yao Qi]

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

gdb/gdbserver:

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

	* mem-break.c (clone_all_breakpoints): Remove all parameters.
	Add new parameters child_thread and parent_proc.  Callers
	updated.
	* mem-break.h (clone_all_breakpoints): Update declaration.
---
 gdb/gdbserver/linux-low.c |  4 +---
 gdb/gdbserver/mem-break.c | 14 +++++++-------
 gdb/gdbserver/mem-break.h |  8 ++++----
 3 files changed, 12 insertions(+), 14 deletions(-)

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index a63cc7a..539d787 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -542,9 +542,7 @@ handle_extended_wait (struct lwp_info **orig_event_lwp, int wstat)
 
 	  parent_proc = get_thread_process (event_thr);
 	  child_proc->attached = parent_proc->attached;
-	  clone_all_breakpoints (&child_proc->breakpoints,
-				 &child_proc->raw_breakpoints,
-				 parent_proc->breakpoints);
+	  clone_all_breakpoints (child_thr, parent_proc);
 
 	  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 1fe1d20..4e45c0e 100644
--- a/gdb/gdbserver/mem-break.c
+++ b/gdb/gdbserver/mem-break.c
@@ -2141,21 +2141,21 @@ 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,
+		       struct process_info *parent_proc)
 {
   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 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 280e4f8..757399a 100644
--- a/gdb/gdbserver/mem-break.h
+++ b/gdb/gdbserver/mem-break.h
@@ -258,10 +258,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_PROC.  */
 
-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,
+			    struct process_info *parent_proc);
 
 #endif /* MEM_BREAK_H */
-- 
1.9.1

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

[Antoine Tremblay]

Yao Qi writes:

> Nowadays, reinsert breakpoint is used in GDBserver to step over a
> breakpoint.  I want to use it to handle vCont;s too.  The idea is that
> when GDBserver receives resume_step request from GDB, use the software
> single step logic, insert reinsert_breakpoint on the next pcs, and
> resume the thread.  This means we have multiple reinsert_breakpoints
> for different threads for either step-over or vCont;s.  Patch #6 makes
> reinsert breakpoints thread specific, patch #7 does the major thing,
> and patch #8 claims supporting vCont;s with software single step.
>
> Patch #1 and #2 fixes the existing bugs.  Patch #3, #4, and #5 refactor
> the code.
>
> The whole series are tested on arm-linux, aarch64-linux and
> x86_64-linux.
>
> *** BLURB HERE ***
>
> Yao Qi (8):
>   Switch to current thread before finish_step_over
>   Delete reinsert breakpoints from forked child
>   Pass breakpoint type in set_breakpoint_at
>   Create sub classes of 'struct breakpoint'
>   Refactor clone_all_breakpoints
>   Make reinsert_breakpoint thread specific
>   Use reinsert_breakpoint for vCont;s
>   [GDBserver] Support vCont s and S actions with software single step
>
>  gdb/gdbserver/linux-low.c | 180 ++++++++++++++++++++++++++--
>  gdb/gdbserver/mem-break.c | 293 +++++++++++++++++++++++++++++++++-------------
>  gdb/gdbserver/mem-break.h |  35 +++---
>  gdb/gdbserver/server.c    |  17 +--
>  4 files changed, 412 insertions(+), 113 deletions(-)

Series LGTM. Thanks for working on this!

Antoine

Re: [PATCH 1/8] Switch to current thread before finish_step_over

[Pedro Alves]

On 05/20/2016 04:12 PM, Yao Qi wrote:

> This patch switched current_thread to the step we are doing step-over

s/to the step/to the thread/

> @@ -4741,7 +4749,13 @@ complete_ongoing_step_over (void)
>  
>        lwp = find_lwp_pid (step_over_bkpt);
>        if (lwp != NULL)
> -	finish_step_over (lwp);
> +	{
> +	  struct thread_info *saved_thread = current_thread;
> +
> +	  current_thread = get_lwp_thread (lwp);
> +	  finish_step_over (lwp);
> +	  current_thread = saved_thread;
> +	}

So the pattern should be that if a function takes a thread/lwp
parameter, then it should work with that, instead of relying on
the global state.  So I think that it should be finish_step_over
itself that switches the current thread, because it is an implementation
detail of finish_step_over that is calls into functions that rely
on the global current thread.  Putting the save/restore there
should make it more obvious why it's needed.

Thanks,
Pedro Alves

Re: [PATCH 2/8] Delete reinsert breakpoints from forked child

[Pedro Alves]

On 05/20/2016 04:12 PM, Yao Qi wrote:

> gdb/gdbserver:
> 
> 2016-05-20  Yao Qi  <yao.qi@linaro.org>
> 
> 	* linux-low.c (handle_extended_wait): If the parent is doing
> 	step-over, remove the reinsert breakpoints from the child.

Don't we need to handle vfork differently?  Removing a breakpoint
from the child removes it from the parent too, since they're
sharing memory.

Thanks,
Pedro Alves

Re: [PATCH 3/8] Pass breakpoint type in set_breakpoint_at

[Pedro Alves]

On 05/20/2016 04:12 PM, Yao Qi wrote:

>  
> -/* 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_at_1 (enum bkpt_type type, CORE_ADDR where,
> +		     int (*handler) (CORE_ADDR))

Maybe avoid the bland "_1", like:

 set_breakpoint_type_at (enum bkpt_type type, CORE_ADDR where,
 		        int (*handler) (CORE_ADDR))

But LGTM.

Thanks,
Pedro Alves

Re: [PATCH 4/8] Create sub classes of 'struct breakpoint'

[Pedro Alves]

On 05/20/2016 04:12 PM, Yao Qi wrote:
> Nowadays, there are three types of breakpoint in GDBserver,
> 
>  - gdb breakpoints,
>  - reinsert breakpoints, used for software single step,
>  - other breakpoints, used for tracepoint,

We should probably rename the latter two, so we didn't
have to explain what they are.   (Doesn't have to be
in this patch, but it's true this patch exposes the naming
to even more places...)

Anyway, LGTM.

The clone_one_breakpoint changes are calling for src->vtable->clone();  :-)

Thanks,
Pedro Alves

Re: [PATCH 5/8] Refactor clone_all_breakpoints

[Pedro Alves]

On 05/20/2016 04:12 PM, Yao Qi wrote:
> This patch is to change the interface of clone_all_breakpoints, from
> lists of breakpoints and raw_breakpoints to child thread and parent
> process.  I choose child thread to pass because we need the ptid of
> the child thread in the following patch.

Seems fine to me.  I'd have passed the parent thread instead of
process.

Thanks,
Pedro Alves

Re: [PATCH 6/8] Make reinsert_breakpoint thread specific

[Pedro Alves]

On 05/20/2016 04:12 PM, Yao Qi wrote:
> 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 step-over, and the
> other one for vCont;s.

Here I don't understand what you mean by "the other one".

> 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,
> which is an operation to one thread in one go.

"in one go" ?  WDYM?

> In other words,
> reinsert_breakpoint is globally thread specific, but in an implicit way.

Right.

> index 4e45c0e..5d699c3 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 id;

It's much more usual to call this "ptid".  From the ChangeLog,
I first though you were adding a "breakpoint number" kind of id.

>  };
>  
>  /* Return the breakpoint size from its kind.  */
> @@ -1476,25 +1479,32 @@ gdb_breakpoint_here (CORE_ADDR where)
>  }
>  
>  void
> -set_reinsert_breakpoint (CORE_ADDR stop_at)
> +set_reinsert_breakpoint (CORE_ADDR stop_at, ptid_t id)

Ditto.  (Likewise other places.)

Otherwise LGTM.

Thanks,
Pedro Alves

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

[Pedro Alves]

On 05/20/2016 04:12 PM, Yao Qi wrote:
> 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.  Note that GDBserver may
> or may not report this event back to GDB.
> 
> gdb/gdbserver:
> 
> 2016-05-20  Yao Qi  <yao.qi@linaro.org>
> 
> 	* linux-low.c (resume_stopped_resumed_lwps): If resume request
> 	is resume_step, either set step to 1 or assert that reinsert
> 	breakpoints are installed.
> 	(linux_wait_1): Stop all threads, remove reinsert breakpoints,
> 	and unstop them.
> 	(linux_resume_one_thread): If resume request is resume_step,
> 	either set step to 1 or assert that reinsert breakpoints are
> 	installed.
> 	(linux_resume): Install reinsert breakpoints if the thread is
> 	requested to resume_step.
> 	(proceed_one_lwp): If resume request is resume_step,
>         either set step to 1 or assert that reinsert breakpoints are
>         installed.
> 	(proceed_all_lwps): Install reinsert breakpoints if the thread is
>         requested to resume_step.

Spaces vs tabs, I think.

> ---
>  gdb/gdbserver/linux-low.c | 125 ++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 121 insertions(+), 4 deletions(-)
> 
> diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
> index d5f7097..22cf51a 100644
> --- a/gdb/gdbserver/linux-low.c
> +++ b/gdb/gdbserver/linux-low.c
> @@ -2526,7 +2526,18 @@ 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)
> +	{

...

> +	  if (can_hardware_single_step ())
> +	    step = 1;
> +	  else
> +	    {
> +	      /* GDBserver must insert reinsert breakpoint for vCont;s.  */
> +	      gdb_assert (has_reinsert_breakpoints (thread));
> +	    }

This pattern seems to appear in many places in the series.
Maybe we could add a function to wrap it?  Something like:

static int
maybe_hw_step (struct thread_info *thread)
{
  if (can_hardware_single_step ())
    return 1;
  else
    {
      gdb_assert (has_reinsert_breakpoints (thread));
      return 0;
    }
}

?

> +	}
>  
>        if (debug_threads)
>  	debug_printf ("RSRL: resuming stopped-resumed LWP %s at %s: step=%d\n",
> @@ -3477,6 +3488,18 @@ linux_wait_1 (ptid_t ptid,
>        return ignore_event (ourstatus);
>      }
>  
> +  /* Remove reinsert breakpoints for resume_step.  */
> +  if (can_software_single_step ()
> +      && current_thread->last_resume_kind == resume_step

What if a vCont;t came in after the step had started and
before it finished?  Won't we have lost the resume_step 
linux_set_resume_request then?


> +      && has_reinsert_breakpoints (current_thread))
> +    {
> +      stop_all_lwps (1, event_child);
> +
> +      delete_reinsert_breakpoints (current_ptid);
> +
> +      unstop_all_lwps (1, event_child);
> +    }
> +
>    /* Note that all addresses are always "out of the step range" when
>       there's no range to begin with.  */
>    in_step_range = lwp_in_step_range (event_child);
> @@ -4796,7 +4819,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;
>  
> @@ -4865,10 +4887,22 @@ linux_resume_one_thread (struct inferior_list_entry *entry, void *arg)
>  
>    if (!leave_pending)
>      {
> +      int step = 0;
> +
>        if (debug_threads)
>  	debug_printf ("resuming LWP %ld\n", lwpid_of (thread));
>  
> -      step = (lwp->resume->kind == resume_step);
> +      if (lwp->resume->kind == resume_step)
> +	{
> +	  if (can_hardware_single_step ())
> +	    step = 1;
> +	  else
> +	    {
> +	      /* GDBserver must insert reinsert breakpoint for vCont;s.  */
> +	      gdb_assert (has_reinsert_breakpoints (thread));
> +	    }
> +	}
> +
>        linux_resume_one_lwp (lwp, step, lwp->resume->sig, NULL);
>      }
>    else
> @@ -4909,6 +4943,7 @@ linux_resume (struct thread_resume *resume_info, size_t n)
>    struct thread_info *need_step_over = NULL;
>    int any_pending;
>    int leave_all_stopped;
> +  int resume_step_is_handled = 0;
>  
>    if (debug_threads)
>      {
> @@ -4952,12 +4987,57 @@ linux_resume (struct thread_resume *resume_info, size_t n)
>  	debug_printf ("Resuming, no pending status or step over needed\n");
>      }
>  
> +  /* Before we resume the threads, if resume_step is requested by GDB,
> +     stop all threads and install reinsert breakpoints.  */
> +  if (!leave_all_stopped && can_software_single_step ())
> +    {
> +      struct inferior_list_entry *inf, *tmp;
> +
> +      if (debug_threads)
> +	debug_printf ("Handle resume_step.\n");
> +
> +      ALL_INFERIORS (&all_threads, inf, tmp)
> +	{
> +	  struct thread_info *thread = (struct thread_info *) inf;
> +	  struct lwp_info *lwp = get_thread_lwp (thread);
> +
> +	  if (lwp->resume != NULL && lwp->resume->kind == resume_step)
> +	    {
> +	      struct thread_info *saved_thread;
> +
> +	      if (!resume_step_is_handled)
> +		{
> +		  stop_all_lwps (0, NULL);
> +
> +		  if (debug_threads)
> +		    debug_printf ("Done stopping all threads.\n");
> +
> +		  resume_step_is_handled = 1;
> +		}
> +
> +	      saved_thread = current_thread;
> +	      current_thread = thread;
> +	      install_software_single_step_breakpoints (lwp);
> +	      current_thread = saved_thread;
> +

This current thread save/restore should move to
install_software_single_step_breakpoints, as per the other patch.


> +	      if (debug_threads)
> +		debug_printf ("Insert breakpoint for resume_step LWP %ld\n",
> +			      lwpid_of (thread));
> +	    }
> +	}
> +
> +      if (debug_threads)
> +	debug_printf ("Handle resume_step.  Done\n");
> +    }
> +
>    /* Even if we're leaving threads stopped, queue all signals we'd
>       otherwise deliver.  */
>    find_inferior (&all_threads, linux_resume_one_thread, &leave_all_stopped);
>  
>    if (need_step_over)
>      start_step_over (get_thread_lwp (need_step_over));
> +  else if (resume_step_is_handled)
> +    unstop_all_lwps (0, NULL);
>  
>    if (debug_threads)
>      {
> @@ -5054,7 +5134,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 (can_hardware_single_step ())
> +	step = 1;
> +      else
> +	{
> +	  /* GDBserver must insert reinsert breakpoint for vCont;s.  */
> +	  gdb_assert (has_reinsert_breakpoints (thread));
> +	}
>      }
>    else if (lwp->bp_reinsert != 0)
>      {
> @@ -5124,6 +5211,36 @@ proceed_all_lwps (void)
>    if (debug_threads)
>      debug_printf ("Proceeding, no step-over needed\n");
>  
> +  /* Re-install the reinsert breakpoints on software single step target
> +     if the client wants it step.  */
> +  if (can_software_single_step ())
> +    {
> +      struct inferior_list_entry *inf, *tmp;
> +
> +      ALL_INFERIORS (&all_threads, inf, tmp)
> +	{
> +	  struct thread_info *thread = (struct thread_info *) inf;
> +
> +	  if (thread->last_resume_kind == resume_step)
> +	    {
> +	      struct lwp_info *lwp = get_thread_lwp (thread);
> +	      struct thread_info *saved_thread;
> +
> +	      saved_thread = current_thread;
> +	      current_thread = thread;
> +
> +	      if (!has_reinsert_breakpoints (thread))
> +		install_software_single_step_breakpoints (lwp);
> +
> +	      current_thread = saved_thread;

Ditto.

> +
> +	      if (debug_threads)
> +		debug_printf ("Insert breakpoint for resume_step LWP %ld\n",
> +			      lwpid_of (thread));
> +	    }
> +	}
> +    }
> +
>    find_inferior (&all_threads, proceed_one_lwp, NULL);
>  }
>  
> 

Thanks,
Pedro Alves

Re: [PATCH 8/8] [GDBserver] Support vCont s and S actions with software single step

[Pedro Alves]

On 05/20/2016 04:12 PM, Yao Qi wrote:
> 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-05-20  Yao Qi  <yao.qi@linaro.org>
> 
> 	* server.c (handle_v_requests): Support s and S actions
> 	if target_supports_software_single_step return true.

LGTM, once the rest of the series is in.

Thanks,
Pedro Alves

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

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

>> +  /* Remove reinsert breakpoints for resume_step.  */
>> +  if (can_software_single_step ()
>> +      && current_thread->last_resume_kind == resume_step
>
> What if a vCont;t came in after the step had started and
> before it finished?  Won't we have lost the resume_step 
> linux_set_resume_request then?

Do you mean a case like this?  GDB sends vCont;s, and then GDBserver
started step-over, in the middle of step-over, GDB sends vCont;t, so
thread->last_resume_kind is set from resume_step to resume_stop, and the
check here fails, so reinsert breakpoints are not deleted.

-- 
Yao (齐尧)

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

[Pedro Alves]

On 05/26/2016 05:52 PM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> writes:
> 
>>> +  /* Remove reinsert breakpoints for resume_step.  */
>>> +  if (can_software_single_step ()
>>> +      && current_thread->last_resume_kind == resume_step
>>
>> What if a vCont;t came in after the step had started and
>> before it finished?  Won't we have lost the resume_step 
>> linux_set_resume_request then?
> 
> Do you mean a case like this?  GDB sends vCont;s, and then GDBserver
> started step-over, in the middle of step-over, GDB sends vCont;t, so
> thread->last_resume_kind is set from resume_step to resume_stop, and the
> check here fails, so reinsert breakpoints are not deleted.

Exactly.

Thanks,
Pedro Alves