Re: [PATCH v2 11/14] gdb: move displaced stepping logic to gdbarch, allow starting concurrent displaced steps

[Pedro Alves <pedro@palves.net>]

Very good!  Only a few minor comments below.  Otherwise OK.

On 12/2/20 3:48 PM, Simon Marchi via Gdb-patches wrote:

> +displaced_step_finish_status
> +displaced_step_buffer::finish (gdbarch *arch, thread_info *thread,
> +			       gdb_signal sig)
> +{
> +  gdb_assert (thread->displaced_step_state.in_progress ());
> +  gdb_assert (thread == m_current_thread);
> +
> +  /* Move this to a local variable so it's released in case something goes
> +     wrong.  */
> +  displaced_step_copy_insn_closure_up copy_insn_closure
> +    = std::move (m_copy_insn_closure);
> +  gdb_assert (copy_insn_closure != nullptr);
> +
> +  /* Reset m_current_thread immediately to mark the buffer as available, in case

Uppercase m_current_thread.

> +     something goes wrong below.  */
> +  m_current_thread = nullptr;
> +
> +  /* Now that a buffer gets freed, tell GDB it can ask us to prepare a displaced

How about "tell infrun" instead of "tell GDB"?  This is all in GDB.

> +     step again for this inferior.  Do that here in case something goes wrong
> +     below.  */
> +  thread->inf->displaced_step_state.unavailable = false;
> +
> +  ULONGEST len = gdbarch_max_insn_length (arch);
> +


> @@ -1927,14 +1833,41 @@ static step_over_what thread_still_needs_step_over (struct thread_info *tp);
>  static bool
>  start_step_over (void)
>  {
> -  struct thread_info *tp, *next;
> +  thread_info *next;
>  
>    /* Don't start a new step-over if we already have an in-line
>       step-over operation ongoing.  */
>    if (step_over_info_valid_p ())
>      return false;
>  
> -  for (tp = global_thread_step_over_chain_head; tp != NULL; tp = next)
> +  /* Steal the global thread step over chain.  As we try to initiate displaced
> +     steps, threads will be enqueued in the global chain if no buffers are
> +     available.  If we iterated on the global chain directly, we might iterate
> +     indefinitely.  */
> +  thread_info *threads_to_step = global_thread_step_over_chain_head;
> +  global_thread_step_over_chain_head = NULL;
> +
> +  infrun_debug_printf ("stealing global queue of threads to step, length = %d",
> +		       thread_step_over_chain_length (threads_to_step));
> +
> +  bool started = false;
> +
> +  /* On scope exit (whatever the reason, return or exception), if there are
> +     threads left in the THREADS_TO_STEP chain, put back these threads in the
> +     global list.  */
> +  SCOPE_EXIT {

This format looks un-GNU-ish to me.  I've been putting the {
on the following line, indented by two spaces, like if it was
a if/for/while block, like:

  SCOPE_EXIT
    {

... unless the whole thing fits in one line:

  SCOPE_EXIT { foo (); };

> +    if (threads_to_step == nullptr)
> +      infrun_debug_printf ("step-over queue now empty");
> +    else
> +      {
> +	infrun_debug_printf ("putting back %d threads to step in global queue",
> +			     thread_step_over_chain_length (threads_to_step));
> +
> +	global_thread_step_over_chain_enqueue_chain (threads_to_step);
> +      }
> +  };
> +
> +  for (thread_info *tp = threads_to_step; tp != NULL; tp = next)
>      {
>        struct execution_control_state ecss;
>        struct execution_control_state *ecs = &ecss;
> @@ -1943,12 +1876,23 @@ start_step_over (void)
>  
>        gdb_assert (!tp->stop_requested);
>  
> -      next = global_thread_step_over_chain_next (tp);
> +      next = thread_step_over_chain_next (threads_to_step, tp);
>  
> -      /* If this inferior already has a displaced step in process,
> -	 don't start a new one.  */
> -      if (displaced_step_in_progress (tp->inf))
> -	continue;
> +      if (tp->inf->displaced_step_state.unavailable)
> +	{
> +	  /* The arch told us to not even try preparing another displaced step
> +	     for this inferior.  Just leave the thread in THREADS_TO_STEP, it
> +	     will get moved to the global chain on scope exit.  */
> +	  continue;
> +	}
> +
> +      /* Remove thread from the THREADS_TO_STEP chain.  If anything goes wrong
> +	 while we try to prepare the displaced step, we don't add it back to
> +	 the global step over chain.  This is to avoid a thread staying in the
> +	 step over chain indefinitely if something go wrong when resuming it.

something goes wrong

> +	 If the error is intermittent and it still needs a step over, it will
> +	 get enqueued again when we try to resume it normally.  */
> +      thread_step_over_chain_remove (&threads_to_step, tp);