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

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

[Yao Qi]

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

gdb/gdbserver:

2016-05-06  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 9c50929..03cbc7b 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

[RFC 2/3] use reinsert breakpoint for vCont;s

[Yao Qi]

This patch teaches GDBserver to handle vCont;s using software single
step.  That is, if the resume request is resume_step, insert reinsert
breakpoint at the next pc address, and resume the thread.  These
reinsert breakpoints are removed in linux_wait_1, which I am not sure.

gdb/gdbserver:

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

	* linux-low.c (linux_wait_1): Delete reinsert breakpoints in
	some cases.
	(linux_resume_one_thread): Call single_step if lwp->resume->kind
	is resume_step.
	(proceed_one_lwp): Call single_step.
---
 gdb/gdbserver/linux-low.c | 33 ++++++++++++++++++++++++++++++---
 gdb/gdbserver/mem-break.c | 20 +++++++++++++++++++-
 gdb/gdbserver/mem-break.h |  7 ++++++-
 3 files changed, 55 insertions(+), 5 deletions(-)

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index e539e34..2a9a1ad 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -3270,12 +3270,34 @@ linux_wait_1 (ptid_t ptid,
 	  if (debug_threads)
 	    debug_printf ("Hit a gdbserver breakpoint.\n");
 	}
+
+      if (!step_over_finished && !can_hardware_single_step ())
+	{
+	  /* If the thread resumed by resume_step hits the reinsert
+	     breakpoint, delete the reinsert breakpoint for it.  */
+	  if (current_thread->last_resume_kind == resume_step)
+	    delete_reinsert_breakpoints (current_thread);
+	  else
+	    {
+	      /* If the thread resumed by other kind, like
+		 resume_continue, hits the breakpoint (either
+		 reinsert breakpoint or GDB breakpoint), delete
+		 all reinsert breakpoints if it hits non-reinsert
+		 breakpoints, otherwise, leave reinsert breakpoint there
+		 and step over it.  */
+	      if (non_reinsert_breakpoint_inserted_here (event_child->stop_pc))
+		delete_reinsert_breakpoints (NULL);
+	    }
+	}
     }
   else
     {
       /* We have some other signal, possibly a step-over dance was in
 	 progress, and it should be cancelled too.  */
       step_over_finished = finish_step_over (event_child);
+
+      if (!step_over_finished && !can_hardware_single_step ())
+	delete_reinsert_breakpoints (NULL);
     }
 
   /* We have all the data we need.  Either report the event to GDB, or
@@ -3568,6 +3590,8 @@ linux_wait_1 (ptid_t ptid,
 
   /* Alright, we're going to report a stop.  */
 
+  delete_reinsert_breakpoints (NULL);
+
   if (!stabilizing_threads)
     {
       /* In all-stop, stop all threads.  */
@@ -4765,7 +4789,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;
 
@@ -4834,10 +4857,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 = single_step (lwp);
+
       linux_resume_one_lwp (lwp, step, lwp->resume->sig, NULL);
     }
   else
@@ -5022,7 +5049,7 @@ 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 = single_step (lwp);
     }
   else if (lwp->bp_reinsert != 0)
     {
diff --git a/gdb/gdbserver/mem-break.c b/gdb/gdbserver/mem-break.c
index 197d7a1..051325a 100644
--- a/gdb/gdbserver/mem-break.c
+++ b/gdb/gdbserver/mem-break.c
@@ -1431,7 +1431,8 @@ delete_reinsert_breakpoints (struct thread_info *thread)
 
   while (bp)
     {
-      if (bp->type == reinsert_breakpoint && bp->thread == thread)
+      if (bp->type == reinsert_breakpoint
+	  && (bp->thread == thread || thread == NULL))
 	{
 	  *bp_link = bp->next;
 	  release_breakpoint (proc, bp);
@@ -1692,6 +1693,23 @@ reinsert_breakpoint_inserted_here (CORE_ADDR addr)
   return 0;
 }
 
+/* See mem-break.h.  */
+
+int
+non_reinsert_breakpoint_inserted_here (CORE_ADDR addr)
+{
+  struct process_info *proc = current_process ();
+  struct breakpoint *bp;
+
+  for (bp = proc->breakpoints; bp != NULL; bp = bp->next)
+    if (bp->type != reinsert_breakpoint
+	&& bp->raw->pc == addr
+	&& bp->raw->inserted)
+      return 1;
+
+  return 0;
+}
+
 static int
 validate_inserted_breakpoint (struct raw_breakpoint *bp)
 {
diff --git a/gdb/gdbserver/mem-break.h b/gdb/gdbserver/mem-break.h
index 2744584..e2aac80 100644
--- a/gdb/gdbserver/mem-break.h
+++ b/gdb/gdbserver/mem-break.h
@@ -104,6 +104,10 @@ int hardware_breakpoint_inserted_here (CORE_ADDR addr);
 
 int reinsert_breakpoint_inserted_here (CORE_ADDR addr);
 
+/* Returns TRUE if there's any breakpoint other than reinsert at ADDR.  */
+
+int non_reinsert_breakpoint_inserted_here (CORE_ADDR addr);
+
 /* Clear all breakpoint conditions and commands associated with a
    breakpoint.  */
 
@@ -154,7 +158,8 @@ int delete_breakpoint (struct breakpoint *bkpt);
 
 void set_reinsert_breakpoint (CORE_ADDR stop_at, struct thread_info *thread);
 
-/* Delete all reinsert breakpoints of THREAD.  */
+/* Delete all reinsert breakpoints of THREAD.  If THREAD is NULL,
+   delete all from all threads.  */
 
 void delete_reinsert_breakpoints (struct thread_info *thread);
 
-- 
1.9.1

[RFC 0/3] Use reinsert breakpoint for vCont;s

[Yao Qi]

Nowadays, reinsert breakpoint is used in GDBserver to step over a
breakpoint.  I want to use it to handle vCont;s.  The motivation
of this work is to exercise software single step in GDBserver side.
In the past two weeks, I am fixing various test fails, but still
can't fix all of them.  I want to post something here, and hope
people can help me on this area.

Suppose GDB is able to send vCont;s to GDBserver using software
single step (done by patch 3), what should GDBserver do?  It call
function single_step if lwp->resume->kind is resume_step.  See
patch 2.

With this change, reinsert breakpoint is used for two purposes,
1) step over GDBserver breakpoint, 2) handle vCont;s.  Here are some
facts or assumptions in my mind,

 - reinsert breakpoints can be inserted for both step over and
   vCont;s together.  GDBserver should finish all step-overs
   before resuming the threads, see scenario b) below,
 - GDB doesn't send more than one vCont s actions in one vCont
   packet, although RSP doc doesn't say this.

