[PATCH 2/2] Avoid step-over infinite loop in GDBServer

[PATCH 2/2] Avoid step-over infinite loop in GDBServer

[Antoine Tremblay]

Before this patch, GDBServer always executed a step-over if it found a
thread that needed one.

This could be a problem in a situation exposed by non-stop-fair-events.exp
where the code and the breakpoint placement is like so:

instruction A : has a single-step breakpoint installed for thread 1 and 2
instruction B : has a single-step breakpoint installed for thread 3
and is a branch to A.

In this particular case:

 - GDBServer stops on instruction A in thread 1.
 - Deletes thread 1 single-step breakpoint.
 - Starts a step-over of thread 1 to step-over the thread 2 breakpoint.
 - GDBServer finishes a step-over and is at instruction B.
 - GDBserver starts a step-over of thread 1 to step-over the thread 3
   breakpoint at instruction B.
 - GDBServer stops on instuction A in thread 1.
 - GDBServer is now in an infinite loop.

This patch avoids this issue by counting the number of times a thread does
a step-over consecutively.  If the thread reaches a threshold, which is
currently 32, GDBServer will not step-over but rather restart all the
threads.

I chose a threshold of 32, so to trigger this there needs to be 32
consecutive instructions with breakpoints installed that one thread needs
to step over. I think this should be rare enought to trigger only in this
case but suggestions on the threshold value are welcome.

If the threshold is hit and the step-over is delayed, when the thread
that needed a step-over runs it will simply hit the breakpoint it needed
to step-over and retry to start a step-over.

This makes it possible for other threads to run and start a step-over
dance of their own or pending events like signals to be handled.

This patch fixes the intermittent FAILs for
gdb.threads/non-stop-fair-events.exp on ARM like discussed here:
https://sourceware.org/ml/gdb-patches/2016-11/msg00132.html

No regressions, tested on ubuntu 14.04 ARMv7.
With gdbserver-native/-m{arm,thumb}

gdb/gdbserver/ChangeLog:

	* linux-low.c (class step_over_limiter): New class.
	(_step_over_limiter): New global variable.
	(linux_wait_1): Count step-overs.
	(proceed_all_lwps): Delay step-over if threshold is reached.
---
 gdb/gdbserver/linux-low.c | 76 +++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 70 insertions(+), 6 deletions(-)

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index 15fb726..b84b62a 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -282,6 +282,53 @@ static int proceed_one_lwp (struct inferior_list_entry *entry, void *except);
    being stepped.  */
 ptid_t step_over_bkpt;
 
+/* Class limiting the number of consecutive step-overs for a thread.  */
+
+class step_over_limiter
+{
+ public:
+
+  step_over_limiter () : m_ptid (null_ptid), m_count (0), m_max_count (32) {}
+
+  void step_over_done (struct lwp_info *lwp)
+  {
+    ptid_t ptid = lwp->thread->entry.id;
+
+    if (!ptid_equal (ptid, m_ptid))
+      {
+	m_ptid = ptid;
+	m_count = 0;
+      }
+
+    m_count++;
+  }
+
+  bool can_step_over (struct lwp_info *lwp)
+  {
+    if (!ptid_equal(lwp->thread->entry.id, m_ptid)
+	|| m_count < m_max_count)
+      return true;
+    else
+      {
+	/* Reset here so that the step_over is retried.  */
+	m_ptid = null_ptid;
+	m_count = 0;
+	return false;
+      }
+  }
+
+ private:
+
+  ptid_t m_ptid;
+  int m_count;
+
+  /* Maximum step overs for a thread, before all threads are run instead of
+     stepping over.  */
+  const int m_max_count;
+};
+
+step_over_limiter _step_over_limiter;
+
 /* True if the low target can hardware single-step.  */
 
 static int
@@ -3607,6 +3654,8 @@ linux_wait_1 (ptid_t ptid,
 	     doesn't lose it.  */
 	  enqueue_pending_signal (event_child, WSTOPSIG (w), info_p);
 
+	  _step_over_limiter.step_over_done (event_child);
+
 	  proceed_all_lwps ();
 	}
       else
@@ -3694,6 +3743,8 @@ linux_wait_1 (ptid_t ptid,
 	     We're going to keep waiting, so use proceed, which
 	     handles stepping over the next breakpoint.  */
 	  unsuspend_all_lwps (event_child);
+
+	  _step_over_limiter.step_over_done (event_child);
 	}
       else
 	{
@@ -5400,13 +5451,26 @@ proceed_all_lwps (void)
 
       if (need_step_over != NULL)
 	{
-	  if (debug_threads)
-	    debug_printf ("proceed_all_lwps: found "
-			  "thread %ld needing a step-over\n",
-			  lwpid_of (need_step_over));
+	  /* Don't step over if we're looping too much.  */
+	  if (_step_over_limiter.can_step_over
+	      (get_thread_lwp (need_step_over)))
+	    {
+	      if (debug_threads)
+		debug_printf ("proceed_all_lwps: found "
+			      "thread %ld needing a step-over\n",
+			      lwpid_of (need_step_over));
 
-	  start_step_over (get_thread_lwp (need_step_over));
-	  return;
+	      start_step_over (get_thread_lwp (need_step_over));
+	      return;
+	    }
+	  else
+	    {
+	      if (debug_threads)
+		debug_printf ("proceed_all_lwps: found "
+			      "thread %ld needing a step-over "
+			      "but max consecutive step-overs reached\n",
+			      lwpid_of (need_step_over));
+	    }
 	}
     }
 
-- 
2.9.2

[PATCH 1/2] This patch fixes GDBServer's run control for single stepping

[Antoine Tremblay]

** Note these patches depend on:
https://sourceware.org/ml/gdb-patches/2016-11/msg00914.html

Before, installing single-step breakpoints was done in proceed_one_lwp as
each thread was started.

This caused a problem on ARM, which is the only architecture using
software single-step on which it is not safe to modify an instruction
to insert a breakpoint in one thread while the other threads are running.
See the architecture manual section "A3.5.4 Concurrent modification and
execution of instructions":

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

Since we want the single-step logic to work for any instruction GDBServer
needs to stop all the threads to install a single-step breakpoint.

Note that we could introduce arch specific code to check if we can do it
for each particular instruction but I think that introducing 2 run control
paths for single stepping would just add too much complexity to the code
for little benefit.

This patch fixes the intermittent FAILs for gdb.threads/schedlock.exp on
ARM like discussed here:
https://sourceware.org/ml/gdb-patches/2016-11/msg00132.html
Tested with RACY_ITER=100 on two different boards, -marm/-thumb.

Note that the FAILs in non-stop-fair-events.exp discussed in that thread
will be fixed in an upcoming patch. They are not caused by what this patch
fixes.

Here's a few implementation details notes:

Before in all-stop mode, GDBServer deleted all single-step breakpoints
when reporting an event since it assumed that this canceled all previous
resume requests.

However, this is not true as GDBServer may hit a breakpoint instruction
written directly in memory so not a GDB breakpoint nor a GDBServer
breakpoint like is the case with displaced-stepping on.

Considering this, this patch only deletes the current thread single-step
breakpoint even in all-stop mode.

Also with this patch, single-step breakpoints inserted in proceed_all_lwp
are inserted before GDBServer checks if it needs to step-over.

This is done in preparation for a patch that allows GDBServer to delay a
step-over. In which case single-step breakpoints need to be inserted
before trying to step-over.

It may also be more clear that GDBServer always insert single-step
breakpoints when calling proceed_all_lwps rather than delaying this
insertion until a step-over is done.

No regressions, tested on ubuntu 14.04 ARMv7.
With gdbserver-native/-m{arm,thumb}

gdb/gdbserver/ChangeLog:

	* linux-low.c (linux_wait_1): Don't remove all single-step
	breakpoints in all-stop.
	(install_all_software_single_step_breakpoints): New function.
	(linux_resume): Install software single-step breakpoints if needed.
	(proceed_one_lwp): Don't install software single-step here.
	(proceed_all_lwps): Install software single-step breakpoints if needed.
---
 gdb/gdbserver/linux-low.c | 145 ++++++++++++++++++++++++++--------------------
 1 file changed, 81 insertions(+), 64 deletions(-)

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index b441ebc..15fb726 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -3747,64 +3747,14 @@ linux_wait_1 (ptid_t ptid,
 
   /* Alright, we're going to report a stop.  */
 
-  /* Remove single-step breakpoints.  */
-  if (can_software_single_step ())
+  if (can_software_single_step ()
+      && has_single_step_breakpoints (current_thread))
     {
-      /* Remove single-step breakpoints or not.  It it is true, stop all
-	 lwps, so that other threads won't hit the breakpoint in the
-	 staled memory.  */
-      int remove_single_step_breakpoints_p = 0;
-
-      if (non_stop)
-	{
-	  remove_single_step_breakpoints_p
-	    = has_single_step_breakpoints (current_thread);
-	}
-      else
-	{
-	  /* In all-stop, a stop reply cancels all previous resume
-	     requests.  Delete all single-step breakpoints.  */
-	  struct inferior_list_entry *inf, *tmp;
-
-	  ALL_INFERIORS (&all_threads, inf, tmp)
-	    {
-	      struct thread_info *thread = (struct thread_info *) inf;
-
-	      if (has_single_step_breakpoints (thread))
-		{
-		  remove_single_step_breakpoints_p = 1;
-		  break;
-		}
-	    }
-	}
-
-      if (remove_single_step_breakpoints_p)
-	{
-	  /* If we remove single-step breakpoints from memory, stop all lwps,
-	     so that other threads won't hit the breakpoint in the staled
-	     memory.  */
-	  stop_all_lwps (0, event_child);
-
-	  if (non_stop)
-	    {
-	      gdb_assert (has_single_step_breakpoints (current_thread));
-	      delete_single_step_breakpoints (current_thread);
-	    }
-	  else
-	    {
-	      struct inferior_list_entry *inf, *tmp;
-
-	      ALL_INFERIORS (&all_threads, inf, tmp)
-		{
-		  struct thread_info *thread = (struct thread_info *) inf;
-
-		  if (has_single_step_breakpoints (thread))
-		    delete_single_step_breakpoints (thread);
-		}
-	    }
-
-	  unstop_all_lwps (0, event_child);
-	}
+      /* If we remove single-step breakpoints from memory, stop all lwps,
+	 so that other threads won't hit invalid memory.  */
+      stop_all_lwps (0, event_child);
+      delete_single_step_breakpoints (current_thread);
+      unstop_all_lwps (0, event_child);
     }
 
   if (!stabilizing_threads)
@@ -4336,6 +4286,52 @@ install_software_single_step_breakpoints (struct lwp_info *lwp)
   do_cleanups (old_chain);
 }
 
+/* Install breakpoints for software single stepping on all threads.
+   Install the breakpoints on a thread only if COND returns true.
+   Return true if we stopped the threads while doing so and they need to be
+   restarted.
+*/
+
+static bool
+install_all_software_single_step_breakpoints (
+  bool (*cond)  (struct lwp_info *lwp))
+{
+  struct inferior_list_entry *inf, *tmp;
+  bool stopped_threads = false;
+
+  ALL_INFERIORS (&all_threads, inf, tmp)
+    {
+      struct thread_info *thread = (struct thread_info *) inf;
+      struct lwp_info *lwp = get_thread_lwp (thread);
+
+      if (cond (lwp))
+	{
+	  if (!stopped_threads)
+	    {
+	      /* We need to stop all threads to modify the inferior
+		 instructions safely.  */
+	      stop_all_lwps (0, NULL);
+
+	      if (debug_threads)
+		debug_printf ("Done stopping all threads.\n");
+
+	      stopped_threads = true;
+	    }
+
+	  if (!has_single_step_breakpoints (thread))
+	    {
+	      install_software_single_step_breakpoints (lwp);
+
+	      if (debug_threads)
+		debug_printf ("Insert breakpoint for resume_step LWP %ld\n",
+			      lwpid_of (thread));
+	    }
+	}
+    }
+
+  return stopped_threads;
+}
+
 /* Single step via hardware or software single step.
    Return 1 if hardware single stepping, 0 if software single stepping
    or can't single step.  */
