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

Yao Qi
2015-04-21T08:28:19Z

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 (齐尧)