It is straightforward to insert reinsert breakpoints for vCont;s,
but I am not sure when to delete them.  Here are some scenarios,

a) vCont;s thread A, and vCont;c thread B.  Thread A hits the reinsert
   breakpoints, and GDBserver can remove them.  What is the proper
   place to remove them?

b) vCont;s thread A, and vCont;c thread B.  Thread B hits breakpoints
   (not reinsert), do we remove reinsert breakpoints?  My answer is
   no.  In the following step-over, reinsert breakpoints for step-over
   are deleted, but reinsert breakpoints for vCont;s (thread A) are still
   there.

c) vCont;s thread A, and vCont;c thread B.  Thread B hits the reinsert
   breakpoints (for thread A vCont;s), do we remove reinsert breakpoints?
   I think no, we can just step over it for thread B.

d) vCont;s thread A, and vCont;c thread B.  A signal arrives, do we remove
   reinsert breakpoints?  Yes, I think so.

IMO, b) requires reinsert breakpoint thread specific, so that we can delete
reinsert breakpoints for step-over of thread B, but keep reinsert breakpoints
for vCont;s of thread A.  That is what patch 1 does.

I tried different ways to remove reinsert breakpoints in GDBserver, but still
can't fix fails in gdb.threads/schedlock.exp, that the program gets SIGILL or
SIGSEGV.  These fails can't happen in every run, and they are disappeared
when I turn on debugging output in GDBserver.  I suspect they are about the
improper management to reinsert breakpoints.

(gdb) PASS: gdb.threads/schedlock.exp: schedlock=off: cmd=next: call_function=1: next to increment (5)
next^M
78          while (*myp > 0)^M
(gdb) next^M
^M
Thread 1 "schedlock" received signal SIGILL, Illegal instruction.^M
[Switching to Thread 3797.3797]^M
0x000087f8 in thread_function (arg=0x0) at /home/yao/SourceCode/gnu/gdb/git/gdb/testsuite/gdb.threads/schedlock.c:78^M
78          while (*myp > 0)^M
(gdb) FAIL: gdb.threads/schedlock.exp: schedlock=off: cmd=next: call_function=1: next to increment (6)

Any ideas on the overall design, how to handle vCont;s in GDBserver using
software single step? or is it a completely wrong thing to handle vCont;s
using software single step?

*** BLURB HERE ***

Yao Qi (3):
  make reinsert breakpoint thread specific
  use reinsert breakpoint for vCont;s
  [GDBserver] Support vCont s and S actions with software single step

 gdb/gdbserver/linux-low.c | 37 ++++++++++++++++++++++++++++++++-----
 gdb/gdbserver/mem-break.c | 29 ++++++++++++++++++++++++++---
 gdb/gdbserver/mem-break.h | 13 +++++++++----
 gdb/gdbserver/server.c    | 13 ++++++++-----
 4 files changed, 75 insertions(+), 17 deletions(-)

-- 
1.9.1

[RFC 1/3] make reinsert breakpoint thread specific

[Yao Qi]

gdb/gdbserver:

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

	* mem-break.c (struct breakpoint) <thread>: New field.
	(set_reinsert_breakpoint): New parameter thread.  Callers
	updated.
	(delete_reinsert_breakpoints): Likewise.
	* mem-break.h (set_reinsert_breakpoint): Update declaration.
	(delete_reinsert_breakpoints): Likewise.
