[PATCH 00/12 V2] Use reinsert breakpoint for vCont;s

[PATCH 00/12 V2] Use reinsert breakpoint for vCont;s

[Yao Qi]

Here is the V2 of this patch series, V1 can be found
https://sourceware.org/ml/gdb-patches/2016-05/msg00358.html

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.

Patches 2~5 are the fixes to existing GDBserver problems on step-over
with reinsert_breakpoint in various cases (exit, fork, and vfork).
Although these 4 patches can be sent as another patch series, I still
include them in this series, as they are derived from the V1 review.

Patches 7 and 12 aren't changed.  All review comments are addressed
in the rest of patches.  Once this series goes in, I'll start to rename
'reinsert_breakpoint' and 'other_breakpoint' to match their usages in
GDBserver.

Regression tested on {x86_64, arm, aarch64}-linux.

*** BLURB HERE ***

Yao Qi (12):
  Switch to current thread in finish_step_over
  More assert checks on reinsert breakpoint
  Step over exit with reinsert breakpoints
  Delete reinsert breakpoints from forked child
  Handle reinsert breakpoints for vforked child
  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 reinsert_breakpoint for vCont;s
  Support vCont s and S actions with software single step

 gdb/gdbserver/gdbthread.h                   |   3 +
 gdb/gdbserver/inferiors.c                   |  12 +
 gdb/gdbserver/linux-low.c                   | 206 +++++++++++++++--
 gdb/gdbserver/mem-break.c                   | 341 +++++++++++++++++++++-------
 gdb/gdbserver/mem-break.h                   |  44 ++--
 gdb/gdbserver/server.c                      |  17 +-
 gdb/testsuite/gdb.base/step-over-exit.c     |  50 ++++
 gdb/testsuite/gdb.base/step-over-exit.exp   | 127 +++++++++++
 gdb/testsuite/gdb.base/step-over-fork-1.c   |  51 +++++
 gdb/testsuite/gdb.base/step-over-fork-1.exp | 106 +++++++++
 10 files changed, 842 insertions(+), 115 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/step-over-exit.c
 create mode 100644 gdb/testsuite/gdb.base/step-over-exit.exp
 create mode 100644 gdb/testsuite/gdb.base/step-over-fork-1.c
 create mode 100644 gdb/testsuite/gdb.base/step-over-fork-1.exp

-- 
1.9.1

[PATCH 02/12] More assert checks on reinsert breakpoint

[Yao Qi]

This patch adds more asserts, so the incorrect or sub-optimal
reinsert breakpoints manipulations (from the tests in the following
patches) can trigger them.

gdb/gdbserver:

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

	* linux-low.c (linux_resume_one_lwp_throw): Assert
	has_reinsert_breakpoints returns false.
	* mem-break.c (delete_disabled_breakpoints): Assert
	bp type isn't reinsert_breakpoint.
---
 gdb/gdbserver/linux-low.c | 6 ++++++
 gdb/gdbserver/mem-break.c | 7 ++++++-
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index 77c296c..5f025b5 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -4237,6 +4237,12 @@ 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 (proc));
+    }
 
   if (fast_tp_collecting == 1)
     {
diff --git a/gdb/gdbserver/mem-break.c b/gdb/gdbserver/mem-break.c
index 3313459..c27e803 100644
--- a/gdb/gdbserver/mem-break.c
+++ b/gdb/gdbserver/mem-break.c
@@ -1740,7 +1740,12 @@ delete_disabled_breakpoints (void)
     {
       next = bp->next;
       if (bp->raw->inserted < 0)
-	delete_breakpoint_1 (proc, bp);
+	{
+	  /* If reinsert_breakpoints become disabled, that means the
+	     manipulations (insertion and removal) of them are wrong.  */
+	  gdb_assert (bp->type != reinsert_breakpoint);
+	  delete_breakpoint_1 (proc, bp);
+	}
     }
 }
 
-- 
1.9.1

Re: [PATCH 02/12] More assert checks on reinsert breakpoint

[Pedro Alves]

On 06/02/2016 10:30 AM, Yao Qi wrote:
> This patch adds more asserts, so the incorrect or sub-optimal
> reinsert breakpoints manipulations (from the tests in the following
> patches) can trigger them.
> 
> gdb/gdbserver:
> 
> 2016-05-25  Yao Qi  <yao.qi@linaro.org>
> 
> 	* linux-low.c (linux_resume_one_lwp_throw): Assert
> 	has_reinsert_breakpoints returns false.
> 	* mem-break.c (delete_disabled_breakpoints): Assert
> 	bp type isn't reinsert_breakpoint.

OK.

Thanks,
Pedro Alves

[PATCH 06/12] Pass breakpoint type in set_breakpoint_at

[Yao Qi]

V2: rename set_breakpoint_at_1 to set_breakpoint_type_at

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-05-31  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/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 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 06/12] Pass breakpoint type in set_breakpoint_at

[Pedro Alves]

On 06/02/2016 10:30 AM, Yao Qi wrote:
> V2: rename set_breakpoint_at_1 to set_breakpoint_type_at
> 
> 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-05-31  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.

OK.

Thanks,
Pedro Alves

[PATCH 09/12] Make reinsert_breakpoint thread specific

[Yao Qi]

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-05-20  Yao Qi  <yao.qi@linaro.org>

	* mem-break.c (struct reinsert_breakpoint) <ptid>: New field.
	(set_reinsert_breakpoint): New parameter ptid.  Callers updated.
	(delete_reinsert_breakpoints): Likewise.
	(has_reinsert_breakpoints): Change parameter to thread.  Callers
	updated.
	(clone_one_breakpoint): 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/linux-low.c | 25 +++++++++++--------------
 gdb/gdbserver/mem-break.c | 44 +++++++++++++++++++++++++++++++-------------
 gdb/gdbserver/mem-break.h | 23 ++++++++++++-----------
 3 files changed, 54 insertions(+), 38 deletions(-)

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index b0af178..414117b 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -555,7 +555,7 @@ handle_extended_wait (struct lwp_info **orig_event_lwp, int wstat)
 		 hit it, so uninsert reinsert breakpoints from parent
 		 (and child).  Once vfork child is done, reinsert
 		 them back to parent.  */
-	      uninsert_reinsert_breakpoints ();
+	      uninsert_reinsert_breakpoints (ptid_of_lwp (event_lwp));
 	      current_thread = saved_thread;
 	    }
 
@@ -597,11 +597,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.  */
@@ -656,13 +656,12 @@ 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);
 
 	  current_thread = event_thr;
-	  reinsert_reinsert_breakpoints ();
+	  reinsert_reinsert_breakpoints (ptid_of_lwp (event_lwp));
 	  current_thread = saved_thread;
 
-	  gdb_assert (has_reinsert_breakpoints (proc));
+	  gdb_assert (has_reinsert_breakpoints (event_thr));
 	}
 
       /* Report the event.  */
@@ -2559,11 +2558,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;
     }
 }
@@ -4153,7 +4150,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);
 }
@@ -4296,7 +4293,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)
@@ -4789,8 +4786,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 (ptid_of_lwp (lwp));
 	}
 
       step_over_bkpt = null_ptid;
diff --git a/gdb/gdbserver/mem-break.c b/gdb/gdbserver/mem-break.c
index 6a38e6a..e85309d 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,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 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 (ptid_t ptid)
 {
   struct process_info *proc = current_process ();
   struct breakpoint *bp, **bp_link;
 
+  gdb_assert (ptid_get_pid (current_ptid) == ptid_get_pid (ptid));
+
   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)->ptid, ptid))
 	{
 	  *bp_link = bp->next;
 	  release_breakpoint (proc, bp);
@@ -1578,14 +1588,15 @@ uninsert_all_breakpoints (void)
 }
 
 void
-uninsert_reinsert_breakpoints (void)
+uninsert_reinsert_breakpoints (ptid_t ptid)
 {
   struct process_info *proc = current_process ();
   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))
       {
 	gdb_assert (bp->raw->inserted > 0);
 
@@ -1642,8 +1653,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 +1663,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,14 +1691,15 @@ reinsert_all_breakpoints (void)
 }
 
 void
-reinsert_reinsert_breakpoints (void)
+reinsert_reinsert_breakpoints (ptid_t ptid)
 {
   struct process_info *proc = current_process ();
   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))
 	{
 	  gdb_assert (bp->raw->inserted > 0);
 
@@ -2113,7 +2128,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 +2189,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 +2219,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 d4af890..aff0c32 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 represented by PTID.  */
 
-void delete_reinsert_breakpoints (void);
+void delete_reinsert_breakpoints (ptid_t ptid);
 
-/* Reinsert all reinsert breakpoints of the current process.  */
+/* Reinsert all reinsert breakpoints of thread represented by PTID.  */
 
-void reinsert_reinsert_breakpoints (void);
+void reinsert_reinsert_breakpoints (ptid_t ptid);
 
-/* Uninsert all reinsert breakpoints of the current process.  This
-   still leaves the reinsert breakpoints in the table.  */
+/* Uninsert all reinsert breakpoints of thread represented by PTID.
+   This still leaves the reinsert breakpoints in the table.  */
 
-void uninsert_reinsert_breakpoints (void);
+void uninsert_reinsert_breakpoints (ptid_t ptid);
 
 /* 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

Re: [PATCH 09/12] Make reinsert_breakpoint thread specific

[Pedro Alves]

On 06/02/2016 10:30 AM, Yao Qi wrote:
> V2: rewrite commit log to make it easy to read,
>     "id" -> "ptid",

Thanks.

> 
> 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-05-20  Yao Qi  <yao.qi@linaro.org>
> 
> 	* mem-break.c (struct reinsert_breakpoint) <ptid>: New field.
> 	(set_reinsert_breakpoint): New parameter ptid.  Callers updated.
> 	(delete_reinsert_breakpoints): Likewise.
> 	(has_reinsert_breakpoints): Change parameter to thread.  Callers
> 	updated.
> 	(clone_one_breakpoint): 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/linux-low.c | 25 +++++++++++--------------
>  gdb/gdbserver/mem-break.c | 44 +++++++++++++++++++++++++++++++-------------
>  gdb/gdbserver/mem-break.h | 23 ++++++++++++-----------
>  3 files changed, 54 insertions(+), 38 deletions(-)
> 
> diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
> index b0af178..414117b 100644
> --- a/gdb/gdbserver/linux-low.c
> +++ b/gdb/gdbserver/linux-low.c
> @@ -555,7 +555,7 @@ handle_extended_wait (struct lwp_info **orig_event_lwp, int wstat)
>  		 hit it, so uninsert reinsert breakpoints from parent
>  		 (and child).  Once vfork child is done, reinsert
>  		 them back to parent.  */
> -	      uninsert_reinsert_breakpoints ();
> +	      uninsert_reinsert_breakpoints (ptid_of_lwp (event_lwp));
>  	      current_thread = saved_thread;

Would it work to pass down the thread pointer directly?

That would then allow pushing down the save current thread / restore
current thread juggling to uninsert_reinsert_breakpoints too.

>  	    }
>  
> @@ -597,11 +597,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;

Likewise here, ptid is the child_thr's ptid here, I believe.

>  
> -	      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.  */
> @@ -656,13 +656,12 @@ 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);
>  
>  	  current_thread = event_thr;
> -	  reinsert_reinsert_breakpoints ();
> +	  reinsert_reinsert_breakpoints (ptid_of_lwp (event_lwp));
>  	  current_thread = saved_thread;

Ditto.  Etc.

Thanks,
Pedro Alves

Re: [PATCH 09/12] Make reinsert_breakpoint thread specific

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

>> -	      uninsert_reinsert_breakpoints ();
>> +	      uninsert_reinsert_breakpoints (ptid_of_lwp (event_lwp));
>>  	      current_thread = saved_thread;
>
> Would it work to pass down the thread pointer directly?
>
> That would then allow pushing down the save current thread / restore
> current thread juggling to uninsert_reinsert_breakpoints too.

Yes, see the patch below,

-- 
Yao (齐尧)

From dd8c2dde049af0972d1da51506d990a97bf97193 Mon Sep 17 00:00:00 2001
From: Yao Qi <yao.qi@linaro.org>
Date: Thu, 19 May 2016 16:02:55 +0100
Subject: [PATCH] Make reinsert_breakpoint thread specific

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-14  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.

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index b0af178..60759c8 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.  */
@@ -2559,11 +2546,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;
     }
 }
@@ -4153,7 +4138,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);
 }
@@ -4296,7 +4281,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)
@@ -4789,8 +4774,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.  */

