Re: [PATCH v3 05/17] Embed the pending step-over chain in thread_info objects

[Yao Qi <qiyaoltc at gmail dot com>]

Pedro Alves <palves@redhat.com> writes:

Hi Pedro,
This patch looks good to me, some questions below.

>	(displaced_step_prepare): Assert that trap_expected is set.  Use
>	thread_step_over_chain_enqueue.  Split starting a new displaced
>	step to ...
>	(start_step_over): ... this new function.

If I read this patch correctly,  start_step_over is moved from
displaced_step_fixup.  That is why we call start_step_over after each
displaced_step_fixup.

> v3:
>
> 	More comments.  The step-over chain is now a global instead of
> 	being per-inferior.  Previous versions had actually broken
> 	multiple-processes displaced stepping at the same time.  Added new

How does per-inferior step-over chain (or displaced stepping queue)
break multi-process displaced stepping?

> @@ -2972,35 +2983,17 @@ infrun_thread_stop_requested_callback (struct thread_info *info, void *arg)
>  static void
>  infrun_thread_stop_requested (ptid_t ptid)
>  {
> -  struct displaced_step_inferior_state *displaced;
> -
> -  /* PTID was requested to stop.  Remove it from the displaced
> -     stepping queue, so we don't try to resume it automatically.  */
> -
> -  for (displaced = displaced_step_inferior_states;
> -       displaced;
> -       displaced = displaced->next)
> -    {
> -      struct displaced_step_request *it, **prev_next_p;
> -
> -      it = displaced->step_request_queue;
> -      prev_next_p = &displaced->step_request_queue;
> -      while (it)
> -	{
> -	  if (ptid_match (it->ptid, ptid))
> -	    {
> -	      *prev_next_p = it->next;
> -	      it->next = NULL;
> -	      xfree (it);
> -	    }
> -	  else
> -	    {
> -	      prev_next_p = &it->next;
> -	    }
> +  struct thread_info *tp;
>  
> -	  it = *prev_next_p;
> -	}
> -    }
> +  /* PTID was requested to stop.  Remove matching threads from the
> +     step-over queue, so we don't try to resume them
> +     automatically.  */

I can understand the code below, except the comment "we don't try to
resume them automatically".  It looks not necessary here.

> +  ALL_NON_EXITED_THREADS (tp)
> +    if (ptid_match (tp->ptid, ptid))
> +      {
> +	if (thread_is_in_step_over_chain (tp))
> +	  thread_step_over_chain_remove (tp);
> +      }
>  
>    iterate_over_threads (infrun_thread_stop_requested_callback, &ptid);
>  }
> @@ -4051,6 +4044,9 @@ Cannot fill $_exitsignal with the correct signal number.\n"));
>  	       that this operation also cleans up the child process for vfork,
>  	       because their pages are shared.  */
>  	    displaced_step_fixup (ecs->ptid, GDB_SIGNAL_TRAP);
> +	    /* Start a new step-over in another thread if there's one
> +	       that needs it.  */
> +	    start_step_over ();

The comment is confusing to me, especially the "one" and the "it".  Do
you mean "in another thread if there is one thread that needs step-over"?

> @@ -323,6 +403,10 @@ delete_thread_1 (ptid_t ptid, int silent)
>    if (!tp)
>      return;
>  
> +  /* Dead threads don't need to step-over.  Remove from queue.  */
> +  if (tp->step_over_next != NULL)
> +    thread_step_over_chain_remove (tp);
> +

I am wondering how this can happen?  A thread needs step-over becomes dead?

>    /* If this is the current thread, or there's code out there that
>       relies on it existing (refcount > 0) we can't delete yet.  Mark
>       it as exited, and notify it.  */

-- 
Yao (éå)