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

[Antoine Tremblay <antoine dot tremblay at ericsson dot com>]

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