Re: [PATCH 09/12] Make reinsert_breakpoint thread specific

[Pedro Alves]

On 06/14/2016 01:52 PM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> writes:
> 
>>> -	      uninsert_reinsert_breakpoints ();
>>> +	      uninsert_reinsert_breakpoints (ptid_of_lwp (event_lwp));
>>>  	      current_thread = saved_thread;
>>
>> Would it work to pass down the thread pointer directly?
>>
>> That would then allow pushing down the save current thread / restore
>> current thread juggling to uninsert_reinsert_breakpoints too.
> 
> Yes, see the patch below,
> 

LGTM.

Thanks,
Pedro Alves

[PATCH 10/12] 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-05-25  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/gdbthread.h |  3 +++
 gdb/gdbserver/inferiors.c | 12 ++++++++++++
 gdb/gdbserver/linux-low.c |  8 ++++++--
 3 files changed, 21 insertions(+), 2 deletions(-)

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 4bea4fd..c15b2e1 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 414117b..6f32911 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -4143,10 +4143,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

Re: [PATCH 10/12] Switch current_thread to lwp's thread in install_software_single_step_breakpoints

[Pedro Alves]

On 06/02/2016 10:30 AM, Yao Qi wrote:
> 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-05-25  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.

OK.

Thanks,
Pedro Alves

[PATCH 05/12] Handle reinsert breakpoints for vforked child

[Yao Qi]

When a thread is doing step-over with reinsert breakpoint, and the
instruction executed is a syscall doing vfork, both parent and child
share the memory, so the reinsert breakpoint in the space is visible
to both of them.  Also, removing the reinsert breakpoints from the
child will effectively remove them from the parent.  We should
carefully manipulate reinsert breakpoints for both processes.

What we are doing here is that

 - uninsert reinsert breakpoints from the parent before cloning the
   breakpoint list.  We use "uninsert" instead of "remove", because
   we need to "reinsert" them back after vfork is done.  In fact,
   "uninsert" removes them from both child and parent process space.
 - reinsert breakpoints in parent process are still copied to child's
   breakpoint list,
 - remove them from child's breakpoint list as what we did for fork,
   at this point, reinsert breakpoints are removed from the child and
   the parent, but they are still tracked by the parent's breakpoint
   list,
 - once vfork is done, "reinsert" them back to the parent,

gdb/gdbserver:

2016-05-26  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
	the parent process when vfork is done.
	* mem-break.c (uninsert_reinsert_breakpoints): New function.
	(reinsert_reinsert_breakpoints): New function.
	* mem-break.h (uninsert_reinsert_breakpoints): Declare
	(reinsert_reinsert_breakpoints): Declare.

gdb/testsuite:

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

	* gdb.base/step-over-fork-1.exp: Extend the test for vfork.
---
 gdb/gdbserver/linux-low.c                   | 38 +++++++++++++++++++++++++----
 gdb/gdbserver/mem-break.c                   | 38 +++++++++++++++++++++++++++++
 gdb/gdbserver/mem-break.h                   |  9 +++++++
 gdb/testsuite/gdb.base/step-over-fork-1.exp |  2 +-
 4 files changed, 81 insertions(+), 6 deletions(-)

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index 3f2d08e..dd92e78 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -543,6 +543,22 @@ handle_extended_wait (struct lwp_info **orig_event_lwp, int wstat)
 
 	  parent_proc = get_thread_process (event_thr);
 	  child_proc->attached = parent_proc->attached;
+
+	  if (event_lwp->bp_reinsert != 0
+	      && 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;
+	    }
+
 	  clone_all_breakpoints (&child_proc->breakpoints,
 				 &child_proc->raw_breakpoints,
 				 parent_proc->breakpoints);
@@ -570,12 +586,12 @@ handle_extended_wait (struct lwp_info **orig_event_lwp, int wstat)
 	  event_lwp->status_pending = wstat;
 
 	  /* If the parent thread is doing step-over with reinsert
-	     breakpoints, the reinsert breakpoints are still in forked
-	     child's process space and cloned to its breakpoint list
-	     from the parent's.  Remove them from the child process.  */
+	     breakpoints, the list of reinsert breakpoints are cloned
+	     from the parent's.  Remove them from the child process.
+	     In case of vfork, we'll reinsert them back once vforked
+	     child is done.  */
 	  if (event_lwp->bp_reinsert != 0
-	      && can_software_single_step ()
-	      && event == PTRACE_EVENT_FORK)
+	      && can_software_single_step ())
 	    {
 	      struct thread_info *saved_thread = current_thread;
 
@@ -639,6 +655,18 @@ handle_extended_wait (struct lwp_info **orig_event_lwp, int wstat)
     {
       event_lwp->waitstatus.kind = TARGET_WAITKIND_VFORK_DONE;
 
+      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);
+
+	  current_thread = event_thr;
+	  reinsert_reinsert_breakpoints ();
+	  current_thread = saved_thread;
+
+	  gdb_assert (has_reinsert_breakpoints (proc));
+	}
+
       /* Report the event.  */
       return 0;
     }
diff --git a/gdb/gdbserver/mem-break.c b/gdb/gdbserver/mem-break.c
index c27e803..5c73326 100644
--- a/gdb/gdbserver/mem-break.c
+++ b/gdb/gdbserver/mem-break.c
@@ -1509,6 +1509,26 @@ uninsert_all_breakpoints (void)
       uninsert_raw_breakpoint (bp);
 }
 
+void
+uninsert_reinsert_breakpoints (void)
+{
+  struct process_info *proc = current_process ();
+  struct breakpoint *bp;
+
+  for (bp = proc->breakpoints; bp != NULL; bp = bp->next)
+    {
+    if (bp->type == reinsert_breakpoint)
+      {
+	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);
+      }
+    }
+}
+
 static void
 reinsert_raw_breakpoint (struct raw_breakpoint *bp)
 {
@@ -1589,6 +1609,24 @@ reinsert_all_breakpoints (void)
 }
 
 void
