Re: [PATCH 10/31] Thread options & clone events (Linux GDBserver)

Andrew Burgess via Gdb-patches
2023-06-06T14:12:49Z

Pedro Alves <pedro@palves.net> writes:

> This patch teaches the Linux GDBserver backend to report clone events
> to GDB, when GDB has requested them with the GDB_THREAD_OPTION_CLONE
> thread option, via the new QThreadOptions packet.
>
> This shuffles code in linux_process_target::handle_extended_wait
> around to a more logical order when we now have to handle and
> potentially report all of fork/vfork/clone.
>
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=19675
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=27830
> Change-Id: I3a19bc98801ec31e5c6fdbe1ebe17df855142bb2
> ---
> gdbserver/linux-low.cc | 235 ++++++++++++++++++++++-------------------
> gdbserver/linux-low.h | 2 +
> 2 files changed, 129 insertions(+), 108 deletions(-)
>
> diff --git a/gdbserver/linux-low.cc b/gdbserver/linux-low.cc
> index 6f96e16d6f0..d755cda0e44 100644
> --- a/gdbserver/linux-low.cc
> +++ b/gdbserver/linux-low.cc
> @@ -491,7 +491,6 @@ linux_process_target::handle_extended_wait (lwp_info **orig_event_lwp,
> struct lwp_info *event_lwp = *orig_event_lwp;
> int event = linux_ptrace_get_extended_event (wstat);
> struct thread_info *event_thr = get_lwp_thread (event_lwp);
> - struct lwp_info *new_lwp;
>
> gdb_assert (event_lwp->waitstatus.kind () == TARGET_WAITKIND_IGNORE);
>
> @@ -503,7 +502,6 @@ linux_process_target::handle_extended_wait (lwp_info **orig_event_lwp,
> if ((event == PTRACE_EVENT_FORK) || (event == PTRACE_EVENT_VFORK)
> || (event == PTRACE_EVENT_CLONE))
> {
> - ptid_t ptid;
> unsigned long new_pid;
> int ret, status;
>
> @@ -527,61 +525,65 @@ linux_process_target::handle_extended_wait (lwp_info **orig_event_lwp,
> warning ("wait returned unexpected status 0x%x", status);
> }
>
> - if (event == PTRACE_EVENT_FORK || event == PTRACE_EVENT_VFORK)
> + if (debug_threads)
> {
> - struct process_info *parent_proc;
> - struct process_info *child_proc;
> - struct lwp_info *child_lwp;
> - struct thread_info *child_thr;
> + debug_printf ("HEW: Got %s event from LWP %ld, new child is %ld\n",
> + (event == PTRACE_EVENT_FORK ? "fork"
> + : event == PTRACE_EVENT_VFORK ? "vfork"
> + : event == PTRACE_EVENT_CLONE ? "clone"
> + : "???"),
> + ptid_of (event_thr).lwp (),
> + new_pid);
> + }
> +
> + ptid_t child_ptid = (event != PTRACE_EVENT_CLONE
> + ? ptid_t (new_pid, new_pid)
> + : ptid_t (ptid_of (event_thr).pid (), new_pid));
>
> - ptid = ptid_t (new_pid, new_pid);
> + lwp_info *child_lwp = add_lwp (child_ptid);
> + gdb_assert (child_lwp != NULL);
> + child_lwp->stopped = 1;
> + if (event != PTRACE_EVENT_CLONE)
> + child_lwp->must_set_ptrace_flags = 1;
> + child_lwp->status_pending_p = 0;
>
> - threads_debug_printf ("Got fork event from LWP %ld, "
> - "new child is %d",
> - ptid_of (event_thr).lwp (),
> - ptid.pid ());
> + thread_info *child_thr = get_lwp_thread (child_lwp);
>
> + /* If we're suspending all threads, leave this one suspended
> + too. If the fork/clone parent is stepping over a breakpoint,
> + all other threads have been suspended already. Leave the
> + child suspended too. */
> + if (stopping_threads == STOPPING_AND_SUSPENDING_THREADS
> + || event_lwp->bp_reinsert != 0)
> + {
> + threads_debug_printf ("leaving child suspended");
> + child_lwp->suspended = 1;
> + }
> +
> + if (event_lwp->bp_reinsert != 0
> + && supports_software_single_step ()
> + && event == PTRACE_EVENT_VFORK)
> + {
> + /* If we leave single-step breakpoints there, child will
> + hit it, so uninsert single-step breakpoints from parent
> + (and child). Once vfork child is done, reinsert
> + them back to parent. */
> + uninsert_single_step_breakpoints (event_thr);
> + }
> +
> + if (event != PTRACE_EVENT_CLONE)
> + {
> /* Add the new process to the tables and clone the breakpoint
> lists of the parent. We need to do this even if the new process
> will be detached, since we will need the process object and the
> breakpoints to remove any breakpoints from memory when we
> detach, and the client side will access registers. */
> - child_proc = add_linux_process (new_pid, 0);
> + process_info *child_proc = add_linux_process (new_pid, 0);
> gdb_assert (child_proc != NULL);
> - child_lwp = add_lwp (ptid);
> - gdb_assert (child_lwp != NULL);
> - child_lwp->stopped = 1;
> - child_lwp->must_set_ptrace_flags = 1;
> - child_lwp->status_pending_p = 0;
> - child_thr = get_lwp_thread (child_lwp);
> - child_thr->last_resume_kind = resume_stop;
> - child_thr->last_status.set_stopped (GDB_SIGNAL_0);
> -
> - /* If we're suspending all threads, leave this one suspended
> - too. If the fork/clone parent is stepping over a breakpoint,
> - all other threads have been suspended already. Leave the
> - child suspended too. */
> - if (stopping_threads == STOPPING_AND_SUSPENDING_THREADS
> - || event_lwp->bp_reinsert != 0)
> - {
> - threads_debug_printf ("leaving child suspended");
> - child_lwp->suspended = 1;
> - }
>
> - parent_proc = get_thread_process (event_thr);
> + process_info *parent_proc = get_thread_process (event_thr);
> child_proc->attached = parent_proc->attached;
>
> - if (event_lwp->bp_reinsert != 0
> - && supports_software_single_step ()
> - && event == PTRACE_EVENT_VFORK)
> - {
> - /* If we leave single-step breakpoints there, child will
> - hit it, so uninsert single-step breakpoints from parent
> - (and child). Once vfork child is done, reinsert
> - them back to parent. */
> - uninsert_single_step_breakpoints (event_thr);
> - }
> -
> clone_all_breakpoints (child_thr, event_thr);
>
> target_desc_up tdesc = allocate_target_description ();
> @@ -590,88 +592,97 @@ linux_process_target::handle_extended_wait (lwp_info **orig_event_lwp,
>
> /* Clone arch-specific process data. */
> low_new_fork (parent_proc, child_proc);
> + }
>
> - /* Save fork info in the parent thread. */
> - if (event == PTRACE_EVENT_FORK)
> - event_lwp->waitstatus.set_forked (ptid);
> - else if (event == PTRACE_EVENT_VFORK)
> - event_lwp->waitstatus.set_vforked (ptid);
> -
> + /* Save fork/clone info in the parent thread. */
> + if (event == PTRACE_EVENT_FORK)
> + event_lwp->waitstatus.set_forked (child_ptid);
> + else if (event == PTRACE_EVENT_VFORK)
> + event_lwp->waitstatus.set_vforked (child_ptid);
> + else if (event == PTRACE_EVENT_CLONE
> + && (event_thr->thread_options & GDB_THREAD_OPTION_CLONE) != 0)
> + event_lwp->waitstatus.set_thread_cloned (child_ptid);
> +
> + if (event != PTRACE_EVENT_CLONE
> + || (event_thr->thread_options & GDB_THREAD_OPTION_CLONE) != 0)
> + {
> /* The status_pending field contains bits denoting the
> - extended event, so when the pending event is handled,
> - the handler will look at lwp->waitstatus. */
> + extended event, so when the pending event is handled, the
> + handler will look at lwp->waitstatus. */
> event_lwp->status_pending_p = 1;
> event_lwp->status_pending = wstat;
>
> - /* Link the threads until the parent event is passed on to
> - higher layers. */
> + /* Link the threads until the parent's event is passed on to
> + GDB. */
> event_lwp->fork_relative = child_lwp;
> child_lwp->fork_relative = event_lwp;
> -
> - /* If the parent thread is doing step-over with single-step
> - breakpoints, the list of single-step breakpoints are cloned
> - from the parent's. Remove them from the child process.
> - In case of vfork, we'll reinsert them back once vforked
> - child is done. */
> - if (event_lwp->bp_reinsert != 0
> - && supports_software_single_step ())
> - {
> - /* The child process is forked and stopped, so it is safe
> - to access its memory without stopping all other threads
> - from other processes. */
> - delete_single_step_breakpoints (child_thr);
> -
> - gdb_assert (has_single_step_breakpoints (event_thr));
> - gdb_assert (!has_single_step_breakpoints (child_thr));
> - }
> -
> - /* Report the event. */
> - return 0;
> }
>
> - threads_debug_printf
> - ("Got clone event from LWP %ld, new child is LWP %ld",
> - lwpid_of (event_thr), new_pid);
> -
> - ptid = ptid_t (pid_of (event_thr), new_pid);
> - new_lwp = add_lwp (ptid);
> -
> - /* Either we're going to immediately resume the new thread
> - or leave it stopped. resume_one_lwp is a nop if it
> - thinks the thread is currently running, so set this first
> - before calling resume_one_lwp. */
> - new_lwp->stopped = 1;
> + /* If the parent thread is doing step-over with single-step
> + breakpoints, the list of single-step breakpoints are cloned
> + from the parent's. Remove them from the child process.
> + In case of vfork, we'll reinsert them back once vforked
> + child is done. */
> + if (event_lwp->bp_reinsert != 0
> + && supports_software_single_step ())
> + {
> + /* The child process is forked and stopped, so it is safe
> + to access its memory without stopping all other threads
> + from other processes. */
> + delete_single_step_breakpoints (child_thr);
>
> - /* If we're suspending all threads, leave this one suspended
> - too. If the fork/clone parent is stepping over a breakpoint,
> - all other threads have been suspended already. Leave the
> - child suspended too. */
> - if (stopping_threads == STOPPING_AND_SUSPENDING_THREADS
> - || event_lwp->bp_reinsert != 0)
> - new_lwp->suspended = 1;
> + gdb_assert (has_single_step_breakpoints (event_thr));
> + gdb_assert (!has_single_step_breakpoints (child_thr));
> + }
>
> /* Normally we will get the pending SIGSTOP. But in some cases
> we might get another signal delivered to the group first.
> If we do get another signal, be sure not to lose it. */
> if (WSTOPSIG (status) != SIGSTOP)
> {
> - new_lwp->stop_expected = 1;
> - new_lwp->status_pending_p = 1;
> - new_lwp->status_pending = status;
> + child_lwp->stop_expected = 1;
> + child_lwp->status_pending_p = 1;
> + child_lwp->status_pending = status;
> }
> - else if (cs.report_thread_events)
> + else if (event == PTRACE_EVENT_CLONE && cs.report_thread_events)
> {
> - new_lwp->waitstatus.set_thread_created ();
> - new_lwp->status_pending_p = 1;
> - new_lwp->status_pending = status;
> + child_lwp->waitstatus.set_thread_created ();
> + child_lwp->status_pending_p = 1;
> + child_lwp->status_pending = status;
> }
>
> + if (event == PTRACE_EVENT_CLONE)
> + {
> #ifdef USE_THREAD_DB
> - thread_db_notice_clone (event_thr, ptid);
> + thread_db_notice_clone (event_thr, child_ptid);
> #endif
> + }
>
> - /* Don't report the event. */
> - return 1;
> + if (event == PTRACE_EVENT_CLONE
> + && (event_thr->thread_options & GDB_THREAD_OPTION_CLONE) == 0)
> + {
> + threads_debug_printf
> + ("not reporting clone event from LWP %ld, new child is %ld\n",
> + ptid_of (event_thr).lwp (),
> + new_pid);
> + return 1;
> + }
> +
> + /* Leave the child stopped until GDB processes the parent
> + event. */
> + child_thr->last_resume_kind = resume_stop;
> + child_thr->last_status.set_stopped (GDB_SIGNAL_0);
> +
> + /* Report the event. */
> + threads_debug_printf
> + ("reporting %s event from LWP %ld, new child is %ld\n",
> + (event == PTRACE_EVENT_FORK ? "fork"
> + : event == PTRACE_EVENT_VFORK ? "vfork"
> + : event == PTRACE_EVENT_CLONE ? "clone"
> + : "???"),
> + ptid_of (event_thr).lwp (),
> + new_pid);
> + return 0;
> }
> else if (event == PTRACE_EVENT_VFORK_DONE)
> {
> @@ -3527,7 +3538,8 @@ linux_process_target::wait_1 (ptid_t ptid, target_waitstatus *ourstatus,
>
> /* Break the unreported fork relationship chain. */
> if (event_child->waitstatus.kind () == TARGET_WAITKIND_FORKED
> - || event_child->waitstatus.kind () == TARGET_WAITKIND_VFORKED)
> + || event_child->waitstatus.kind () == TARGET_WAITKIND_VFORKED
> + || event_child->waitstatus.kind () == TARGET_WAITKIND_THREAD_CLONED)

Can we not use:

if (is_new_child_status (event_child->waitstatus.kind ()))

here?

I almost asked here: do we really need to check for THREAD_CLONED here,
given the body of the `if` only changes the `fork_relative` field, which
surely is only related to forks, right?

... but then I dug deeper and spotted that the field also used for clone
events now, so: I wonder if we should rename `fork_relative` to
something better, maybe `fork_clone_relative`, or maybe go generic with
something like `related_lwp`?

It just seemed a little confusing (to me).

But otherwise, this patch looks good.

Reviewed-By: Andrew Burgess <aburgess@redhat.com>

Thanks,
Andrew

> {
> event_child->fork_relative->fork_relative = NULL;
> event_child->fork_relative = NULL;
> @@ -4263,15 +4275,14 @@ linux_set_resume_request (thread_info *thread, thread_resume *resume, size_t n)
> continue;
> }
>
> - /* Don't let wildcard resumes resume fork children that GDB
> - does not yet know are new fork children. */
> + /* Don't let wildcard resumes resume fork/vfork/clone
> + children that GDB does not yet know are new children. */
> if (lwp->fork_relative != NULL)
> {
> struct lwp_info *rel = lwp->fork_relative;
>
> if (rel->status_pending_p
> - && (rel->waitstatus.kind () == TARGET_WAITKIND_FORKED
> - || rel->waitstatus.kind () == TARGET_WAITKIND_VFORKED))
> + && is_new_child_status (rel->waitstatus.kind ()))
> {
> threads_debug_printf
> ("not resuming LWP %ld: has queued stop reply",
> @@ -5894,6 +5905,14 @@ linux_process_target::supports_vfork_events ()
> return true;
> }
>
> +/* Return the set of supported thread options. */
> +
> +gdb_thread_options
> +linux_process_target::supported_thread_options ()
> +{
> + return GDB_THREAD_OPTION_CLONE;
> +}
> +
> /* Check if exec events are supported. */
>
> bool
> diff --git a/gdbserver/linux-low.h b/gdbserver/linux-low.h
> index 1594f063f47..69a34fb96fc 100644
> --- a/gdbserver/linux-low.h
> +++ b/gdbserver/linux-low.h
> @@ -234,6 +234,8 @@ class linux_process_target : public process_stratum_target
>
> bool supports_vfork_events () override;
>
> + gdb_thread_options supported_thread_options () override;
> +
> bool supports_exec_events () override;
>
> void handle_new_gdb_connection () override;
> --
> 2.36.0