@@ -5178,6 +5174,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;
+  bool stopped_threads = false;
 
   if (debug_threads)
     {
@@ -5221,12 +5218,29 @@ 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 ())
+    {
+      stopped_threads = install_all_software_single_step_breakpoints
+	([] (struct lwp_info *lwp)
+	 {
+	   return (lwp->resume != NULL && lwp->resume->kind == resume_step);
+	 });
+
+      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 (stopped_threads)
+    unstop_all_lwps (0, NULL);
 
   if (debug_threads)
     {
@@ -5323,13 +5337,6 @@ proceed_one_lwp (struct inferior_list_entry *entry, void *except)
 	debug_printf ("   stepping LWP %ld, client wants it stepping\n",
 		      lwpid_of (thread));
 
-      /* If resume_step is requested by GDB, install single-step
-	 breakpoints when the thread is about to be actually resumed if
-	 the single-step breakpoints weren't removed.  */
-      if (can_software_single_step ()
-	  && !has_single_step_breakpoints (thread))
-	install_software_single_step_breakpoints (lwp);
-
       step = maybe_hw_step (thread);
     }
   else if (lwp->bp_reinsert != 0)
@@ -5370,6 +5377,16 @@ proceed_all_lwps (void)
 {
   struct thread_info *need_step_over;
 
+  /* Always install software single step breakpoints if any.  */
+  if (can_software_single_step ())
+    {
+      install_all_software_single_step_breakpoints
+	([] (struct lwp_info *lwp)
+	 {
+	   return (get_lwp_thread (lwp)->last_resume_kind == resume_step);
+	 });
+    }
+
   /* If there is a thread which would otherwise be resumed, which is
      stopped at a breakpoint that needs stepping over, then don't
      resume any threads - have it step over the breakpoint with all
-- 
2.9.2

Re: [PATCH 1/2] This patch fixes GDBServer's run control for single stepping

[Antoine Tremblay]

Oops the subject got messed-up a bit subject should read:

Fix GDBServer's run control for single stepping

Then 1st line:

This patch fixes GDBServer's run control for single stepping.


Sorry about that

Re: [PATCH 2/2] Avoid step-over infinite loop in GDBServer

[Antoine Tremblay]

Ping.

Antoine Tremblay writes:

> Before this patch, GDBServer always executed a step-over if it found a
> thread that needed one.
>
> This could be a problem in a situation exposed by non-stop-fair-events.exp
> where the code and the breakpoint placement is like so:
>
> instruction A : has a single-step breakpoint installed for thread 1 and 2
> instruction B : has a single-step breakpoint installed for thread 3
> and is a branch to A.
>
> In this particular case:
>
>  - GDBServer stops on instruction A in thread 1.
>  - Deletes thread 1 single-step breakpoint.
>  - Starts a step-over of thread 1 to step-over the thread 2 breakpoint.
>  - GDBServer finishes a step-over and is at instruction B.
>  - GDBserver starts a step-over of thread 1 to step-over the thread 3
>    breakpoint at instruction B.
>  - GDBServer stops on instuction A in thread 1.
>  - GDBServer is now in an infinite loop.
>
> This patch avoids this issue by counting the number of times a thread does
> a step-over consecutively.  If the thread reaches a threshold, which is
> currently 32, GDBServer will not step-over but rather restart all the
> threads.
>
> I chose a threshold of 32, so to trigger this there needs to be 32
> consecutive instructions with breakpoints installed that one thread needs
> to step over. I think this should be rare enought to trigger only in this
> case but suggestions on the threshold value are welcome.
>
> If the threshold is hit and the step-over is delayed, when the thread
> that needed a step-over runs it will simply hit the breakpoint it needed
> to step-over and retry to start a step-over.
>
> This makes it possible for other threads to run and start a step-over
> dance of their own or pending events like signals to be handled.
>
> This patch fixes the intermittent FAILs for
> gdb.threads/non-stop-fair-events.exp on ARM like discussed here:
> https://sourceware.org/ml/gdb-patches/2016-11/msg00132.html
>
> No regressions, tested on ubuntu 14.04 ARMv7.
> With gdbserver-native/-m{arm,thumb}
>
> gdb/gdbserver/ChangeLog:
>
> 	* linux-low.c (class step_over_limiter): New class.
> 	(_step_over_limiter): New global variable.
> 	(linux_wait_1): Count step-overs.
> 	(proceed_all_lwps): Delay step-over if threshold is reached.
> ---
>  gdb/gdbserver/linux-low.c | 76 +++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 70 insertions(+), 6 deletions(-)
>
> diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
> index 15fb726..b84b62a 100644
> --- a/gdb/gdbserver/linux-low.c
> +++ b/gdb/gdbserver/linux-low.c
> @@ -282,6 +282,53 @@ static int proceed_one_lwp (struct inferior_list_entry *entry, void *except);
>     being stepped.  */
>  ptid_t step_over_bkpt;
>  
> +/* Class limiting the number of consecutive step-overs for a thread.  */
> +
> +class step_over_limiter
> +{
> + public:
> +
> +  step_over_limiter () : m_ptid (null_ptid), m_count (0), m_max_count (32) {}
> +
> +  void step_over_done (struct lwp_info *lwp)
> +  {
> +    ptid_t ptid = lwp->thread->entry.id;
> +
> +    if (!ptid_equal (ptid, m_ptid))
> +      {
> +	m_ptid = ptid;
> +	m_count = 0;
> +      }
> +
> +    m_count++;
> +  }
> +
> +  bool can_step_over (struct lwp_info *lwp)
> +  {
> +    if (!ptid_equal(lwp->thread->entry.id, m_ptid)
> +	|| m_count < m_max_count)
> +      return true;
> +    else
> +      {
> +	/* Reset here so that the step_over is retried.  */
> +	m_ptid = null_ptid;
> +	m_count = 0;
> +	return false;
> +      }
> +  }
> +
> + private:
> +
> +  ptid_t m_ptid;
> +  int m_count;
> +
> +  /* Maximum step overs for a thread, before all threads are run instead of
> +     stepping over.  */
> +  const int m_max_count;
> +};
> +
> +step_over_limiter _step_over_limiter;
> +
>  /* True if the low target can hardware single-step.  */
>  
>  static int
> @@ -3607,6 +3654,8 @@ linux_wait_1 (ptid_t ptid,
>  	     doesn't lose it.  */
>  	  enqueue_pending_signal (event_child, WSTOPSIG (w), info_p);
>  
> +	  _step_over_limiter.step_over_done (event_child);
> +
>  	  proceed_all_lwps ();
>  	}
>        else
> @@ -3694,6 +3743,8 @@ linux_wait_1 (ptid_t ptid,
>  	     We're going to keep waiting, so use proceed, which
>  	     handles stepping over the next breakpoint.  */
>  	  unsuspend_all_lwps (event_child);
> +
> +	  _step_over_limiter.step_over_done (event_child);
>  	}
>        else
>  	{
> @@ -5400,13 +5451,26 @@ proceed_all_lwps (void)
>  
>        if (need_step_over != NULL)
>  	{
> -	  if (debug_threads)
> -	    debug_printf ("proceed_all_lwps: found "
> -			  "thread %ld needing a step-over\n",
> -			  lwpid_of (need_step_over));
> +	  /* Don't step over if we're looping too much.  */
> +	  if (_step_over_limiter.can_step_over
> +	      (get_thread_lwp (need_step_over)))
> +	    {
> +	      if (debug_threads)
> +		debug_printf ("proceed_all_lwps: found "
> +			      "thread %ld needing a step-over\n",
> +			      lwpid_of (need_step_over));
>  
> -	  start_step_over (get_thread_lwp (need_step_over));
> -	  return;
> +	      start_step_over (get_thread_lwp (need_step_over));
> +	      return;
> +	    }
> +	  else
> +	    {
> +	      if (debug_threads)
> +		debug_printf ("proceed_all_lwps: found "
> +			      "thread %ld needing a step-over "
> +			      "but max consecutive step-overs reached\n",
> +			      lwpid_of (need_step_over));
> +	    }
>  	}
>      }

Re: [PATCH 1/2] This patch fixes GDBServer's run control for single stepping

[Antoine Tremblay]

Ping.

Antoine Tremblay writes:

> ** Note these patches depend on:
> https://sourceware.org/ml/gdb-patches/2016-11/msg00914.html
>
> Before, installing single-step breakpoints was done in proceed_one_lwp as
> each thread was started.
>
> This caused a problem on ARM, which is the only architecture using
> software single-step on which it is not safe to modify an instruction
> to insert a breakpoint in one thread while the other threads are running.
> See the architecture manual section "A3.5.4 Concurrent modification and
> execution of instructions":
>
> "The ARMv7 architecture limits the set of instructions that can be
> executed by one thread of execution as they are being modified by another
> thread of execution without requiring explicit synchronization.  Except
> for the instructions identified in this section, the effect of the
> concurrent modification and execution of an instruction is UNPREDICTABLE
> ."
>
> Since we want the single-step logic to work for any instruction GDBServer
> needs to stop all the threads to install a single-step breakpoint.
>
> Note that we could introduce arch specific code to check if we can do it
> for each particular instruction but I think that introducing 2 run control
> paths for single stepping would just add too much complexity to the code
> for little benefit.
>
> This patch fixes the intermittent FAILs for gdb.threads/schedlock.exp on
> ARM like discussed here:
> https://sourceware.org/ml/gdb-patches/2016-11/msg00132.html
> Tested with RACY_ITER=100 on two different boards, -marm/-thumb.
>
> Note that the FAILs in non-stop-fair-events.exp discussed in that thread
> will be fixed in an upcoming patch. They are not caused by what this patch
> fixes.
>
> Here's a few implementation details notes:
>
> Before in all-stop mode, GDBServer deleted all single-step breakpoints
> when reporting an event since it assumed that this canceled all previous
> resume requests.
>
> However, this is not true as GDBServer may hit a breakpoint instruction
> written directly in memory so not a GDB breakpoint nor a GDBServer
> breakpoint like is the case with displaced-stepping on.
>
> Considering this, this patch only deletes the current thread single-step
> breakpoint even in all-stop mode.
>
> Also with this patch, single-step breakpoints inserted in proceed_all_lwp
> are inserted before GDBServer checks if it needs to step-over.
>
> This is done in preparation for a patch that allows GDBServer to delay a
> step-over. In which case single-step breakpoints need to be inserted
> before trying to step-over.
>
> It may also be more clear that GDBServer always insert single-step
> breakpoints when calling proceed_all_lwps rather than delaying this
> insertion until a step-over is done.
>
> No regressions, tested on ubuntu 14.04 ARMv7.
> With gdbserver-native/-m{arm,thumb}
>
> gdb/gdbserver/ChangeLog:
>
> 	* linux-low.c (linux_wait_1): Don't remove all single-step
> 	breakpoints in all-stop.
> 	(install_all_software_single_step_breakpoints): New function.
> 	(linux_resume): Install software single-step breakpoints if needed.
> 	(proceed_one_lwp): Don't install software single-step here.
> 	(proceed_all_lwps): Install software single-step breakpoints if needed.
> ---
>  gdb/gdbserver/linux-low.c | 145 ++++++++++++++++++++++++++--------------------
>  1 file changed, 81 insertions(+), 64 deletions(-)
>
> diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
> index b441ebc..15fb726 100644
> --- a/gdb/gdbserver/linux-low.c
> +++ b/gdb/gdbserver/linux-low.c
> @@ -3747,64 +3747,14 @@ linux_wait_1 (ptid_t ptid,
>  
>    /* Alright, we're going to report a stop.  */
>  
> -  /* Remove single-step breakpoints.  */
> -  if (can_software_single_step ())
> +  if (can_software_single_step ()
> +      && has_single_step_breakpoints (current_thread))
>      {
> -      /* Remove single-step breakpoints or not.  It it is true, stop all
> -	 lwps, so that other threads won't hit the breakpoint in the
> -	 staled memory.  */
> -      int remove_single_step_breakpoints_p = 0;
> -
> -      if (non_stop)
> -	{
> -	  remove_single_step_breakpoints_p
> -	    = has_single_step_breakpoints (current_thread);
> -	}
> -      else
> -	{
> -	  /* In all-stop, a stop reply cancels all previous resume
> -	     requests.  Delete all single-step breakpoints.  */
> -	  struct inferior_list_entry *inf, *tmp;
> -
> -	  ALL_INFERIORS (&all_threads, inf, tmp)
> -	    {
> -	      struct thread_info *thread = (struct thread_info *) inf;
> -
> -	      if (has_single_step_breakpoints (thread))
> -		{
> -		  remove_single_step_breakpoints_p = 1;
> -		  break;
> -		}
> -	    }
> -	}
> -
> -      if (remove_single_step_breakpoints_p)
> -	{
> -	  /* If we remove single-step breakpoints from memory, stop all lwps,
> -	     so that other threads won't hit the breakpoint in the staled
> -	     memory.  */
> -	  stop_all_lwps (0, event_child);
> -
> -	  if (non_stop)
> -	    {
> -	      gdb_assert (has_single_step_breakpoints (current_thread));
> -	      delete_single_step_breakpoints (current_thread);
> -	    }
> -	  else
> -	    {
> -	      struct inferior_list_entry *inf, *tmp;
> -
> -	      ALL_INFERIORS (&all_threads, inf, tmp)
> -		{
> -		  struct thread_info *thread = (struct thread_info *) inf;
> -
> -		  if (has_single_step_breakpoints (thread))
> -		    delete_single_step_breakpoints (thread);
> -		}
> -	    }
> -
> -	  unstop_all_lwps (0, event_child);
> -	}
> +      /* If we remove single-step breakpoints from memory, stop all lwps,
> +	 so that other threads won't hit invalid memory.  */
> +      stop_all_lwps (0, event_child);
> +      delete_single_step_breakpoints (current_thread);
> +      unstop_all_lwps (0, event_child);
>      }
>  
>    if (!stabilizing_threads)
> @@ -4336,6 +4286,52 @@ install_software_single_step_breakpoints (struct lwp_info *lwp)
>    do_cleanups (old_chain);
>  }
>  
> +/* Install breakpoints for software single stepping on all threads.
> +   Install the breakpoints on a thread only if COND returns true.
> +   Return true if we stopped the threads while doing so and they need to be
> +   restarted.
> +*/
> +
> +static bool
> +install_all_software_single_step_breakpoints (
> +  bool (*cond)  (struct lwp_info *lwp))
> +{
> +  struct inferior_list_entry *inf, *tmp;
> +  bool stopped_threads = false;
> +
> +  ALL_INFERIORS (&all_threads, inf, tmp)
> +    {
> +      struct thread_info *thread = (struct thread_info *) inf;
> +      struct lwp_info *lwp = get_thread_lwp (thread);
> +
> +      if (cond (lwp))
> +	{
> +	  if (!stopped_threads)
> +	    {
> +	      /* We need to stop all threads to modify the inferior
> +		 instructions safely.  */
> +	      stop_all_lwps (0, NULL);
> +
> +	      if (debug_threads)
> +		debug_printf ("Done stopping all threads.\n");
> +
> +	      stopped_threads = true;
> +	    }
> +
> +	  if (!has_single_step_breakpoints (thread))
> +	    {
> +	      install_software_single_step_breakpoints (lwp);
> +
> +	      if (debug_threads)
> +		debug_printf ("Insert breakpoint for resume_step LWP %ld\n",
> +			      lwpid_of (thread));
> +	    }
> +	}
> +    }
> +
> +  return stopped_threads;
> +}
> +
>  /* Single step via hardware or software single step.
>     Return 1 if hardware single stepping, 0 if software single stepping
>     or can't single step.  */
> @@ -5178,6 +5174,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;
> +  bool stopped_threads = false;
>  
>    if (debug_threads)
>      {
> @@ -5221,12 +5218,29 @@ 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 ())
> +    {
> +      stopped_threads = install_all_software_single_step_breakpoints
> +	([] (struct lwp_info *lwp)
> +	 {
> +	   return (lwp->resume != NULL && lwp->resume->kind == resume_step);
> +	 });
> +
> +      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 (stopped_threads)
> +    unstop_all_lwps (0, NULL);
>  
>    if (debug_threads)
>      {
> @@ -5323,13 +5337,6 @@ proceed_one_lwp (struct inferior_list_entry *entry, void *except)
>  	debug_printf ("   stepping LWP %ld, client wants it stepping\n",
>  		      lwpid_of (thread));
>  
> -      /* If resume_step is requested by GDB, install single-step
> -	 breakpoints when the thread is about to be actually resumed if
> -	 the single-step breakpoints weren't removed.  */
> -      if (can_software_single_step ()
> -	  && !has_single_step_breakpoints (thread))
> -	install_software_single_step_breakpoints (lwp);
> -
>        step = maybe_hw_step (thread);
>      }
>    else if (lwp->bp_reinsert != 0)
> @@ -5370,6 +5377,16 @@ proceed_all_lwps (void)
>  {
>    struct thread_info *need_step_over;
>  
> +  /* Always install software single step breakpoints if any.  */
> +  if (can_software_single_step ())
> +    {
> +      install_all_software_single_step_breakpoints
> +	([] (struct lwp_info *lwp)
> +	 {
> +	   return (get_lwp_thread (lwp)->last_resume_kind == resume_step);
> +	 });
> +    }
> +
>    /* If there is a thread which would otherwise be resumed, which is
>       stopped at a breakpoint that needs stepping over, then don't
>       resume any threads - have it step over the breakpoint with all

Re: [PATCH 2/2] Avoid step-over infinite loop in GDBServer

[Antoine Tremblay]

Antoine Tremblay writes:

> Ping.
>

FYI, I just realised that this can break traceframes and breakpoint counts,
I'll try to fix that or find a better way.

Re: [PATCH 1/2] This patch fixes GDBServer's run control for single stepping

[Yao Qi]

On 16-11-29 07:07:01, Antoine Tremblay wrote:
> ** Note these patches depend on:
> https://sourceware.org/ml/gdb-patches/2016-11/msg00914.html
> 
> Before, installing single-step breakpoints was done in proceed_one_lwp as
> each thread was started.
> 
> This caused a problem on ARM, which is the only architecture using
> software single-step on which it is not safe to modify an instruction
> to insert a breakpoint in one thread while the other threads are running.
> See the architecture manual section "A3.5.4 Concurrent modification and
> execution of instructions":
> 
> "The ARMv7 architecture limits the set of instructions that can be
> executed by one thread of execution as they are being modified by another
> thread of execution without requiring explicit synchronization.  Except
> for the instructions identified in this section, the effect of the
> concurrent modification and execution of an instruction is UNPREDICTABLE
> ."

This doesn't apply to ptrace.  PTRACE_POKETEXT on a word aligned address
is atomic, so threads observe the coherent result, either breakpoint
instruction or the original instruction.  We don't see any fails in -marm
mode, do we?

In -mthumb mode, breakpoint can be 16-bit or 32-bit, but GDBserver still
PTRACE_POKETEXT on a word aligned address (in linux-low.c:linux_write_memory)
and write a word each time.  It should be atomic, however, if the 32-bit
thumb-2 instruction is 2-byte aligned, we end up two PTRACE_POKETEXT,
which is non-atomic.  That is the root cause of the problem, AFAICS.

32-bit thumb-2 breakpoint was invented for single step 32-bit thumb-2
instruction in IT block,
http://lists.infradead.org/pipermail/linux-arm-kernel/2010-January/007488.html
but we can use 16-bit thumb breakpoint instruction anywhere else.  If I
force GDBserver to use 16-bit thumb breakpoint instruction even on 32-bit
thumb-2 instruction, I can't see any fails in schedlock.exp anymore!  See
the patch below.

Out of IT block, we can use 16-bit thumb breakpoint for any thumb code.  In
IT block, we can use 16-bit thumb breakpoint for 16-bit instruction, and
32-bit thumb-2 breakpoint for 4-byte aligned 32-bit thumb-2 instruction.
However, we can not use either 16-bit or 32-bit breakpoint for 2-byte
aligned 32-bit thumb-2 instruction in IT block.

The reason we can't use 16-bit breakpoint on a 32-bit instruction in IT
block is that 16-bit breakpoint instruction makes the second half of
32-bit instruction to another different 16-bit instruction, and the 16-bit
breakpoint instruction may not be executed at all, so the second half
of instruction may be executed, and the result is completely unknown.
GDB sets two breakpoints on two branches, "if" branch and "else" branch,
because GDB doesn't know how does the instruction to be executed affect
the flag.

	      /* There are conditional instructions after this one.
		 If this instruction modifies the flags, then we can
		 not predict what the next executed instruction will
		 be.  Fortunately, this instruction is architecturally
		 forbidden to branch; we know it will fall through.
		 Start by skipping past it.  */

GDB/GDBserver has to emulate the instruction on how does it affect the
flag, and only insert the breakpoint on the "true" branch.  Since the
target instruction will be definitely executed, we can safely use
16-bit breakpoint instruction.

-- 
Yao (齐尧)

diff --git a/gdb/gdbserver/linux-aarch32-low.c b/gdb/gdbserver/linux-aarch32-low.c
index 2b710ba..789e0e6 100644
--- a/gdb/gdbserver/linux-aarch32-low.c
+++ b/gdb/gdbserver/linux-aarch32-low.c
@@ -243,7 +243,7 @@ arm_breakpoint_kind_from_pc (CORE_ADDR *pcptr)
 
 	  target_read_memory (*pcptr, (gdb_byte *) &inst1, 2);
 	  if (thumb_insn_size (inst1) == 4)
-	    return ARM_BP_KIND_THUMB2;
+	    return ARM_BP_KIND_THUMB;
 	}
       return ARM_BP_KIND_THUMB;
     }

Re: [PATCH 1/2] This patch fixes GDBServer's run control for single stepping

[Antoine Tremblay]

Yao Qi writes:

> On 16-11-29 07:07:01, Antoine Tremblay wrote:
>> ** Note these patches depend on:
>> https://sourceware.org/ml/gdb-patches/2016-11/msg00914.html
>>
>> Before, installing single-step breakpoints was done in proceed_one_lwp as
>> each thread was started.
>>
>> This caused a problem on ARM, which is the only architecture using
>> software single-step on which it is not safe to modify an instruction
>> to insert a breakpoint in one thread while the other threads are running.
>> See the architecture manual section "A3.5.4 Concurrent modification and
>> execution of instructions":
>>
>> "The ARMv7 architecture limits the set of instructions that can be
>> executed by one thread of execution as they are being modified by another
>> thread of execution without requiring explicit synchronization.  Except
>> for the instructions identified in this section, the effect of the
>> concurrent modification and execution of an instruction is UNPREDICTABLE
>> ."
>
> This doesn't apply to ptrace.  PTRACE_POKETEXT on a word aligned address
> is atomic, so threads observe the coherent result, either breakpoint
> instruction or the original instruction.  We don't see any fails in -marm
> mode, do we?
>

Indeed.

> In -mthumb mode, breakpoint can be 16-bit or 32-bit, but GDBserver still
> PTRACE_POKETEXT on a word aligned address (in linux-low.c:linux_write_memory)
> and write a word each time.  It should be atomic, however, if the 32-bit
> thumb-2 instruction is 2-byte aligned, we end up two PTRACE_POKETEXT,
> which is non-atomic.  That is the root cause of the problem, AFAICS.
>

Haaa makes total sense now :) thx!

> 32-bit thumb-2 breakpoint was invented for single step 32-bit thumb-2
> instruction in IT block,
> http://lists.infradead.org/pipermail/linux-arm-kernel/2010-January/007488.html
> but we can use 16-bit thumb breakpoint instruction anywhere else.  If I
> force GDBserver to use 16-bit thumb breakpoint instruction even on 32-bit
> thumb-2 instruction, I can't see any fails in schedlock.exp anymore!  See
> the patch below.
>
> Out of IT block, we can use 16-bit thumb breakpoint for any thumb code.  In
> IT block, we can use 16-bit thumb breakpoint for 16-bit instruction, and
> 32-bit thumb-2 breakpoint for 4-byte aligned 32-bit thumb-2 instruction.
> However, we can not use either 16-bit or 32-bit breakpoint for 2-byte
> aligned 32-bit thumb-2 instruction in IT block.
>
> The reason we can't use 16-bit breakpoint on a 32-bit instruction in IT
> block is that 16-bit breakpoint instruction makes the second half of
> 32-bit instruction to another different 16-bit instruction, and the 16-bit
> breakpoint instruction may not be executed at all, so the second half
> of instruction may be executed, and the result is completely unknown.
> GDB sets two breakpoints on two branches, "if" branch and "else" branch,
> because GDB doesn't know how does the instruction to be executed affect
> the flag.
>
> 	      /* There are conditional instructions after this one.
> 		 If this instruction modifies the flags, then we can
> 		 not predict what the next executed instruction will
> 		 be.  Fortunately, this instruction is architecturally
> 		 forbidden to branch; we know it will fall through.
> 		 Start by skipping past it.  */
>
> GDB/GDBserver has to emulate the instruction on how does it affect the
> flag, and only insert the breakpoint on the "true" branch.  Since the
> target instruction will be definitely executed, we can safely use
> 16-bit breakpoint instruction.

Ouch, reading the kernel thread it looks like this emulation would be
complex to say the least.

I think it would be better to get the current single stepping working
with the stop all threads logic since GDBServer was working like that
when GDB was doing the single stepping anyway. This would fix the current
regression.

Then work could be done to improve GDBServer to be better at
non-stopping.

WDYT ?

Thanks,
Antoine

Re: [PATCH 1/2] This patch fixes GDBServer's run control for single stepping

[Yao Qi]

On Fri, Jan 27, 2017 at 4:06 PM, Antoine Tremblay
<antoine.tremblay@ericsson.com> wrote:
>> GDB/GDBserver has to emulate the instruction on how does it affect the
>> flag, and only insert the breakpoint on the "true" branch.  Since the
>> target instruction will be definitely executed, we can safely use
>> 16-bit breakpoint instruction.
>
> Ouch, reading the kernel thread it looks like this emulation would be
> complex to say the least.
>

There are some other ideas discussed in the kernel threads, but I didn't
go through them.  They may work.  If emulation is complex, probably
we can partially fix this problem by "always using 16-bit thumb instruction
if program is out of IT block".

> I think it would be better to get the current single stepping working
> with the stop all threads logic since GDBServer was working like that
> when GDB was doing the single stepping anyway. This would fix the current
> regression.
>
> Then work could be done to improve GDBServer to be better at
> non-stopping.
>
> WDYT ?
>

Sounds like we are applying the ARM linux limitation to a general place.
Other software single step targets may write breakpoint in atomic way,
so we don't need to stop all threads.  Even in -marm mode, we don't
have to stop all threads on inserting breakpoints.

-- 
Yao (齐尧)

Re: [PATCH 1/2] This patch fixes GDBServer's run control for single stepping

[Antoine Tremblay]

Yao Qi writes:

> On Fri, Jan 27, 2017 at 4:06 PM, Antoine Tremblay
> <antoine.tremblay@ericsson.com> wrote:
>>> GDB/GDBserver has to emulate the instruction on how does it affect the
>>> flag, and only insert the breakpoint on the "true" branch.  Since the
>>> target instruction will be definitely executed, we can safely use
>>> 16-bit breakpoint instruction.
>>
>> Ouch, reading the kernel thread it looks like this emulation would be
>> complex to say the least.
>>
>
> There are some other ideas discussed in the kernel threads, but I didn't
> go through them.  They may work.

There was this one
http://lists.infradead.org/pipermail/linux-arm-kernel/2010-January/007515.html

But it got discarded: http://lists.infradead.org/pipermail/linux-arm-kernel/2010-January/007517.html

> If emulation is complex, probably
> we can partially fix this problem by "always using 16-bit thumb instruction
> if program is out of IT block".
>

I would be against that since it would leave the feature partially
working and crashing the program when it fails...

It would also be a regression compared to previous GDBServer.
Also, IT blocks are a common place to have a breakpoint/tracepoint.

>> I think it would be better to get the current single stepping working
>> with the stop all threads logic since GDBServer was working like that
>> when GDB was doing the single stepping anyway. This would fix the current
>> regression.
>>
>> Then work could be done to improve GDBServer to be better at
>> non-stopping.
>>
>> WDYT ?
>>
>
> Sounds like we are applying the ARM linux limitation to a general
> place.
> Other software single step targets may write breakpoint in atomic way,
> so we don't need to stop all threads.  Even in -marm mode, we don't
> have to stop all threads on inserting breakpoints.

Well ARM is the only software single stepping target, while I agreee
that we would be affecting general code, I think that since there is
no software single step breakpoint target that supports atomic
breakpoint writes at the moment it's normal that the run control
doesn't support that.

I don't count -marm as an arch since there no way to check that all the
program including shared libs etc is -marm, I don't think we could make
the distinction in GDBServer AFAIK.

Should an arch support this in the future we could adapt the run control
to support both cases possibly via some arch specific property.

I think also that given the current trends in architecture design the
odds of another arch needing that in the future are also unlikely ?

WDYT ?

Re: [PATCH 1/2] This patch fixes GDBServer's run control for single stepping

[Yao Qi]

On Fri, Jan 27, 2017 at 6:24 PM, Antoine Tremblay
<antoine.tremblay@ericsson.com> wrote:
>
> Yao Qi writes:
>
>> On Fri, Jan 27, 2017 at 4:06 PM, Antoine Tremblay
>> <antoine.tremblay@ericsson.com> wrote:

>> If emulation is complex, probably
>> we can partially fix this problem by "always using 16-bit thumb instruction
>> if program is out of IT block".
>>
>
> I would be against that since it would leave the feature partially
> working and crashing the program when it fails...
>
> It would also be a regression compared to previous GDBServer.

There won't be any regression.  16-bit thumb breakpoint works pretty well
for any thumb instructions (16-bit and 32-bit) if program is out of IT block.
The 32-bit thumb-2 breakpoint was added for single step IT block.

> Also, IT blocks are a common place to have a breakpoint/tracepoint.
>

We don't change anything when setting breakpoint inside IT block.

>>> I think it would be better to get the current single stepping working
>>> with the stop all threads logic since GDBServer was working like that
>>> when GDB was doing the single stepping anyway. This would fix the current
>>> regression.
>>>
>>> Then work could be done to improve GDBServer to be better at
>>> non-stopping.
>>>
>>> WDYT ?
>>>
>>
>> Sounds like we are applying the ARM linux limitation to a general
>> place.
>> Other software single step targets may write breakpoint in atomic way,
>> so we don't need to stop all threads.  Even in -marm mode, we don't
>> have to stop all threads on inserting breakpoints.
>
> Well ARM is the only software single stepping target, while I agreee
> that we would be affecting general code, I think that since there is
> no software single step breakpoint target that supports atomic
> breakpoint writes at the moment it's normal that the run control
> doesn't support that.

No, ARM Linux ptrace POKETEXT is _atomic_ if the address is 4-byte
aligned, so 32-bit arm breakpoint and 16-bit thumb breakpoint can be
written atomically.  32-bit thumb-2 breakpoint can be written atomically
too if the address is 4-byte aligned.

The only problem we have is inserting a breakpoint on a 2-byte aligned
32-bit thumb-2 instruction inside IT block, we can not use neither 16-bit thumb
breakpoint nor 32-bit thumb breakpoint.  Everything works fine in other
cases.

>
> I don't count -marm as an arch since there no way to check that all the
> program including shared libs etc is -marm, I don't think we could make
> the distinction in GDBServer AFAIK.

We can check with -mthumb.  My hack in last email fixes fails in
schedlock.exp in thumb mode.

-- 
Yao (齐尧)

Re: [PATCH 1/2] This patch fixes GDBServer's run control for single stepping

[Antoine Tremblay]

Yao Qi writes:

> On Fri, Jan 27, 2017 at 6:24 PM, Antoine Tremblay
> <antoine.tremblay@ericsson.com> wrote:
>>
>> Yao Qi writes:
>>
>>> On Fri, Jan 27, 2017 at 4:06 PM, Antoine Tremblay
>>> <antoine.tremblay@ericsson.com> wrote:
>
>>> If emulation is complex, probably
>>> we can partially fix this problem by "always using 16-bit thumb instruction
>>> if program is out of IT block".
>>>
>>
>> I would be against that since it would leave the feature partially
>> working and crashing the program when it fails...
>>
>> It would also be a regression compared to previous GDBServer.
>
> There won't be any regression.  16-bit thumb breakpoint works pretty well
> for any thumb instructions (16-bit and 32-bit) if program is out of IT block.
> The 32-bit thumb-2 breakpoint was added for single step IT block.

Yes so there will be a regression for single step inside an IT block if
we keep using the 32 bit breakpoint since this one can be non atomic if
it's not aligned properly.

We could write a test case for it and it would fail like schedlock
did. But it would not with the single stepping controlled by GDB like it
was before.

>
>> Also, IT blocks are a common place to have a breakpoint/tracepoint.
>>
>
> We don't change anything when setting breakpoint inside IT block.

Well that's a problem if we write a 32 bit thumb2 breakpoint aligned on
2 bytes like discussed before.

>
>>>> I think it would be better to get the current single stepping working
>>>> with the stop all threads logic since GDBServer was working like that
>>>> when GDB was doing the single stepping anyway. This would fix the current
>>>> regression.
>>>>
>>>> Then work could be done to improve GDBServer to be better at
>>>> non-stopping.
>>>>
>>>> WDYT ?
>>>>
>>>
>>> Sounds like we are applying the ARM linux limitation to a general
>>> place.
>>> Other software single step targets may write breakpoint in atomic way,
>>> so we don't need to stop all threads.  Even in -marm mode, we don't
>>> have to stop all threads on inserting breakpoints.
>>
>> Well ARM is the only software single stepping target, while I agreee
>> that we would be affecting general code, I think that since there is
>> no software single step breakpoint target that supports atomic
>> breakpoint writes at the moment it's normal that the run control
>> doesn't support that.
>
> No, ARM Linux ptrace POKETEXT is _atomic_ if the address is 4-byte
> aligned, so 32-bit arm breakpoint and 16-bit thumb breakpoint can be
> written atomically.  32-bit thumb-2 breakpoint can be written atomically
> too if the address is 4-byte aligned.
>
> The only problem we have is inserting a breakpoint on a 2-byte aligned
> 32-bit thumb-2 instruction inside IT block, we can not use neither 16-bit thumb
> breakpoint nor 32-bit thumb breakpoint.  Everything works fine in other
> cases.
>

I mean software single stepping as a whole, which means considering all
cases, is not atomic. I don't see how we could leave that case
unaddressed ?

Especially since it will crash the inferior ?

>>
>> I don't count -marm as an arch since there no way to check that all the
>> program including shared libs etc is -marm, I don't think we could make
>> the distinction in GDBServer AFAIK.
>
> We can check with -mthumb.  My hack in last email fixes fails in
> schedlock.exp in thumb mode.

Yes but schedlock.exp is not made to test the 2 byte aligned thumb2
32-bit breakpoint IIRC.

Re: [PATCH 1/2] This patch fixes GDBServer's run control for single stepping

[Pedro Alves]

On 01/30/2017 01:28 PM, Antoine Tremblay wrote:

>> We don't change anything when setting breakpoint inside IT block.
> 
> Well that's a problem if we write a 32 bit thumb2 breakpoint aligned on
> 2 bytes like discussed before.

Can we restrict the stopping-all-threads to the case that
needs it, only?

An optimization that would avoid this would be to use a
hardware watchpoint/breakpoint (if available) for single-stepping.
IIRC, you can ARM a breakpoint (or was it a watchpoint) on ARM for 
triggering when the PC is different from the current PC (or really,
some specified address).

In IT blocks, this would probably make the thread stop on
instructions that aren't really executed (e.g., the then/else
branches when the condition is correspondingly false/true),
unlike the current solution where breakpoint instructions are
not executed by the CPU when it falls on an instruction that
isn't executed (because the breakpoint opcode we use it just
some magic invalid instruction, node the BKPT instruction), but
I think that when the thread stops, and we're stepping an IT
block, we could look at the status registers and decide whether
to single-step again.