+reinsert_reinsert_breakpoints (void)
+{
+  struct process_info *proc = current_process ();
+  struct breakpoint *bp;
+
+  for (bp = proc->breakpoints; bp != NULL; bp = bp->next)
+    {
+      if (bp->type == reinsert_breakpoint)
+	{
+	  gdb_assert (bp->raw->inserted > 0);
+
+	  if (bp->raw->refcount == 1)
+	    reinsert_raw_breakpoint (bp->raw);
+	}
+    }
+}
+
+void
 check_breakpoints (CORE_ADDR stop_pc)
 {
   struct process_info *proc = current_process ();
diff --git a/gdb/gdbserver/mem-break.h b/gdb/gdbserver/mem-break.h
index b84dc1e..6a06c0c 100644
--- a/gdb/gdbserver/mem-break.h
+++ b/gdb/gdbserver/mem-break.h
@@ -158,6 +158,15 @@ void set_reinsert_breakpoint (CORE_ADDR stop_at);
 
 void delete_reinsert_breakpoints (void);
 
+/* Reinsert all reinsert breakpoints of the current process.  */
+
+void reinsert_reinsert_breakpoints (void);
+
+/* Uninsert all reinsert breakpoints of the current process.  This
+   still leaves the reinsert breakpoints in the table.  */
+
+void uninsert_reinsert_breakpoints (void);
+
 /* Reinsert breakpoints at WHERE (and change their status to
    inserted).  */
 
diff --git a/gdb/testsuite/gdb.base/step-over-fork-1.exp b/gdb/testsuite/gdb.base/step-over-fork-1.exp
index 4a34e61..6791c8f 100644
--- a/gdb/testsuite/gdb.base/step-over-fork-1.exp
+++ b/gdb/testsuite/gdb.base/step-over-fork-1.exp
@@ -101,6 +101,6 @@ proc test { syscall } {
     gdb_exit
 }
 
-foreach_with_prefix syscall { "fork" } {
+foreach_with_prefix syscall { "fork" "vfork" } {
     test $syscall
 }
-- 
1.9.1

Re: [PATCH 05/12] Handle reinsert breakpoints for vforked child

[Pedro Alves]

On 06/02/2016 10:30 AM, Yao Qi wrote:
> When a thread is doing step-over with reinsert breakpoint, and the
> instruction executed is a syscall doing vfork, both parent and child
> share the memory, so the reinsert breakpoint in the space is visible
> to both of them.  Also, removing the reinsert breakpoints from the
> child will effectively remove them from the parent.  We should
> carefully manipulate reinsert breakpoints for both processes.
> 
> What we are doing here is that
> 
>  - uninsert reinsert breakpoints from the parent before cloning the
>    breakpoint list.  We use "uninsert" instead of "remove", because
>    we need to "reinsert" them back after vfork is done.  In fact,
>    "uninsert" removes them from both child and parent process space.
>  - reinsert breakpoints in parent process are still copied to child's
>    breakpoint list,
>  - remove them from child's breakpoint list as what we did for fork,
>    at this point, reinsert breakpoints are removed from the child and
>    the parent, but they are still tracked by the parent's breakpoint
>    list,
>  - once vfork is done, "reinsert" them back to the parent,
> 
> gdb/gdbserver:
> 
> 2016-05-26  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
> 	the parent process when vfork is done.
> 	* mem-break.c (uninsert_reinsert_breakpoints): New function.
> 	(reinsert_reinsert_breakpoints): New function.
> 	* mem-break.h (uninsert_reinsert_breakpoints): Declare
> 	(reinsert_reinsert_breakpoints): Declare.
> 
> gdb/testsuite:
> 
> 2016-05-26  Yao Qi  <yao.qi@linaro.org>
> 
> 	* gdb.base/step-over-fork-1.exp: Extend the test for vfork.

OK.

Thanks,
Pedro Alves

[PATCH 08/12] Refactor clone_all_breakpoints

[Yao Qi]

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-05-20  Yao Qi  <yao.qi@linaro.org>

	* 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/linux-low.c |  4 +---
 gdb/gdbserver/mem-break.c | 15 ++++++++-------
 gdb/gdbserver/mem-break.h |  8 ++++----
 3 files changed, 13 insertions(+), 14 deletions(-)

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index dd92e78..b0af178 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..6a38e6a 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,
+		       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..d4af890 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,
+			    struct thread_info *parent_thread);
 
 #endif /* MEM_BREAK_H */
-- 
1.9.1

Re: [PATCH 08/12] Refactor clone_all_breakpoints

[Pedro Alves]

On 06/02/2016 10:30 AM, Yao Qi wrote:
> V2: pass parent thread instead of parent process to
>     clone_all_breakpoints,

Thanks.

>  
> -/* 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 thread_info *parent_thread)

Something that I failed to notice on v1:

Can you make parent_thread be const, like src_list was?
That helps make the src vs dest more obvious.

I think you'd just need to make get_thread_process take a
const pointer, which should be a two line change.

Otherwise looks good.

Thanks,
Pedro Alves

[PATCH 04/12] 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, its bp_reinsert is zero, but has
reinsert breakpoints, so the following assert is triggered.

gdb/gdbserver/linux-low.c:4292: A problem internal to GDBserver has been detected.^M
void linux_resume_one_lwp_throw(lwp_info*, int, int, siginfo_t*): Assertion `!has_reinsert_breakpoints (proc)' failed.

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 forked child.

gdb/testsuite:

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

	* gdb.base/step-over-fork-1.c: New file.
	* gdb.base/step-over-fork-1.exp: New file.
---
 gdb/gdbserver/linux-low.c                   |  21 ++++++
 gdb/testsuite/gdb.base/step-over-fork-1.c   |  51 +++++++++++++
 gdb/testsuite/gdb.base/step-over-fork-1.exp | 106 ++++++++++++++++++++++++++++
 3 files changed, 178 insertions(+)
 create mode 100644 gdb/testsuite/gdb.base/step-over-fork-1.c
 create mode 100644 gdb/testsuite/gdb.base/step-over-fork-1.exp

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index 95104d2..3f2d08e 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -569,6 +569,27 @@ 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 forked
+	     child's process space and cloned to its breakpoint list
+	     from the parent's.  Remove them from the child process.  */
+	  if (event_lwp->bp_reinsert != 0
+	      && can_software_single_step ()
+	      && event == PTRACE_EVENT_FORK)
+	    {
+	      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;
 	}
diff --git a/gdb/testsuite/gdb.base/step-over-fork-1.c b/gdb/testsuite/gdb.base/step-over-fork-1.c
new file mode 100644
index 0000000..2ca145b
--- /dev/null
+++ b/gdb/testsuite/gdb.base/step-over-fork-1.c
@@ -0,0 +1,51 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2011-2016 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include <stdlib.h>
+#include <unistd.h>
+
+static void
+marker (void)
+{}
+
+int
+main (void)
+{
+  int  pid;
+
+  pid = FUNC ();
+  if (pid == 0) /* child */
+    {
+      exit (0);
+    }
+  else
+    {
+    }
+
+  marker ();
+
+  pid = FUNC ();
+  if (pid == 0) /* child */
+    {
+      marker ();
+      exit (0);
+    }
+  else
+    {
+      marker ();
+    }
+}
diff --git a/gdb/testsuite/gdb.base/step-over-fork-1.exp b/gdb/testsuite/gdb.base/step-over-fork-1.exp
new file mode 100644
index 0000000..4a34e61
--- /dev/null
+++ b/gdb/testsuite/gdb.base/step-over-fork-1.exp
@@ -0,0 +1,106 @@
+#   Copyright 2016 Free Software Foundation, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Test a thread is stepping over a syscall instruction which does
+# fork, and GDB (or GDBserver) can update its state on both parent
+# process and child process correctly.
+
+standard_testfile
+
+proc test { syscall } {
+    global testfile srcfile
+    global gdb_prompt
+    global decimal hex
+
+    if {[prepare_for_testing $testfile.exp $testfile $srcfile "debug additional_flags=-DFUNC=$syscall"]} {
+	untested $testfile.exp
+	return -1
+    }
+
+    # Start with a fresh gdb.
+    clean_restart ${testfile}
+    if ![runto_main] {
+	fail "Can't run to main"
+	return -1
+    }
+
+    # Step 1, find the syscall instruction address by "catch $syscall".
+    # Verify that the system supports "catch $syscall".
+    gdb_test "catch $syscall" "Catchpoint \[0-9\]* \\($syscall\\)" \
+	"insert first catchpoint"
+    set test "continue to first catchpoint"
+    gdb_test_multiple "continue" $test {
+	-re ".*Your system does not support this type\r\nof catchpoint.*$gdb_prompt $" {
+	    unsupported "catchpoint $syscall"
+	    return
+	}
+	-re ".*Catchpoint.*$gdb_prompt $" {
+	    pass $test
+	}
+    }
+
+    set syscall_insn_addr ""
+    set test "get syscall_insn_addr"
+    gdb_test_multiple "disassemble \$pc - 20,+30" $test {
+	-re " ($hex)\[^\r\n\]+\r\n=> .*$gdb_prompt $" {
+	    set syscall_insn_addr $expect_out(1,string)
+	    pass $test
+	}
+    }
+
+    if { $syscall_insn_addr == "" } {
+	fail $test
+	return
+    }
+
+    delete_breakpoints
+
+    gdb_test "break marker"
+
+    gdb_test "continue" "Continuing\\..*Breakpoint $decimal, .*" \
+	"continue to marker (1)"
+
+    gdb_test "set follow-fork-mode child"
+    gdb_test "set detach-on-fork off"
+
+    set test "set breakpoint condition-evaluation target"
+    gdb_test_multiple $test $test {
+	-re "warning: Target does not support breakpoint condition evaluation.\r\nUsing host evaluation mode instead.\r\n$gdb_prompt $" {
+	    # Target doesn't support breakpoint condition
+	    # evaluation on its side.
+	}
+	-re "^$test\r\n$gdb_prompt $" {
+	}
+    }
+
+    # Create a breakpoint which evaluates false.
+    gdb_test "break \*$syscall_insn_addr if main == 0" \
+	"Breakpoint \[0-9\]* at .*"
+
+    gdb_test "continue" "Continuing\\..*Breakpoint $decimal, marker \\(\\).*" \
+	"continue to marker (2)"
+
+    gdb_test "continue" "exited normally.*" "continue to end of inf 2"
+
+    gdb_test "inferior 1"
+    gdb_test "continue" \
+	"Continuing\\..*Breakpoint $decimal, marker \\(\\).*" \
+	"continue to marker (3)"
+    gdb_exit
+}
+
+foreach_with_prefix syscall { "fork" } {
+    test $syscall
+}
-- 
1.9.1

Re: [PATCH 04/12] Delete reinsert breakpoints from forked child

[Pedro Alves]

The gdbserver hunk looks fine to me.  Comments on the test below.

On 06/02/2016 10:30 AM, Yao Qi wrote:

> +    set syscall_insn_addr ""
> +    set test "get syscall_insn_addr"
> +    gdb_test_multiple "disassemble \$pc - 20,+30" $test {

Hmm, "\$pc - 20" doesn't look right for e.g., x86 with variable
length instructions.  I think that can well start disassembling
in the middle of an instruction, and produce garbage.

> +	-re " ($hex)\[^\r\n\]+\r\n=> .*$gdb_prompt $" {
> +	    set syscall_insn_addr $expect_out(1,string)
> +	    pass $test
> +	}
> +    }
> +
> +    if { $syscall_insn_addr == "" } {
> +	fail $test
> +	return
> +    }
> +
> +    delete_breakpoints
> +
> +    gdb_test "break marker"
> +
> +    gdb_test "continue" "Continuing\\..*Breakpoint $decimal, .*" \
> +	"continue to marker (1)"

No " ($foo)".

> +    set test "set breakpoint condition-evaluation target"
> +    gdb_test_multiple $test $test {
> +	-re "warning: Target does not support breakpoint condition evaluation.\r\nUsing host evaluation mode instead.\r\n$gdb_prompt $" {
> +	    # Target doesn't support breakpoint condition
> +	    # evaluation on its side.
> +	}
> +	-re "^$test\r\n$gdb_prompt $" {
> +	}
> +    }

No pass call?

> +    # Create a breakpoint which evaluates false.
> +    gdb_test "break \*$syscall_insn_addr if main == 0" \
> +	"Breakpoint \[0-9\]* at .*"

This ends up with "$syscall_insn_addr" in the test message.


I'm thinking that it might be good for these tests to also have
a displaced-stepping on/off test axis.  Or better still:

 out-of-line-step-over-bp / in-line-step-over-bp / plain-single-step

with the single-step variant doing a single-step over the
syscall instruction, with no breakpoint at PC at all.

Thanks,
Pedro Alves

Re: [PATCH 04/12] Delete reinsert breakpoints from forked child

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

> Hmm, "\$pc - 20" doesn't look right for e.g., x86 with variable
> length instructions.  I think that can well start disassembling
> in the middle of an instruction, and produce garbage.
>

I thought 20 is big enough to include the previous instruction in.
The max instruction length of x86 is 16.  If we disassemble in the
middle of an instruction, and garbage is printed, it is a bug, and we
should fix in disassembler.

I can't find another way to show the previous instruction.

>> +	-re " ($hex)\[^\r\n\]+\r\n=> .*$gdb_prompt $" {
>> +	    set syscall_insn_addr $expect_out(1,string)
>> +	    pass $test
>> +	}
>> +    }
>> +
>> +    if { $syscall_insn_addr == "" } {
>> +	fail $test
>> +	return
>> +    }
>> +
>> +    delete_breakpoints
>> +
>> +    gdb_test "break marker"
>> +
>> +    gdb_test "continue" "Continuing\\..*Breakpoint $decimal, .*" \
>> +	"continue to marker (1)"
>
> No " ($foo)".
>

I'll fix it.

>> +    set test "set breakpoint condition-evaluation target"
>> +    gdb_test_multiple $test $test {
>> + -re "warning: Target does not support breakpoint condition
>> evaluation.\r\nUsing host evaluation mode instead.\r\n$gdb_prompt $"
>> {
>> +	    # Target doesn't support breakpoint condition
>> +	    # evaluation on its side.
>> +	}
>> +	-re "^$test\r\n$gdb_prompt $" {
>> +	}
>> +    }
>
> No pass call?
>

I'll fix it.

>> +    # Create a breakpoint which evaluates false.
>> +    gdb_test "break \*$syscall_insn_addr if main == 0" \
>> +	"Breakpoint \[0-9\]* at .*"
>
> This ends up with "$syscall_insn_addr" in the test message.
>
>
> I'm thinking that it might be good for these tests to also have
> a displaced-stepping on/off test axis.  Or better still:
>
>  out-of-line-step-over-bp / in-line-step-over-bp / plain-single-step
>

What is difference between the second one and third one?  I think
they've already covered by gdb.base/step-over-syscall.exp.

> with the single-step variant doing a single-step over the
> syscall instruction, with no breakpoint at PC at all.

-- 
Yao (齐尧)

Re: [PATCH 04/12] Delete reinsert breakpoints from forked child

[Pedro Alves]

On 06/13/2016 05:53 PM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> writes:
> 
>> Hmm, "\$pc - 20" doesn't look right for e.g., x86 with variable
>> length instructions.  I think that can well start disassembling
>> in the middle of an instruction, and produce garbage.
>>
> 
> I thought 20 is big enough to include the previous instruction in.
> The max instruction length of x86 is 16.  If we disassemble in the
> middle of an instruction, 

Yes, we do.

> and garbage is printed, 

Yes, it does.  Easily confirmed:

(gdb) disassemble $pc,+10
Dump of assembler code from 0x7ffff78c55bb to 0x7ffff78c55c5:
=> 0x00007ffff78c55bb <__libc_fork+187>:        cmp    $0xfffffffffffff000,%rax
   0x00007ffff78c55c1 <__libc_fork+193>:        ja     0x7ffff78c5716 <__libc_fork+534>
End of assembler dump.
(gdb) disassemble $pc-1,+10
Dump of assembler code from 0x7ffff78c55ba to 0x7ffff78c55c4:
   0x00007ffff78c55ba <__libc_fork+186>:        add    $0xf0003d48,%eax
   0x00007ffff78c55bf <__libc_fork+191>:        (bad)  
   0x00007ffff78c55c0 <__libc_fork+192>:        decl   (%rdi)
   0x00007ffff78c55c2 <__libc_fork+194>:        xchg   %ecx,0x1(%rdi)
End of assembler dump.
(gdb) 

> it is a bug, and we should fix in disassembler.

The problem is that with variable length instructions,
there's no way to tell where an instruction starts by going
backwards...  All you can do is disassemble forward from some
known instruction boundary.  "disassemble" without a closed range
uses the the function's address to know where to start.

> I can't find another way to show the previous instruction.

Now that the negative repeat count for 'x' patch [1] is in,
you can just do "x/-i $pc".  Maybe "disassemble" could learn
to find boundaries similarly.  

But that x/-i trick only works if the code has line info available,
which you won't for _exit, unless you have debug info for glibc
installed.  Maybe better is to just do "disassemble", with no args,
which disassembles the whole function.

Or do it like step-over-syscall.exp does.

[1] https://sourceware.org/ml/gdb-patches/2016-06/msg00021.html

>> I'm thinking that it might be good for these tests to also have
>> a displaced-stepping on/off test axis.  Or better still:
>>
>>  out-of-line-step-over-bp / in-line-step-over-bp / plain-single-step
>>
> 
> What is difference between the second one and third one?  

As I mention in the quoted sentence below, the last one
would single-step the instruction with no breakpoint
installed.  The second one would have a breakpoint at PC,
which forces a step-over operation.  With displaced step
off, that'd be an in-line step over.  Thus the second one stops
all threads (and thus requires restarting them), while the
third one doesn't.

> I think
> they've already covered by gdb.base/step-over-syscall.exp.

In that case, shouldn't we be extending that test instead?

> 
>> with the single-step variant doing a single-step over the
>> syscall instruction, with no breakpoint at PC at all.
> 

Thanks,
Pedro Alves

Re: [PATCH 04/12] Delete reinsert breakpoints from forked child

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

>> I can't find another way to show the previous instruction.
>
> Now that the negative repeat count for 'x' patch [1] is in,
> you can just do "x/-i $pc".  Maybe "disassemble" could learn
> to find boundaries similarly.  
>
> But that x/-i trick only works if the code has line info available,
> which you won't for _exit, unless you have debug info for glibc
> installed.  Maybe better is to just do "disassemble", with no args,
> which disassembles the whole function.

Yes, "disassemble" without args can do that.

>
> Or do it like step-over-syscall.exp does.
>
> [1] https://sourceware.org/ml/gdb-patches/2016-06/msg00021.html
>

I tried to avoid that approach in step-over-syscall.exp, because it may
take so many instructions to single-step from fork () to the syscall
instruction (due to spin lock in the execution path, I suspect), and
test takes too long.  I'll fix step-over-syscall.exp separately.