---
 gdb/gdbserver/linux-low.c |  4 ++--
 gdb/gdbserver/mem-break.c | 11 ++++++++---
 gdb/gdbserver/mem-break.h |  8 ++++----
 3 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index 8e1e2fc..e539e34 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -4080,7 +4080,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_thread);
 
   do_cleanups (old_chain);
 }
@@ -4705,7 +4705,7 @@ 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 ();
+	delete_reinsert_breakpoints (get_lwp_thread (lwp));
 
       step_over_bkpt = null_ptid;
       return 1;
diff --git a/gdb/gdbserver/mem-break.c b/gdb/gdbserver/mem-break.c
index 363d7ca..197d7a1 100644
--- a/gdb/gdbserver/mem-break.c
+++ b/gdb/gdbserver/mem-break.c
@@ -190,6 +190,10 @@ struct breakpoint
      the breakpoint shall be deleted; 0 or if this callback is NULL,
      it will be left inserted.  */
   int (*handler) (CORE_ADDR);
+
+  /* The owner of this breakpoint if it is not NULL.  It is only used
+     by reinsert breakpoint so far.  */
+  struct thread_info *thread;
 };
 
 /* Return the breakpoint size from its kind.  */
@@ -1407,16 +1411,17 @@ gdb_breakpoint_here (CORE_ADDR where)
 }
 
 void
-set_reinsert_breakpoint (CORE_ADDR stop_at)
+set_reinsert_breakpoint (CORE_ADDR stop_at, struct thread_info *thread)
 {
   struct breakpoint *bp;
 
   bp = set_breakpoint_at (stop_at, NULL);
   bp->type = reinsert_breakpoint;
+  bp->thread = thread;
 }
 
 void
-delete_reinsert_breakpoints (void)
+delete_reinsert_breakpoints (struct thread_info *thread)
 {
   struct process_info *proc = current_process ();
   struct breakpoint *bp, **bp_link;
@@ -1426,7 +1431,7 @@ delete_reinsert_breakpoints (void)
 
   while (bp)
     {
-      if (bp->type == reinsert_breakpoint)
+      if (bp->type == reinsert_breakpoint && bp->thread == thread)
 	{
 	  *bp_link = bp->next;
 	  release_breakpoint (proc, bp);
diff --git a/gdb/gdbserver/mem-break.h b/gdb/gdbserver/mem-break.h
index 4d9a76c..2744584 100644
--- a/gdb/gdbserver/mem-break.h
+++ b/gdb/gdbserver/mem-break.h
@@ -150,13 +150,13 @@ 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.  */
 
-void set_reinsert_breakpoint (CORE_ADDR stop_at);
+void set_reinsert_breakpoint (CORE_ADDR stop_at, struct thread_info *thread);
 
-/* Delete all reinsert breakpoints.  */
+/* Delete all reinsert breakpoints of THREAD.  */
 
-void delete_reinsert_breakpoints (void);
+void delete_reinsert_breakpoints (struct thread_info *thread);
 
 /* Reinsert breakpoints at WHERE (and change their status to
    inserted).  */
-- 
1.9.1

Re: [RFC 0/3] Use reinsert breakpoint for vCont;s

[Antoine Tremblay]

Yao Qi writes:

> Nowadays, reinsert breakpoint is used in GDBserver to step over a
> breakpoint.  I want to use it to handle vCont;s.  The motivation
> of this work is to exercise software single step in GDBserver side.
> In the past two weeks, I am fixing various test fails, but still
> can't fix all of them.  I want to post something here, and hope
> people can help me on this area.
>
> Suppose GDB is able to send vCont;s to GDBserver using software
> single step (done by patch 3), what should GDBserver do?  It call
> function single_step if lwp->resume->kind is resume_step.  See
> patch 2.
>
> With this change, reinsert breakpoint is used for two purposes,
> 1) step over GDBserver breakpoint, 2) handle vCont;s.  Here are some
> facts or assumptions in my mind,
>
>  - reinsert breakpoints can be inserted for both step over and
>    vCont;s together.  GDBserver should finish all step-overs
>    before resuming the threads, see scenario b) below,
>  - GDB doesn't send more than one vCont s actions in one vCont
>    packet, although RSP doc doesn't say this.
>
> It is straightforward to insert reinsert breakpoints for vCont;s,
> but I am not sure when to delete them.  Here are some scenarios,
>
> a) vCont;s thread A, and vCont;c thread B.  Thread A hits the reinsert
>    breakpoints, and GDBserver can remove them.  What is the proper
>    place to remove them?
>
> b) vCont;s thread A, and vCont;c thread B.  Thread B hits breakpoints
>    (not reinsert), do we remove reinsert breakpoints?  My answer is
>    no.  In the following step-over, reinsert breakpoints for step-over
>    are deleted, but reinsert breakpoints for vCont;s (thread A) are still
>    there.
>
> c) vCont;s thread A, and vCont;c thread B.  Thread B hits the reinsert
>    breakpoints (for thread A vCont;s), do we remove reinsert breakpoints?
>    I think no, we can just step over it for thread B.
>
> d) vCont;s thread A, and vCont;c thread B.  A signal arrives, do we remove
>    reinsert breakpoints?  Yes, I think so.
>
> IMO, b) requires reinsert breakpoint thread specific, so that we can delete
> reinsert breakpoints for step-over of thread B, but keep reinsert breakpoints
> for vCont;s of thread A.  That is what patch 1 does.
>
> I tried different ways to remove reinsert breakpoints in GDBserver, but still
> can't fix fails in gdb.threads/schedlock.exp, that the program gets SIGILL or
> SIGSEGV.  These fails can't happen in every run, and they are disappeared
> when I turn on debugging output in GDBserver.  I suspect they are about the
> improper management to reinsert breakpoints.
>