TBC, I'm not suggesting that we need to do that right now.

The emulation solution discussed on the lkml thread made
me thing of displaced stepping -- the ARM implementation
emulates some instructions.  I wonder whether displaced
stepping over IT blocks is already handled correctly.

Thanks,
Pedro Alves

Re: [PATCH 2/2] Avoid step-over infinite loop in GDBServer

[Pedro Alves]

On 11/29/2016 12:07 PM, Antoine Tremblay wrote:
>  - GDBServer stops on instruction A in thread 1.
>  - Deletes thread 1 single-step breakpoint.
>  - Starts a step-over of thread 1 to step-over the thread 2 breakpoint.
>  - GDBServer finishes a step-over and is at instruction B.
>  - GDBserver starts a step-over of thread 1 to step-over the thread 3
>    breakpoint at instruction B.
>  - GDBServer stops on instuction A in thread 1.
>  - GDBServer is now in an infinite loop.

This sounds to me very much like a fairness issue.  There were
three threads stopped that needed to move past a breakpoint, but
gdbserver always picks thread 1.  Why?

Thanks,
Pedro Alves

Re: [PATCH 1/2] This patch fixes GDBServer's run control for single stepping

[Yao Qi]

On 17-01-30 08:28:29, Antoine Tremblay wrote:
>
> Yao Qi writes:
>
> > On Fri, Jan 27, 2017 at 6:24 PM, Antoine Tremblay
> > <antoine.tremblay@ericsson.com> wrote:
> >>
> >> Yao Qi writes:
> >>
> >>> On Fri, Jan 27, 2017 at 4:06 PM, Antoine Tremblay
> >>> <antoine.tremblay@ericsson.com> wrote:
> >
> >>> If emulation is complex, probably
> >>> we can partially fix this problem by "always using 16-bit thumb instruction
> >>> if program is out of IT block".
> >>>
> >>
> >> I would be against that since it would leave the feature partially
> >> working and crashing the program when it fails...
> >>
> >> It would also be a regression compared to previous GDBServer.
> >
> > There won't be any regression.  16-bit thumb breakpoint works pretty well
> > for any thumb instructions (16-bit and 32-bit) if program is out of IT block.
> > The 32-bit thumb-2 breakpoint was added for single step IT block.
>
> Yes so there will be a regression for single step inside an IT block if
> we keep using the 32 bit breakpoint since this one can be non atomic if
> it's not aligned properly.
>

It does fail, but not a regression, because current GDBserver fails to
write 32-bit thumb breakpoint to 2-byte aligned 32-bit instruction
atomically, regardless the program is within IT block or not.  My
suggestion is that "let us fix this problem when program is out of IT
block by using 16-bit thumb breakpoint".  That will fix the issue
of atomic write when program is out of IT block, and leave the problem
there when program is within IT block.  Why is it a regression?

> We could write a test case for it and it would fail like schedlock
> did. But it would not with the single stepping controlled by GDB like it
> was before.
>
> >
> >> Also, IT blocks are a common place to have a breakpoint/tracepoint.
> >>
> >
> > We don't change anything when setting breakpoint inside IT block.
>
> Well that's a problem if we write a 32 bit thumb2 breakpoint aligned on
> 2 bytes like discussed before.
>

That is just the sub-set of the original problem.

> >
> >>>> I think it would be better to get the current single stepping working
> >>>> with the stop all threads logic since GDBServer was working like that
> >>>> when GDB was doing the single stepping anyway. This would fix the current
> >>>> regression.
> >>>>
> >>>> Then work could be done to improve GDBServer to be better at
> >>>> non-stopping.
> >>>>
> >>>> WDYT ?
> >>>>
> >>>
> >>> Sounds like we are applying the ARM linux limitation to a general
> >>> place.
> >>> Other software single step targets may write breakpoint in atomic way,
> >>> so we don't need to stop all threads.  Even in -marm mode, we don't
> >>> have to stop all threads on inserting breakpoints.
> >>
> >> Well ARM is the only software single stepping target, while I agreee
> >> that we would be affecting general code, I think that since there is
> >> no software single step breakpoint target that supports atomic
> >> breakpoint writes at the moment it's normal that the run control
> >> doesn't support that.
> >
> > No, ARM Linux ptrace POKETEXT is _atomic_ if the address is 4-byte
> > aligned, so 32-bit arm breakpoint and 16-bit thumb breakpoint can be
> > written atomically.  32-bit thumb-2 breakpoint can be written atomically
> > too if the address is 4-byte aligned.
> >
> > The only problem we have is inserting a breakpoint on a 2-byte aligned
> > 32-bit thumb-2 instruction inside IT block, we can not use neither 16-bit thumb
> > breakpoint nor 32-bit thumb breakpoint.  Everything works fine in other
> > cases.
> >
>
> I mean software single stepping as a whole, which means considering all
> cases, is not atomic. I don't see how we could leave that case
> unaddressed ?
>
> Especially since it will crash the inferior ?
>

I am thinking how do we make progress here.  Nowadays, the multi-threaded
program may crash almost everywhere, but we can partially fix it to an
extent that the multi-threaded program may only crash in side IT block.
Is it a progress?  I feel it is a good example to apply "divide and
conquer" to a complicated engineering problem.  We can easily fix the
first half of this problem, and then think about the second half.

> >>
> >> I don't count -marm as an arch since there no way to check that all the
> >> program including shared libs etc is -marm, I don't think we could make
> >> the distinction in GDBServer AFAIK.
> >
> > We can check with -mthumb.  My hack in last email fixes fails in
> > schedlock.exp in thumb mode.
>
> Yes but schedlock.exp is not made to test the 2 byte aligned thumb2
> 32-bit breakpoint IIRC.
>

We can write one test for single step 2-byte aligned thumb-2 32-bit
instruction.  No problem.

-- 
Yao (齐尧)

Re: [PATCH 1/2] This patch fixes GDBServer's run control for single stepping

[Antoine Tremblay]

Pedro Alves writes:

> On 01/30/2017 01:28 PM, Antoine Tremblay wrote:
>
>>> We don't change anything when setting breakpoint inside IT block.
>>
>> Well that's a problem if we write a 32 bit thumb2 breakpoint aligned on
>> 2 bytes like discussed before.
>

Sorry for the delay I just saw your mail...

> Can we restrict the stopping-all-threads to the case that
> needs it, only?

Possibly but I would like to avoid introducing 2 execution paths in the
run control, it's already hard to follow as it is and it would introduce
a lot of code in the arch independant code just for this case, that's
something I would like to avoid too.

>
> An optimization that would avoid this would be to use a
> hardware watchpoint/breakpoint (if available) for single-stepping.
> IIRC, you can ARM a breakpoint (or was it a watchpoint) on ARM for
> triggering when the PC is different from the current PC (or really,
> some specified address).

I did not know that but I'm worried about the limited number of hardware
watchpoints available. IIRC on my board I had only 4, if GDBServer can't
find one available would it refuse to single step ? That would be
weird... ?

It's an interesting approch however I'll dig about it more.

>
> In IT blocks, this would probably make the thread stop on
> instructions that aren't really executed (e.g., the then/else
> branches when the condition is correspondingly false/true),

Really ? I need to find something about that in the arch man...

> unlike the current solution where breakpoint instructions are
> not executed by the CPU when it falls on an instruction that
> isn't executed (because the breakpoint opcode we use it just
> some magic invalid instruction, node the BKPT instruction), but
> I think that when the thread stops, and we're stepping an IT
> block, we could look at the status registers and decide whether
> to single-step again.
>
> TBC, I'm not suggesting that we need to do that right now.
>
> The emulation solution discussed on the lkml thread made
> me thing of displaced stepping -- the ARM implementation
> emulates some instructions.  I wonder whether displaced
> stepping over IT blocks is already handled correctly.
>