>>> I'm thinking that it might be good for these tests to also have
>>> a displaced-stepping on/off test axis.  Or better still:
>>>
>>>  out-of-line-step-over-bp / in-line-step-over-bp / plain-single-step
>>>
>> 
>> What is difference between the second one and third one?  
>
> As I mention in the quoted sentence below, the last one
> would single-step the instruction with no breakpoint
> installed.  The second one would have a breakpoint at PC,
> which forces a step-over operation.  With displaced step
> off, that'd be an in-line step over.  Thus the second one stops
> all threads (and thus requires restarting them), while the
> third one doesn't.
>
>> I think
>> they've already covered by gdb.base/step-over-syscall.exp.
>
> In that case, shouldn't we be extending that test instead?

OK, I extend step-over-syscall.exp by setting different
combinations of follow-fork and detach-on-fork modes, and it still can
trigger GDBserver internal errors.  How about the patch below?  It
should be patch 5.5, after the bug fix patches (4 and 5) in this series.

-- 
Yao (齐尧)

From a48fbab0961b5112bafd199f0ad298715a5e8eff Mon Sep 17 00:00:00 2001
From: Yao Qi <yao.qi@linaro.org>
Date: Tue, 14 Jun 2016 11:32:01 +0100
Subject: [PATCH] Extend step-over-syscall.exp with different detach-on-fork
 and follow-fork modes

This patch extends step-over-syscall.exp by setting different values to
detach-on-fork and follow-fork.

gdb/testsuite:

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

	* gdb.base/step-over-syscall.exp (break_cond_on_syscall): New
	parameters follow_fork and detach_on_fork.  Set follow-fork-mode
	and detach-on-fork.  Adjust tests.
	(top level): Invoke break_cond_on_syscall with combinations of
	syscall, follow-fork-mode and detach-on-fork.

diff --git a/gdb/testsuite/gdb.base/step-over-syscall.exp b/gdb/testsuite/gdb.base/step-over-syscall.exp
index 7e5a719..2809004 100644
--- a/gdb/testsuite/gdb.base/step-over-syscall.exp
+++ b/gdb/testsuite/gdb.base/step-over-syscall.exp
@@ -176,9 +176,11 @@ proc step_over_syscall { syscall } {
 
 # Set a breakpoint with a condition that evals false on syscall
 # instruction.  In fact, it tests GDBserver steps over syscall
-# instruction.
+# instruction.  SYSCALL is the syscall the program calls.
+# FOLLOW_FORK is either "parent" or "child".  DETACH_ON_FORK is
+# "on" or "off".
 
-proc break_cond_on_syscall { syscall } {
+proc break_cond_on_syscall { syscall follow_fork detach_on_fork } {
     with_test_prefix "break cond on target : $syscall" {
 	set testfile "step-over-$syscall"
 
@@ -195,6 +197,8 @@ proc break_cond_on_syscall { syscall } {
 	# Delete breakpoint syscall insns to avoid interference with other syscalls.
 	delete_breakpoints
 
+	gdb_test "set follow-fork-mode $follow_fork"
+	gdb_test "set detach-on-fork $detach_on_fork"
 
 	# Create a breakpoint with a condition that evals false.
 	gdb_test "break \*$syscall_insn_addr if main == 0" \
@@ -212,9 +216,27 @@ proc break_cond_on_syscall { syscall } {
 	    gdb_test "break clone_fn if main == 0"
 	}
 
-	gdb_test "break marker" "Breakpoint.*at.* file .*${testfile}.c, line.*"
-	gdb_test "continue" "Continuing\\..*Breakpoint \[0-9\]+, marker \\(\\) at.*" \
-	    "continue to marker ($syscall)"
+	if { $syscall == "clone" } {
+	    # follow-fork and detach-on-fork only make sense to
+	    # fork and vfork.
+	    gdb_test "break marker" "Breakpoint.*at.* file .*${testfile}.c, line.*"
+	    gdb_test "continue" "Continuing\\..*Breakpoint \[0-9\]+, marker \\(\\) at.*" \
+		"continue to marker"
+	} else {
+	    if { $follow_fork == "child" } {
+		gdb_test "continue" "exited normally.*" "continue to end of inf 2"
+		if { $detach_on_fork == "off" } {
+		    gdb_test "inferior 1"
+		    gdb_test "break marker" "Breakpoint.*at.*"
+		    gdb_test "continue" "Continuing\\..*Breakpoint \[0-9\]+, marker \\(\\) at.*" \
+			"continue to marker"
+		}
+	    } else {
+		gdb_test "break marker" "Breakpoint.*at.* file .*${testfile}.c, line.*"
+		gdb_test "continue" "Continuing\\..*Breakpoint \[0-9\]+, marker \\(\\) at.*" \
+		    "continue to marker"
+	    }
+	}
     }
 }
 
@@ -243,7 +265,21 @@ gdb_test_multiple $test $test {
 }
 
 if { $cond_bp_target } {
-    break_cond_on_syscall "fork"
-    break_cond_on_syscall "vfork"
-    break_cond_on_syscall "clone"
+
+    foreach_with_prefix detach-on-fork {"on" "off"} {
+	foreach_with_prefix follow-fork {"parent" "child"} {
+	    foreach syscall { "fork" "vfork" "clone" } {
+
+		if { $syscall != "vfork"
+		     || ${follow-fork} != "parent"
+		     || ${detach-on-fork} != "off" } {
+		    # Both vforked child process and parent process are
+		    # under GDB's control, but GDB follows the parent
+		    # process only, which can't be run until vforked child
+		    # finishes.  Skip the test in this scenario.
+		    break_cond_on_syscall $syscall ${follow-fork} ${detach-on-fork}
+		}
+	    }
+	}
+    }
 }

Re: [PATCH 04/12] Delete reinsert breakpoints from forked child

[Pedro Alves]

On 06/14/2016 12:16 PM, Yao Qi wrote:

>> >
>>> >> I think
>>> >> they've already covered by gdb.base/step-over-syscall.exp.
>> >
>> > In that case, shouldn't we be extending that test instead?
> OK, I extend step-over-syscall.exp by setting different
> combinations of follow-fork and detach-on-fork modes, and it still can
> trigger GDBserver internal errors.  How about the patch below?  

Fine with me.  One suggestion below.

> +    foreach_with_prefix detach-on-fork {"on" "off"} {
> +	foreach_with_prefix follow-fork {"parent" "child"} {
> +	    foreach syscall { "fork" "vfork" "clone" } {
> +
> +		if { $syscall != "vfork"
> +		     || ${follow-fork} != "parent"
> +		     || ${detach-on-fork} != "off" } {
> +		    # Both vforked child process and parent process are
> +		    # under GDB's control, but GDB follows the parent
> +		    # process only, which can't be run until vforked child
> +		    # finishes.  Skip the test in this scenario.
> +		    break_cond_on_syscall $syscall ${follow-fork} ${detach-on-fork}
> +		}

I'd suggest reversing the condition logic, making it match the
comment, like this:

		if { $syscall == "vfork"
		     && ${follow-fork} == "parent"
		     && ${detach-on-fork} == "off" } {
		    # Both vforked child process and parent process are
		    # under GDB's control, but GDB follows the parent
		    # process only, which can't be run until vforked child
		    # finishes.  Skip the test in this scenario.
                    continue
		}

	        break_cond_on_syscall $syscall ${follow-fork} ${detach-on-fork}


This way makes it easier to add more "skip" conditions in the future too.  E.g.,

		if { $syscall == "vfork"
		     && ${follow-fork} == "parent"
		     && ${detach-on-fork} == "off" } {
		    # Both vforked child process and parent process are
		    # under GDB's control, but GDB follows the parent
		    # process only, which can't be run until vforked child
		    # finishes.  Skip the test in this scenario.
                    continue
		}

+		if { $syscall == "whatnot"
+		     && ${follow-fork} == "parent"} {
+		    # For whatever reason, this shouldn't be tested either.
+                   continue
+		}

	        break_cond_on_syscall $syscall ${follow-fork} ${detach-on-fork}

> +	    }
> +	}
> +    }
>  }

Thanks,
Pedro Alves

Re: [PATCH 04/12] Delete reinsert breakpoints from forked child

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

> I'd suggest reversing the condition logic, making it match the
> comment, like this:
>
> 		if { $syscall == "vfork"
> 		     && ${follow-fork} == "parent"
> 		     && ${detach-on-fork} == "off" } {
> 		    # Both vforked child process and parent process are
> 		    # under GDB's control, but GDB follows the parent
> 		    # process only, which can't be run until vforked child
> 		    # finishes.  Skip the test in this scenario.
>                     continue
> 		}
>
> 	        break_cond_on_syscall $syscall ${follow-fork} ${detach-on-fork}

OK, revert the condition logic, and patch below is pushed in.

-- 
Yao (齐尧)

From 21a770913c24ab085fe66a5274ebe7cf9e031982 Mon Sep 17 00:00:00 2001
From: Yao Qi <yao.qi@linaro.org>
Date: Fri, 17 Jun 2016 10:25:13 +0100
Subject: [PATCH] Extend step-over-syscall.exp with different detach-on-fork
 and follow-fork modes

This patch extends step-over-syscall.exp by setting different values to
detach-on-fork and follow-fork.

gdb/testsuite:

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

	* gdb.base/step-over-syscall.exp (break_cond_on_syscall): New
	parameters follow_fork and detach_on_fork.  Set follow-fork-mode
	and detach-on-fork.  Adjust tests.
	(top level): Invoke break_cond_on_syscall with combinations of
	syscall, follow-fork-mode and detach-on-fork.

diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index fbdcd2b..69aad53 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,5 +1,13 @@
 2016-06-17  Yao Qi  <yao.qi@linaro.org>
 
+	* gdb.base/step-over-syscall.exp (break_cond_on_syscall): New
+	parameters follow_fork and detach_on_fork.  Set follow-fork-mode
+	and detach-on-fork.  Adjust tests.
+	(top level): Invoke break_cond_on_syscall with combinations of
+	syscall, follow-fork-mode and detach-on-fork.
+
+2016-06-17  Yao Qi  <yao.qi@linaro.org>
+
 	* gdb.base/step-over-exit.c: New.
 	* gdb.base/step-over-exit.exp: New.
 
diff --git a/gdb/testsuite/gdb.base/step-over-syscall.exp b/gdb/testsuite/gdb.base/step-over-syscall.exp
index 7e5a719..e1d5ba1 100644
--- a/gdb/testsuite/gdb.base/step-over-syscall.exp
+++ b/gdb/testsuite/gdb.base/step-over-syscall.exp
@@ -176,9 +176,11 @@ proc step_over_syscall { syscall } {
 
 # Set a breakpoint with a condition that evals false on syscall
 # instruction.  In fact, it tests GDBserver steps over syscall
-# instruction.
+# instruction.  SYSCALL is the syscall the program calls.
+# FOLLOW_FORK is either "parent" or "child".  DETACH_ON_FORK is
+# "on" or "off".
 
-proc break_cond_on_syscall { syscall } {
+proc break_cond_on_syscall { syscall follow_fork detach_on_fork } {
     with_test_prefix "break cond on target : $syscall" {
 	set testfile "step-over-$syscall"
 
@@ -195,6 +197,8 @@ proc break_cond_on_syscall { syscall } {
 	# Delete breakpoint syscall insns to avoid interference with other syscalls.
 	delete_breakpoints
 
+	gdb_test "set follow-fork-mode $follow_fork"
+	gdb_test "set detach-on-fork $detach_on_fork"
 
 	# Create a breakpoint with a condition that evals false.
 	gdb_test "break \*$syscall_insn_addr if main == 0" \
@@ -212,9 +216,27 @@ proc break_cond_on_syscall { syscall } {
 	    gdb_test "break clone_fn if main == 0"
 	}
 
-	gdb_test "break marker" "Breakpoint.*at.* file .*${testfile}.c, line.*"
-	gdb_test "continue" "Continuing\\..*Breakpoint \[0-9\]+, marker \\(\\) at.*" \
-	    "continue to marker ($syscall)"
+	if { $syscall == "clone" } {
+	    # follow-fork and detach-on-fork only make sense to
+	    # fork and vfork.
+	    gdb_test "break marker" "Breakpoint.*at.* file .*${testfile}.c, line.*"
+	    gdb_test "continue" "Continuing\\..*Breakpoint \[0-9\]+, marker \\(\\) at.*" \
+		"continue to marker"
+	} else {
+	    if { $follow_fork == "child" } {
+		gdb_test "continue" "exited normally.*" "continue to end of inf 2"
+		if { $detach_on_fork == "off" } {
+		    gdb_test "inferior 1"
+		    gdb_test "break marker" "Breakpoint.*at.*"
+		    gdb_test "continue" "Continuing\\..*Breakpoint \[0-9\]+, marker \\(\\) at.*" \
+			"continue to marker"
+		}
+	    } else {
+		gdb_test "break marker" "Breakpoint.*at.* file .*${testfile}.c, line.*"
+		gdb_test "continue" "Continuing\\..*Breakpoint \[0-9\]+, marker \\(\\) at.*" \
+		    "continue to marker"
+	    }
+	}
     }
 }
 
@@ -243,7 +265,22 @@ gdb_test_multiple $test $test {
 }
 
 if { $cond_bp_target } {
-    break_cond_on_syscall "fork"
-    break_cond_on_syscall "vfork"
-    break_cond_on_syscall "clone"
+
+    foreach_with_prefix detach-on-fork {"on" "off"} {
+	foreach_with_prefix follow-fork {"parent" "child"} {
+	    foreach syscall { "fork" "vfork" "clone" } {
+
+		if { $syscall == "vfork"
+		     && ${follow-fork} == "parent"
+		     && ${detach-on-fork} == "off" } {
+		    # Both vforked child process and parent process are
+		    # under GDB's control, but GDB follows the parent
+		    # process only, which can't be run until vforked child
+		    # finishes.  Skip the test in this scenario.
+		    continue
+		}
+		break_cond_on_syscall $syscall ${follow-fork} ${detach-on-fork}
+	    }
+	}
+    }
 }

[PATCH 01/12] Switch to current thread in finish_step_over

[Yao Qi]

V2: move current_thread switch into finsh_step_over

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 thread we are doing step-over
in complete_ongoing_step_over.

gdb/gdbserver:

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

	* linux-low.c (maybe_hw_step): New function.
	(linux_resume_one_lwp_throw): Call maybe_hw_step.
	(finish_step_over): Switch current_thread to lwp temporarily,
	and assert has_reinsert_breakpoints returns true.
	(proceed_one_lwp): Call maybe_hw_step.
	* mem-break.c (has_reinsert_breakpoints): New function.
	* mem-break.h (has_reinsert_breakpoints): Declare.
---
 gdb/gdbserver/linux-low.c | 35 +++++++++++++++++++++++++++++++----
 gdb/gdbserver/mem-break.c | 22 ++++++++++++++++++++++
 gdb/gdbserver/mem-break.h |  4 ++++
 3 files changed, 57 insertions(+), 4 deletions(-)

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index 81134b0..77c296c 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -2495,6 +2495,24 @@ linux_low_filter_event (int lwpid, int wstat)
   return child;
 }
 
+/* Return true if THREAD is doing hardware single step.  */
+
+static int
+maybe_hw_step (struct thread_info *thread)
+{
+  if (can_hardware_single_step ())
+    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));
+      return 0;
+    }
+}
+
 /* Resume LWPs that are currently stopped without any pending status
    to report, but are resumed from the core's perspective.  */
 
@@ -4215,9 +4233,9 @@ linux_resume_one_lwp_throw (struct lwp_info *lwp,
 		fprintf (stderr, "BAD - reinserting and suspended(%d).\n",
 			 lwp->suspended);
 	    }
-
-	  step = 1;
 	}