I'm not sure at the moment but this makes me think of an issue I sent
here: https://www.sourceware.org/ml/gdb/2015-11/msg00030.html

Actually thanks to a good discussion with LTTng maintainer, (Mathieu
Desnoyer) last week I got the hint that this could be due to the
situation described here:
https://community.arm.com/groups/processors/blog/2010/02/17/caches-and-self-modifying-code

Short story, the instruction cache could be out of sync after we've
removed a software breakpoint and the processor would execute the
breakpoint instruction causing a SIGILL rather then the original memory.

The solution however, calling __clear_cache(...) requires that we're in
the same process for the memory addresses to be valid, I think, so I'm
not sure how to fix this yet...

I'm still early in my investigation but it may be something to consider.

> (gdb) PASS: gdb.threads/schedlock.exp: schedlock=off: cmd=next: call_function=1: next to increment (5)
> next^M
> 78          while (*myp > 0)^M
> (gdb) next^M
> ^M
> Thread 1 "schedlock" received signal SIGILL, Illegal instruction.^M
> [Switching to Thread 3797.3797]^M
> 0x000087f8 in thread_function (arg=0x0) at /home/yao/SourceCode/gnu/gdb/git/gdb/testsuite/gdb.threads/schedlock.c:78^M
> 78          while (*myp > 0)^M
> (gdb) FAIL: gdb.threads/schedlock.exp: schedlock=off: cmd=next: call_function=1: next to increment (6)
>
> Any ideas on the overall design, how to handle vCont;s in GDBserver using
> software single step? or is it a completely wrong thing to handle vCont;s
> using software single step?
>
> *** BLURB HERE ***
>
> Yao Qi (3):
>   make reinsert breakpoint thread specific
>   use reinsert breakpoint for vCont;s
>   [GDBserver] Support vCont s and S actions with software single step
>
>  gdb/gdbserver/linux-low.c | 37 ++++++++++++++++++++++++++++++++-----
>  gdb/gdbserver/mem-break.c | 29 ++++++++++++++++++++++++++---
>  gdb/gdbserver/mem-break.h | 13 +++++++++----
>  gdb/gdbserver/server.c    | 13 ++++++++-----
>  4 files changed, 75 insertions(+), 17 deletions(-)

Re: [RFC 0/3] Use reinsert breakpoint for vCont;s

[Antoine Tremblay]

Antoine Tremblay writes:

>> I tried different ways to remove reinsert breakpoints in GDBserver, but still
>> can't fix fails in gdb.threads/schedlock.exp, that the program gets SIGILL or
>> SIGSEGV.  These fails can't happen in every run, and they are disappeared
>> when I turn on debugging output in GDBserver.  I suspect they are about the
>> improper management to reinsert breakpoints.
>>
>
> I'm not sure at the moment but this makes me think of an issue I sent
> here: https://www.sourceware.org/ml/gdb/2015-11/msg00030.html
>
> Actually thanks to a good discussion with LTTng maintainer, (Mathieu
> Desnoyer) last week I got the hint that this could be due to the
> situation described here:
> https://community.arm.com/groups/processors/blog/2010/02/17/caches-and-self-modifying-code
>
> Short story, the instruction cache could be out of sync after we've
> removed a software breakpoint and the processor would execute the
> breakpoint instruction causing a SIGILL rather then the original memory.
>
> The solution however, calling __clear_cache(...) requires that we're in
> the same process for the memory addresses to be valid, I think, so I'm
> not sure how to fix this yet...
>
> I'm still early in my investigation but it may be something to consider.
>

Note I tracked that down to __flush_ptrace_access in the kernel, and
tried to hardcode the instruction cache flush (Thanks to Will Deacon for
the hints) but that did not help, at least on the odroid ux4.

Not an easy problem, I'm installing your patch series today and I'll
start checking it out in more detail.

Thanks,
Antoine

Re: [RFC 0/3] Use reinsert breakpoint for vCont;s

[Yao Qi]

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

> Not an easy problem, I'm installing your patch series today and I'll
> start checking it out in more detail.