It does look like displaced stepping would be preferable to trying to
emulate and validate that the emulation is correct.

Simon and I are looking into displaced stepping but we've yet to have
this discussion with Yao...

Re: [PATCH 1/2] This patch fixes GDBServer's run control for single stepping

[Pedro Alves]

On 02/17/2017 01:41 AM, Antoine Tremblay wrote:
> 
> Pedro Alves writes:
> 
>> On 01/30/2017 01:28 PM, Antoine Tremblay wrote:
>>
>>>> We don't change anything when setting breakpoint inside IT block.
>>>
>>> Well that's a problem if we write a 32 bit thumb2 breakpoint aligned on
>>> 2 bytes like discussed before.
>>
> 
> Sorry for the delay I just saw your mail...
> 
>> Can we restrict the stopping-all-threads to the case that
>> needs it, only?
> 
> Possibly but I would like to avoid introducing 2 execution paths in the
> run control, it's already hard to follow as it is and it would introduce
> a lot of code in the arch independant code just for this case, that's
> something I would like to avoid too.

I don't immediately see how this would imply introducing lots of code
in the run control.  We shouldn't be imposing this stop-all-threads
on all archs, since it adds a lot of slowness (and
the more threads the inferior has, the worse it gets).  So if
we already need the 2 execution paths, making the condition that
determines whether to pause all threads consider more state
doesn't seem to add that much complexity to the run control
part itself.

>> An optimization that would avoid this would be to use a
>> hardware watchpoint/breakpoint (if available) for single-stepping.
>> IIRC, you can ARM a breakpoint (or was it a watchpoint) on ARM for
>> triggering when the PC is different from the current PC (or really,
>> some specified address).
> 
> I did not know that but I'm worried about the limited number of hardware
> watchpoints available. IIRC on my board I had only 4, if GDBServer can't
> find one available would it refuse to single step ? That would be
> weird... ?

My thought was that we'd give preference to user-requested
watchpoints.  I.e., treat this as an optimization.  We always need to
pause all threads in order to install watchpoints in all
threads (watchpoints must be inserted with the thread paused, and
we do that on thread resume).  So if a request for a user-watchpoints
comes in, we'd just interrupt the current single-step as we currently
do, install the watchpoints, and go back to single-stepping, if it didn't
manage to finish, as we currently do.  At this point, we notice that we
don't have free hardware watchpoints left, and fallback to do
the slow software single-step as before.

> 
> It's an interesting approch however I'll dig about it more.
> 
>>
>> In IT blocks, this would probably make the thread stop on
>> instructions that aren't really executed (e.g., the then/else
>> branches when the condition is correspondingly false/true),
> 
> Really ? I need to find something about that in the arch man...

AFAIK, in IT blocks, all instructions always "execute", but, when
the condition is false, the instruction becomes as-if a nop.
The only reason breakpoints don't stop there currently is that
breakpoints are just another instruction (actually an undefined
instruction the kernel is aware of, that causes an undefined
instruction exception that the kernel translates to a SIGTRAP
instead of a SIGILL), and when the condition is false, the breakpoint
instruction becomes a nop too...  If you have a hardware trap
set to trap at such an address though, then it should trap, I believe,
as if you had armed a hardware breakpoint to trap on the address
of a real nop instruction.

Thanks,
Pedro Alves

Re: [PATCH 1/2] This patch fixes GDBServer's run control for single stepping

[Antoine Tremblay]

Yao Qi writes:

> On 17-01-30 08:28:29, Antoine Tremblay wrote:
>>
>> Yao Qi writes:
>>
>> > On Fri, Jan 27, 2017 at 6:24 PM, Antoine Tremblay
>> > <antoine.tremblay@ericsson.com> wrote:
>> >>
>> >> Yao Qi writes:
>> >>
>> >>> On Fri, Jan 27, 2017 at 4:06 PM, Antoine Tremblay
>> >>> <antoine.tremblay@ericsson.com> wrote:
>> >
>> >>> If emulation is complex, probably
>> >>> we can partially fix this problem by "always using 16-bit thumb instruction
>> >>> if program is out of IT block".
>> >>>
>> >>
>> >> I would be against that since it would leave the feature partially
>> >> working and crashing the program when it fails...
>> >>
>> >> It would also be a regression compared to previous GDBServer.
>> >
>> > There won't be any regression.  16-bit thumb breakpoint works pretty well
>> > for any thumb instructions (16-bit and 32-bit) if program is out of IT block.
>> > The 32-bit thumb-2 breakpoint was added for single step IT block.
>>
>> Yes so there will be a regression for single step inside an IT block if
>> we keep using the 32 bit breakpoint since this one can be non atomic if
>> it's not aligned properly.
>>
>
> It does fail, but not a regression, because current GDBserver fails to
> write 32-bit thumb breakpoint to 2-byte aligned 32-bit instruction
> atomically, regardless the program is within IT block or not.

I mean it's a regression compared to GDB 7.11. In 7.11 GDBServer will
stop all threads write the 4 bytes and then restart all threads so the
issue is not present.

Yes it wasn't atomic before either but it did not create an issue due to
the stopping of the threads.

Or maybe I misunderstand what you mean... ?

> My
> suggestion is that "let us fix this problem when program is out of IT
> block by using 16-bit thumb breakpoint".  That will fix the issue
> of atomic write when program is out of IT block, and leave the problem
> there when program is within IT block.  Why is it a regression?
>

Well like I said before in 7.11 because GDBServer stopped the threads to
write the 4 bytes, it did not have to be atomic and schedlock.exp etc
passed, single stepping a program even with a IT block did not fail.

I would like to re-validate this since you're introducing doubt into my
mind but I can't at the moment, I hope my memory serves me right but I
have not retested this now.

>> We could write a test case for it and it would fail like schedlock
>> did. But it would not with the single stepping controlled by GDB like it
>> was before.
>>
>> >
>> >> Also, IT blocks are a common place to have a breakpoint/tracepoint.
>> >>
>> >
>> > We don't change anything when setting breakpoint inside IT block.
>>
>> Well that's a problem if we write a 32 bit thumb2 breakpoint aligned on
>> 2 bytes like discussed before.
>>
>
> That is just the sub-set of the original problem.
>
>> >
>> >>>> I think it would be better to get the current single stepping working
>> >>>> with the stop all threads logic since GDBServer was working like that
>> >>>> when GDB was doing the single stepping anyway. This would fix the current
>> >>>> regression.
>> >>>>
>> >>>> Then work could be done to improve GDBServer to be better at
>> >>>> non-stopping.
>> >>>>
>> >>>> WDYT ?
>> >>>>
>> >>>
>> >>> Sounds like we are applying the ARM linux limitation to a general
>> >>> place.
>> >>> Other software single step targets may write breakpoint in atomic way,
>> >>> so we don't need to stop all threads.  Even in -marm mode, we don't
>> >>> have to stop all threads on inserting breakpoints.
>> >>
>> >> Well ARM is the only software single stepping target, while I agreee
>> >> that we would be affecting general code, I think that since there is
>> >> no software single step breakpoint target that supports atomic
>> >> breakpoint writes at the moment it's normal that the run control
>> >> doesn't support that.
>> >
>> > No, ARM Linux ptrace POKETEXT is _atomic_ if the address is 4-byte
>> > aligned, so 32-bit arm breakpoint and 16-bit thumb breakpoint can be
>> > written atomically.  32-bit thumb-2 breakpoint can be written atomically
>> > too if the address is 4-byte aligned.
>> >
>> > The only problem we have is inserting a breakpoint on a 2-byte aligned
>> > 32-bit thumb-2 instruction inside IT block, we can not use neither 16-bit thumb
>> > breakpoint nor 32-bit thumb breakpoint.  Everything works fine in other
>> > cases.
>> >
>>
>> I mean software single stepping as a whole, which means considering all
>> cases, is not atomic. I don't see how we could leave that case
>> unaddressed ?
>>
>> Especially since it will crash the inferior ?
>>
>
> I am thinking how do we make progress here.  Nowadays, the multi-threaded
> program may crash almost everywhere, but we can partially fix it to an
> extent that the multi-threaded program may only crash in side IT block.
> Is it a progress?  I feel it is a good example to apply "divide and
> conquer" to a complicated engineering problem.  We can easily fix the
> first half of this problem, and then think about the second half.
>

Well I agree with the divide an conquer approach, I would just have
divided it another way by stopping all threads so that we fix all the
problem right now, and then think about the second half which would be
to allow non-stop operations.

The solution to the program crashing in the IT block seems non-trivial
and I don't know how much time will pass before a fix is done.  I'm
afraid this would linger for a long time but maybe I'm wrong, do you plan
to address the second part for 8.0 too ? 

I would feel better if GDB worked for all cases meanwhile.

This is more important to me than not stopping the threads, especially
since in 7.11 the threads were stopped.

My point is that if we can fix the problem completely now while we think
about a better solution isn't that preferable to leaving GDB partially
fixed ?

>> >>
>> >> I don't count -marm as an arch since there no way to check that all the
>> >> program including shared libs etc is -marm, I don't think we could make
>> >> the distinction in GDBServer AFAIK.
>> >
>> > We can check with -mthumb.  My hack in last email fixes fails in
>> > schedlock.exp in thumb mode.
>>
>> Yes but schedlock.exp is not made to test the 2 byte aligned thumb2
>> 32-bit breakpoint IIRC.
>>
>
> We can write one test for single step 2-byte aligned thumb-2 32-bit
> instruction.  No problem.

Re: [PATCH 1/2] This patch fixes GDBServer's run control for single stepping

[Antoine Tremblay]

Pedro Alves writes:

> On 02/17/2017 01:41 AM, Antoine Tremblay wrote:
>>
>> Pedro Alves writes:
>>
>>> On 01/30/2017 01:28 PM, Antoine Tremblay wrote:
>>>
>>>>> We don't change anything when setting breakpoint inside IT block.
>>>>
>>>> Well that's a problem if we write a 32 bit thumb2 breakpoint aligned on
>>>> 2 bytes like discussed before.
>>>
>>
>> Sorry for the delay I just saw your mail...
>>
>>> Can we restrict the stopping-all-threads to the case that
>>> needs it, only?
>>
>> Possibly but I would like to avoid introducing 2 execution paths in the
>> run control, it's already hard to follow as it is and it would introduce
>> a lot of code in the arch independant code just for this case, that's
>> something I would like to avoid too.
>
> I don't immediately see how this would imply introducing lots of code
> in the run control.

Well lots can be debatable, but at first glance you would basically need
this current posted patch + what it removes and a switch between the 2.

And I'm not sure how the IT block detection would be done.

It just seems like much to me, I'm actually very surprised that you're
suggesting having those two paths.

It seems like it creates much to maintain to support this particular
problem with the ARM breakpoints.

Maybe it's just that I had such a hard time getting all that run control
right, it's full of traps and corner cases with all-stop/non-stop,
stopping, suspending, deleting the breakpoints, re-adding them at the
right time etc... Adding more state is giving me quite a headache.

> We shouldn't be imposing this stop-all-threads
> on all archs, since it adds a lot of slowness (and
> the more threads the inferior has, the worse it gets).

I would be the first the agree usually that's something I would like to
avoid but considering that it was crashing the inferior in the only
architecture using this, not stopping seemed less important.

Also I'm not arguing to keep it like this forever but until there's a
better solution to the problem it seemed reasonable to me to take the
performance hit.

And I was pretty much certain that before another arch uses this we
would have figured it out.

Is there another arch on the horizon that would use this ?

>So if
> we already need the 2 execution paths, making the condition that
> determines whether to pause all threads consider more state
> doesn't seem to add that much complexity to the run control
> part itself.
>
>>> An optimization that would avoid this would be to use a
>>> hardware watchpoint/breakpoint (if available) for single-stepping.
>>> IIRC, you can ARM a breakpoint (or was it a watchpoint) on ARM for
>>> triggering when the PC is different from the current PC (or really,
>>> some specified address).
>>
>> I did not know that but I'm worried about the limited number of hardware
>> watchpoints available. IIRC on my board I had only 4, if GDBServer can't
>> find one available would it refuse to single step ? That would be
>> weird... ?
>
> My thought was that we'd give preference to user-requested
> watchpoints.  I.e., treat this as an optimization.  We always need to
> pause all threads in order to install watchpoints in all
> threads (watchpoints must be inserted with the thread paused, and
> we do that on thread resume).  So if a request for a user-watchpoints
> comes in, we'd just interrupt the current single-step as we currently
> do, install the watchpoints, and go back to single-stepping, if it didn't
> manage to finish, as we currently do.  At this point, we notice that we
> don't have free hardware watchpoints left, and fallback to do
> the slow software single-step as before.
>

OK I see. Interesting.

>>
>> It's an interesting approch however I'll dig about it more.
>>
>>>
>>> In IT blocks, this would probably make the thread stop on
>>> instructions that aren't really executed (e.g., the then/else
>>> branches when the condition is correspondingly false/true),
>>
>> Really ? I need to find something about that in the arch man...
>
> AFAIK, in IT blocks, all instructions always "execute", but, when
> the condition is false, the instruction becomes as-if a nop.
> The only reason breakpoints don't stop there currently is that
> breakpoints are just another instruction (actually an undefined
> instruction the kernel is aware of, that causes an undefined
> instruction exception that the kernel translates to a SIGTRAP
> instead of a SIGILL), and when the condition is false, the breakpoint
> instruction becomes a nop too...  If you have a hardware trap
> set to trap at such an address though, then it should trap, I believe,
> as if you had armed a hardware breakpoint to trap on the address
> of a real nop instruction.
>

Seems to make sense :) I'll test it out with a hardware breakpoint.

Thanks!,
Antoine

Re: [PATCH 2/2] Avoid step-over infinite loop in GDBServer

[Antoine Tremblay]

Pedro Alves writes:

> On 11/29/2016 12:07 PM, Antoine Tremblay wrote:
>>  - GDBServer stops on instruction A in thread 1.
>>  - Deletes thread 1 single-step breakpoint.
>>  - Starts a step-over of thread 1 to step-over the thread 2 breakpoint.
>>  - GDBServer finishes a step-over and is at instruction B.
>>  - GDBserver starts a step-over of thread 1 to step-over the thread 3
>>    breakpoint at instruction B.
>>  - GDBServer stops on instuction A in thread 1.
>>  - GDBServer is now in an infinite loop.
>
> This sounds to me very much like a fairness issue.  There were
> three threads stopped that needed to move past a breakpoint, but
> gdbserver always picks thread 1.  Why?

It is a fairness issue but not between the stepping over threads,
gdbserver could pick any thread rather than thread 1 and still get into
the same loop.

The problem is one of fairness between stepping over threads and
non-stepping-over threads.

For example if all the instructions in the thread's execution
path have a breakpoint on it such that this thread always needs to
step-over and that the only way to break that loop is to let a thread
that doesn't need a step-over run then we're in an infinite loop since
GDBServer always gives precedence to the threads needing a step-over.

Thus this patch created more fairness by giving non-stepping over
threads a chance to run.

But it doesn't work... I'm not sure this can be fixed it might be more
a problem like putting a breakpoint on a mutex and wanting a thread that
depends on that mutex to continue... so more a user problem... but
non-stop-fair-events is done in a way that this can happen...

Ideas are welcome :)

Re: [PATCH 1/2] This patch fixes GDBServer's run control for single stepping

[Yao Qi]

On Fri, Feb 17, 2017 at 3:05 AM, Antoine Tremblay
<antoine.tremblay@ericsson.com> wrote:
>
> And I'm not sure how the IT block detection would be done.
>

In ARM ARM, we have the pseudo code,

boolean InITBlock()
return (ITSTATE.IT<3:0> != ‘0000’);

ITSTATE can be got from CPSR.

-- 
Yao (齐尧)

Re: [PATCH 1/2] This patch fixes GDBServer's run control for single stepping

[Antoine Tremblay]

Yao Qi writes:

> On Fri, Feb 17, 2017 at 3:05 AM, Antoine Tremblay
> <antoine.tremblay@ericsson.com> wrote:
>>
>> And I'm not sure how the IT block detection would be done.
>>
>
> In ARM ARM, we have the pseudo code,
>
> boolean InITBlock()
> return (ITSTATE.IT<3:0> != ‘0000’);
>
> ITSTATE can be got from CPSR.

Yes that's good if you're inserting a breakpoint at current PC but
otherwise you will need something else...

Re: [PATCH 1/2] This patch fixes GDBServer's run control for single stepping

[Yao Qi]

On 17-02-17 19:17:56, Antoine Tremblay wrote:
> > In ARM ARM, we have the pseudo code,
> >
> > boolean InITBlock()
> > return (ITSTATE.IT<3:0> != ‘0000’);
> >
> > ITSTATE can be got from CPSR.
>
> Yes that's good if you're inserting a breakpoint at current PC but
> otherwise you will need something else...

In software single step, we calculate the next pcs, and select
breakpoint kinds of them, according to current pc.  If current
pc is not within IT block (!InITBlock ()) or the last instruction
in IT block (LastInITBlock ()), we can safely use 16-bit thumb
breakpoint for any thumb instruction.  That is, in
gdbserver/linux-aarch32-low.c:arm_breakpoint_kind_from_current_state,
we can return ARM_BP_KIND_THUMB if (!InITBlock () || LastInITBlock ()).

Then, in some level, when installing software single step breakpoints,
if one breakpoint type is ARM_BP_KIND_THUMB2 and installed
address is 2-byte aligned, stop all threads.

-- 
Yao (齐尧)

Re: [PATCH 1/2] This patch fixes GDBServer's run control for single stepping

[Antoine Tremblay]

Yao Qi writes:

> On 17-02-17 19:17:56, Antoine Tremblay wrote:
>> > In ARM ARM, we have the pseudo code,
>> >
>> > boolean InITBlock()
>> > return (ITSTATE.IT<3:0> != ‘0000’);
>> >
>> > ITSTATE can be got from CPSR.
>>
>> Yes that's good if you're inserting a breakpoint at current PC but
>> otherwise you will need something else...
>
> In software single step, we calculate the next pcs, and select
> breakpoint kinds of them, according to current pc.  If current
> pc is not within IT block (!InITBlock ()) or the last instruction
> in IT block (LastInITBlock ()), we can safely use 16-bit thumb
> breakpoint for any thumb instruction.  That is, in
> gdbserver/linux-aarch32-low.c:arm_breakpoint_kind_from_current_state,
> we can return ARM_BP_KIND_THUMB if (!InITBlock () || LastInITBlock ()).
>
> Then, in some level, when installing software single step breakpoints,
> if one breakpoint type is ARM_BP_KIND_THUMB2 and installed
> address is 2-byte aligned, stop all threads.