+
+      step = maybe_hw_step (thread);
     }
 
   if (fast_tp_collecting == 1)
@@ -4689,9 +4707,13 @@ finish_step_over (struct lwp_info *lwp)
 {
   if (lwp->bp_reinsert != 0)
     {
+      struct thread_info *saved_thread = current_thread;
+
       if (debug_threads)
 	debug_printf ("Finished step over.\n");
 
+      current_thread = get_lwp_thread (lwp);
+
       /* Reinsert any breakpoint at LWP->BP_REINSERT.  Note that there
 	 may be no breakpoint to reinsert there by now.  */
       reinsert_breakpoints_at (lwp->bp_reinsert);
@@ -4705,9 +4727,13 @@ 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;
+      current_thread = saved_thread;
       return 1;
     }
   else
@@ -5029,7 +5055,8 @@ 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;
+
+      step = maybe_hw_step (thread);
     }
   else
     step = 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

Re: [PATCH 01/12] Switch to current thread in finish_step_over

[Pedro Alves]

On 06/02/2016 10:30 AM, Yao Qi wrote:
> V2: move current_thread switch into finsh_step_over

Thanks.

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

^ This sentence is now stale.

> gdb/gdbserver:
> 
> 2016-05-31  Yao Qi  <yao.qi@linaro.org>
> 
> 	* linux-low.c (maybe_hw_step): New function.
> 	(linux_resume_one_lwp_throw): Call maybe_hw_step.
> 	(finish_step_over): Switch current_thread to lwp temporarily,
> 	and assert has_reinsert_breakpoints returns true.
> 	(proceed_one_lwp): Call maybe_hw_step.
> 	* mem-break.c (has_reinsert_breakpoints): New function.
> 	* mem-break.h (has_reinsert_breakpoints): Declare.

Otherwise OK.

Thanks,
Pedro Alves

[PATCH 03/12] Step over exit with reinsert breakpoints

[Yao Qi]

This patch fixes a GDBserver crash when one thread is stepping over
a syscall instruction which is exit.  Step-over isn't finished due
to the exit, but GDBserver doesn't clean up the state of step-over,
so in the wait next time, GDBserver will wait on step_over_bkpt,
which is already exited, and GDBserver crashes because
'requested_child' is NULL.  See gdbserver logs below,

Need step over [LWP 14858]? yes, found breakpoint at 0x2aaaaad91307^M
proceed_all_lwps: found thread 14858 needing a step-over^M
Starting step-over on LWP 14858.  Stopping all threads^M
>>>> entering void stop_all_lwps(int, lwp_info*)
....
<<<< exiting void stop_all_lwps(int, lwp_info*)^M
Done stopping all threads for step-over.^M
pc is 0x2aaaaad91307^M
Writing 0f to 0x2aaaaad91307 in process 14858^M
Could not find fast tracepoint jump at 0x2aaaaad91307 in list (uninserting).^M
  pending reinsert at 0x2aaaaad91307^M
  step from pc 0x2aaaaad91307^M
Resuming lwp 14858 (step, signal 0, stop not expected)^M

 # Start step-over for LWP 14858

>>>> entering ptid_t linux_wait_1(ptid_t, target_waitstatus*, int)
....
LLFE: 14858 exited.
...
<<<< exiting ptid_t linux_wait_1(ptid_t, target_waitstatus*, int)

  # LWP 14858 exited
.....
>>>> entering ptid_t linux_wait_1(ptid_t, target_waitstatus*, int)^M
linux_wait_1: [<all threads>]^M
step_over_bkpt set [LWP 14858.14858], doing a blocking wait

  # but step_over_bkpt is still LWP 14858, which is wrong

The fix is to finish step-over if it is ongoing, and unsuspend other
threads.  Without the fix in linux-low.c, GDBserver will crash in
with running gdb.base/step-over-exit.exp.

gdb/gdbserver:

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

	* linux-low.c (unsuspend_all_lwps): Declare.
	(linux_low_filter_event): If thread exited, call finish_step_over.
	If step-over is finished, unsuspend other threads.

gdb/testsuite:

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

	* gdb.base/step-over-exit.c: New.
	* gdb.base/step-over-exit.exp: New.