Thanks for looking at this.  I managed to reproduce the SIGILL with
GDBserver debugging logs.  Note that current GDB test harness doesn't
fetch GDBserver debugging output to gdb.log.

-- 
Yao (齐尧)

Re: [RFC 2/3] use reinsert breakpoint for vCont;s

[Yao Qi]

Yao Qi <qiyaoltc@gmail.com> writes:

> +
> +      if (!step_over_finished && !can_hardware_single_step ())
> +	{
> +	  /* If the thread resumed by resume_step hits the reinsert
> +	     breakpoint, delete the reinsert breakpoint for it.  */
> +	  if (current_thread->last_resume_kind == resume_step)
> +	    delete_reinsert_breakpoints (current_thread);
> +	  else
> +	    {
> +	      /* If the thread resumed by other kind, like
> +		 resume_continue, hits the breakpoint (either
> +		 reinsert breakpoint or GDB breakpoint), delete
> +		 all reinsert breakpoints if it hits non-reinsert
> +		 breakpoints, otherwise, leave reinsert breakpoint there
> +		 and step over it.  */
> +	      if (non_reinsert_breakpoint_inserted_here (event_child->stop_pc))
> +		delete_reinsert_breakpoints (NULL);
> +	    }
> +	}
>      }
>    else
>      {
>        /* We have some other signal, possibly a step-over dance was in
>  	 progress, and it should be cancelled too.  */
>        step_over_finished = finish_step_over (event_child);
> +
> +      if (!step_over_finished && !can_hardware_single_step ())
> +	delete_reinsert_breakpoints (NULL);
>      }
>  
>    /* We have all the data we need.  Either report the event to GDB, or
> @@ -3568,6 +3590,8 @@ linux_wait_1 (ptid_t ptid,
>  
>    /* Alright, we're going to report a stop.  */
>  
> +  delete_reinsert_breakpoints (NULL);
> +

The SIGILL is caused by removing these reinsert breakpoints when threads
are still running.  I adjust the code removing reinsert breakpoints when
threads stop, the SIGILL goes away.

-- 
Yao (齐尧)

Re: [RFC 0/3] Use reinsert breakpoint for vCont;s

[Antoine Tremblay]

Yao Qi writes:

> Antoine Tremblay <antoine.tremblay@ericsson.com> writes:
>
>> Not an easy problem, I'm installing your patch series today and I'll
>> start checking it out in more detail.
>
> Thanks for looking at this.  I managed to reproduce the SIGILL with
> GDBserver debugging logs.  Note that current GDB test harness doesn't
> fetch GDBserver debugging output to gdb.log.

Yes that the GDB testsuite can't handle GDBServer output is very
annoying. I have a TODO item to add a log to file option to GDBServer so
that at least we can run the test with GDBServer in debug without
interfering with the tests and keep the logs. (I usually hack it like that)

I'm glad you found the source of your SIGILL problem, I've found the
source of mine too thanks to Will Daecon, I will post my findings as a
follow-up to https://www.sourceware.org/ml/gdb/2015-11/msg00030.html

And I'll keep looking at this series...

Thanks,
Antoine

Re: [RFC 2/3] use reinsert breakpoint for vCont;s

[Antoine Tremblay]

Yao Qi writes:

> Yao Qi <qiyaoltc@gmail.com> writes:
>
>> +
>> +      if (!step_over_finished && !can_hardware_single_step ())
>> +	{
>> +	  /* If the thread resumed by resume_step hits the reinsert
>> +	     breakpoint, delete the reinsert breakpoint for it.  */
>> +	  if (current_thread->last_resume_kind == resume_step)
>> +	    delete_reinsert_breakpoints (current_thread);
>> +	  else
>> +	    {
>> +	      /* If the thread resumed by other kind, like
>> +		 resume_continue, hits the breakpoint (either
>> +		 reinsert breakpoint or GDB breakpoint), delete
>> +		 all reinsert breakpoints if it hits non-reinsert
>> +		 breakpoints, otherwise, leave reinsert breakpoint there
>> +		 and step over it.  */
>> +	      if (non_reinsert_breakpoint_inserted_here (event_child->stop_pc))
>> +		delete_reinsert_breakpoints (NULL);
>> +	    }
>> +	}
>>      }
>>    else
>>      {
>>        /* We have some other signal, possibly a step-over dance was in
>>  	 progress, and it should be cancelled too.  */
>>        step_over_finished = finish_step_over (event_child);
>> +
>> +      if (!step_over_finished && !can_hardware_single_step ())
>> +	delete_reinsert_breakpoints (NULL);
>>      }
>>  
>>    /* We have all the data we need.  Either report the event to GDB, or
>> @@ -3568,6 +3590,8 @@ linux_wait_1 (ptid_t ptid,
>>  
>>    /* Alright, we're going to report a stop.  */
>>  
>> +  delete_reinsert_breakpoints (NULL);
>> +
>
> The SIGILL is caused by removing these reinsert breakpoints when threads
> are still running.  I adjust the code removing reinsert breakpoints when
> threads stop, the SIGILL goes away.