Yes that would make sense but I think we can be calling get_next_pc
to insert a software single step breakpoint at an address different then
the current PCs next address.

See: https://sourceware.org/ml/gdb-patches/2016-06/msg00268.html

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

This is still true AFAIK so GDBServer may be at any PC stopped on a
pending event and need to insert a single step breakpoint at an address
unrelated to that event so in that case CPSR can't be used...

Thanks,
Antoine

Re: [PATCH 1/2] This patch fixes GDBServer's run control for single stepping

[Antoine Tremblay]

Antoine Tremblay writes:

> Yao Qi writes:
>
>> On 17-02-17 19:17:56, Antoine Tremblay wrote:
>>> > In ARM ARM, we have the pseudo code,
>>> >
>>> > boolean InITBlock()
>>> > return (ITSTATE.IT<3:0> != ‘0000’);
>>> >
>>> > ITSTATE can be got from CPSR.
>>>
>>> Yes that's good if you're inserting a breakpoint at current PC but
>>> otherwise you will need something else...
>>
>> In software single step, we calculate the next pcs, and select
>> breakpoint kinds of them, according to current pc.  If current
>> pc is not within IT block (!InITBlock ()) or the last instruction
>> in IT block (LastInITBlock ()), we can safely use 16-bit thumb
>> breakpoint for any thumb instruction.  That is, in
>> gdbserver/linux-aarch32-low.c:arm_breakpoint_kind_from_current_state,
>> we can return ARM_BP_KIND_THUMB if (!InITBlock () || LastInITBlock ()).
>>
>> Then, in some level, when installing software single step breakpoints,
>> if one breakpoint type is ARM_BP_KIND_THUMB2 and installed
>> address is 2-byte aligned, stop all threads.
>
> Yes that would make sense but I think we can be calling get_next_pc
> to insert a software single step breakpoint at an address different then
> the current PCs next address.
>
> See: https://sourceware.org/ml/gdb-patches/2016-06/msg00268.html
>
> "> 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."
>
> This is still true AFAIK so GDBServer may be at any PC stopped on a
> pending event and need to insert a single step breakpoint at an address
> unrelated to that event so in that case CPSR can't be used...
>
> Thanks,
> Antoine

Oops looks like it had been a while since I checked the get_next_pc
code, so we switch the current thread and use that PC so it should work.

Disregard my last mail :)

Looks like this solution is a good way forward.

Re: [PATCH 2/2] Avoid step-over infinite loop in GDBServer

[Yao Qi]

On Tue, Nov 29, 2016 at 12:07 PM, Antoine Tremblay
<antoine.tremblay@ericsson.com> wrote:
> Before this patch, GDBServer always executed a step-over if it found a
> thread that needed one.
>
> This could be a problem in a situation exposed by non-stop-fair-events.exp
> where the code and the breakpoint placement is like so:
>
> instruction A : has a single-step breakpoint installed for thread 1 and 2
> instruction B : has a single-step breakpoint installed for thread 3
> and is a branch to A.
>

Is instruction B following instruction A?  Is it like

.L1:
 nop
 b .L1

> In this particular case:
>
>  - GDBServer stops on instruction A in thread 1.
>  - Deletes thread 1 single-step breakpoint.
>  - Starts a step-over of thread 1 to step-over the thread 2 breakpoint.
>  - GDBServer finishes a step-over and is at instruction B.
>  - GDBserver starts a step-over of thread 1 to step-over the thread 3
>    breakpoint at instruction B.

Why does GDBserver starts a step-over again?  is it because
need_step_over_p doing checks like this,

  if (breakpoint_here (pc) || fast_tracepoint_jump_here (pc))
    {
      /* Don't step over a breakpoint that GDB expects to hit
         though.  If the condition is being evaluated on the target's side
         and it evaluate to false, step over this breakpoint as well.  */
      if (gdb_breakpoint_here (pc)
          && gdb_condition_true_at_breakpoint (pc)
          && gdb_no_commands_at_breakpoint (pc))
        {
          if (debug_threads)
            debug_printf ("Need step over [LWP %ld]? yes, but found"
                          " GDB breakpoint at 0x%s; skipping step over\n",
                          lwpid_of (thread), paddress (pc));

          current_thread = saved_thread;
          return 0;
        }
      else
        {
          if (debug_threads)
            debug_printf ("Need step over [LWP %ld]? yes, "
                          "found breakpoint at 0x%s\n",
                          lwpid_of (thread), paddress (pc));

          /* We've found an lwp that needs stepping over --- return 1 so
             that find_inferior stops looking.  */
          current_thread = saved_thread;

          return 1;
        }
    }

there is a single step breakpoint on pc, and it is obviously not a
gdb breakpoint, so 1 is returned.

>  - GDBServer stops on instuction A in thread 1.
>  - GDBServer is now in an infinite loop.
>

I am wondering can we take the information that we've already step
over a breakpoint for thread A into need_step_over_p, if we see pc
is on another single step breakpoint for thread B, don't do step over.

-- 
Yao (齐尧)

Re: [PATCH 2/2] Avoid step-over infinite loop in GDBServer

[Antoine Tremblay]

Yao Qi writes:

> On Tue, Nov 29, 2016 at 12:07 PM, Antoine Tremblay
> <antoine.tremblay@ericsson.com> wrote:
>> Before this patch, GDBServer always executed a step-over if it found a
>> thread that needed one.
>>
>> This could be a problem in a situation exposed by non-stop-fair-events.exp
>> where the code and the breakpoint placement is like so:
>>
>> instruction A : has a single-step breakpoint installed for thread 1 and 2
>> instruction B : has a single-step breakpoint installed for thread 3
>> and is a branch to A.
>>
>
> Is instruction B following instruction A?  Is it like
>
> .L1:
>  nop
>  b .L1
>

Yes, assuming A is nop an B is b .L1.

I reduced it to that from the real world case in non-stop-fair-events.c
with :

0x0000000000400767 <+38>:	mov    0x200963(%rip),%eax        # 0x6010d0 <got_sig>
0x000000000040076d <+44>:	test   %eax,%eax
0x000000000040076f <+46>:	je     0x400767 <child_function+38>

And a single-step breakpoint on all instructions.

>> In this particular case:
>>
>>  - GDBServer stops on instruction A in thread 1.
>>  - Deletes thread 1 single-step breakpoint.
>>  - Starts a step-over of thread 1 to step-over the thread 2 breakpoint.
>>  - GDBServer finishes a step-over and is at instruction B.
>>  - GDBserver starts a step-over of thread 1 to step-over the thread 3
>>    breakpoint at instruction B.
>
> Why does GDBserver starts a step-over again?  is it because
> need_step_over_p doing checks like this,
>
>   if (breakpoint_here (pc) || fast_tracepoint_jump_here (pc))
>     {
>       /* Don't step over a breakpoint that GDB expects to hit
>          though.  If the condition is being evaluated on the target's side
>          and it evaluate to false, step over this breakpoint as well.  */
>       if (gdb_breakpoint_here (pc)
>           && gdb_condition_true_at_breakpoint (pc)
>           && gdb_no_commands_at_breakpoint (pc))
>         {
>           if (debug_threads)
>             debug_printf ("Need step over [LWP %ld]? yes, but found"
>                           " GDB breakpoint at 0x%s; skipping step over\n",
>                           lwpid_of (thread), paddress (pc));
>
>           current_thread = saved_thread;
>           return 0;
>         }
>       else
>         {
>           if (debug_threads)
>             debug_printf ("Need step over [LWP %ld]? yes, "
>                           "found breakpoint at 0x%s\n",
>                           lwpid_of (thread), paddress (pc));
>
>           /* We've found an lwp that needs stepping over --- return 1 so
>              that find_inferior stops looking.  */
>           current_thread = saved_thread;
>
>           return 1;
>         }
>     }
>
> there is a single step breakpoint on pc, and it is obviously not a
> gdb breakpoint, so 1 is returned.
>

Yes.

>>  - GDBServer stops on instuction A in thread 1.
>>  - GDBServer is now in an infinite loop.
>>
>
> I am wondering can we take the information that we've already step
> over a breakpoint for thread A into need_step_over_p, if we see pc
> is on another single step breakpoint for thread B, don't do step over.

There's 3 parts to consider here:

 - What to restart ?

My first thought with this was step-over the other threads that needed
to step-over in a queue like fashion, but this may not be enough since we
may depend on another thread to be able to make progress on the single
stepped threads like is the case in non-stop-fair-events.

So like this patch does I though it was better to restart all
threads...

 - What does it mean to not step-over ?

The don't do step over part is more tricky then it seems since
let's say GDBServer:

 - hits a breakpoint on A with thread 1
 - doesn't step-over and restart all threads
 - any thread hits the breakpoint on A

At this point thread 1 has already hit the breakpoint on A even if we do
not allow it to step-over, so it's stuck there and could hit the
breakpoint a number of times, thus breaking the breakpoints/trace frames
counts and maybe some other stuff... I'm not sure about the solution to
that, ideas ? How to "undo" a breakpoint hit ? Or some other solution...

- When not to step-over ?

After some thought I think we could make it so that we expect the
single-step breakpoints for PC to be hit in the order of insertion.

Since the breakpoints are already a linked list we could make it so that
if you hit a single step breakpoint and that this breakpoint is
installed on multiple threads then single-step only if the thread stopped
matches the thread of the first breakpoint for that PC.

Otherwise continue all threads.

I think this would be simpler than to record the last executed step-over
for all threads and check/reset that flag so that all threads wait for
each other, but that may be possible too.

WDYT ?

Re: [PATCH 1/2] This patch fixes GDBServer's run control for single stepping

[Antoine Tremblay]

Yao Qi writes:

> On 17-02-17 19:17:56, Antoine Tremblay wrote:
>> > In ARM ARM, we have the pseudo code,
>> >
>> > boolean InITBlock()
>> > return (ITSTATE.IT<3:0> != ‘0000’);
>> >
>> > ITSTATE can be got from CPSR.
>>
>> Yes that's good if you're inserting a breakpoint at current PC but
>> otherwise you will need something else...
>
> In software single step, we calculate the next pcs, and select
> breakpoint kinds of them, according to current pc.  If current
> pc is not within IT block (!InITBlock ()) or the last instruction
> in IT block (LastInITBlock ()), we can safely use 16-bit thumb
> breakpoint for any thumb instruction.  That is, in
> gdbserver/linux-aarch32-low.c:arm_breakpoint_kind_from_current_state,
> we can return ARM_BP_KIND_THUMB if (!InITBlock () || LastInITBlock ()).

This is not entirely true since we have to check if the next pcs are in
an IT block rather then only the current one, so there's multiple
scenarios.

Consider if current PC is the IT instruction for example, then there's
at least 2 next pcs inside the IT block where we will need to install an THUMB2
breakpoint and get_next_pcs will return that.

Now I find myself trying to replicate the logic in thumb_get_next_pcs_raw
for IT blocks in arm_breakpoint_kind_from_current_state and it doesn't
feel right.

It gives something like:

void
set_single_step_breakpoint (CORE_ADDR stop_at, ptid_t ptid)
{
  struct single_step_breakpoint *bp;

  gdb_assert (ptid_get_pid (current_ptid) == ptid_get_pid (ptid));

  CORE_ADDR placed_address = stop_at;
  int breakpoint_kind
    = target_breakpoint_kind_from_current_state (&placed_address);

  bp = (struct single_step_breakpoint *) set_breakpoint_type_at_with_kind
    (single_step_breakpoint, placed_address, NULL, breakpoint_kind);
  bp->ptid = ptid;
}