---
 gdb/gdbserver/linux-low.c                 |   8 ++
 gdb/testsuite/gdb.base/step-over-exit.c   |  50 ++++++++++++
 gdb/testsuite/gdb.base/step-over-exit.exp | 127 ++++++++++++++++++++++++++++++
 3 files changed, 185 insertions(+)
 create mode 100644 gdb/testsuite/gdb.base/step-over-exit.c
 create mode 100644 gdb/testsuite/gdb.base/step-over-exit.exp

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index 5f025b5..95104d2 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -252,6 +252,7 @@ static void linux_resume_one_lwp (struct lwp_info *lwp,
 static void linux_resume (struct thread_resume *resume_info, size_t n);
 static void stop_all_lwps (int suspend, struct lwp_info *except);
 static void unstop_all_lwps (int unsuspend, struct lwp_info *except);
+static void unsuspend_all_lwps (struct lwp_info *except);
 static int linux_wait_for_event_filtered (ptid_t wait_ptid, ptid_t filter_ptid,
 					  int *wstat, int options);
 static int linux_wait_for_event (ptid_t ptid, int *wstat, int options);
@@ -2357,6 +2358,13 @@ linux_low_filter_event (int lwpid, int wstat)
     {
       if (debug_threads)
 	debug_printf ("LLFE: %d exited.\n", lwpid);
+
+      if (finish_step_over (child))
+	{
+	  /* Unsuspend all other LWPs, and set them back running again.  */
+	  unsuspend_all_lwps (child);
+	}
+
       /* If there is at least one more LWP, then the exit signal was
 	 not the end of the debugged application and should be
 	 ignored, unless GDB wants to hear about thread exits.  */
diff --git a/gdb/testsuite/gdb.base/step-over-exit.c b/gdb/testsuite/gdb.base/step-over-exit.c
new file mode 100644
index 0000000..fd0de71
--- /dev/null
+++ b/gdb/testsuite/gdb.base/step-over-exit.c
@@ -0,0 +1,50 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2016 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include <stdlib.h>
+#include <unistd.h>
+#include <string.h>
+
+static void
+marker (void)
+{}
+
+int
+main (void)
+{
+  int  pid;
+
+  pid = fork ();
+  if (pid == 0) /* child */
+    {
+      _exit (0);
+    }
+  else
+    {
+    }
+
+  pid = fork ();
+  if (pid == 0) /* child */
+    {
+      marker ();
+      _exit (0);
+    }
+  else
+    {
+      marker ();
+    }
+}
diff --git a/gdb/testsuite/gdb.base/step-over-exit.exp b/gdb/testsuite/gdb.base/step-over-exit.exp
new file mode 100644
index 0000000..9827cde
--- /dev/null
+++ b/gdb/testsuite/gdb.base/step-over-exit.exp
@@ -0,0 +1,127 @@
+#   Copyright 2016 Free Software Foundation, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+standard_testfile
+
+# Test a thread is doing step-over a syscall instruction which is exit,
+# and GDBserver should cleanup its state of step-over properly.
+
+set syscall_insn ""
+
+# Define the syscall instruction for each target.
+
+if { [istarget "i\[34567\]86-*-linux*"] || [istarget "x86_64-*-linux*"] } {
+    set syscall_insn "\[ \t\](int|syscall|sysenter)\[ \t\]"
+} elseif { [istarget "aarch64*-*-linux*"] || [istarget "arm*-*-linux*"] } {
+    set syscall_insn "\[ \t\](swi|svc)\[ \t\]"
+} else {
+    unsupported "unknown syscall instruction"
+    return -1
+}
+
+if {[prepare_for_testing $testfile.exp $testfile $srcfile debug]} {
+    untested $testfile.exp
+    return -1
+}
+
+# Start with a fresh gdb.
+clean_restart ${testfile}
+if ![runto_main] {
+    fail "Can't run to main"
+    return -1
+}
+
+gdb_test "set follow-fork-mode child"
+gdb_test "set detach-on-fork off"
+
+# Step 1, find the syscall instruction address.
+
+gdb_test "break _exit" "Breakpoint $decimal at .*"
+
+# Hit the breakpoint on _exit.  The address of syscall insn is recorded.
+
+gdb_test "continue" \
+    "Continuing\\..*Breakpoint $decimal.*_exit \\(.*\\).*" \
+    "continue to exit"
+
+gdb_test "display/i \$pc" ".*"
+
+# Single step until we see a syscall insn or we reach the
+# upper bound of loop iterations.
+set msg "find syscall insn in exit"
+set steps 0
+set max_steps 1000
+gdb_test_multiple "stepi" $msg {
+    -re ".*$syscall_insn.*$gdb_prompt $" {
+	pass $msg
+    }
+    -re "x/i .*=>.*\r\n$gdb_prompt $" {
+	incr steps
+	if {$steps == $max_steps} {
+	    fail $msg
+	} else {
+	    send_gdb "stepi\n"
+	    exp_continue
+	}
+    }
+}
+
+if {$steps == $max_steps} {
+    return
+}
+
+# Remove the display
+gdb_test_no_output "delete display 1"
+
+set syscall_insn_addr [get_hexadecimal_valueof "\$pc" "0"]
+
+gdb_test "continue" "exited normally.*" "continue to end (2)"
+gdb_test "inferior 1" ".*Switching to inferior 1.*" \
+    "switch back to inferior 1 (2)"
+
+delete_breakpoints
+
+gdb_test "break marker"
+
+gdb_test "continue" "Continuing\\..*Breakpoint $decimal, .*" \
+    "continue to marker (1)"
+
+# Step 2, create a breakpoint which evaluates false, and force it
+# evaluated on the target side.
+
+set test "set breakpoint condition-evaluation target"
+gdb_test_multiple $test $test {
+    -re "warning: Target does not support breakpoint condition evaluation.\r\nUsing host evaluation mode instead.\r\n$gdb_prompt $" {
+	# Target doesn't support breakpoint condition evaluation
+	# on its side, but it is no harm to run the test.
+    }
+    -re "^$test\r\n$gdb_prompt $" {
+    }
+}
+
+gdb_test "break \*$syscall_insn_addr if main == 0" \
+    "Breakpoint \[0-9\]* at .*"
+
+# Resume the child process, and the step-over is being done.
+
+gdb_test "continue" "exited normally.*" "continue to end (3)"
+gdb_test "inferior 1" ".*Switching to inferior 1.*" \
+    "switch back to inferior 1 (3)"
+
+# Switch back to the parent process, continue to the marker to
+# test GDBserver's state is still correct.
+
+gdb_test "continue" "Continuing\\..*Breakpoint $decimal, .*" \
+    "continue to marker (2)"
-- 
1.9.1

Re: [PATCH 03/12] Step over exit with reinsert breakpoints

[Pedro Alves]

On 06/02/2016 10:30 AM, Yao Qi wrote:

> +gdb_test "continue" "exited normally.*" "continue to end (2)"
> +gdb_test "inferior 1" ".*Switching to inferior 1.*" \
> +    "switch back to inferior 1 (2)"

Please don't use trailing " ($foo)" to differentiate tests (throughout).

Otherwise OK.

Thanks,
Pedro Alves

Re: [PATCH 03/12] Step over exit with reinsert breakpoints

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

>> +gdb_test "continue" "exited normally.*" "continue to end (2)"
>> +gdb_test "inferior 1" ".*Switching to inferior 1.*" \
>> +    "switch back to inferior 1 (2)"
>
> Please don't use trailing " ($foo)" to differentiate tests (throughout).

so, I use "first time" and "second time" to differentiate the test, like

  gdb_test "inferior 1" ".*Switching to inferior 1.*" \
    "switch back to inferior 1, second time"

-- 
Yao (齐尧)

Re: [PATCH 03/12] Step over exit with reinsert breakpoints

[Pedro Alves]

On 06/13/2016 03:52 PM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> writes:
> 
>>> +gdb_test "continue" "exited normally.*" "continue to end (2)"
>>> +gdb_test "inferior 1" ".*Switching to inferior 1.*" \
>>> +    "switch back to inferior 1 (2)"
>>
>> Please don't use trailing " ($foo)" to differentiate tests (throughout).
> 
> so, I use "first time" and "second time" to differentiate the test, like
> 
>   gdb_test "inferior 1" ".*Switching to inferior 1.*" \
>     "switch back to inferior 1, second time"

Yes, that's fine.

Thanks,
Pedro Alves

Re: [PATCH 03/12] Step over exit with reinsert breakpoints

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

>> so, I use "first time" and "second time" to differentiate the test, like
>> 
>>   gdb_test "inferior 1" ".*Switching to inferior 1.*" \
>>     "switch back to inferior 1, second time"
>
> Yes, that's fine.

Patch below is what I pushed in,

-- 
Yao (齐尧)

From f50bf8e5153e3cdddd1ad5d3f7d16f2b4e5adb3c Mon Sep 17 00:00:00 2001
From: Yao Qi <yao.qi@linaro.org>
Date: Fri, 17 Jun 2016 10:25:12 +0100
Subject: [PATCH] Step over exit with reinsert breakpoints

This patch fixes a GDBserver crash when one thread is stepping over
a syscall instruction which is exit.  Step-over isn't finished due
to the exit, but GDBserver doesn't clean up the state of step-over,
so in the wait next time, GDBserver will wait on step_over_bkpt,
which is already exited, and GDBserver crashes because
'requested_child' is NULL.  See gdbserver logs below,

Need step over [LWP 14858]? yes, found breakpoint at 0x2aaaaad91307^M
proceed_all_lwps: found thread 14858 needing a step-over^M
Starting step-over on LWP 14858.  Stopping all threads^M
>>>> entering void stop_all_lwps(int, lwp_info*)
....
<<<< exiting void stop_all_lwps(int, lwp_info*)^M
Done stopping all threads for step-over.^M
pc is 0x2aaaaad91307^M
Writing 0f to 0x2aaaaad91307 in process 14858^M
Could not find fast tracepoint jump at 0x2aaaaad91307 in list (uninserting).^M
  pending reinsert at 0x2aaaaad91307^M
  step from pc 0x2aaaaad91307^M
Resuming lwp 14858 (step, signal 0, stop not expected)^M

 # Start step-over for LWP 14858

>>>> entering ptid_t linux_wait_1(ptid_t, target_waitstatus*, int)
....
LLFE: 14858 exited.
...
<<<< exiting ptid_t linux_wait_1(ptid_t, target_waitstatus*, int)

  # LWP 14858 exited
.....
>>>> entering ptid_t linux_wait_1(ptid_t, target_waitstatus*, int)^M
linux_wait_1: [<all threads>]^M
step_over_bkpt set [LWP 14858.14858], doing a blocking wait

  # but step_over_bkpt is still LWP 14858, which is wrong

The fix is to finish step-over if it is ongoing, and unsuspend other
threads.  Without the fix in linux-low.c, GDBserver will crash in
with running gdb.base/step-over-exit.exp.

gdb/gdbserver:

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

	* linux-low.c (unsuspend_all_lwps): Declare.
	(linux_low_filter_event): If thread exited, call finish_step_over.
	If step-over is finished, unsuspend other threads.

gdb/testsuite:

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

	* gdb.base/step-over-exit.c: New.
	* gdb.base/step-over-exit.exp: New.

diff --git a/gdb/gdbserver/ChangeLog b/gdb/gdbserver/ChangeLog
index c7a9595..fd70f59 100644
--- a/gdb/gdbserver/ChangeLog
+++ b/gdb/gdbserver/ChangeLog
@@ -1,5 +1,11 @@
 2016-06-17  Yao Qi  <yao.qi@linaro.org>
 
+	* linux-low.c (unsuspend_all_lwps): Declare.
+	(linux_low_filter_event): If thread exited, call finish_step_over.
+	If step-over is finished, unsuspend other threads.
+
+2016-06-17  Yao Qi  <yao.qi@linaro.org>
+
 	* linux-low.c (linux_resume_one_lwp_throw): Assert
 	has_reinsert_breakpoints returns false.
 	* mem-break.c (delete_disabled_breakpoints): Assert
diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index 5f025b5..95104d2 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -252,6 +252,7 @@ static void linux_resume_one_lwp (struct lwp_info *lwp,
 static void linux_resume (struct thread_resume *resume_info, size_t n);
 static void stop_all_lwps (int suspend, struct lwp_info *except);
 static void unstop_all_lwps (int unsuspend, struct lwp_info *except);
+static void unsuspend_all_lwps (struct lwp_info *except);
 static int linux_wait_for_event_filtered (ptid_t wait_ptid, ptid_t filter_ptid,
 					  int *wstat, int options);
 static int linux_wait_for_event (ptid_t ptid, int *wstat, int options);
@@ -2357,6 +2358,13 @@ linux_low_filter_event (int lwpid, int wstat)
     {
       if (debug_threads)
 	debug_printf ("LLFE: %d exited.\n", lwpid);
+
+      if (finish_step_over (child))
+	{
+	  /* Unsuspend all other LWPs, and set them back running again.  */
+	  unsuspend_all_lwps (child);
+	}
+
       /* If there is at least one more LWP, then the exit signal was
 	 not the end of the debugged application and should be
 	 ignored, unless GDB wants to hear about thread exits.  */
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 3dad273..fbdcd2b 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,8 @@
+2016-06-17  Yao Qi  <yao.qi@linaro.org>
+
+	* gdb.base/step-over-exit.c: New.
+	* gdb.base/step-over-exit.exp: New.
+
 2016-06-17  Yan-Ting Lin  <currygt52@gmail.com>
 
 	* gdb.base/float.exp: Add target check for nds32*-*-*.
diff --git a/gdb/testsuite/gdb.base/step-over-exit.c b/gdb/testsuite/gdb.base/step-over-exit.c
new file mode 100644
index 0000000..fd0de71
--- /dev/null
+++ b/gdb/testsuite/gdb.base/step-over-exit.c
@@ -0,0 +1,50 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2016 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include <stdlib.h>
+#include <unistd.h>
+#include <string.h>
+
+static void
+marker (void)
+{}
+
+int
+main (void)
+{
+  int  pid;
+
+  pid = fork ();
+  if (pid == 0) /* child */
+    {
+      _exit (0);
+    }
+  else
+    {
+    }
+
+  pid = fork ();
+  if (pid == 0) /* child */
+    {
+      marker ();
+      _exit (0);
+    }
+  else
+    {
+      marker ();
+    }
+}
diff --git a/gdb/testsuite/gdb.base/step-over-exit.exp b/gdb/testsuite/gdb.base/step-over-exit.exp
new file mode 100644
index 0000000..9f4c826
--- /dev/null
+++ b/gdb/testsuite/gdb.base/step-over-exit.exp
@@ -0,0 +1,127 @@
+#   Copyright 2016 Free Software Foundation, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+standard_testfile
+
+# Test a thread is doing step-over a syscall instruction which is exit,
+# and GDBserver should cleanup its state of step-over properly.
+
+set syscall_insn ""
+
+# Define the syscall instruction for each target.
+
+if { [istarget "i\[34567\]86-*-linux*"] || [istarget "x86_64-*-linux*"] } {
+    set syscall_insn "\[ \t\](int|syscall|sysenter)\[ \t\]"
+} elseif { [istarget "aarch64*-*-linux*"] || [istarget "arm*-*-linux*"] } {
+    set syscall_insn "\[ \t\](swi|svc)\[ \t\]"
+} else {
+    unsupported "unknown syscall instruction"
+    return -1
+}
+
+if {[prepare_for_testing $testfile.exp $testfile $srcfile debug]} {
+    untested $testfile.exp
+    return -1
+}
+
+# Start with a fresh gdb.
+clean_restart ${testfile}
+if ![runto_main] {
+    fail "Can't run to main"
+    return -1
+}
+
+gdb_test "set follow-fork-mode child"
+gdb_test "set detach-on-fork off"
+
+# Step 1, find the syscall instruction address.
+
+gdb_test "break _exit" "Breakpoint $decimal at .*"
+
+# Hit the breakpoint on _exit.  The address of syscall insn is recorded.
+
+gdb_test "continue" \
+    "Continuing\\..*Breakpoint $decimal.*_exit \\(.*\\).*" \
+    "continue to exit"
+
+gdb_test "display/i \$pc" ".*"
+
+# Single step until we see a syscall insn or we reach the
+# upper bound of loop iterations.
+set msg "find syscall insn in exit"
+set steps 0
+set max_steps 1000
+gdb_test_multiple "stepi" $msg {
+    -re ".*$syscall_insn.*$gdb_prompt $" {
+	pass $msg
+    }
+    -re "x/i .*=>.*\r\n$gdb_prompt $" {
+	incr steps
+	if {$steps == $max_steps} {
+	    fail $msg
+	} else {
+	    send_gdb "stepi\n"
+	    exp_continue
+	}
+    }
+}
+
+if {$steps == $max_steps} {
+    return
+}
+
+# Remove the display
+gdb_test_no_output "delete display 1"
+
+set syscall_insn_addr [get_hexadecimal_valueof "\$pc" "0"]
+
+gdb_test "continue" "exited normally.*" "continue to end, first time"
+gdb_test "inferior 1" ".*Switching to inferior 1.*" \
+    "switch back to inferior 1, first time"
+
+delete_breakpoints
+
+gdb_test "break marker"
+
+gdb_test "continue" "Continuing\\..*Breakpoint $decimal, .*" \
+    "continue to marker, first time"
+
+# Step 2, create a breakpoint which evaluates false, and force it
+# evaluated on the target side.
+
+set test "set breakpoint condition-evaluation target"
+gdb_test_multiple $test $test {
+    -re "warning: Target does not support breakpoint condition evaluation.\r\nUsing host evaluation mode instead.\r\n$gdb_prompt $" {
+	# Target doesn't support breakpoint condition evaluation
+	# on its side, but it is no harm to run the test.
+    }
+    -re "^$test\r\n$gdb_prompt $" {
+    }
+}
+
+gdb_test "break \*$syscall_insn_addr if main == 0" \
+    "Breakpoint \[0-9\]* at .*"
+
+# Resume the child process, and the step-over is being done.
+
+gdb_test "continue" "exited normally.*" "continue to end, second time"
+gdb_test "inferior 1" ".*Switching to inferior 1.*" \
+    "switch back to inferior 1, second time"
+
+# Switch back to the parent process, continue to the marker to
+# test GDBserver's state is still correct.
+
+gdb_test "continue" "Continuing\\..*Breakpoint $decimal, .*" \
+    "continue to marker, second time"

[PATCH 12/12] 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

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

[Pedro Alves]

OK.

Thanks,
Pedro Alves

[PATCH 07/12] 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 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

Re: [PATCH 07/12] Create sub classes of 'struct breakpoint'

[Pedro Alves]

On 06/02/2016 10:30 AM, 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,
> 
> 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 *'.

OK.

Thanks,
Pedro Alves

[PATCH 11/12] Use reinsert_breakpoint for vCont;s

[Yao Qi]

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.  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, call maybe_hw_step.
	(linux_wait_1): Stop all threads, remove reinsert breakpoints,
	and unstop them.
	(linux_resume_one_thread): If resume request is resume_step,
	call maybe_hw_step.
	(linux_resume): Install reinsert breakpoints if the thread is
	requested to resume_step.
	(proceed_one_lwp): If resume request is resume_step, call
	maybe_hw_step.
	(proceed_all_lwps): Install reinsert breakpoints if the thread is
	requested to resume_step.
---
 gdb/gdbserver/linux-low.c | 99 ++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 94 insertions(+), 5 deletions(-)

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index 6f32911..4e79ec1 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -2579,7 +2579,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",
@@ -3530,6 +3533,23 @@ linux_wait_1 (ptid_t ptid,
       return ignore_event (ourstatus);
     }
 
+  /* Remove reinsert breakpoints ...  */
+  if (can_software_single_step ()
+      && has_reinsert_breakpoints (current_thread)
+      /*... if GDB requests this thread doing resume_step or ...*/
+      && (current_thread->last_resume_kind == resume_step
+	  /* GDBserver has already started the step-over for vCont;s,
+	     but it gets some other signal, like SIGSTOP sent by
+	     GDBserver for vCont;t or other signal program received.  */
+	  || !maybe_internal_trap))
+    {
+      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);
@@ -4293,7 +4313,7 @@ linux_resume_one_lwp_throw (struct lwp_info *lwp,
 
       step = maybe_hw_step (thread);
     }
-  else
+  else if (lwp->resume != NULL && lwp->resume->kind != resume_step)
     {
       /* If the thread isn't doing step-over, there shouldn't be any
 	 reinsert breakpoints.  */
@@ -4853,7 +4873,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;
 
@@ -4922,10 +4941,14 @@ 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)
+	step = maybe_hw_step (thread);
+
       linux_resume_one_lwp (lwp, step, lwp->resume->sig, NULL);
     }
   else
@@ -4966,6 +4989,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)
     {
@@ -5009,12 +5033,52 @@ 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)
+	    {
+	      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;
+		}
+
+	      install_software_single_step_breakpoints (lwp);
+
+	      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)
     {
@@ -5110,7 +5174,8 @@ 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;
+
+      step = maybe_hw_step (thread);
     }
   else if (lwp->bp_reinsert != 0)
     {
@@ -5176,6 +5241,30 @@ 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);
+
+	      if (!has_reinsert_breakpoints (thread))
+		install_software_single_step_breakpoints (lwp);
+
+	      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

Re: [PATCH 11/12] Use reinsert_breakpoint for vCont;s

[Pedro Alves]

On 06/02/2016 10:30 AM, Yao Qi wrote:

> @@ -4293,7 +4313,7 @@ linux_resume_one_lwp_throw (struct lwp_info *lwp,
>  
>        step = maybe_hw_step (thread);
>      }
> -  else
> +  else if (lwp->resume != NULL && lwp->resume->kind != resume_step)
>      {
>        /* If the thread isn't doing step-over, there shouldn't be any
>  	 reinsert breakpoints.  */

Consider (non-stop RSP):

 -> vCont;s:1
 <- OK
 -> vCont;s:2
 <- OK

The handling of the second vCont sets thread 1's lwp->resume to NULL.
The lwp->resume pointer is only meaningful within linux_resume
and its callees.  (But this function is called in other contexts.)

> @@ -5009,12 +5033,52 @@ 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.  */

Looking again, I think the rationale for stopping threads should
be mentioned here, as it's not obvious.

> +  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)
> +	    {
> +	      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;
> +		}
> +
> +	      install_software_single_step_breakpoints (lwp);
> +
> +	      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)
>      {
> @@ -5110,7 +5174,8 @@ 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;
> +
> +      step = maybe_hw_step (thread);
>      }
>    else if (lwp->bp_reinsert != 0)
>      {
> @@ -5176,6 +5241,30 @@ 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 ())