I think the insertion of the breakpoints may also be unsafe in non-stop
mode, since correct me if am wrong but in linux_resume we can't assume
that all threads are stopped and thus when we call single_step from:
linux_resume->linux_resume_one_lwp->single_step another thread could
hit the memory we're writing to.

We should stop all threads before the breakpoint insertion like done in
start_step_over.

Actually I think we should have a function like
start_software_vCont that does stops all threads, insert the
breakpoints, resume all threads...

I have not pinpointed the design of this however as I'd rather not call
it from the linux_resume_one_lwp callback. It would be weird to mess
with the thread running state there.

I'm thinking, maybe again close to what step over is doing having a:

if (software_single_step && !hardware_single_step)
   find_inferior (&all_threads, need_software_vCont..)

if (need_software_vCont)
  start_software_vCont
        - stop all threads
        - call single_step to insert the breakpoints
        - resume all threads

And I guess we can have a stop_software_vCont to match it.

I have not gone through the stop scenarios enough yet to tell where however...

Re: [RFC 2/3] use reinsert breakpoint for vCont;s

[Antoine Tremblay]

Yao Qi writes:

> Yao Qi <qiyaoltc@gmail.com> writes:
>
>> +
>> +      if (!step_over_finished && !can_hardware_single_step ())
>> +	{
>> +	  /* If the thread resumed by resume_step hits the reinsert
>> +	     breakpoint, delete the reinsert breakpoint for it.  */
>> +	  if (current_thread->last_resume_kind == resume_step)
>> +	    delete_reinsert_breakpoints (current_thread);
>> +	  else
>> +	    {
>> +	      /* If the thread resumed by other kind, like
>> +		 resume_continue, hits the breakpoint (either
>> +		 reinsert breakpoint or GDB breakpoint), delete
>> +		 all reinsert breakpoints if it hits non-reinsert
>> +		 breakpoints, otherwise, leave reinsert breakpoint there
>> +		 and step over it.  */
>> +	      if (non_reinsert_breakpoint_inserted_here (event_child->stop_pc))
>> +		delete_reinsert_breakpoints (NULL);
>> +	    }
>> +	}
>>      }
>>    else
>>      {
>>        /* We have some other signal, possibly a step-over dance was in
>>  	 progress, and it should be cancelled too.  */
>>        step_over_finished = finish_step_over (event_child);
>> +
>> +      if (!step_over_finished && !can_hardware_single_step ())
>> +	delete_reinsert_breakpoints (NULL);
>>      }
>>  
>>    /* We have all the data we need.  Either report the event to GDB, or
>> @@ -3568,6 +3590,8 @@ linux_wait_1 (ptid_t ptid,
>>  
>>    /* Alright, we're going to report a stop.  */
>>  
>> +  delete_reinsert_breakpoints (NULL);
>> +
>
> The SIGILL is caused by removing these reinsert breakpoints when threads
> are still running.  I adjust the code removing reinsert breakpoints when
> threads stop, the SIGILL goes away.

I though we would have to explicitly stop the threads, I'm curious has
to where you moved it so that the threads are stopped ?

Re: [RFC 2/3] use reinsert breakpoint for vCont;s

[Yao Qi]

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

> I though we would have to explicitly stop the threads, I'm curious has
> to where you moved it so that the threads are stopped ?

I moved the code deleting reinsert breakpoint to the place where
GDBserver is ready to report a stop back to GDB, because on this point,
all threads stop in all-stop mode, and the event thread stops in
no-stop mode.  In non-stop mode, I delete the reinsert breakpoint of
event thread, while in all-stop mode, I delete the reinsert breakpoint
of all threads.

You are right that it is unsafe to remove reinsert breakpoint while some
threads are still running (in non-stop mode).

-- 
Yao (齐尧)

Re: [RFC 2/3] use reinsert breakpoint for vCont;s

[Antoine Tremblay]

Antoine Tremblay writes:

> Yao Qi writes:
>
>> Yao Qi <qiyaoltc@gmail.com> writes:
>>
>>> +
>>> +      if (!step_over_finished && !can_hardware_single_step ())
>>> +	{
>>> +	  /* If the thread resumed by resume_step hits the reinsert
>>> +	     breakpoint, delete the reinsert breakpoint for it.  */
>>> +	  if (current_thread->last_resume_kind == resume_step)
>>> +	    delete_reinsert_breakpoints (current_thread);
>>> +	  else
>>> +	    {
>>> +	      /* If the thread resumed by other kind, like
>>> +		 resume_continue, hits the breakpoint (either
>>> +		 reinsert breakpoint or GDB breakpoint), delete
>>> +		 all reinsert breakpoints if it hits non-reinsert
>>> +		 breakpoints, otherwise, leave reinsert breakpoint there
>>> +		 and step over it.  */
>>> +	      if (non_reinsert_breakpoint_inserted_here (event_child->stop_pc))
>>> +		delete_reinsert_breakpoints (NULL);
>>> +	    }
>>> +	}
>>>      }
>>>    else
>>>      {
>>>        /* We have some other signal, possibly a step-over dance was in
>>>  	 progress, and it should be cancelled too.  */
>>>        step_over_finished = finish_step_over (event_child);
>>> +
>>> +      if (!step_over_finished && !can_hardware_single_step ())
>>> +	delete_reinsert_breakpoints (NULL);
>>>      }
>>>  
>>>    /* We have all the data we need.  Either report the event to GDB, or
>>> @@ -3568,6 +3590,8 @@ linux_wait_1 (ptid_t ptid,
>>>  
>>>    /* Alright, we're going to report a stop.  */
>>>  
>>> +  delete_reinsert_breakpoints (NULL);
>>> +
>>
>> The SIGILL is caused by removing these reinsert breakpoints when threads
>> are still running.  I adjust the code removing reinsert breakpoints when
>> threads stop, the SIGILL goes away.
>
> I think the insertion of the breakpoints may also be unsafe in non-stop
> mode, since correct me if am wrong but in linux_resume we can't assume
> that all threads are stopped and thus when we call single_step from:
> linux_resume->linux_resume_one_lwp->single_step another thread could
> hit the memory we're writing to.
>
> We should stop all threads before the breakpoint insertion like done in
> start_step_over.
>

Note that on second though, I think it's best to just call
prepare/done_to_access_memory on every aceess and not have to worry
about this. I'm trying this out now.

And then it's easy to just select what were we want to remove the
breakpoint like this patch does.

Re: [RFC 0/3] Use reinsert breakpoint for vCont;s

[Antoine Tremblay]

Yao Qi writes:

> Nowadays, reinsert breakpoint is used in GDBserver to step over a
> breakpoint.  I want to use it to handle vCont;s.  The motivation
> of this work is to exercise software single step in GDBserver side.
> In the past two weeks, I am fixing various test fails, but still
> can't fix all of them.  I want to post something here, and hope
> people can help me on this area.
>
> Suppose GDB is able to send vCont;s to GDBserver using software
> single step (done by patch 3), what should GDBserver do?  It call
> function single_step if lwp->resume->kind is resume_step.  See
> patch 2.
>
> With this change, reinsert breakpoint is used for two purposes,
> 1) step over GDBserver breakpoint, 2) handle vCont;s.  Here are some
> facts or assumptions in my mind,
>
>  - reinsert breakpoints can be inserted for both step over and
>    vCont;s together.  GDBserver should finish all step-overs
>    before resuming the threads, see scenario b) below,
>  - GDB doesn't send more than one vCont s actions in one vCont
>    packet, although RSP doc doesn't say this.
>
> It is straightforward to insert reinsert breakpoints for vCont;s,
> but I am not sure when to delete them.  Here are some scenarios,
>
> a) vCont;s thread A, and vCont;c thread B.  Thread A hits the reinsert
>    breakpoints, and GDBserver can remove them.  What is the proper
>    place to remove them?
>

I think like you did in patch 2 before we know we're reporting to GDB
the right place too, but adding a
prepare_to_access_memory/done_accessing_memory lock around the delete /
insert reinsert breakpoints is needed.

I think this is better than waiting to know that we're going to
report, since this an unrelated condition.

> b) vCont;s thread A, and vCont;c thread B.  Thread B hits breakpoints
>    (not reinsert), do we remove reinsert breakpoints?  My answer is
>    no.  In the following step-over, reinsert breakpoints for step-over
>    are deleted, but reinsert breakpoints for vCont;s (thread A) are still
>    there.
>

Right.

> c) vCont;s thread A, and vCont;c thread B.  Thread B hits the reinsert
>    breakpoints (for thread A vCont;s), do we remove reinsert breakpoints?
>    I think no, we can just step over it for thread B.
>

Indeed.

> d) vCont;s thread A, and vCont;c thread B.  A signal arrives, do we remove
>    reinsert breakpoints?  Yes, I think so.

Indeed.

>
> IMO, b) requires reinsert breakpoint thread specific, so that we can delete
> reinsert breakpoints for step-over of thread B, but keep reinsert breakpoints
> for vCont;s of thread A.  That is what patch 1 does.
>

Indeed.

> I tried different ways to remove reinsert breakpoints in GDBserver, but still
> can't fix fails in gdb.threads/schedlock.exp, that the program gets SIGILL or
> SIGSEGV.  These fails can't happen in every run, and they are disappeared
> when I turn on debugging output in GDBserver.  I suspect they are about the
> improper management to reinsert breakpoints.
>
> (gdb) PASS: gdb.threads/schedlock.exp: schedlock=off: cmd=next: call_function=1: next to increment (5)
> next^M
> 78          while (*myp > 0)^M
> (gdb) next^M
> ^M
> Thread 1 "schedlock" received signal SIGILL, Illegal instruction.^M
> [Switching to Thread 3797.3797]^M
> 0x000087f8 in thread_function (arg=0x0) at /home/yao/SourceCode/gnu/gdb/git/gdb/testsuite/gdb.threads/schedlock.c:78^M
> 78          while (*myp > 0)^M
> (gdb) FAIL: gdb.threads/schedlock.exp: schedlock=off: cmd=next: call_function=1: next to increment (6)
>
> Any ideas on the overall design, how to handle vCont;s in GDBserver using
> software single step? or is it a completely wrong thing to handle vCont;s
> using software single step?
>