int
arm_breakpoint_kind_from_current_state (CORE_ADDR *pcptr)
{
  if (arm_is_thumb_mode ())
    {

    if ( current_pcs_or_expected_next_pcs_are_in_it_block  ??? /* That's
    not right... */

	{
	  *pcptr = UNMAKE_THUMB_ADDR (*pcptr);
	  return ARM_BP_KIND_THUMB;
	}
      else
	{
	  *pcptr = MAKE_THUMB_ADDR (*pcptr);
	  return arm_breakpoint_kind_from_pc (pcptr);
	}
    }
  else
    {
      return arm_breakpoint_kind_from_pc (pcptr);
    }
}

It would be much better if get_next_pcs could select the breakpoint kind
itself and hint that to set_single_step_breakpoint like :

 VEC (struct { next_pc, breakpoint_kind })  = (*the_low_target.get_next_pcs) (regcache);

  for (i = 0; VEC_iterate (struct  { CORE_ADDR, kind }, next_pcs, i, pc); ++i)
    set_single_step_breakpoint (pc, kind, current_ptid);

But of course that means changing that virtual function for all archs
etc... :(

I'm thinking of going with that approch but I would like to know if you
have any other solutions to propose or if that sounds OK before writing
all that code ?

Thanks,
Antoine

Re: [PATCH 1/2] This patch fixes GDBServer's run control for single stepping

[Antoine Tremblay]

Antoine Tremblay writes:

> Yao Qi writes:
>
>> On 17-02-17 19:17:56, Antoine Tremblay wrote:
>>> > In ARM ARM, we have the pseudo code,
>>> >
>>> > boolean InITBlock()
>>> > return (ITSTATE.IT<3:0> != ‘0000’);
>>> >
>>> > ITSTATE can be got from CPSR.
>>>
>>> Yes that's good if you're inserting a breakpoint at current PC but
>>> otherwise you will need something else...
>>
>> In software single step, we calculate the next pcs, and select
>> breakpoint kinds of them, according to current pc.  If current
>> pc is not within IT block (!InITBlock ()) or the last instruction
>> in IT block (LastInITBlock ()), we can safely use 16-bit thumb
>> breakpoint for any thumb instruction.  That is, in
>> gdbserver/linux-aarch32-low.c:arm_breakpoint_kind_from_current_state,
>> we can return ARM_BP_KIND_THUMB if (!InITBlock () || LastInITBlock ()).
>
> This is not entirely true since we have to check if the next pcs are in
> an IT block rather then only the current one, so there's multiple
> scenarios.
>
> Consider if current PC is the IT instruction for example, then there's
> at least 2 next pcs inside the IT block where we will need to install an THUMB2
> breakpoint and get_next_pcs will return that.
>
> Now I find myself trying to replicate the logic in thumb_get_next_pcs_raw
> for IT blocks in arm_breakpoint_kind_from_current_state and it doesn't
> feel right.
>
> It gives something like:
>
> void
> set_single_step_breakpoint (CORE_ADDR stop_at, ptid_t ptid)
> {
>   struct single_step_breakpoint *bp;
>
>   gdb_assert (ptid_get_pid (current_ptid) == ptid_get_pid (ptid));
>
>   CORE_ADDR placed_address = stop_at;
>   int breakpoint_kind
>     = target_breakpoint_kind_from_current_state (&placed_address);
>
>   bp = (struct single_step_breakpoint *) set_breakpoint_type_at_with_kind
>     (single_step_breakpoint, placed_address, NULL, breakpoint_kind);
>   bp->ptid = ptid;
> }
>
> int
> arm_breakpoint_kind_from_current_state (CORE_ADDR *pcptr)
> {
>   if (arm_is_thumb_mode ())
>     {
>
>     if ( current_pcs_or_expected_next_pcs_are_in_it_block  ??? /* That's
>     not right... */
>
> 	{
> 	  *pcptr = UNMAKE_THUMB_ADDR (*pcptr);
> 	  return ARM_BP_KIND_THUMB;
> 	}
>       else
> 	{
> 	  *pcptr = MAKE_THUMB_ADDR (*pcptr);
> 	  return arm_breakpoint_kind_from_pc (pcptr);
> 	}
>     }
>   else
>     {
>       return arm_breakpoint_kind_from_pc (pcptr);
>     }
> }
>
> It would be much better if get_next_pcs could select the breakpoint kind
> itself and hint that to set_single_step_breakpoint like :
>
>  VEC (struct { next_pc, breakpoint_kind })  = (*the_low_target.get_next_pcs) (regcache);
>
>   for (i = 0; VEC_iterate (struct  { CORE_ADDR, kind }, next_pcs, i, pc); ++i)
>     set_single_step_breakpoint (pc, kind, current_ptid);
>
> But of course that means changing that virtual function for all archs
> etc... :(
>
> I'm thinking of going with that approch but I would like to know if you
> have any other solutions to propose or if that sounds OK before writing
> all that code ?
>
> Thanks,
> Antoine


Just a small follow-up on this idea, I think it would simplify GDB's
implementation too, it would have to change anyway since the interface
is shared.

See commit 833b7ab5008b769dca6db6d5ee1d21d33e730132
introduces a special case in arm_breakpoint_kind_from_current_state
where it's checked if GDB is single stepping and if so redoes the
arm_get_next_pc to get the execution mode of the next instruction.

I could do the same kind of thing in GDBServer recall get_next_pcs and
if I get > 1 PC in the vect it would mean I have an IT block, but it
sounds like a hack.

I also find that confusing given that the documentation for
breakpoint_kind_from_current_state is:

# Return the breakpoint kind for this target based on the current
# processor state (e.g. the current instruction mode on ARM) and the
# *PCPTR.  In default, it is gdbarch->breakpoint_kind_from_pc.

Finding the next PCs kind with this function seems like adding another
meaning to it...

Re: [PATCH 1/2] This patch fixes GDBServer's run control for single stepping

[Antoine Tremblay]

Antoine Tremblay writes:

>> Consider if current PC is the IT instruction for example, then there's
>> at least 2 next pcs inside the IT block where we will need to install an THUMB2
>> breakpoint and get_next_pcs will return that.

Oops please read that "Consider if the current instruction is the CMP instruction
before an IT instruction...."

Basically so that we get into the arm-get-next-pcs.c:351 case... in fact
now that I think of it maybe that would be OK if I were to add that check in
breakpoint_kind_from_current_state also but the previous comments still
apply about the possibly hackish state of this..

Thanks,
Antoine

Re: [PATCH 1/2] This patch fixes GDBServer's run control for single stepping

[Yao Qi]

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

>> In software single step, we calculate the next pcs, and select
>> breakpoint kinds of them, according to current pc.  If current
>> pc is not within IT block (!InITBlock ()) or the last instruction
>> in IT block (LastInITBlock ()), we can safely use 16-bit thumb
>> breakpoint for any thumb instruction.  That is, in
>> gdbserver/linux-aarch32-low.c:arm_breakpoint_kind_from_current_state,
>> we can return ARM_BP_KIND_THUMB if (!InITBlock () || LastInITBlock ()).
>
> This is not entirely true since we have to check if the next pcs are in
> an IT block rather then only the current one, so there's multiple
> scenarios.

In fact, we need to know whether the next pcs will be conditionally
executed or not, right?  If they won't be conditionally executed, we can
use 16-bit thumb breakpoint instruction.  By checking CPSR, if
current PC is not in IT block or on the last instruction in IT block,
the next pcs can't be conditionally executed.  I've already had a patch
below to implement what I described.

The problem of this patch is that we end up inserting different
kinds of breakpoints on the same instruction.  For a given 32-bit thumb
instruction, GDB and GDBserver knows 32-bit thumb breakpoint instruction
is used for GDB breakpoint, but only GDBserver knows 16-bit thumb
breakpoint is used for GDBserver single-step breakpoint, so GDB will be
confused on this.  I stopped here, and start to do something else.

>
> Consider if current PC is the IT instruction for example, then there's
> at least 2 next pcs inside the IT block where we will need to install an THUMB2
> breakpoint and get_next_pcs will return that.

-- 
Yao (齐尧)

From fad6162c9365535a09ca1072f15469e6007c0fd2 Mon Sep 17 00:00:00 2001
From: Yao Qi <yao.qi@linaro.org>
Date: Fri, 24 Feb 2017 09:25:43 +0000
Subject: [PATCH] Only use 32-bit thumb-2 breakpoint instruction on necessary

It takes two PTRACE_POKETEXT calls to write 32-bit thumb-2 breakpoint to
a 2-byte aligned address, and other threads can observe the partially
modified instruction in the memory between these two calls.  This causes
problems on single stepping multi-thread program in GDBserver.

32-bit thumb-2 breakpoint was invented for single step 32-bit thumb-2
instruction in IT block,
http://lists.infradead.org/pipermail/linux-arm-kernel/2010-January/007488.html
but we can use 16-bit thumb breakpoint instruction anywhere else.
That is what this patch does.  Change in set_breakpoint_type_at is similar
to breakpoint.c:breakpoint_kind.

This patch fixes fails in gdb.threads/schedlock.exp.

Even with patch, 32-bit thumb-2 breakpoint is still used for 32-bit thumb-2
instructions in IT block, so the problem is still there.  This patch is a
partial fix to PR 21169.

gdb/gdbserver:

2017-02-24  Yao Qi  <yao.qi@linaro.org>

	PR server/21169
	* linux-aarch32-low.c (arm_breakpoint_kind_from_current_state):
	Set kind to ARM_BP_KIND_THUMB if program is out of IT block or
	on the last instruction of IT block.
	* mem-break.c (set_breakpoint_type_at): Call
	target_breakpoint_kind_from_current_state to get breakpoint kind
	for single_step breakpoint.

diff --git a/gdb/gdbserver/linux-aarch32-low.c b/gdb/gdbserver/linux-aarch32-low.c
index 2b710ba..7409050 100644
--- a/gdb/gdbserver/linux-aarch32-low.c
+++ b/gdb/gdbserver/linux-aarch32-low.c
@@ -288,7 +288,30 @@ arm_breakpoint_kind_from_current_state (CORE_ADDR *pcptr)
   if (arm_is_thumb_mode ())
     {
       *pcptr = MAKE_THUMB_ADDR (*pcptr);
-      return arm_breakpoint_kind_from_pc (pcptr);
+      int kind = arm_breakpoint_kind_from_pc (pcptr);
+
+      if (kind == ARM_BP_KIND_THUMB2)
+	{
+	    unsigned long cpsr;
+	    struct regcache *regcache
+	      = get_thread_regcache (current_thread, 1);
+
+	    collect_register_by_name (regcache, "cpsr", &cpsr);
+	    /* Only use 32-bit thumb-2 breakpoint if *PCPTR is within
+	       IT block, because it takes two PTRACE_POKETEXT calls to
+	       write 32-bit thumb-2 breakpoint to a 2-byte aligned
+	       address, and other threads can observe the partially
+	       modified instruction in the memory between two
+	       PTRACE_POKETEXT calls.
+
+	       Don't use 32-bit thumb-2 breakpoint if program is not
+	       in IT block or on the last instruction of IT block,
+	       (ITSTATE.IT<2:0> == 000).  These bits are from CPSR bit
+	       10, 25, and 26.  */
+	    if ((cpsr & 0x06000400) == 0)
+	      kind = ARM_BP_KIND_THUMB;
+	}
+      return kind;
     }
   else
     {
diff --git a/gdb/gdbserver/mem-break.c b/gdb/gdbserver/mem-break.c
index 6e6926a..f3845cf 100644
--- a/gdb/gdbserver/mem-break.c
+++ b/gdb/gdbserver/mem-break.c
@@ -855,7 +855,21 @@ set_breakpoint_type_at (enum bkpt_type type, CORE_ADDR where,
 {
   int err_ignored;
   CORE_ADDR placed_address = where;
-  int breakpoint_kind = target_breakpoint_kind_from_pc (&placed_address);
+  int breakpoint_kind;
+
+  /* Get the kind of breakpoint to PLACED_ADDRESS except single-step
+     breakpoint.  Get the kind of single-step breakpoint according to
+     the current register state.  */
+  if (type == single_step_breakpoint)
+    {
+      breakpoint_kind
+	= target_breakpoint_kind_from_current_state (&placed_address);
+    }
+  else
+    {
+      breakpoint_kind
+	= target_breakpoint_kind_from_pc (&placed_address);
+    }
 
   return set_breakpoint (type, raw_bkpt_type_sw,
 			 placed_address, breakpoint_kind, handler,

Re: [PATCH 1/2] This patch fixes GDBServer's run control for single stepping

[Antoine Tremblay]

Yao Qi writes:

> Antoine Tremblay <antoine.tremblay@ericsson.com> writes:
>
>>> In software single step, we calculate the next pcs, and select
>>> breakpoint kinds of them, according to current pc.  If current
>>> pc is not within IT block (!InITBlock ()) or the last instruction
>>> in IT block (LastInITBlock ()), we can safely use 16-bit thumb
>>> breakpoint for any thumb instruction.  That is, in
>>> gdbserver/linux-aarch32-low.c:arm_breakpoint_kind_from_current_state,
>>> we can return ARM_BP_KIND_THUMB if (!InITBlock () || LastInITBlock ()).
>>
>> This is not entirely true since we have to check if the next pcs are in
>> an IT block rather then only the current one, so there's multiple
>> scenarios.
>
> In fact, we need to know whether the next pcs will be conditionally
> executed or not, right?  If they won't be conditionally executed, we can
> use 16-bit thumb breakpoint instruction.  By checking CPSR, if
> current PC is not in IT block or on the last instruction in IT block,
> the next pcs can't be conditionally executed.  I've already had a patch
> below to implement what I described.
>

You have to add if the current instruction is an IT instruction in wich
case the next instruction will be in an IT block.

Also if you have a conditional instruction that would evalutate to
true and is not the last one, get_next_pcs may return an instruction
after the IT block, arm_breakpoint_kind_from_current_state will be
called from the IT block with that PC and return a THUMB2_KIND when it
should not. See the else case in arm-get-next-pcs.c:~351

My point was that we should use get_next_pc directly since it's the best
place to detect if the next_pc is in the IT block. And the intent would
be clear.

It would give something like the patch below. (Note the GDB part of this
is missing but it works with GDBServer)

> The problem of this patch is that we end up inserting different
> kinds of breakpoints on the same instruction.  For a given 32-bit thumb
> instruction, GDB and GDBserver knows 32-bit thumb breakpoint instruction
> is used for GDB breakpoint, but only GDBserver knows 16-bit thumb
> breakpoint is used for GDBserver single-step breakpoint, so GDB will be
> confused on this.  I stopped here, and start to do something else.

Humm but how will the GDBServer 16-bit breakpoint be reported to GDB ?
Won't it always be hit and handled by GDBServer ?

And if you have a GDB breakpoint on an instruction and GDBServer puts
a single step breakpoint on that GDB breakpoint instruction, GDBServer
still knows of the GDB and GDBServer breakpoint types.

So how does GDB get confused ?

----
commit a18806af57b6c8906594ded036009c6b30c5b7db
Author: Antoine Tremblay <antoine.tremblay@ericsson.com>
Date:   Thu Mar 30 12:57:29 2017 -0400

    getnextpc encodes the kind

diff --git a/gdb/arch/arm-get-next-pcs.c b/gdb/arch/arm-get-next-pcs.c
index 398dd59a..f9b5bf1 100644
--- a/gdb/arch/arm-get-next-pcs.c
+++ b/gdb/arch/arm-get-next-pcs.c
@@ -315,6 +315,7 @@ thumb_get_next_pcs_raw (struct arm_get_next_pcs *self)
 	      itstate = thumb_advance_itstate (itstate);
 	    }
 
+	  pc = MAKE_THUMB2_BP_KIND (pc);
 	  VEC_safe_push (CORE_ADDR, next_pcs, MAKE_THUMB_ADDR (pc));
 	  return next_pcs;
 	}
@@ -335,6 +336,7 @@ thumb_get_next_pcs_raw (struct arm_get_next_pcs *self)
 		  itstate = thumb_advance_itstate (itstate);
 		}
 
+	      pc = MAKE_THUMB2_BP_KIND (pc);
 	      VEC_safe_push (CORE_ADDR, next_pcs, MAKE_THUMB_ADDR (pc));
 	      return next_pcs;
 	    }
@@ -361,8 +363,13 @@ thumb_get_next_pcs_raw (struct arm_get_next_pcs *self)
 
 	      /* Set a breakpoint on the following instruction.  */
 	      gdb_assert ((itstate & 0x0f) != 0);
+	      pc = MAKE_THUMB2_BP_KIND (pc);
 	      VEC_safe_push (CORE_ADDR, next_pcs, MAKE_THUMB_ADDR (pc));
 
+	      /* Reset the thumb2 kind bit to avoid affecting the next pc
+		 value.  */
+	      pc = UNMAKE_THUMB2_BP_KIND (pc);
+
 	      cond_negated = (itstate >> 4) & 1;
 
 	      /* Skip all following instructions with the same
@@ -378,6 +385,11 @@ thumb_get_next_pcs_raw (struct arm_get_next_pcs *self)
 		}
 	      while (itstate != 0 && ((itstate >> 4) & 1) == cond_negated);
 
+	      /* Only set a thumb2 breakpoint kind if we are still in an
+		 IT block.  */
+	      if (itstate != 0 && ((itstate >> 4) & 1) == cond_negated)
+		pc = MAKE_THUMB2_BP_KIND (pc);
+
 	      VEC_safe_push (CORE_ADDR, next_pcs, MAKE_THUMB_ADDR (pc));
 
 	      return next_pcs;
diff --git a/gdb/arch/arm-linux.c b/gdb/arch/arm-linux.c
index 943efe7..4b20f77 100644
--- a/gdb/arch/arm-linux.c
+++ b/gdb/arch/arm-linux.c
@@ -73,7 +73,7 @@ arm_linux_get_next_pcs_fixup (struct arm_get_next_pcs *self,
      step this instruction, this instruction isn't executed yet, and LR
      may not be updated yet.  In other words, GDB can get the target
      address from LR if this instruction isn't BL or BLX.  */
-  if (nextpc > 0xffff0000)
+  if ((nextpc & 0xffffffff) > 0xffff0000)
     {
       int bl_blx_p = 0;
       CORE_ADDR pc = regcache_read_pc (self->regcache);
diff --git a/gdb/arch/arm.h b/gdb/arch/arm.h
index a86dd37..b83fc79 100644
--- a/gdb/arch/arm.h
+++ b/gdb/arch/arm.h
@@ -101,6 +101,9 @@ enum arm_breakpoint_kinds
 #define IS_THUMB_ADDR(addr)    ((addr) & 1)
 #define MAKE_THUMB_ADDR(addr)  ((addr) | 1)
 #define UNMAKE_THUMB_ADDR(addr) ((addr) & ~1)
+#define IS_THUMB2_BP_KIND(addr) (((addr) & 1ULL << 32) >> 32)
+#define MAKE_THUMB2_BP_KIND(addr) ((addr) | (1ULL << 32))
+#define UNMAKE_THUMB2_BP_KIND(addr) ((addr) & ~(1ULL << 32))
 
 /* Support routines for instruction parsing.  */
 #define submask(x) ((1L << ((x) + 1)) - 1)
diff --git a/gdb/gdbserver/linux-aarch32-low.c b/gdb/gdbserver/linux-aarch32-low.c
index 2b710ba..fc66028 100644
--- a/gdb/gdbserver/linux-aarch32-low.c
+++ b/gdb/gdbserver/linux-aarch32-low.c
@@ -242,10 +242,16 @@ arm_breakpoint_kind_from_pc (CORE_ADDR *pcptr)
 	  unsigned short inst1 = 0;
 
 	  target_read_memory (*pcptr, (gdb_byte *) &inst1, 2);
-	  if (thumb_insn_size (inst1) == 4)
-	    return ARM_BP_KIND_THUMB2;
+	  if (thumb_insn_size (inst1) == 4 && IS_THUMB2_BP_KIND (*pcptr))
+	    {
+	      *pcptr = UNMAKE_THUMB2_BP_KIND (*pcptr);
+	      return ARM_BP_KIND_THUMB2;
+	    }
+
+	  return ARM_BP_KIND_THUMB;
 	}
-      return ARM_BP_KIND_THUMB;
+      else
+	return ARM_BP_KIND_THUMB;
     }
   else
     return ARM_BP_KIND_ARM;

Re: [PATCH 1/2] This patch fixes GDBServer's run control for single stepping

[Yao Qi]

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

> You have to add if the current instruction is an IT instruction in wich
> case the next instruction will be in an IT block.
>

Oh, you are right.

> Also if you have a conditional instruction that would evalutate to
> true and is not the last one, get_next_pcs may return an instruction
> after the IT block, arm_breakpoint_kind_from_current_state will be
> called from the IT block with that PC and return a THUMB2_KIND when it
> should not. See the else case in arm-get-next-pcs.c:~351

With the current PC and CPSR, it is not difficult to know whether
next_pc is still within IT block nor not, because all instructions in IT
block should be sequentially executed or skipped.

>
> My point was that we should use get_next_pc directly since it's the best
> place to detect if the next_pc is in the IT block. And the intent would
> be clear.

Yeah, we can record the information of breakpoint type in the return
value of get_next_pc, ...

>
> It would give something like the patch below. (Note the GDB part of this
> is missing but it works with GDBServer)
>

... but using extra bit in CORE_ADDR is not a good idea to me.

>> The problem of this patch is that we end up inserting different
>> kinds of breakpoints on the same instruction.  For a given 32-bit thumb
>> instruction, GDB and GDBserver knows 32-bit thumb breakpoint instruction
>> is used for GDB breakpoint, but only GDBserver knows 16-bit thumb
>> breakpoint is used for GDBserver single-step breakpoint, so GDB will be
>> confused on this.  I stopped here, and start to do something else.
>
> Humm but how will the GDBServer 16-bit breakpoint be reported to GDB ?
> Won't it always be hit and handled by GDBServer ?
>
> And if you have a GDB breakpoint on an instruction and GDBServer puts
> a single step breakpoint on that GDB breakpoint instruction, GDBServer
> still knows of the GDB and GDBServer breakpoint types.
>
> So how does GDB get confused ?

That was my conclusion at that point.  I got some regressions in
gdb.threads/*.exp when I tested my patch (gdb running is on
x86_64-linux), but I can't remember more details.

I am also wondering that we can use some code in
arm_adjust_breakpoint_address about detecting BPADDR is within IT block
or not by scanning instructions backward, if none of two bytes (can be
16-bit thumb instruction or the 2nd half of 32-bit thumb instruction)
matches IT instruction, the PC is not within IT block.

-- 
Yao (齐尧)

Re: [PATCH 1/2] This patch fixes GDBServer's run control for single stepping

[Antoine Tremblay]

Yao Qi writes:

> Antoine Tremblay <antoine.tremblay@ericsson.com> writes:
>
>> You have to add if the current instruction is an IT instruction in wich
>> case the next instruction will be in an IT block.
>>
>
> Oh, you are right.
>
>> Also if you have a conditional instruction that would evalutate to
>> true and is not the last one, get_next_pcs may return an instruction
>> after the IT block, arm_breakpoint_kind_from_current_state will be
>> called from the IT block with that PC and return a THUMB2_KIND when it
>> should not. See the else case in arm-get-next-pcs.c:~351
>
> With the current PC and CPSR, it is not difficult to know whether
> next_pc is still within IT block nor not, because all instructions in IT
> block should be sequentially executed or skipped.
>

I don't think you could figure out that this last instruction that
get_next_pc is returning after the IT block is or not in it however
without redoing much of the work.

I think maybe the best solution would be to abstract only that part of
get_next_pc in a function: the if block starting with if
(self->has_thumb2_breakpoint) around line 301.

And have only that part return the next_pc + the breakpoint kind, this
would avoid breaking all the virtual get_next_pc functions just for that
case and allow the same code to be used in kind_from_current_state.

We'll still redo the work but at least the code will be in one
place. WDYT ?

>>
>> My point was that we should use get_next_pc directly since it's the best
>> place to detect if the next_pc is in the IT block. And the intent would
>> be clear.
>
> Yeah, we can record the information of breakpoint type in the return
> value of get_next_pc, ...
>
>>
>> It would give something like the patch below. (Note the GDB part of this
>> is missing but it works with GDBServer)
>>
>
> ... but using extra bit in CORE_ADDR is not a good idea to me.
>

Yes it was quite hackish I was able to test with the CORE_ADDR patch
that it somewhat works at least.

Note that I say somewhat because stopping all the threads is proving
more problematic then I thought, I took the inspiration from your
original patch series V2 and installed all the single step breakpoints in
linux_resume and proceed_all_lwp.

But in linux_wait_event_filtered, linux_resume_one is also called with
the possibility of a stepping thread in 2 places. And you can't
call stop_all_lwps there...

So I'm scratching my head on how to stop the threads there thinking
about like calling  sigprocmask (SIG_SETMASK, &prev_mask, NULL); in
there to allow  linux_wait_event_filtered to be called
recursively... possibly, I just don't see a clean way.

There's other issues too where I get GDB adjusting a breakpoint and the
inferior crashing....

Might be other issues too :(

>>> The problem of this patch is that we end up inserting different
>>> kinds of breakpoints on the same instruction.  For a given 32-bit thumb
>>> instruction, GDB and GDBserver knows 32-bit thumb breakpoint instruction
>>> is used for GDB breakpoint, but only GDBserver knows 16-bit thumb
>>> breakpoint is used for GDBserver single-step breakpoint, so GDB will be
>>> confused on this.  I stopped here, and start to do something else.
>>
>> Humm but how will the GDBServer 16-bit breakpoint be reported to GDB ?
>> Won't it always be hit and handled by GDBServer ?
>>
>> And if you have a GDB breakpoint on an instruction and GDBServer puts
>> a single step breakpoint on that GDB breakpoint instruction, GDBServer
>> still knows of the GDB and GDBServer breakpoint types.
>>
>> So how does GDB get confused ?
>
> That was my conclusion at that point.  I got some regressions in
> gdb.threads/*.exp when I tested my patch (gdb running is on
> x86_64-linux), but I can't remember more details.
>

OK. Do you remember if it had to do with displaced stepping on ? There's
a problem with that and the current code in all-stop.

I had fixed that with the original patch from this thread by not
removing all single step breakpoints in all-stop...

> I am also wondering that we can use some code in
> arm_adjust_breakpoint_address about detecting BPADDR is within IT block
> or not by scanning instructions backward, if none of two bytes (can be
> 16-bit thumb instruction or the 2nd half of 32-bit thumb instruction)
> matches IT instruction, the PC is not within IT block.

Looking at the code, I think reusing parts of get_next_pc would be
simpler.

Note I'm including the test I use in case it's useful to you.

---

commit ad0288b35d85e96b6c676c665b0063b74a293dab
Author: Antoine Tremblay <antoine.tremblay@ericsson.com>
Date:   Thu Mar 30 11:14:17 2017 -0400

    test single step

diff --git a/gdb/testsuite/gdb.arch/arm-single-step.c b/gdb/testsuite/gdb.arch/arm-single-step.c
new file mode 100644
index 0000000..e18f4ed
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/arm-single-step.c
@@ -0,0 +1,110 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2017 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 <pthread.h>
+#include <unistd.h>
+#include <stdlib.h>
+#include <signal.h>
+
+#define NUM_THREADS 2
+#define TIMEOUT 5
+
+const int num_threads = NUM_THREADS;
+pthread_t child_thread[NUM_THREADS];
+volatile pthread_t signal_thread;
+volatile int run = 1;
+
+void
+handler (int signo)
+{
+  run = 0;
+}
+
+/* Align the instruction on a 2 byte boundary
+   Missalign it with a 4 byte boundary.  */
+#define THUMB2_INST			\
+  asm ("    .align 4 \n"		\
+       "    nop\n"				\
+       "    nop.w\n"				\
+       )				\
+
+#define ITBLOCK					\
+  asm ("    .align 4 \n"			\
+       "    nop\n"				\
+       "    cmp r0, #0\n"			\
+       "    ite eq\n"				\
+       "    nop.w \n"				\
+       "    nop.w \n"				\
+       )					\
+
+#define LOOP(macro)			\
+  while (run)					\
+    {						\
+      macro;					\
+    }						\
+
+void out_of_loop ()
+{
+  return;
+}
+
+void *
+thumb2_function (void *arg)
+{
+  LOOP (THUMB2_INST); /* break thumb2 */
+  out_of_loop ();
+  pthread_exit (NULL);
+}
+
+void *
+itblock_function (void *arg)
+{
+  LOOP (ITBLOCK); /* break itblock */
+  out_of_loop ();
+  pthread_exit (NULL);
+}
+
+int
+main (void)
+{
+  int res;
+  int i, j;
+  void *(*functions[2]) (void *);
+
+  functions[0] = thumb2_function;
+  functions[1] = itblock_function;
+
+  signal (SIGALRM, handler);
+
+  for (i = 0; i < 2; i++)
+    {
+      alarm (TIMEOUT);
+
+      for (j = 0; j < NUM_THREADS; j++)
+	{
+	  res = pthread_create (&child_thread[j], NULL, functions[i], NULL);
+	}
+
+      for (j = 0; j < NUM_THREADS; j++)
+	{
+	  res = pthread_join (child_thread[j], NULL);
+	}
+
+      run = 1;
+    }
+  exit(EXIT_SUCCESS);
+}
diff --git a/gdb/testsuite/gdb.arch/arm-single-step.exp b/gdb/testsuite/gdb.arch/arm-single-step.exp
new file mode 100644
index 0000000..c85f981
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/arm-single-step.exp
@@ -0,0 +1,42 @@
+# Copyright 2017 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
+set executable ${testfile}
+
+if { [gdb_compile_pthreads "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}] != "" } {
+    untested "failed to compile"
+    return -1
+}
+
+clean_restart $executable
+
+if ![runto_main] {
+    return -1
+}
+
+gdb_test_no_output "set scheduler-locking off"
+
+
+# Test each instruction type, thumb2 is a plain 2 byte aligned but not 4
+# byte aligned thumb2 instruction.  itblock is the same but in an itblock.
+foreach_with_prefix inst { "thumb2" "itblock" } {
+    gdb_breakpoint [gdb_get_line_number "break $inst"]
+    gdb_continue_to_breakpoint "break thumb2" ".* break $inst .*"
+    delete_breakpoints
+#    gdb_breakpoint "out_of_loop"
+    gdb_test "step" ".*out_of_loop.*" "stepping $inst"
+ #   delete_breakpoints
+}

Re: [PATCH 1/2] This patch fixes GDBServer's run control for single stepping

[Yao Qi]

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

> I think maybe the best solution would be to abstract only that part of
> get_next_pc in a function: the if block starting with if
> (self->has_thumb2_breakpoint) around line 301.
>
> And have only that part return the next_pc + the breakpoint kind, this
> would avoid breaking all the virtual get_next_pc functions just for that
> case and allow the same code to be used in kind_from_current_state.
>
> We'll still redo the work but at least the code will be in one
> place. WDYT ?

That should be fine, although I am not exactly sure what are you
going to do.

-- 
Yao (齐尧)

Re: [PATCH 1/2] This patch fixes GDBServer's run control for single stepping

[Antoine Tremblay]

Yao Qi writes:

> Antoine Tremblay <antoine.tremblay@ericsson.com> writes:
>
>> I think maybe the best solution would be to abstract only that part of
>> get_next_pc in a function: the if block starting with if
>> (self->has_thumb2_breakpoint) around line 301.
>>
>> And have only that part return the next_pc + the breakpoint kind, this
>> would avoid breaking all the virtual get_next_pc functions just for that
>> case and allow the same code to be used in kind_from_current_state.
>>
>> We'll still redo the work but at least the code will be in one
>> place. WDYT ?
>
> That should be fine, although I am not exactly sure what are you
> going to do.

It looks like this, comments ?:

----
commit 9a8b46f9038e9d3805c8e6ec1bdf0dff1c95c065
Author: Antoine Tremblay <antoine.tremblay@ericsson.com>
Date:   Mon Apr 3 09:13:20 2017 -0400

    refactor get-next-pc
---
 gdb/arch/arm-get-next-pcs.c       | 199 ++++++++++++++++++++++----------------
 gdb/arch/arm-get-next-pcs.h       |   9 ++
 gdb/gdbserver/linux-aarch32-low.c |  46 ++++++++-
 gdb/gdbserver/linux-aarch32-low.h |   3 +
 gdb/gdbserver/linux-arm-low.c     |  21 ----
 gdb/gdbserver/mem-break.c         |  16 ++-
 6 files changed, 188 insertions(+), 106 deletions(-)

diff --git a/gdb/arch/arm-get-next-pcs.c b/gdb/arch/arm-get-next-pcs.c
index 398dd59a..d967e16 100644
--- a/gdb/arch/arm-get-next-pcs.c
+++ b/gdb/arch/arm-get-next-pcs.c
@@ -22,6 +22,7 @@
 #include "common-regcache.h"
 #include "arm.h"
 #include "arm-get-next-pcs.h"
+#include <vector>
 
 /* See arm-get-next-pcs.h.  */
 
@@ -258,6 +259,115 @@ arm_deal_with_atomic_sequence_raw (struct arm_get_next_pcs *self)
   return next_pcs;
 }
 
+/* See arm-get-next-pcs.h.  */
+
+std::vector <std::pair<CORE_ADDR, arm_breakpoint_kinds>>
+thumb_get_next_pcs_raw_itblock
+(CORE_ADDR pc, unsigned short inst1,
+ ULONGEST status, ULONGEST itstate,
+ ULONGEST (*read_mem_uint) (CORE_ADDR memaddr, int len, int byte_order),
+ int byte_order_for_code)
+{
+  std::vector <std::pair <CORE_ADDR, arm_breakpoint_kinds>> next_pcs;
+
+  if ((inst1 & 0xff00) == 0xbf00 && (inst1 & 0x000f) != 0)
+    {
+      /* An IT instruction.  Because this instruction does not
+	 modify the flags, we can accurately predict the next
+	 executed instruction.  */
+      itstate = inst1 & 0x00ff;
+      pc += thumb_insn_size (inst1);
+
+      while (itstate != 0 && ! condition_true (itstate >> 4, status))
+	{
+	  inst1 = read_mem_uint (pc, 2,byte_order_for_code);
+	  pc += thumb_insn_size (inst1);
+	  itstate = thumb_advance_itstate (itstate);
+	}
+      next_pcs.push_back (std::pair<CORE_ADDR, arm_breakpoint_kinds>
+			  (MAKE_THUMB_ADDR (pc), ARM_BP_KIND_THUMB2));
+      return next_pcs;
+    }
+  else if (itstate != 0)
+    {
+      /* We are in a conditional block.  Check the condition.  */
+      if (! condition_true (itstate >> 4, status))
+	{
+	  /* Advance to the next executed instruction.  */
+	  pc += thumb_insn_size (inst1);
+	  itstate = thumb_advance_itstate (itstate);
+
+	  while (itstate != 0 && ! condition_true (itstate >> 4, status))
+	    {
+	      inst1 = read_mem_uint (pc, 2, byte_order_for_code);
+
+	      pc += thumb_insn_size (inst1);
+	      itstate = thumb_advance_itstate (itstate);
+	    }
+
+	  next_pcs.push_back (std::pair<CORE_ADDR, arm_breakpoint_kinds>
+			      (MAKE_THUMB_ADDR (pc), ARM_BP_KIND_THUMB2));
+	  return next_pcs;
+	}
+      else if ((itstate & 0x0f) == 0x08)
+	{
+	  /* This is the last instruction of the conditional
+	     block, and it is executed.  We can handle it normally
+	     because the following instruction is not conditional,
+	     and we must handle it normally because it is
+	     permitted to branch.  Fall through.  */
+	}
+      else
+	{
+	  int cond_negated;
+
+	  /* There are conditional instructions after this one.
+	     If this instruction modifies the flags, then we can
+	     not predict what the next executed instruction will
+	     be.  Fortunately, this instruction is archi2tecturally
+	     forbidden to branch; we know it will fall through.
+	     Start by skipping past it.  */
+	  pc += thumb_insn_size (inst1);
+	  itstate = thumb_advance_itstate (itstate);
+
+	  /* Set a breakpoint on the following instruction.  */
+	  gdb_assert ((itstate & 0x0f) != 0);
+	  next_pcs.push_back (std::pair<CORE_ADDR, arm_breakpoint_kinds>
+			      (MAKE_THUMB_ADDR (pc), ARM_BP_KIND_THUMB2));
+
+	  cond_negated = (itstate >> 4) & 1;
+
+	  /* Skip all following instructions with the same
+	     condition.  If there is a later instruction in the IT
+	     block with the opposite condition, set the other
+	     breakpoint there.  If not, then set a breakpoint on
+	     the instruction after the IT block.  */
+	  do
+	    {
+	      inst1 = read_mem_uint (pc, 2, byte_order_for_code);
+	      pc += thumb_insn_size (inst1);
+	      itstate = thumb_advance_itstate (itstate);
+	    }
+	  while (itstate != 0 && ((itstate >> 4) & 1) == cond_negated);
+
+	  if (itstate != 0 && ((itstate >> 4) & 1) == cond_negated)
+	    {
+	      next_pcs.push_back (std::pair<CORE_ADDR, arm_breakpoint_kinds>
+				  (MAKE_THUMB_ADDR (pc), ARM_BP_KIND_THUMB2));
+	    }
+	  else
+	    {
+	      next_pcs.push_back (std::pair<CORE_ADDR, arm_breakpoint_kinds>
+				  (MAKE_THUMB_ADDR (pc), ARM_BP_KIND_THUMB));
+	    }
+
+	  return next_pcs;
+	}
+    }
+
+  return next_pcs;
+}
+
 /* Find the next possible PCs for thumb mode.  */
 
 static VEC (CORE_ADDR) *
@@ -300,89 +410,14 @@ thumb_get_next_pcs_raw (struct arm_get_next_pcs *self)
 
   if (self->has_thumb2_breakpoint)
     {
-      if ((inst1 & 0xff00) == 0xbf00 && (inst1 & 0x000f) != 0)
-	{
-	  /* An IT instruction.  Because this instruction does not
-	     modify the flags, we can accurately predict the next
-	     executed instruction.  */
-	  itstate = inst1 & 0x00ff;
-	  pc += thumb_insn_size (inst1);
-
-	  while (itstate != 0 && ! condition_true (itstate >> 4, status))
-	    {
-	      inst1 = self->ops->read_mem_uint (pc, 2,byte_order_for_code);
-	      pc += thumb_insn_size (inst1);
-	      itstate = thumb_advance_itstate (itstate);
-	    }
-
-	  VEC_safe_push (CORE_ADDR, next_pcs, MAKE_THUMB_ADDR (pc));
-	  return next_pcs;
-	}
-      else if (itstate != 0)
-	{
-	  /* We are in a conditional block.  Check the condition.  */
-	  if (! condition_true (itstate >> 4, status))
-	    {
-	      /* Advance to the next executed instruction.  */
-	      pc += thumb_insn_size (inst1);
-	      itstate = thumb_advance_itstate (itstate);
+      std::vector <std::pair <CORE_ADDR, arm_breakpoint_kinds>> itblock_next_pcs
+	= thumb_get_next_pcs_raw_itblock
+	(pc, inst1, status, itstate, self->ops->read_mem_uint,
+	 byte_order_for_code);
 
-	      while (itstate != 0 && ! condition_true (itstate >> 4, status))
-		{
-		  inst1 = self->ops->read_mem_uint (pc, 2, byte_order_for_code);
-
-		  pc += thumb_insn_size (inst1);
-		  itstate = thumb_advance_itstate (itstate);
-		}
-
-	      VEC_safe_push (CORE_ADDR, next_pcs, MAKE_THUMB_ADDR (pc));
-	      return next_pcs;
-	    }
-	  else if ((itstate & 0x0f) == 0x08)
-	    {
-	      /* This is the last instruction of the conditional
-		 block, and it is executed.  We can handle it normally
-		 because the following instruction is not conditional,
-		 and we must handle it normally because it is
-		 permitted to branch.  Fall through.  */
-	    }
-	  else
-	    {
-	      int cond_negated;
-
-	      /* There are conditional instructions after this one.
-		 If this instruction modifies the flags, then we can
-		 not predict what the next executed instruction will
-		 be.  Fortunately, this instruction is architecturally
-		 forbidden to branch; we know it will fall through.
-		 Start by skipping past it.  */
-	      pc += thumb_insn_size (inst1);
-	      itstate = thumb_advance_itstate (itstate);
-
-	      /* Set a breakpoint on the following instruction.  */
-	      gdb_assert ((itstate & 0x0f) != 0);
-	      VEC_safe_push (CORE_ADDR, next_pcs, MAKE_THUMB_ADDR (pc));
-
-	      cond_negated = (itstate >> 4) & 1;
-
-	      /* Skip all following instructions with the same
-		 condition.  If there is a later instruction in the IT
-		 block with the opposite condition, set the other
-		 breakpoint there.  If not, then set a breakpoint on
-		 the instruction after the IT block.  */
-	      do
-		{
-		  inst1 = self->ops->read_mem_uint (pc, 2, byte_order_for_code);
-		  pc += thumb_insn_size (inst1);
-		  itstate = thumb_advance_itstate (itstate);
-		}
-	      while (itstate != 0 && ((itstate >> 4) & 1) == cond_negated);
-
-	      VEC_safe_push (CORE_ADDR, next_pcs, MAKE_THUMB_ADDR (pc));
-
-	      return next_pcs;
-	    }
-	}
+      for(auto const &nextpc : itblock_next_pcs) {
+	VEC_safe_push (CORE_ADDR, next_pcs, nextpc.first);
+      }
     }
   else if (itstate & 0x0f)
     {
diff --git a/gdb/arch/arm-get-next-pcs.h b/gdb/arch/arm-get-next-pcs.h
index 2300ac1..830844b 100644
--- a/gdb/arch/arm-get-next-pcs.h
+++ b/gdb/arch/arm-get-next-pcs.h
@@ -20,6 +20,7 @@
 #ifndef ARM_GET_NEXT_PCS_H
 #define ARM_GET_NEXT_PCS_H 1
 #include "gdb_vecs.h"
+#include <vector>
 
 /* Forward declaration.  */
 struct arm_get_next_pcs;
@@ -63,4 +64,12 @@ void arm_get_next_pcs_ctor (struct arm_get_next_pcs *self,
 /* Find the next possible PCs after the current instruction executes.  */
 VEC (CORE_ADDR) *arm_get_next_pcs (struct arm_get_next_pcs *self);
 
+/* Return the next possible PCs after PC if those are in a thumb2 it block.  */
+std::vector <std::pair <CORE_ADDR, arm_breakpoint_kinds>>
+thumb_get_next_pcs_raw_itblock
+(CORE_ADDR pc, unsigned short inst1,
+ ULONGEST status, ULONGEST itstate,
+ ULONGEST (*read_mem_uint) (CORE_ADDR memaddr, int len, int byte_order),
+ int byte_order_for_code);
+
 #endif /* ARM_GET_NEXT_PCS_H */
diff --git a/gdb/gdbserver/linux-aarch32-low.c b/gdb/gdbserver/linux-aarch32-low.c
index 2b710ba..ff4869f 100644
--- a/gdb/gdbserver/linux-aarch32-low.c
+++ b/gdb/gdbserver/linux-aarch32-low.c
@@ -18,6 +18,7 @@
 #include "server.h"
 #include "arch/arm.h"
 #include "arch/arm-linux.h"
+#include "arch/arm-get-next-pcs.h"
 #include "linux-low.h"
 #include "linux-aarch32-low.h"
 
@@ -28,6 +29,8 @@
 #include <elf.h>
 #endif
 
+#include <vector>
+
 /* Correct in either endianness.  */
 #define arm_abi_breakpoint 0xef9f0001UL
 
@@ -287,8 +290,31 @@ arm_breakpoint_kind_from_current_state (CORE_ADDR *pcptr)
 {
   if (arm_is_thumb_mode ())
     {
-      *pcptr = MAKE_THUMB_ADDR (*pcptr);
-      return arm_breakpoint_kind_from_pc (pcptr);
+      struct regcache *regcache = get_thread_regcache (current_thread, 1);
+      CORE_ADDR pc = regcache_read_pc (regcache);
+      ULONGEST status, itstate;
+      unsigned short inst1;
+
+      *pcptr = UNMAKE_THUMB_ADDR (*pcptr);
+
+      inst1 = get_next_pcs_read_memory_unsigned_integer
+	(pc, 2, 0);
+      status = regcache_raw_get_unsigned (regcache, ARM_PS_REGNUM);
+      itstate = ((status >> 8) & 0xfc) | ((status >> 25) & 0x3);
+
+      std::vector <std::pair <CORE_ADDR, arm_breakpoint_kinds>> itblock_next_pcs
+	= thumb_get_next_pcs_raw_itblock
+	(pc, inst1, status, itstate,
+	 get_next_pcs_read_memory_unsigned_integer,
+	 0);
+
+      /* If this PC is in an itblock let thumb_get_next_pcs_raw_itblock
+	 decide the kind.  */
+	for(auto const &nextpc : itblock_next_pcs) {
+	  if (MAKE_THUMB_ADDR (*pcptr) == nextpc.first)
+	    return nextpc.second;
+	}
+	return ARM_BP_KIND_THUMB;
     }
   else
     {
@@ -296,6 +322,22 @@ arm_breakpoint_kind_from_current_state (CORE_ADDR *pcptr)
     }
 }
 
+/* Read memory from the inferiror.
+   BYTE_ORDER is ignored and there to keep compatiblity with GDB's
+   read_memory_unsigned_integer. */
+ULONGEST
+get_next_pcs_read_memory_unsigned_integer (CORE_ADDR memaddr,
+					   int len,
+					   int byte_order)
+{
+  ULONGEST res;
+
+  res = 0;
+  target_read_memory (memaddr, (unsigned char *) &res, len);
+
+  return res;
+}
+
 void
 initialize_low_arch_aarch32 (void)
 {
diff --git a/gdb/gdbserver/linux-aarch32-low.h b/gdb/gdbserver/linux-aarch32-low.h
index fecfcbe..77fca32 100644
--- a/gdb/gdbserver/linux-aarch32-low.h
+++ b/gdb/gdbserver/linux-aarch32-low.h
@@ -27,6 +27,9 @@ int arm_breakpoint_kind_from_pc (CORE_ADDR *pcptr);
 const gdb_byte *arm_sw_breakpoint_from_kind (int kind , int *size);
 int arm_breakpoint_kind_from_current_state (CORE_ADDR *pcptr);
 int arm_breakpoint_at (CORE_ADDR where);
+ULONGEST get_next_pcs_read_memory_unsigned_integer (CORE_ADDR memaddr,
+						    int len,
+						    int byte_order);
 
 void initialize_low_arch_aarch32 (void);
 
diff --git a/gdb/gdbserver/linux-arm-low.c b/gdb/gdbserver/linux-arm-low.c
index fc2b348..fc6b5cc 100644
--- a/gdb/gdbserver/linux-arm-low.c
+++ b/gdb/gdbserver/linux-arm-low.c
@@ -139,11 +139,6 @@ static int arm_regmap[] = {
   64
 };
 
-/* Forward declarations needed for get_next_pcs ops.  */
-static ULONGEST get_next_pcs_read_memory_unsigned_integer (CORE_ADDR memaddr,
-							   int len,
-							   int byte_order);
-
 static CORE_ADDR get_next_pcs_addr_bits_remove (struct arm_get_next_pcs *self,
 						CORE_ADDR val);
 
@@ -252,22 +247,6 @@ get_next_pcs_is_thumb (struct arm_get_next_pcs *self)
   return arm_is_thumb_mode ();
 }
 
-/* Read memory from the inferiror.
-   BYTE_ORDER is ignored and there to keep compatiblity with GDB's
-   read_memory_unsigned_integer. */
-static ULONGEST
-get_next_pcs_read_memory_unsigned_integer (CORE_ADDR memaddr,
-					   int len,
-					   int byte_order)
-{
-  ULONGEST res;
-
-  res = 0;
-  target_read_memory (memaddr, (unsigned char *) &res, len);
-
-  return res;
-}
-
 /* Fetch the thread-local storage pointer for libthread_db.  */
 
 ps_err_e
diff --git a/gdb/gdbserver/mem-break.c b/gdb/gdbserver/mem-break.c
index 6e6926a..f3845cf 100644
--- a/gdb/gdbserver/mem-break.c
+++ b/gdb/gdbserver/mem-break.c
@@ -855,7 +855,21 @@ set_breakpoint_type_at (enum bkpt_type type, CORE_ADDR where,
 {
   int err_ignored;
   CORE_ADDR placed_address = where;
-  int breakpoint_kind = target_breakpoint_kind_from_pc (&placed_address);
+  int breakpoint_kind;
+
+  /* Get the kind of breakpoint to PLACED_ADDRESS except single-step
+     breakpoint.  Get the kind of single-step breakpoint according to
+     the current register state.  */
+  if (type == single_step_breakpoint)
+    {
+      breakpoint_kind
+	= target_breakpoint_kind_from_current_state (&placed_address);
+    }
+  else
+    {
+      breakpoint_kind
+	= target_breakpoint_kind_from_pc (&placed_address);
+    }
 
   return set_breakpoint (type, raw_bkpt_type_sw,
 			 placed_address, breakpoint_kind, handler,

Re: [PATCH 1/2] This patch fixes GDBServer's run control for single stepping

[Yao Qi]

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

> +  if ((inst1 & 0xff00) == 0xbf00 && (inst1 & 0x000f) != 0)
> +    {
> +      /* An IT instruction.  Because this instruction does not
> +	 modify the flags, we can accurately predict the next
> +	 executed instruction.  */
> +      itstate = inst1 & 0x00ff;
> +      pc += thumb_insn_size (inst1);
> +
> +      while (itstate != 0 && ! condition_true (itstate >> 4, status))
> +	{
> +	  inst1 = read_mem_uint (pc, 2,byte_order_for_code);
> +	  pc += thumb_insn_size (inst1);
> +	  itstate = thumb_advance_itstate (itstate);
> +	}
> +      next_pcs.push_back (std::pair<CORE_ADDR, arm_breakpoint_kinds>
> +			  (MAKE_THUMB_ADDR (pc), ARM_BP_KIND_THUMB2));

It is incorrect to choose ARM_BP_KIND_THUMB2 if the instruction is
16-bit.  IMO, this function should only tell whether PC is in IT block
nor not.  It shouldn't involve any breakpoint kinds selection.

> +      return next_pcs;
> +    }
> +  else if (itstate != 0)
> +    {
> +      /* We are in a conditional block.  Check the condition.  */
> +      if (! condition_true (itstate >> 4, status))
> +	{
> +	  /* Advance to the next executed instruction.  */
> +	  pc += thumb_insn_size (inst1);
> +	  itstate = thumb_advance_itstate (itstate);
> +
> +	  while (itstate != 0 && ! condition_true (itstate >> 4, status))
> +	    {
> +	      inst1 = read_mem_uint (pc, 2, byte_order_for_code);
> +
> +	      pc += thumb_insn_size (inst1);
> +	      itstate = thumb_advance_itstate (itstate);
> +	    }
> +

If all the following instructions' condition is false, breakpoint should
be set on the first instruction out side of IT block.  We can still use
16-bit thumb breakpoint.

> +	  next_pcs.push_back (std::pair<CORE_ADDR, arm_breakpoint_kinds>
> +			      (MAKE_THUMB_ADDR (pc),
> ARM_BP_KIND_THUMB2));

The same issue.

> +	  return next_pcs;
> +	}
> +      else if ((itstate & 0x0f) == 0x08)
> +	{
> +	  /* This is the last instruction of the conditional
> +	     block, and it is executed.  We can handle it normally
> +	     because the following instruction is not conditional,
> +	     and we must handle it normally because it is
> +	     permitted to branch.  Fall through.  */

How do we fall through now?

> +	}
> +      else
> +	{
> +	  int cond_negated;
> +
> +	  /* There are conditional instructions after this one.
> +	     If this instruction modifies the flags, then we can
> +	     not predict what the next executed instruction will
> +	     be.  Fortunately, this instruction is archi2tecturally
> +	     forbidden to branch; we know it will fall through.
> +	     Start by skipping past it.  */
> +	  pc += thumb_insn_size (inst1);
> +	  itstate = thumb_advance_itstate (itstate);
> +
> +	  /* Set a breakpoint on the following instruction.  */
> +	  gdb_assert ((itstate & 0x0f) != 0);
> +	  next_pcs.push_back (std::pair<CORE_ADDR, arm_breakpoint_kinds>
> +			      (MAKE_THUMB_ADDR (pc), ARM_BP_KIND_THUMB2));
> +
> +	  cond_negated = (itstate >> 4) & 1;
> +
> +	  /* Skip all following instructions with the same
> +	     condition.  If there is a later instruction in the IT
> +	     block with the opposite condition, set the other
> +	     breakpoint there.  If not, then set a breakpoint on
> +	     the instruction after the IT block.  */
> +	  do
> +	    {
> +	      inst1 = read_mem_uint (pc, 2, byte_order_for_code);
> +	      pc += thumb_insn_size (inst1);
> +	      itstate = thumb_advance_itstate (itstate);
> +	    }
> +	  while (itstate != 0 && ((itstate >> 4) & 1) == cond_negated);
> +
> +	  if (itstate != 0 && ((itstate >> 4) & 1) == cond_negated)
> +	    {
> +	      next_pcs.push_back (std::pair<CORE_ADDR, arm_breakpoint_kinds>
> +				  (MAKE_THUMB_ADDR (pc), ARM_BP_KIND_THUMB2));
> +	    }
> +	  else
> +	    {
> +	      next_pcs.push_back (std::pair<CORE_ADDR, arm_breakpoint_kinds>
> +				  (MAKE_THUMB_ADDR (pc), ARM_BP_KIND_THUMB));
> +	    }

Why do you choose breakpoint in this way?

> diff --git a/gdb/gdbserver/mem-break.c b/gdb/gdbserver/mem-break.c
> index 6e6926a..f3845cf 100644
> --- a/gdb/gdbserver/mem-break.c
> +++ b/gdb/gdbserver/mem-break.c
> @@ -855,7 +855,21 @@ set_breakpoint_type_at (enum bkpt_type type, CORE_ADDR where,
>  {
>    int err_ignored;
>    CORE_ADDR placed_address = where;
> -  int breakpoint_kind = target_breakpoint_kind_from_pc (&placed_address);
> +  int breakpoint_kind;
> +
> +  /* Get the kind of breakpoint to PLACED_ADDRESS except single-step
> +     breakpoint.  Get the kind of single-step breakpoint according to
> +     the current register state.  */
> +  if (type == single_step_breakpoint)
> +    {
> +      breakpoint_kind
> +	= target_breakpoint_kind_from_current_state (&placed_address);

I read my patch again, but it looks wrong.  If we single-step an
instruction with a state change, like bx or blx, current get_next_pcs
correctly marked the address bit.  However, with the change like this,
we'll get the wrong breakpoint kind.

-- 
Yao (齐尧)

Re: [PATCH 1/2] This patch fixes GDBServer's run control for single stepping

[Antoine Tremblay]

Yao Qi writes:

> Antoine Tremblay <antoine.tremblay@ericsson.com> writes:
>
>> +  if ((inst1 & 0xff00) == 0xbf00 && (inst1 & 0x000f) != 0)
>> +    {
>> +      /* An IT instruction.  Because this instruction does not
>> +	 modify the flags, we can accurately predict the next
>> +	 executed instruction.  */
>> +      itstate = inst1 & 0x00ff;
>> +      pc += thumb_insn_size (inst1);
>> +
>> +      while (itstate != 0 && ! condition_true (itstate >> 4, status))
>> +	{
>> +	  inst1 = read_mem_uint (pc, 2,byte_order_for_code);
>> +	  pc += thumb_insn_size (inst1);
>> +	  itstate = thumb_advance_itstate (itstate);
>> +	}
>> +      next_pcs.push_back (std::pair<CORE_ADDR, arm_breakpoint_kinds>
>> +			  (MAKE_THUMB_ADDR (pc), ARM_BP_KIND_THUMB2));
>
> It is incorrect to choose ARM_BP_KIND_THUMB2 if the instruction is
> 16-bit.

Good point this does not look correct indeed, a call to
breakpoint_from_pc would be better at this point.

> IMO, this function should only tell whether PC is in IT block
> nor not.  It shouldn't involve any breakpoint kinds selection.
>

Yes I think you're right, I could make it return std::pair<CORE_ADDR,
bool (is_it_block ())>

And use breakpoint_from_pc if true , and BP_KIND_THUMB if false.

>> +      return next_pcs;
>> +    }
>> +  else if (itstate != 0)
>> +    {
>> +      /* We are in a conditional block.  Check the condition.  */
>> +      if (! condition_true (itstate >> 4, status))
>> +	{
>> +	  /* Advance to the next executed instruction.  */
>> +	  pc += thumb_insn_size (inst1);
>> +	  itstate = thumb_advance_itstate (itstate);
>> +
>> +	  while (itstate != 0 && ! condition_true (itstate >> 4, status))
>> +	    {
>> +	      inst1 = read_mem_uint (pc, 2, byte_order_for_code);
>> +
>> +	      pc += thumb_insn_size (inst1);
>> +	      itstate = thumb_advance_itstate (itstate);
>> +	    }
>> +
>
> If all the following instructions' condition is false, breakpoint should
> be set on the first instruction out side of IT block.  We can still use
> 16-bit thumb breakpoint.
>
>> +	  next_pcs.push_back (std::pair<CORE_ADDR, arm_breakpoint_kinds>
>> +			      (MAKE_THUMB_ADDR (pc),
>> ARM_BP_KIND_THUMB2));
>
> The same issue.
>
>> +	  return next_pcs;
>> +	}
>> +      else if ((itstate & 0x0f) == 0x08)
>> +	{
>> +	  /* This is the last instruction of the conditional
>> +	     block, and it is executed.  We can handle it normally
>> +	     because the following instruction is not conditional,
>> +	     and we must handle it normally because it is
>> +	     permitted to branch.  Fall through.  */
>
> How do we fall through now?

No breakpoints are added to the vector, so it falls through.

>
>> +	}
>> +      else
>> +	{
>> +	  int cond_negated;
>> +
>> +	  /* There are conditional instructions after this one.
>> +	     If this instruction modifies the flags, then we can
>> +	     not predict what the next executed instruction will
>> +	     be.  Fortunately, this instruction is archi2tecturally
>> +	     forbidden to branch; we know it will fall through.
>> +	     Start by skipping past it.  */
>> +	  pc += thumb_insn_size (inst1);
>> +	  itstate = thumb_advance_itstate (itstate);
>> +
>> +	  /* Set a breakpoint on the following instruction.  */
>> +	  gdb_assert ((itstate & 0x0f) != 0);
>> +	  next_pcs.push_back (std::pair<CORE_ADDR, arm_breakpoint_kinds>
>> +			      (MAKE_THUMB_ADDR (pc), ARM_BP_KIND_THUMB2));
>> +
>> +	  cond_negated = (itstate >> 4) & 1;
>> +
>> +	  /* Skip all following instructions with the same
>> +	     condition.  If there is a later instruction in the IT
>> +	     block with the opposite condition, set the other
>> +	     breakpoint there.  If not, then set a breakpoint on
>> +	     the instruction after the IT block.  */
>> +	  do
>> +	    {
>> +	      inst1 = read_mem_uint (pc, 2, byte_order_for_code);
>> +	      pc += thumb_insn_size (inst1);
>> +	      itstate = thumb_advance_itstate (itstate);
>> +	    }
>> +	  while (itstate != 0 && ((itstate >> 4) & 1) == cond_negated);
>> +
>> +	  if (itstate != 0 && ((itstate >> 4) & 1) == cond_negated)
>> +	    {
>> +	      next_pcs.push_back (std::pair<CORE_ADDR, arm_breakpoint_kinds>
>> +				  (MAKE_THUMB_ADDR (pc), ARM_BP_KIND_THUMB2));
>> +	    }
>> +	  else
>> +	    {
>> +	      next_pcs.push_back (std::pair<CORE_ADDR, arm_breakpoint_kinds>
>> +				  (MAKE_THUMB_ADDR (pc), ARM_BP_KIND_THUMB));
>> +	    }
>
> Why do you choose breakpoint in this way?
>

In the case of if (itstate != 0 && ((itstate >> 4) & 1) == cond_negated)
we are in the IT block

But if that is false we're after the IT block so it doesn't need a
thumb2 breakpoint. (See the comment above in the code)

>> diff --git a/gdb/gdbserver/mem-break.c b/gdb/gdbserver/mem-break.c
>> index 6e6926a..f3845cf 100644
>> --- a/gdb/gdbserver/mem-break.c
>> +++ b/gdb/gdbserver/mem-break.c
>> @@ -855,7 +855,21 @@ set_breakpoint_type_at (enum bkpt_type type, CORE_ADDR where,
>>  {
>>    int err_ignored;
>>    CORE_ADDR placed_address = where;
>> -  int breakpoint_kind = target_breakpoint_kind_from_pc (&placed_address);
>> +  int breakpoint_kind;
>> +
>> +  /* Get the kind of breakpoint to PLACED_ADDRESS except single-step
>> +     breakpoint.  Get the kind of single-step breakpoint according to
>> +     the current register state.  */
>> +  if (type == single_step_breakpoint)
>> +    {
>> +      breakpoint_kind
>> +	= target_breakpoint_kind_from_current_state (&placed_address);
>
> I read my patch again, but it looks wrong.  If we single-step an
> instruction with a state change, like bx or blx, current get_next_pcs
> correctly marked the address bit.  However, with the change like this,
> we'll get the wrong breakpoint kind.

You're right, that's a problem however I think it could be fixed in
arm_breakpoint_kind_from_current_state by adding a case where
placed_address != current_pc in which case the thumb bit would be the
deciding factor rather then arm_is_thumb_mode ()...

I'll resubmit a proper patch with those fixes.