Not immediately obvious to why is this necessary.  Where were they
removed in the first place?  I'm it must be necessary, but maybe
extending the comment helps.

> +    {
> +      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);
> +
> +	      if (!has_reinsert_breakpoints (thread))
> +		install_software_single_step_breakpoints (lwp);
> +
> +	      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 11/12] Use reinsert_breakpoint for vCont;s

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

>> @@ -4293,7 +4313,7 @@ linux_resume_one_lwp_throw (struct lwp_info *lwp,
>>  
>>        step = maybe_hw_step (thread);
>>      }
>> -  else
>> +  else if (lwp->resume != NULL && lwp->resume->kind != resume_step)
>>      {
>>        /* If the thread isn't doing step-over, there shouldn't be any
>>  	 reinsert breakpoints.  */
>
> Consider (non-stop RSP):
>
>  -> vCont;s:1
>  <- OK
>  -> vCont;s:2
>  <- OK
>
> The handling of the second vCont sets thread 1's lwp->resume to NULL.

If so, the assert won't be called for thread 1.

> The lwp->resume pointer is only meaningful within linux_resume
> and its callees.  (But this function is called in other contexts.)
>

When I wrote the patch, it took me a while to think about this condition
check.  I wanted to remove this condition and assert, but finally
decided to leave it there, as it is not harmful.  If lwp->resume is only
meaningful within linux_resume and its callees, how about remove the
condition check and assert here?

>> @@ -5009,12 +5033,52 @@ 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.  */
>
> Looking again, I think the rationale for stopping threads should
> be mentioned here, as it's not obvious.
>

How about this,

  /* Before we resume the threads, if resume_step is requested by GDB,
     we need to access the inferior memory to install reinsert
     breakpoints, so stop all threads.  */

>> @@ -5110,7 +5174,8 @@ 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;
>> +
>> +      step = maybe_hw_step (thread);
>>      }
>>    else if (lwp->bp_reinsert != 0)
>>      {
>> @@ -5176,6 +5241,30 @@ 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 ())
>
> Not immediately obvious to why is this necessary.  Where were they
> removed in the first place?  I'm it must be necessary, but maybe
> extending the comment helps.

How about this

  /* On software single step target, we removed reinsert breakpoints
     after we get any events from the inferior.  If the client wants
     thread step, re-install these reinsert breakpoints.  */

-- 
Yao (齐尧)

Re: [PATCH 11/12] Use reinsert_breakpoint for vCont;s

[Pedro Alves]

On 06/14/2016 02:14 PM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> writes:
> 
>>> @@ -4293,7 +4313,7 @@ linux_resume_one_lwp_throw (struct lwp_info *lwp,
>>>  
>>>        step = maybe_hw_step (thread);
>>>      }
>>> -  else
>>> +  else if (lwp->resume != NULL && lwp->resume->kind != resume_step)
>>>      {
>>>        /* If the thread isn't doing step-over, there shouldn't be any
>>>  	 reinsert breakpoints.  */
>>
>> Consider (non-stop RSP):
>>
>>  -> vCont;s:1
>>  <- OK
>>  -> vCont;s:2
>>  <- OK
>>
>> The handling of the second vCont sets thread 1's lwp->resume to NULL.
> 
> If so, the assert won't be called for thread 1.
> 
>> The lwp->resume pointer is only meaningful within linux_resume
>> and its callees.  (But this function is called in other contexts.)
>>
> 
> When I wrote the patch, it took me a while to think about this condition
> check.  I wanted to remove this condition and assert, but finally
> decided to leave it there, as it is not harmful.  If lwp->resume is only
> meaningful within linux_resume and its callees, how about remove the
> condition check and assert here?

Yes, if it's only for the assert, then let's remove it.

> 
>>> @@ -5009,12 +5033,52 @@ 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.  */
>>
>> Looking again, I think the rationale for stopping threads should
>> be mentioned here, as it's not obvious.
>>
> 
> How about this,
> 
>   /* Before we resume the threads, if resume_step is requested by GDB,
>      we need to access the inferior memory to install reinsert
>      breakpoints, so stop all threads.  */

That doesn't tell the reader why we need to stop _all_ threads.  The
threads that are about to be resumed are obviously stopped, and
thus we could already _access_ inferior memory through them.

I guess this is about flushing instruction caches?

>>> @@ -5176,6 +5241,30 @@ 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 ())
>>
>> Not immediately obvious to why is this necessary.  Where were they
>> removed in the first place?  I'm it must be necessary, but maybe
>> extending the comment helps.
> 
> How about this
> 
>   /* On software single step target, we removed reinsert breakpoints
>      after we get any events from the inferior.  

Is that all events, even internal events?  From the patch, it seemed
like it was only before reporting an event to gdb.

> If the client wants
>      thread step, re-install these reinsert breakpoints.  */
> 

If we only remove before reporting an event to gdb, then I don't
understand this.  We already insert single-step breakpoints when
we process the resume request from gdb, no?

Thanks,
Pedro Alves

Re: [PATCH 11/12] Use reinsert_breakpoint for vCont;s

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

> That doesn't tell the reader why we need to stop _all_ threads.  The
> threads that are about to be resumed are obviously stopped, and
> thus we could already _access_ inferior memory through them.

GDB may only resume some threads, and leave other threads running.  In
order to access inferior memory safely, we must stop all threads.

>
> I guess this is about flushing instruction caches?
>

No, it is not about flushing instruction caches.

>>>> @@ -5176,6 +5241,30 @@ 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 ())
>>>
>>> Not immediately obvious to why is this necessary.  Where were they
>>> removed in the first place?  I'm it must be necessary, but maybe
>>> extending the comment helps.
>> 
>> How about this
>> 
>>   /* On software single step target, we removed reinsert breakpoints
>>      after we get any events from the inferior.  
>
> Is that all events, even internal events?  From the patch, it seemed
> like it was only before reporting an event to gdb.
>

You are right, I though too much about supporting range-stepping.
I rewrite the comments in the patch below,

>> If the client wants
>>      thread step, re-install these reinsert breakpoints.  */
>> 
>
> If we only remove before reporting an event to gdb, then I don't
> understand this.  We already insert single-step breakpoints when
> we process the resume request from gdb, no?

We insert single-step breakpoints when we process the resume requests
and threads are about to be resumed.  If threads still have pending
status, single-step breakpoints are not installed, so we need to install
them in proceed_all_lwps.

-- 
Yao (齐尧)
From f7237f92a32f0ab98ad5db91833f85c36eaf25a3 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

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.  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, call maybe_hw_step.
	(linux_wait_1): Stop all threads, remove reinsert breakpoints,
	and unstop them.
	(linux_resume_one_thread): If resume request is resume_step,
	call maybe_hw_step.
	(linux_resume): Install reinsert breakpoints if the thread is
	requested to resume_step.
	(proceed_one_lwp): If resume request is resume_step, call
	maybe_hw_step.
	(proceed_all_lwps): Install reinsert breakpoints if the thread is
	requested to resume_step.

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index 5d0eafc..dc28c0d 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -2567,7 +2567,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",
@@ -3518,6 +3521,23 @@ linux_wait_1 (ptid_t ptid,
       return ignore_event (ourstatus);
     }
 
+  /* Remove reinsert breakpoints ...  */
+  if (can_software_single_step ()
+      && has_reinsert_breakpoints (current_thread)
+      /*... if GDB requests this thread doing resume_step or ...*/
+      && (current_thread->last_resume_kind == resume_step
+	  /* GDBserver has already started the step-over for vCont;s,
+	     but it gets some other signal, like SIGSTOP sent by
+	     GDBserver for vCont;t or other signal program received.  */
+	  || !maybe_internal_trap))
+    {
+      stop_all_lwps (1, event_child);
+
+      delete_reinsert_breakpoints (current_thread);
+
+      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);
@@ -4281,12 +4301,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)
     {
@@ -4841,7 +4855,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;
 
@@ -4910,10 +4923,14 @@ 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)
+	step = maybe_hw_step (thread);
+
       linux_resume_one_lwp (lwp, step, lwp->resume->sig, NULL);
     }
   else