Actually pretty much the only thing that single step reinsert breakpoints have
in common with step over reinsert breakpoints is that they're
inserted as a GDBServer breakpoint. No other code path is the same, afaick.

I think it would be more clear to have a different kind of breakpoint so that :

 - We can protect these breakpoints with prepare_to_access_memory
 without affecting the step over reinsert breakpoints, that do not need this.
 - Have these breakpoints thread specific, again something that
 step-over breakpoints do not need.

The added logic to the control flow should be about the same or less
than by sharing the reinsert_breakpoints.

Also, when changing code related to either of the 2 scenarios we would not
fear breaking one or the other. Things are already mangled enough
in that area ?

I'm testing this out...

Re: [RFC 0/3] Use reinsert breakpoint for vCont;s

[Yao Qi]

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

> I think like you did in patch 2 before we know we're reporting to GDB
> the right place too, but adding a
> prepare_to_access_memory/done_accessing_memory lock around the delete /
> insert reinsert breakpoints is needed.

prepare_to_access_memory and done_accessing_memory are used when *GDB*
wants to access memory, not GDBserver.

>
> Actually pretty much the only thing that single step reinsert breakpoints have
> in common with step over reinsert breakpoints is that they're
> inserted as a GDBServer breakpoint. No other code path is the same, afaick.
>

They use the_low_target.get_next_pcs to know the next pcs.

> I think it would be more clear to have a different kind of breakpoint so that :
>
>  - We can protect these breakpoints with prepare_to_access_memory
>  without affecting the step over reinsert breakpoints, that do not need this.

prepare_to_access_memory can't be used here, because it is "prepare for
the memory access requested by GDB".

>  - Have these breakpoints thread specific, again something that
>  step-over breakpoints do not need.

Nowadays, we do step-over once per thread, so it is not harmful to make
reinsert breakpoint thread specific.

>
> The added logic to the control flow should be about the same or less
> than by sharing the reinsert_breakpoints.
>
> Also, when changing code related to either of the 2 scenarios we would not
> fear breaking one or the other. Things are already mangled enough
> in that area ?

I don't think we can deal with the control flow or logic separately,
because we add breakpoint for vCont;s, and breakpoint and event
management should be done in linux_wait_1 and linux_resume.  Adding a
new kind of breakpoint doesn't help, IMO.

I've got a regression-free patch series, but need to remove some
redundant code, and post the out for review.

-- 
Yao (齐尧)

Re: [RFC 0/3] Use reinsert breakpoint for vCont;s

[Antoine Tremblay]

Yao Qi writes:

> Antoine Tremblay <antoine.tremblay@ericsson.com> writes:
>
>> I think like you did in patch 2 before we know we're reporting to GDB
>> the right place too, but adding a
>> prepare_to_access_memory/done_accessing_memory lock around the delete /
>> insert reinsert breakpoints is needed.
>
> prepare_to_access_memory and done_accessing_memory are used when *GDB*
> wants to access memory, not GDBserver.
>
Yes I know, I don't see what prevents us to extend it to GDBServer to
protect non-stop operations however ?

>>
>> Actually pretty much the only thing that single step reinsert breakpoints have
>> in common with step over reinsert breakpoints is that they're
>> inserted as a GDBServer breakpoint. No other code path is the same, afaick.
>>
>
> They use the_low_target.get_next_pcs to know the next pcs.
>

Right, good point.

>> I think it would be more clear to have a different kind of breakpoint so that :
>>
>>  - We can protect these breakpoints with prepare_to_access_memory
>>  without affecting the step over reinsert breakpoints, that do not need this.
>
> prepare_to_access_memory can't be used here, because it is "prepare for
> the memory access requested by GDB".
>

Same question as above...

>>  - Have these breakpoints thread specific, again something that
>>  step-over breakpoints do not need.
>
> Nowadays, we do step-over once per thread, so it is not harmful to make
> reinsert breakpoint thread specific.
>
>>
>> The added logic to the control flow should be about the same or less
>> than by sharing the reinsert_breakpoints.
>>
>> Also, when changing code related to either of the 2 scenarios we would not
>> fear breaking one or the other. Things are already mangled enough
>> in that area ?
>
> I don't think we can deal with the control flow or logic separately,
> because we add breakpoint for vCont;s, and breakpoint and event
> management should be done in linux_wait_1 and linux_resume.  Adding a
> new kind of breakpoint doesn't help, IMO.
>
OK.

> I've got a regression-free patch series, but need to remove some
> redundant code, and post the out for review.

OK thanks, I'll wait for that.