@@ -4954,6 +4971,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)
     {
@@ -4997,12 +5015,55 @@ linux_resume (struct thread_resume *resume_info, size_t n)
 	debug_printf ("Resuming, no pending status or step over needed\n");
     }
 
+  /* If resume_step is requested by GDB, install reinsert breakpoints
+     when the thread is about to be actually resumed.  IOW, we don't
+     insert reinsert breakpoints if any thread has pending status.  */
+  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)
+	    {
+	      if (!resume_step_is_handled)
+		{
+		  /* We need to access the inferior memory to install
+		     reinsert breakpoints, so stop all threads.  */
+		  stop_all_lwps (0, NULL);
+
+		  if (debug_threads)
+		    debug_printf ("Done stopping all threads.\n");
+
+		  resume_step_is_handled = 1;
+		}
+
+	      install_software_single_step_breakpoints (lwp);
+
+	      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)
     {
@@ -5098,7 +5159,8 @@ 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;
+
+      step = maybe_hw_step (thread);
     }
   else if (lwp->bp_reinsert != 0)
     {
@@ -5164,6 +5226,33 @@ proceed_all_lwps (void)
   if (debug_threads)
     debug_printf ("Proceeding, no step-over needed\n");
 
+  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;
+
+	  /* On software single step target, we insert reinsert
+	     breakpoints when the threads are about to be actually
+	     resumed.  IOW, we don't insert them if any thread has
+	     pending status.  Before we proceed threads, insert
+	     reinsert breakpoints if the client wants it step.  */
+	  if (thread->last_resume_kind == resume_step)
+	    {
+	      struct lwp_info *lwp = get_thread_lwp (thread);
+
+	      if (!has_reinsert_breakpoints (thread))
+		install_software_single_step_breakpoints (lwp);
+
+	      if (debug_threads)
+		debug_printf ("Insert breakpoint for resume_step LWP %ld\n",
+			      lwpid_of (thread));
+	    }
+	}
+    }
+
   find_inferior (&all_threads, proceed_one_lwp, NULL);
 }
 

Re: [PATCH 11/12] Use reinsert_breakpoint for vCont;s

[Pedro Alves]

On 06/15/2016 05:41 PM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> writes:
> 
>> That doesn't tell the reader why we need to stop _all_ threads.  The
>> threads that are about to be resumed are obviously stopped, and
>> thus we could already _access_ inferior memory through them.
> 
> GDB may only resume some threads, and leave other threads running.  In
> order to access inferior memory safely, we must stop all threads.

But what do you mean by "safely" ?  What goes wrong if we
don't stop all threads?

> 
>>
>> I guess this is about flushing instruction caches?
>>
> 
> No, it is not about flushing instruction caches.

Then what is it about?

> 
>>>>> @@ -5176,6 +5241,30 @@ 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 ())
>>>>
>>>> Not immediately obvious to why is this necessary.  Where were they
>>>> removed in the first place?  I'm it must be necessary, but maybe
>>>> extending the comment helps.
>>>
>>> How about this
>>>
>>>   /* On software single step target, we removed reinsert breakpoints
>>>      after we get any events from the inferior.  
>>
>> Is that all events, even internal events?  From the patch, it seemed
>> like it was only before reporting an event to gdb.
>>
> 
> You are right, I though too much about supporting range-stepping.
> I rewrite the comments in the patch below,
> 
>>> If the client wants
>>>      thread step, re-install these reinsert breakpoints.  */
>>>
>>
>> If we only remove before reporting an event to gdb, then I don't
>> understand this.  We already insert single-step breakpoints when
>> we process the resume request from gdb, no?
> 
> We insert single-step breakpoints when we process the resume requests
> and threads are about to be resumed.  If threads still have pending
> status, single-step breakpoints are not installed, so we need to install
> them in proceed_all_lwps.
> 

> @@ -3518,6 +3521,23 @@ linux_wait_1 (ptid_t ptid,
>        return ignore_event (ourstatus);
>      }
>  
> +  /* Remove reinsert breakpoints ...  */
> +  if (can_software_single_step ()
> +      && has_reinsert_breakpoints (current_thread)
> +      /*... if GDB requests this thread doing resume_step or ...*/
> +      && (current_thread->last_resume_kind == resume_step
> +	  /* GDBserver has already started the step-over for vCont;s,
> +	     but it gets some other signal, like SIGSTOP sent by
> +	     GDBserver for vCont;t or other signal program received.  */
> +	  || !maybe_internal_trap))
> +    {
> +      stop_all_lwps (1, event_child);
> +
> +      delete_reinsert_breakpoints (current_thread);
> +
> +      unstop_all_lwps (1, event_child);
> +    }

I'm re-looking at this and wondering if this is really the
right place to do this.  If the thread hits a breakpoint
that ends up not reported to gdb (e.g., condition evals false),
then we'll remove the reinsert breakpoints here, and then
later reinsert them in proceed_all_lwps.  The extra 
stopping/unstopping everything is best avoided if possible.

Thus, couldn't we move this to after:

  /* We found no reason GDB would want us to stop.  We either hit one
     of our own breakpoints, or finished an internal step GDB
     shouldn't know about.  */
  if (!report_to_gdb)
    {
...
    }
 
?


- 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?

> +
>    /* 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);
> @@ -4281,12 +4301,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)
>      {
> @@ -4841,7 +4855,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;
>  
> @@ -4910,10 +4923,14 @@ 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)
> +	step = maybe_hw_step (thread);
> +
>        linux_resume_one_lwp (lwp, step, lwp->resume->sig, NULL);
>      }
>    else
> @@ -4954,6 +4971,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)
>      {
> @@ -4997,12 +5015,55 @@ linux_resume (struct thread_resume *resume_info, size_t n)
>  	debug_printf ("Resuming, no pending status or step over needed\n");
>      }
>  
> +  /* If resume_step is requested by GDB, install reinsert breakpoints
> +     when the thread is about to be actually resumed.  IOW, we don't
> +     insert reinsert breakpoints if any thread has pending status.  */
> +  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)
> +	    {
> +	      if (!resume_step_is_handled)
> +		{
> +		  /* We need to access the inferior memory to install
> +		     reinsert breakpoints, so stop all threads.  */
> +		  stop_all_lwps (0, NULL);
> +
> +		  if (debug_threads)
> +		    debug_printf ("Done stopping all threads.\n");
> +
> +		  resume_step_is_handled = 1;
> +		}
> +
> +	      install_software_single_step_breakpoints (lwp);
> +
> +	      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)
>      {
> @@ -5098,7 +5159,8 @@ 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;
> +
> +      step = maybe_hw_step (thread);
>      }
>    else if (lwp->bp_reinsert != 0)
>      {
> @@ -5164,6 +5226,33 @@ proceed_all_lwps (void)
>    if (debug_threads)
>      debug_printf ("Proceeding, no step-over needed\n");
>  
> +  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;
> +
> +	  /* On software single step target, we insert reinsert
> +	     breakpoints when the threads are about to be actually
> +	     resumed.  IOW, we don't insert them if any thread has
> +	     pending status.  Before we proceed threads, insert
> +	     reinsert breakpoints if the client wants it step.  */
> +	  if (thread->last_resume_kind == resume_step)
> +	    {
> +	      struct lwp_info *lwp = get_thread_lwp (thread);
> +
> +	      if (!has_reinsert_breakpoints (thread))
> +		install_software_single_step_breakpoints (lwp);
> +
> +	      if (debug_threads)
> +		debug_printf ("Insert breakpoint for resume_step LWP %ld\n",
> +			      lwpid_of (thread));
> +	    }
> +	}
> +    }
> +
>    find_inferior (&all_threads, proceed_one_lwp, NULL);
>  }
>  

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.

Thanks,
Pedro Alves

Re: [PATCH 11/12] Use reinsert_breakpoint for vCont;s

[Antoine Tremblay]

Pedro Alves writes:

>> @@ -3518,6 +3521,23 @@ linux_wait_1 (ptid_t ptid,
>>        return ignore_event (ourstatus);
>>      }
>>  
>> +  /* Remove reinsert breakpoints ...  */
>> +  if (can_software_single_step ()
>> +      && has_reinsert_breakpoints (current_thread)
>> +      /*... if GDB requests this thread doing resume_step or ...*/
>> +      && (current_thread->last_resume_kind == resume_step
>> +	  /* GDBserver has already started the step-over for vCont;s,
>> +	     but it gets some other signal, like SIGSTOP sent by
>> +	     GDBserver for vCont;t or other signal program received.  */
>> +	  || !maybe_internal_trap))
>> +    {
>> +      stop_all_lwps (1, event_child);
>> +
>> +      delete_reinsert_breakpoints (current_thread);
>> +
>> +      unstop_all_lwps (1, event_child);
>> +    }
>
> I'm re-looking at this and wondering if this is really the
> right place to do this.  If the thread hits a breakpoint
> that ends up not reported to gdb (e.g., condition evals false),
> then we'll remove the reinsert breakpoints here, and then
> later reinsert them in proceed_all_lwps.  The extra 
> stopping/unstopping everything is best avoided if possible.
>
> Thus, couldn't we move this to after:
>
>   /* We found no reason GDB would want us to stop.  We either hit one
>      of our own breakpoints, or finished an internal step GDB
>      shouldn't know about.  */
>   if (!report_to_gdb)
>     {
> ...
>     }
>  
> ?

A note about avoiding stop/unstop, could we make it so that we really
stop/unstop only when we're actually modifing memory ?

If we place multiple reinsert breakpoints at one location over multiple
threads like the non-stop-fair-events tests does for example, we end up
calling stop/unstop when only the breakpoint refcount gets modified.

This could be applied in general to the stop / breakpoint operation /
unstop case...

I'm testing range-stepping with this patchset and found that with
non-stop-fair-events the single stepping threads stop/unstop so often
that it actually starves the execution of the thread that could make
them stop single stepping.

Thread 2-11 of the test step in an infinite loop waiting for a signal
from thread 1 but thread 1 even if it's resumed, is stopped/started so quickly
that it never gets a chance to really execute.

Most likely limiting this stop/unstop for the refcounted case would not
solve that issue but may help.

I'm not sure what else could be done for that except maybe displaced
stepping ?

Also if you're testing this out, note that there's a bug in the event
selection code I think that can be fixed with :

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index 4e79ec1..9d2f4d9 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -3664,24 +3670,6 @@ linux_wait_1 (ptid_t ptid,
       if (!non_stop)
        stop_all_lwps (0, NULL);
 
-      /* If we're not waiting for a specific LWP, choose an event LWP
-        from among those that have had events.  Giving equal priority
-        to all LWPs that have had events helps prevent
-        starvation.  */
-      if (ptid_equal (ptid, minus_one_ptid))
-       {
-         event_child->status_pending_p = 1;
-         event_child->status_pending = w;
-
-         select_event_lwp (&event_child);
-
-         /* current_thread and event_child must stay in sync.  */
-         current_thread = get_lwp_thread (event_child);
-
-         event_child->status_pending_p = 0;
-         w = event_child->status_pending;
-       }
-
       if (step_over_finished)
        {
          if (!non_stop)
@@ -3706,6 +3694,25 @@ linux_wait_1 (ptid_t ptid,
            }
        }
 
+      /* If we're not waiting for a specific LWP, choose an event LWP
+        from among those that have had events.  Giving equal priority
+        to all LWPs that have had events helps prevent
+        starvation.  */
+      if (ptid_equal (ptid, minus_one_ptid))
+       {
+         event_child->status_pending_p = 1;
+         event_child->status_pending = w;
+
+         select_event_lwp (&event_child);
+
+         /* current_thread and event_child must stay in sync.  */
+         current_thread = get_lwp_thread (event_child);
+
+         event_child->status_pending_p = 0;
+         w = event_child->status_pending;
+       }
+
+
       /* Stabilize threads (move out of jump pads).  */
       if (!non_stop)
        stabilize_threads ();


Otherwise, the suspend refcount gets it wrong since we're changing the
event_child before calling unsuspend_all_lwps/unstop.

I'm waiting to send this one since it I need the range-stepping to test
it but I hope it's usefull to the WIP here.