[04/31] Step over clone syscall w/ breakpoint, TARGET_WAITKIND_THREAD_CLONED
Commit Message
(A good chunk of the problem statement in the commit log below is
Andrew's, adjusted for a different solution, and for covering
displaced stepping too.)
This commit addresses bugs gdb/19675 and gdb/27830, which are about
stepping over a breakpoint set at a clone syscall instruction, one is
about displaced stepping, and the other about in-line stepping.
Currently, when a new thread is created through a clone syscall, GDB
sets the new thread running. With 'continue' this makes sense
(assuming no schedlock):
- all-stop mode, user issues 'continue', all threads are set running,
a newly created thread should also be set running.
- non-stop mode, user issues 'continue', other pre-existing threads
are not affected, but as the new thread is (sort-of) a child of the
thread the user asked to run, it makes sense that the new threads
should be created in the running state.
Similarly, if we are stopped at the clone syscall, and there's no
software breakpoint at this address, then the current behaviour is
fine:
- all-stop mode, user issues 'stepi', stepping will be done in place
(as there's no breakpoint to step over). While stepping the thread
of interest all the other threads will be allowed to continue. A
newly created thread will be set running, and then stopped once the
thread of interest has completed its step.
- non-stop mode, user issues 'stepi', stepping will be done in place
(as there's no breakpoint to step over). Other threads might be
running or stopped, but as with the continue case above, the new
thread will be created running. The only possible issue here is
that the new thread will be left running after the initial thread
has completed its stepi. The user would need to manually select
the thread and interrupt it, this might not be what the user
expects. However, this is not something this commit tries to
change.
The problem then is what happens when we try to step over a clone
syscall if there is a breakpoint at the syscall address.
- For both all-stop and non-stop modes, with in-line stepping:
+ user issues 'stepi',
+ [non-stop mode only] GDB stops all threads. In all-stop mode all
threads are already stopped.
+ GDB removes s/w breakpoint at syscall address,
+ GDB single steps just the thread of interest, all other threads
are left stopped,
+ New thread is created running,
+ Initial thread completes its step,
+ [non-stop mode only] GDB resumes all threads that it previously
stopped.
There are two problems in the in-line stepping scenario above:
1. The new thread might pass through the same code that the initial
thread is in (i.e. the clone syscall code), in which case it will
fail to hit the breakpoint in clone as this was removed so the
first thread can single step,
2. The new thread might trigger some other stop event before the
initial thread reports its step completion. If this happens we
end up triggering an assertion as GDB assumes that only the
thread being stepped should stop. The assert looks like this:
infrun.c:5899: internal-error: int finish_step_over(execution_control_state*): Assertion `ecs->event_thread->control.trap_expected' failed.
- For both all-stop and non-stop modes, with displaced stepping:
+ user issues 'stepi',
+ GDB starts the displaced step, moves thread's PC to the
out-of-line scratch pad, maybe adjusts registers,
+ GDB single steps the thread of interest, [non-stop mode only] all
other threads are left as they were, either running or stopped.
In all-stop, all other threads are left stopped.
+ New thread is created running,
+ Initial thread completes its step, GDB re-adjusts its PC,
restores/releases scratchpad,
+ [non-stop mode only] GDB resumes the thread, now past its
breakpoint.
+ [all-stop mode only] GDB resumes all threads.
There is one problem with the displaced stepping scenario above:
3. When the parent thread completed its step, GDB adjusted its PC,
but did not adjust the child's PC, thus that new child thread
will continue execution in the scratch pad, invoking undefined
behavior. If you're lucky, you see a crash. If unlucky, the
inferior gets silently corrupted.
What is needed is for GDB to have more control over whether the new
thread is created running or not. Issue #1 above requires that the
new thread not be allowed to run until the breakpoint has been
reinserted. The only way to guarantee this is if the new thread is
held in a stopped state until the single step has completed. Issue #3
above requires that GDB is informed of when a thread clones itself,
and of what is the child's ptid, so that GDB can fixup both the parent
and the child.
When looking for solutions to this problem I considered how GDB
handles fork/vfork as these have some of the same issues. The main
difference between fork/vfork and clone is that the clone events are
not reported back to core GDB. Instead, the clone event is handled
automatically in the target code and the child thread is immediately
set running.
Note we have support for requesting thread creation events out of the
target (TARGET_WAITKIND_THREAD_CREATED). However, those are reported
for the new/child thread. That would be sufficient to address in-line
stepping (issue #1), but not for displaced-stepping (issue #3). To
handle displaced-stepping, we need an event that is reported to the
_parent_ of the clone, as the information about the displaced step is
associated with the clone parent. TARGET_WAITKIND_THREAD_CREATED
includes no indication of which thread is the parent that spawned the
new child. In fact, for some targets, like e.g., Windows, it would be
impossible to know which thread that was, as thread creation there
doesn't work by "cloning".
The solution implemented here is to model clone on fork/vfork, and
introduce a new TARGET_WAITKIND_THREAD_CLONED event. This event is
similar to TARGET_WAITKIND_FORKED and TARGET_WAITKIND_VFORKED, except
that we end up with a new thread in the same process, instead of a new
thread of a new process. Like FORKED and VFORKED, THREAD_CLONED
waitstatuses have a child_ptid property, and the child is held stopped
until GDB explicitly resumes it. This addresses the in-line stepping
case (issues #1 and #2).
The infrun code that handles displaced stepping fixup for the child
after a fork/vfork event is thus reused for THREAD_CLONE, with some
minimal conditions added, addressing the displaced stepping case
(issue #3).
The native Linux backend is adjusted to unconditionally report
TARGET_WAITKIND_THREAD_CLONED events to the core.
Following the follow_fork model in core GDB, we introduce a
target_follow_clone target method, which is responsible for making the
new clone child visible to the rest of GDB.
Subsequent patches will add clone events support to the remote
protocol and gdbserver.
A testcase will be added by a later patch.
displaced_step_in_progress_thread becomes unused with this patch, but
a new use will reappear later in the series. To avoid deleting it and
readding it back, this patch marks it with attribute unused, and the
latter patch removes the attribute again. We need to do this because
the function is static, and with no callers, the compiler would warn,
(error with -Werror), breaking the build.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=19675
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=27830
Change-Id: I474e9a7015dd3d33469e322a5764ae83f8a32787
---
gdb/infrun.c | 158 +++++++++++++------------
gdb/linux-nat.c | 248 +++++++++++++++++++++-------------------
gdb/linux-nat.h | 2 +
gdb/target-delegates.c | 24 ++++
gdb/target.c | 7 ++
gdb/target.h | 2 +
gdb/target/waitstatus.c | 1 +
gdb/target/waitstatus.h | 31 ++++-
8 files changed, 281 insertions(+), 192 deletions(-)
Comments
Pedro Alves <pedro@palves.net> writes:
> (A good chunk of the problem statement in the commit log below is
> Andrew's, adjusted for a different solution, and for covering
> displaced stepping too.)
>
> This commit addresses bugs gdb/19675 and gdb/27830, which are about
> stepping over a breakpoint set at a clone syscall instruction, one is
> about displaced stepping, and the other about in-line stepping.
>
> Currently, when a new thread is created through a clone syscall, GDB
> sets the new thread running. With 'continue' this makes sense
> (assuming no schedlock):
>
> - all-stop mode, user issues 'continue', all threads are set running,
> a newly created thread should also be set running.
>
> - non-stop mode, user issues 'continue', other pre-existing threads
> are not affected, but as the new thread is (sort-of) a child of the
> thread the user asked to run, it makes sense that the new threads
> should be created in the running state.
>
> Similarly, if we are stopped at the clone syscall, and there's no
> software breakpoint at this address, then the current behaviour is
> fine:
>
> - all-stop mode, user issues 'stepi', stepping will be done in place
> (as there's no breakpoint to step over). While stepping the thread
> of interest all the other threads will be allowed to continue. A
> newly created thread will be set running, and then stopped once the
> thread of interest has completed its step.
>
> - non-stop mode, user issues 'stepi', stepping will be done in place
> (as there's no breakpoint to step over). Other threads might be
> running or stopped, but as with the continue case above, the new
> thread will be created running. The only possible issue here is
> that the new thread will be left running after the initial thread
> has completed its stepi. The user would need to manually select
> the thread and interrupt it, this might not be what the user
> expects. However, this is not something this commit tries to
> change.
>
> The problem then is what happens when we try to step over a clone
> syscall if there is a breakpoint at the syscall address.
>
> - For both all-stop and non-stop modes, with in-line stepping:
>
> + user issues 'stepi',
> + [non-stop mode only] GDB stops all threads. In all-stop mode all
> threads are already stopped.
> + GDB removes s/w breakpoint at syscall address,
> + GDB single steps just the thread of interest, all other threads
> are left stopped,
> + New thread is created running,
> + Initial thread completes its step,
> + [non-stop mode only] GDB resumes all threads that it previously
> stopped.
>
> There are two problems in the in-line stepping scenario above:
>
> 1. The new thread might pass through the same code that the initial
> thread is in (i.e. the clone syscall code), in which case it will
> fail to hit the breakpoint in clone as this was removed so the
> first thread can single step,
>
> 2. The new thread might trigger some other stop event before the
> initial thread reports its step completion. If this happens we
> end up triggering an assertion as GDB assumes that only the
> thread being stepped should stop. The assert looks like this:
>
> infrun.c:5899: internal-error: int finish_step_over(execution_control_state*): Assertion `ecs->event_thread->control.trap_expected' failed.
>
> - For both all-stop and non-stop modes, with displaced stepping:
>
> + user issues 'stepi',
> + GDB starts the displaced step, moves thread's PC to the
> out-of-line scratch pad, maybe adjusts registers,
> + GDB single steps the thread of interest, [non-stop mode only] all
> other threads are left as they were, either running or stopped.
> In all-stop, all other threads are left stopped.
> + New thread is created running,
> + Initial thread completes its step, GDB re-adjusts its PC,
> restores/releases scratchpad,
> + [non-stop mode only] GDB resumes the thread, now past its
> breakpoint.
> + [all-stop mode only] GDB resumes all threads.
>
> There is one problem with the displaced stepping scenario above:
>
> 3. When the parent thread completed its step, GDB adjusted its PC,
> but did not adjust the child's PC, thus that new child thread
> will continue execution in the scratch pad, invoking undefined
> behavior. If you're lucky, you see a crash. If unlucky, the
> inferior gets silently corrupted.
>
> What is needed is for GDB to have more control over whether the new
> thread is created running or not. Issue #1 above requires that the
> new thread not be allowed to run until the breakpoint has been
> reinserted. The only way to guarantee this is if the new thread is
> held in a stopped state until the single step has completed. Issue #3
> above requires that GDB is informed of when a thread clones itself,
> and of what is the child's ptid, so that GDB can fixup both the parent
> and the child.
>
> When looking for solutions to this problem I considered how GDB
> handles fork/vfork as these have some of the same issues. The main
> difference between fork/vfork and clone is that the clone events are
> not reported back to core GDB. Instead, the clone event is handled
> automatically in the target code and the child thread is immediately
> set running.
>
> Note we have support for requesting thread creation events out of the
> target (TARGET_WAITKIND_THREAD_CREATED). However, those are reported
> for the new/child thread. That would be sufficient to address in-line
> stepping (issue #1), but not for displaced-stepping (issue #3). To
> handle displaced-stepping, we need an event that is reported to the
> _parent_ of the clone, as the information about the displaced step is
> associated with the clone parent. TARGET_WAITKIND_THREAD_CREATED
> includes no indication of which thread is the parent that spawned the
> new child. In fact, for some targets, like e.g., Windows, it would be
> impossible to know which thread that was, as thread creation there
> doesn't work by "cloning".
>
> The solution implemented here is to model clone on fork/vfork, and
> introduce a new TARGET_WAITKIND_THREAD_CLONED event. This event is
> similar to TARGET_WAITKIND_FORKED and TARGET_WAITKIND_VFORKED, except
> that we end up with a new thread in the same process, instead of a new
> thread of a new process. Like FORKED and VFORKED, THREAD_CLONED
> waitstatuses have a child_ptid property, and the child is held stopped
> until GDB explicitly resumes it. This addresses the in-line stepping
> case (issues #1 and #2).
>
> The infrun code that handles displaced stepping fixup for the child
> after a fork/vfork event is thus reused for THREAD_CLONE, with some
> minimal conditions added, addressing the displaced stepping case
> (issue #3).
>
> The native Linux backend is adjusted to unconditionally report
> TARGET_WAITKIND_THREAD_CLONED events to the core.
>
> Following the follow_fork model in core GDB, we introduce a
> target_follow_clone target method, which is responsible for making the
> new clone child visible to the rest of GDB.
>
> Subsequent patches will add clone events support to the remote
> protocol and gdbserver.
>
> A testcase will be added by a later patch.
>
> displaced_step_in_progress_thread becomes unused with this patch, but
> a new use will reappear later in the series. To avoid deleting it and
> readding it back, this patch marks it with attribute unused, and the
> latter patch removes the attribute again. We need to do this because
> the function is static, and with no callers, the compiler would warn,
> (error with -Werror), breaking the build.
>
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=19675
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=27830
>
> Change-Id: I474e9a7015dd3d33469e322a5764ae83f8a32787
> ---
> gdb/infrun.c | 158 +++++++++++++------------
> gdb/linux-nat.c | 248 +++++++++++++++++++++-------------------
> gdb/linux-nat.h | 2 +
> gdb/target-delegates.c | 24 ++++
> gdb/target.c | 7 ++
> gdb/target.h | 2 +
> gdb/target/waitstatus.c | 1 +
> gdb/target/waitstatus.h | 31 ++++-
> 8 files changed, 281 insertions(+), 192 deletions(-)
>
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index 0590310ffac..f7786672004 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -1583,6 +1583,7 @@ step_over_info_valid_p (void)
> /* Return true if THREAD is doing a displaced step. */
>
> static bool
> +ATTRIBUTE_UNUSED
> displaced_step_in_progress_thread (thread_info *thread)
> {
> gdb_assert (thread != nullptr);
> @@ -1897,6 +1898,31 @@ static displaced_step_finish_status
> displaced_step_finish (thread_info *event_thread,
> const target_waitstatus &event_status)
> {
> + /* Check whether the parent is displaced stepping. */
> + struct regcache *regcache = get_thread_regcache (event_thread);
> + struct gdbarch *gdbarch = regcache->arch ();
> + inferior *parent_inf = event_thread->inf;
> +
> + /* If this was a fork/vfork/clone, this event indicates that the
> + displaced stepping of the syscall instruction has been done, so
> + we perform cleanup for parent here. Also note that this
> + operation also cleans up the child for vfork, because their pages
> + are shared. */
> +
> + /* If this is a fork (child gets its own address space copy) and
> + some displaced step buffers were in use at the time of the fork,
> + restore the displaced step buffer bytes in the child process.
> +
> + Architectures which support displaced stepping and fork events
> + must supply an implementation of
> + gdbarch_displaced_step_restore_all_in_ptid. This is not enforced
> + during gdbarch validation to support architectures which support
> + displaced stepping but not forks. */
> + if (event_status.kind () == TARGET_WAITKIND_FORKED
> + && gdbarch_supports_displaced_stepping (gdbarch))
> + gdbarch_displaced_step_restore_all_in_ptid
> + (gdbarch, parent_inf, event_status.child_ptid ());
> +
> displaced_step_thread_state *displaced = &event_thread->displaced_step_state;
>
> /* Was this thread performing a displaced step? */
> @@ -1916,8 +1942,39 @@ displaced_step_finish (thread_info *event_thread,
>
> /* Do the fixup, and release the resources acquired to do the displaced
> step. */
> - return gdbarch_displaced_step_finish (displaced->get_original_gdbarch (),
> - event_thread, event_status);
> + displaced_step_finish_status status
> + = gdbarch_displaced_step_finish (displaced->get_original_gdbarch (),
> + event_thread, event_status);
> +
> + if (event_status.kind () == TARGET_WAITKIND_FORKED
> + || event_status.kind () == TARGET_WAITKIND_VFORKED
> + || event_status.kind () == TARGET_WAITKIND_THREAD_CLONED)
> + {
> + /* Since the vfork/fork/clone syscall instruction was executed
> + in the scratchpad, the child's PC is also within the
> + scratchpad. Set the child's PC to the parent's PC value,
> + which has already been fixed up. Note: we use the parent's
> + aspace here, although we're touching the child, because the
> + child hasn't been added to the inferior list yet at this
> + point. */
> +
> + struct regcache *child_regcache
> + = get_thread_arch_aspace_regcache (parent_inf->process_target (),
> + event_status.child_ptid (),
> + gdbarch,
> + parent_inf->aspace);
> + /* Read PC value of parent. */
> + CORE_ADDR parent_pc = regcache_read_pc (regcache);
> +
> + displaced_debug_printf ("write child pc from %s to %s",
> + paddress (gdbarch,
> + regcache_read_pc (child_regcache)),
> + paddress (gdbarch, parent_pc));
> +
> + regcache_write_pc (child_regcache, parent_pc);
> + }
> +
> + return status;
> }
>
> /* Data to be passed around while handling an event. This data is
> @@ -5663,67 +5720,13 @@ handle_inferior_event (struct execution_control_state *ecs)
>
> case TARGET_WAITKIND_FORKED:
> case TARGET_WAITKIND_VFORKED:
> - /* Check whether the inferior is displaced stepping. */
> - {
> - struct regcache *regcache = get_thread_regcache (ecs->event_thread);
> - struct gdbarch *gdbarch = regcache->arch ();
> - inferior *parent_inf = find_inferior_ptid (ecs->target, ecs->ptid);
> -
> - /* If this is a fork (child gets its own address space copy)
> - and some displaced step buffers were in use at the time of
> - the fork, restore the displaced step buffer bytes in the
> - child process.
> -
> - Architectures which support displaced stepping and fork
> - events must supply an implementation of
> - gdbarch_displaced_step_restore_all_in_ptid. This is not
> - enforced during gdbarch validation to support architectures
> - which support displaced stepping but not forks. */
> - if (ecs->ws.kind () == TARGET_WAITKIND_FORKED
> - && gdbarch_supports_displaced_stepping (gdbarch))
> - gdbarch_displaced_step_restore_all_in_ptid
> - (gdbarch, parent_inf, ecs->ws.child_ptid ());
> -
> - /* If displaced stepping is supported, and thread ecs->ptid is
> - displaced stepping. */
> - if (displaced_step_in_progress_thread (ecs->event_thread))
> - {
> - struct regcache *child_regcache;
> - CORE_ADDR parent_pc;
> -
> - /* GDB has got TARGET_WAITKIND_FORKED or TARGET_WAITKIND_VFORKED,
> - indicating that the displaced stepping of syscall instruction
> - has been done. Perform cleanup for parent process here. Note
> - that this operation also cleans up the child process for vfork,
> - because their pages are shared. */
> - displaced_step_finish (ecs->event_thread, ecs->ws);
> - /* Start a new step-over in another thread if there's one
> - that needs it. */
> - start_step_over ();
> -
> - /* Since the vfork/fork syscall instruction was executed in the scratchpad,
> - the child's PC is also within the scratchpad. Set the child's PC
> - to the parent's PC value, which has already been fixed up.
> - FIXME: we use the parent's aspace here, although we're touching
> - the child, because the child hasn't been added to the inferior
> - list yet at this point. */
> -
> - child_regcache
> - = get_thread_arch_aspace_regcache (parent_inf->process_target (),
> - ecs->ws.child_ptid (),
> - gdbarch,
> - parent_inf->aspace);
> - /* Read PC value of parent process. */
> - parent_pc = regcache_read_pc (regcache);
> -
> - displaced_debug_printf ("write child pc from %s to %s",
> - paddress (gdbarch,
> - regcache_read_pc (child_regcache)),
> - paddress (gdbarch, parent_pc));
> -
> - regcache_write_pc (child_regcache, parent_pc);
> - }
> - }
> + case TARGET_WAITKIND_THREAD_CLONED:
> +
> + displaced_step_finish (ecs->event_thread, ecs->ws);
> +
> + /* Start a new step-over in another thread if there's one that
> + needs it. */
> + start_step_over ();
>
> context_switch (ecs);
>
> @@ -5739,7 +5742,7 @@ handle_inferior_event (struct execution_control_state *ecs)
> need to unpatch at follow/detach time instead to be certain
> that new breakpoints added between catchpoint hit time and
> vfork follow are detached. */
> - if (ecs->ws.kind () != TARGET_WAITKIND_VFORKED)
> + if (ecs->ws.kind () == TARGET_WAITKIND_FORKED)
> {
> /* This won't actually modify the breakpoint list, but will
> physically remove the breakpoints from the child. */
> @@ -5771,14 +5774,24 @@ handle_inferior_event (struct execution_control_state *ecs)
> if (!bpstat_causes_stop (ecs->event_thread->control.stop_bpstat))
> {
> bool follow_child
> - = (follow_fork_mode_string == follow_fork_mode_child);
> + = (ecs->ws.kind () != TARGET_WAITKIND_THREAD_CLONED
> + && follow_fork_mode_string == follow_fork_mode_child);
>
> ecs->event_thread->set_stop_signal (GDB_SIGNAL_0);
>
> process_stratum_target *targ
> = ecs->event_thread->inf->process_target ();
>
> - bool should_resume = follow_fork ();
> + bool should_resume;
> + if (ecs->ws.kind () != TARGET_WAITKIND_THREAD_CLONED)
> + should_resume = follow_fork ();
> + else
> + {
> + should_resume = true;
> + inferior *inf = ecs->event_thread->inf;
> + inf->top_target ()->follow_clone (ecs->ws.child_ptid ());
> + ecs->event_thread->pending_follow.set_spurious ();
> + }
>
> /* Note that one of these may be an invalid pointer,
> depending on detach_fork. */
> @@ -5789,16 +5802,21 @@ handle_inferior_event (struct execution_control_state *ecs)
> child is marked stopped. */
>
> /* If not resuming the parent, mark it stopped. */
> - if (follow_child && !detach_fork && !non_stop && !sched_multi)
> + if (ecs->ws.kind () != TARGET_WAITKIND_THREAD_CLONED
> + && follow_child && !detach_fork && !non_stop && !sched_multi)
> parent->set_running (false);
>
> /* If resuming the child, mark it running. */
> - if (follow_child || (!detach_fork && (non_stop || sched_multi)))
> + if (ecs->ws.kind () == TARGET_WAITKIND_THREAD_CLONED
> + || (follow_child || (!detach_fork && (non_stop || sched_multi))))
> child->set_running (true);
>
> /* In non-stop mode, also resume the other branch. */
> - if (!detach_fork && (non_stop
> - || (sched_multi && target_is_non_stop_p ())))
> + if ((ecs->ws.kind () == TARGET_WAITKIND_THREAD_CLONED
> + && target_is_non_stop_p ())
> + || (!detach_fork && (non_stop
> + || (sched_multi
> + && target_is_non_stop_p ()))))
> {
> if (follow_child)
> switch_to_thread (parent);
> diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
> index 5ee3227f1b9..f3d02b740e8 100644
> --- a/gdb/linux-nat.c
> +++ b/gdb/linux-nat.c
> @@ -1286,64 +1286,79 @@ get_detach_signal (struct lwp_info *lp)
> return 0;
> }
>
> -/* Detach from LP. If SIGNO_P is non-NULL, then it points to the
> - signal number that should be passed to the LWP when detaching.
> - Otherwise pass any pending signal the LWP may have, if any. */
> +/* If LP has a pending fork/vfork/clone status, return it. */
>
> -static void
> -detach_one_lwp (struct lwp_info *lp, int *signo_p)
> +static gdb::optional<target_waitstatus>
> +get_pending_child_status (lwp_info *lp)
> {
> - int lwpid = lp->ptid.lwp ();
> - int signo;
> -
> - gdb_assert (lp->status == 0 || WIFSTOPPED (lp->status));
> -
> - /* If the lwp/thread we are about to detach has a pending fork event,
> - there is a process GDB is attached to that the core of GDB doesn't know
> - about. Detach from it. */
> -
> /* Check in lwp_info::status. */
> if (WIFSTOPPED (lp->status) && linux_is_extended_waitstatus (lp->status))
> {
> int event = linux_ptrace_get_extended_event (lp->status);
>
> - if (event == PTRACE_EVENT_FORK || event == PTRACE_EVENT_VFORK)
> + if (event == PTRACE_EVENT_FORK
> + || event == PTRACE_EVENT_VFORK
> + || event == PTRACE_EVENT_CLONE)
> {
> unsigned long child_pid;
> int ret = ptrace (PTRACE_GETEVENTMSG, lp->ptid.lwp (), 0, &child_pid);
> if (ret == 0)
> - detach_one_pid (child_pid, 0);
> + {
> + target_waitstatus ws;
> +
> + if (event == PTRACE_EVENT_FORK)
> + ws.set_forked (ptid_t (child_pid, child_pid));
> + else if (event == PTRACE_EVENT_VFORK)
> + ws.set_vforked (ptid_t (child_pid, child_pid));
> + else if (event == PTRACE_EVENT_CLONE)
> + ws.set_thread_cloned (ptid_t (lp->ptid.pid (), child_pid));
> + else
> + gdb_assert_not_reached ("unhandled");
> +
> + return ws;
> + }
> else
> - perror_warning_with_name (_("Failed to detach fork child"));
> + {
> + perror_warning_with_name (_("Failed to retrieve event msg"));
> + return {};
> + }
> }
> }
>
> /* Check in lwp_info::waitstatus. */
> - if (lp->waitstatus.kind () == TARGET_WAITKIND_VFORKED
> - || lp->waitstatus.kind () == TARGET_WAITKIND_FORKED)
> - detach_one_pid (lp->waitstatus.child_ptid ().pid (), 0);
> -
> + if (is_new_child_status (lp->waitstatus.kind ()))
> + return lp->waitstatus;
>
> - /* Check in thread_info::pending_waitstatus. */
> thread_info *tp = find_thread_ptid (linux_target, lp->ptid);
> - if (tp->has_pending_waitstatus ())
> - {
> - const target_waitstatus &ws = tp->pending_waitstatus ();
>
> - if (ws.kind () == TARGET_WAITKIND_VFORKED
> - || ws.kind () == TARGET_WAITKIND_FORKED)
> - detach_one_pid (ws.child_ptid ().pid (), 0);
> - }
> + /* Check in thread_info::pending_waitstatus. */
> + if (tp->has_pending_waitstatus ()
> + && is_new_child_status (tp->pending_waitstatus ().kind ()))
> + return tp->pending_waitstatus ();
>
> /* Check in thread_info::pending_follow. */
> - if (tp->pending_follow.kind () == TARGET_WAITKIND_VFORKED
> - || tp->pending_follow.kind () == TARGET_WAITKIND_FORKED)
> - detach_one_pid (tp->pending_follow.child_ptid ().pid (), 0);
> + if (is_new_child_status (tp->pending_follow.kind ()))
> + return tp->pending_follow;
>
> - if (lp->status != 0)
> - linux_nat_debug_printf ("Pending %s for %s on detach.",
> - strsignal (WSTOPSIG (lp->status)),
> - lp->ptid.to_string ().c_str ());
> + return {};
> +}
> +
> +/* Detach from LP. If SIGNO_P is non-NULL, then it points to the
> + signal number that should be passed to the LWP when detaching.
> + Otherwise pass any pending signal the LWP may have, if any. */
> +
> +static void
> +detach_one_lwp (struct lwp_info *lp, int *signo_p)
> +{
> + int lwpid = lp->ptid.lwp ();
> + int signo;
> +
> + /* If the lwp/thread we are about to detach has a pending fork/clone
> + event, there is a process/thread GDB is attached to that the core
> + of GDB doesn't know about. Detach from it. */
> +
> + if (gdb::optional<target_waitstatus> ws = get_pending_child_status (lp))
I know this is just a style issue, but I'm really not a fan of this
assignment in the if condition. I know it probably seems silly, but I'd
find it much clearer if we did:
gdb::optional<target_waitstatus> ws = get_pending_child_status (lp)
if (ws.has_value ())
...
The first time I read the initial line, my first thought was s/=/==/,
then I had to read it again and figure out what was going on...
> + detach_one_pid (ws->child_ptid ().lwp (), 0);
>
> /* If there is a pending SIGSTOP, get rid of it. */
> if (lp->signalled)
> @@ -1821,6 +1836,53 @@ linux_handle_syscall_trap (struct lwp_info *lp, int stopping)
> return 1;
> }
>
I think there should be a:
/* See target.h. */
comment here to point to the documentation in target.h.
> +void
> +linux_nat_target::follow_clone (ptid_t child_ptid)
> +{
> + lwp_info *new_lp = add_lwp (child_ptid);
> + new_lp->stopped = 1;
> +
> + /* If the thread_db layer is active, let it record the user
> + level thread id and status, and add the thread to GDB's
> + list. */
> + if (!thread_db_notice_clone (inferior_ptid, new_lp->ptid))
> + {
> + /* The process is not using thread_db. Add the LWP to
> + GDB's list. */
> + add_thread (linux_target, new_lp->ptid);
> + }
> +
> + /* We just created NEW_LP so it cannot yet contain STATUS. */
> + gdb_assert (new_lp->status == 0);
> +
> + if (!pull_pid_from_list (&stopped_pids, child_ptid.lwp (), &new_lp->status))
> + internal_error (_("no saved status for clone lwp"));
> +
> + if (WSTOPSIG (new_lp->status) != SIGSTOP)
> + {
> + /* This can happen if someone starts sending signals to
> + the new thread before it gets a chance to run, which
> + have a lower number than SIGSTOP (e.g. SIGUSR1).
> + This is an unlikely case, and harder to handle for
> + fork / vfork than for clone, so we do not try - but
> + we handle it for clone events here. */
> +
> + new_lp->signalled = 1;
> +
> + /* Save the wait status to report later. */
> + linux_nat_debug_printf
> + ("waitpid of new LWP %ld, saving status %s",
> + (long) new_lp->ptid.lwp (), status_to_str (new_lp->status).c_str ());
> + }
> + else
> + {
> + new_lp->status = 0;
> +
> + if (report_thread_events)
> + new_lp->waitstatus.set_thread_created ();
> + }
> +}
> +
> /* Handle a GNU/Linux extended wait response. If we see a clone
> event, we need to add the new LWP to our list (and not report the
> trap to higher layers). This function returns non-zero if the
> @@ -1861,11 +1923,9 @@ linux_handle_extended_wait (struct lwp_info *lp, int status)
> internal_error (_("wait returned unexpected status 0x%x"), status);
> }
>
> - ptid_t child_ptid (new_pid, new_pid);
> -
> if (event == PTRACE_EVENT_FORK || event == PTRACE_EVENT_VFORK)
> {
> - open_proc_mem_file (child_ptid);
> + open_proc_mem_file (ptid_t (new_pid, new_pid));
>
> /* The arch-specific native code may need to know about new
> forks even if those end up never mapped to an
> @@ -1902,66 +1962,18 @@ linux_handle_extended_wait (struct lwp_info *lp, int status)
> }
>
> if (event == PTRACE_EVENT_FORK)
> - ourstatus->set_forked (child_ptid);
> + ourstatus->set_forked (ptid_t (new_pid, new_pid));
> else if (event == PTRACE_EVENT_VFORK)
> - ourstatus->set_vforked (child_ptid);
> + ourstatus->set_vforked (ptid_t (new_pid, new_pid));
> else if (event == PTRACE_EVENT_CLONE)
> {
> - struct lwp_info *new_lp;
> -
> - ourstatus->set_ignore ();
> -
> linux_nat_debug_printf
> ("Got clone event from LWP %d, new child is LWP %ld", pid, new_pid);
>
> - new_lp = add_lwp (ptid_t (lp->ptid.pid (), new_pid));
> - new_lp->stopped = 1;
> - new_lp->resumed = 1;
> + /* Save the status again, we'll use it in follow_clone. */
> + add_to_pid_list (&stopped_pids, new_pid, status);
>
> - /* If the thread_db layer is active, let it record the user
> - level thread id and status, and add the thread to GDB's
> - list. */
> - if (!thread_db_notice_clone (lp->ptid, new_lp->ptid))
> - {
> - /* The process is not using thread_db. Add the LWP to
> - GDB's list. */
> - add_thread (linux_target, new_lp->ptid);
> - }
> -
> - /* Even if we're stopping the thread for some reason
> - internal to this module, from the perspective of infrun
> - and the user/frontend, this new thread is running until
> - it next reports a stop. */
> - set_running (linux_target, new_lp->ptid, true);
> - set_executing (linux_target, new_lp->ptid, true);
> -
> - if (WSTOPSIG (status) != SIGSTOP)
> - {
> - /* This can happen if someone starts sending signals to
> - the new thread before it gets a chance to run, which
> - have a lower number than SIGSTOP (e.g. SIGUSR1).
> - This is an unlikely case, and harder to handle for
> - fork / vfork than for clone, so we do not try - but
> - we handle it for clone events here. */
> -
> - new_lp->signalled = 1;
> -
> - /* We created NEW_LP so it cannot yet contain STATUS. */
> - gdb_assert (new_lp->status == 0);
> -
> - /* Save the wait status to report later. */
> - linux_nat_debug_printf
> - ("waitpid of new LWP %ld, saving status %s",
> - (long) new_lp->ptid.lwp (), status_to_str (status).c_str ());
> - new_lp->status = status;
> - }
> - else if (report_thread_events)
> - {
> - new_lp->waitstatus.set_thread_created ();
> - new_lp->status = status;
> - }
> -
> - return 1;
> + ourstatus->set_thread_cloned (ptid_t (lp->ptid.pid (), new_pid));
> }
>
> return 0;
> @@ -3536,59 +3548,55 @@ kill_wait_callback (struct lwp_info *lp)
> return 0;
> }
>
> -/* Kill the fork children of any threads of inferior INF that are
> - stopped at a fork event. */
> +/* Kill the fork/clone child of LP if it has an unfollowed child. */
>
> -static void
> -kill_unfollowed_fork_children (struct inferior *inf)
> +static int
> +kill_unfollowed_child_callback (lwp_info *lp)
> {
> - for (thread_info *thread : inf->non_exited_threads ())
> + if (gdb::optional<target_waitstatus> ws = get_pending_child_status (lp))
> {
> - struct target_waitstatus *ws = &thread->pending_follow;
> -
> - if (ws->kind () == TARGET_WAITKIND_FORKED
> - || ws->kind () == TARGET_WAITKIND_VFORKED)
> - {
> - ptid_t child_ptid = ws->child_ptid ();
> - int child_pid = child_ptid.pid ();
> - int child_lwp = child_ptid.lwp ();
> + ptid_t child_ptid = ws->child_ptid ();
> + int child_pid = child_ptid.pid ();
> + int child_lwp = child_ptid.lwp ();
>
> - kill_one_lwp (child_lwp);
> - kill_wait_one_lwp (child_lwp);
> + kill_one_lwp (child_lwp);
> + kill_wait_one_lwp (child_lwp);
>
> - /* Let the arch-specific native code know this process is
> - gone. */
> - linux_target->low_forget_process (child_pid);
> - }
> + /* Let the arch-specific native code know this process is
> + gone. */
> + if (ws->kind () != TARGET_WAITKIND_THREAD_CLONED)
> + linux_target->low_forget_process (child_pid);
> }
> +
> + return 0;
> }
>
> void
> linux_nat_target::kill ()
> {
> - /* If we're stopped while forking and we haven't followed yet,
> - kill the other task. We need to do this first because the
> + ptid_t pid_ptid (inferior_ptid.pid ());
> +
> + /* If we're stopped while forking/cloning and we haven't followed
> + yet, kill the child task. We need to do this first because the
> parent will be sleeping if this is a vfork. */
> - kill_unfollowed_fork_children (current_inferior ());
> + iterate_over_lwps (pid_ptid, kill_unfollowed_child_callback);
>
> if (forks_exist_p ())
> linux_fork_killall ();
> else
> {
> - ptid_t ptid = ptid_t (inferior_ptid.pid ());
> -
> /* Stop all threads before killing them, since ptrace requires
> that the thread is stopped to successfully PTRACE_KILL. */
> - iterate_over_lwps (ptid, stop_callback);
> + iterate_over_lwps (pid_ptid, stop_callback);
> /* ... and wait until all of them have reported back that
> they're no longer running. */
> - iterate_over_lwps (ptid, stop_wait_callback);
> + iterate_over_lwps (pid_ptid, stop_wait_callback);
>
> /* Kill all LWP's ... */
> - iterate_over_lwps (ptid, kill_callback);
> + iterate_over_lwps (pid_ptid, kill_callback);
>
> /* ... and wait until we've flushed all events. */
> - iterate_over_lwps (ptid, kill_wait_callback);
> + iterate_over_lwps (pid_ptid, kill_wait_callback);
> }
>
> target_mourn_inferior (inferior_ptid);
> diff --git a/gdb/linux-nat.h b/gdb/linux-nat.h
> index a9b91a5e908..3ed25cc5ba4 100644
> --- a/gdb/linux-nat.h
> +++ b/gdb/linux-nat.h
> @@ -129,6 +129,8 @@ class linux_nat_target : public inf_ptrace_target
>
> void follow_fork (inferior *, ptid_t, target_waitkind, bool, bool) override;
>
> + void follow_clone (ptid_t) override;
> +
> std::vector<static_tracepoint_marker>
> static_tracepoint_markers_by_strid (const char *id) override;
>
> diff --git a/gdb/target-delegates.c b/gdb/target-delegates.c
> index daf46821be0..bee46608c38 100644
> --- a/gdb/target-delegates.c
> +++ b/gdb/target-delegates.c
> @@ -76,6 +76,7 @@ struct dummy_target : public target_ops
> int insert_vfork_catchpoint (int arg0) override;
> int remove_vfork_catchpoint (int arg0) override;
> void follow_fork (inferior *arg0, ptid_t arg1, target_waitkind arg2, bool arg3, bool arg4) override;
> + void follow_clone (ptid_t arg0) override;
> int insert_exec_catchpoint (int arg0) override;
> int remove_exec_catchpoint (int arg0) override;
> void follow_exec (inferior *arg0, ptid_t arg1, const char *arg2) override;
> @@ -250,6 +251,7 @@ struct debug_target : public target_ops
> int insert_vfork_catchpoint (int arg0) override;
> int remove_vfork_catchpoint (int arg0) override;
> void follow_fork (inferior *arg0, ptid_t arg1, target_waitkind arg2, bool arg3, bool arg4) override;
> + void follow_clone (ptid_t arg0) override;
> int insert_exec_catchpoint (int arg0) override;
> int remove_exec_catchpoint (int arg0) override;
> void follow_exec (inferior *arg0, ptid_t arg1, const char *arg2) override;
> @@ -1545,6 +1547,28 @@ debug_target::follow_fork (inferior *arg0, ptid_t arg1, target_waitkind arg2, bo
> gdb_puts (")\n", gdb_stdlog);
> }
>
> +void
> +target_ops::follow_clone (ptid_t arg0)
> +{
> + this->beneath ()->follow_clone (arg0);
> +}
> +
> +void
> +dummy_target::follow_clone (ptid_t arg0)
> +{
> + default_follow_clone (this, arg0);
> +}
> +
> +void
> +debug_target::follow_clone (ptid_t arg0)
> +{
> + gdb_printf (gdb_stdlog, "-> %s->follow_clone (...)\n", this->beneath ()->shortname ());
> + this->beneath ()->follow_clone (arg0);
> + gdb_printf (gdb_stdlog, "<- %s->follow_clone (", this->beneath ()->shortname ());
> + target_debug_print_ptid_t (arg0);
> + gdb_puts (")\n", gdb_stdlog);
> +}
> +
> int
> target_ops::insert_exec_catchpoint (int arg0)
> {
> diff --git a/gdb/target.c b/gdb/target.c
> index f781f7e4f96..2fb09c2817d 100644
> --- a/gdb/target.c
> +++ b/gdb/target.c
> @@ -2732,6 +2732,13 @@ default_follow_fork (struct target_ops *self, inferior *child_inf,
> internal_error (_("could not find a target to follow fork"));
> }
>
> +static void
> +default_follow_clone (struct target_ops *self, ptid_t child_ptid)
> +{
> + /* Some target returned a clone event, but did not know how to follow it. */
> + internal_error (_("could not find a target to follow clone"));
> +}
> +
> /* See target.h. */
>
> void
> diff --git a/gdb/target.h b/gdb/target.h
> index 28aa9273893..aab390aec57 100644
> --- a/gdb/target.h
> +++ b/gdb/target.h
> @@ -637,6 +637,8 @@ struct target_ops
> TARGET_DEFAULT_RETURN (1);
> virtual void follow_fork (inferior *, ptid_t, target_waitkind, bool, bool)
> TARGET_DEFAULT_FUNC (default_follow_fork);
> + virtual void follow_clone (ptid_t)
> + TARGET_DEFAULT_FUNC (default_follow_clone);
One thing that sucks about some of the older parts of the target API is
just how undocumented it is. The only way to figure out what things
like follow_fork do is to look at the code.
Some of the newer methods do have good comments. Or at least comments
that give a good hint to what the method does.
I'd be really grateful if new target API methods could be given a good
comment in target.h.
Thanks,
Andrew
> virtual int insert_exec_catchpoint (int)
> TARGET_DEFAULT_RETURN (1);
> virtual int remove_exec_catchpoint (int)
> diff --git a/gdb/target/waitstatus.c b/gdb/target/waitstatus.c
> index ef432bb629d..3e45e4f32fa 100644
> --- a/gdb/target/waitstatus.c
> +++ b/gdb/target/waitstatus.c
> @@ -45,6 +45,7 @@ DIAGNOSTIC_ERROR_SWITCH
>
> case TARGET_WAITKIND_FORKED:
> case TARGET_WAITKIND_VFORKED:
> + case TARGET_WAITKIND_THREAD_CLONED:
> return string_appendf (str, ", child_ptid = %s",
> this->child_ptid ().to_string ().c_str ());
>
> diff --git a/gdb/target/waitstatus.h b/gdb/target/waitstatus.h
> index 63bbd737749..2072eb018ae 100644
> --- a/gdb/target/waitstatus.h
> +++ b/gdb/target/waitstatus.h
> @@ -95,6 +95,13 @@ enum target_waitkind
> /* There are no resumed children left in the program. */
> TARGET_WAITKIND_NO_RESUMED,
>
> + /* The thread was cloned. The event's ptid corresponds to the
> + cloned parent. The cloned child is held stopped at its entry
> + point, and its ptid is in the event's m_child_ptid. The target
> + must not add the cloned child to GDB's thread list until
> + target_ops::follow_clone() is called. */
> + TARGET_WAITKIND_THREAD_CLONED,
> +
> /* The thread was created. */
> TARGET_WAITKIND_THREAD_CREATED,
>
> @@ -102,6 +109,17 @@ enum target_waitkind
> TARGET_WAITKIND_THREAD_EXITED,
> };
>
> +/* Determine if KIND represents an event with a new child - a fork,
> + vfork, or clone. */
> +
> +static inline bool
> +is_new_child_status (target_waitkind kind)
> +{
> + return (kind == TARGET_WAITKIND_FORKED
> + || kind == TARGET_WAITKIND_VFORKED
> + || kind == TARGET_WAITKIND_THREAD_CLONED);
> +}
> +
> /* Return KIND as a string. */
>
> static inline const char *
> @@ -125,6 +143,8 @@ DIAGNOSTIC_ERROR_SWITCH
> return "FORKED";
> case TARGET_WAITKIND_VFORKED:
> return "VFORKED";
> + case TARGET_WAITKIND_THREAD_CLONED:
> + return "THREAD_CLONED";
> case TARGET_WAITKIND_EXECD:
> return "EXECD";
> case TARGET_WAITKIND_VFORK_DONE:
> @@ -325,6 +345,14 @@ struct target_waitstatus
> return *this;
> }
>
> + target_waitstatus &set_thread_cloned (ptid_t child_ptid)
> + {
> + this->reset ();
> + m_kind = TARGET_WAITKIND_THREAD_CLONED;
> + m_value.child_ptid = child_ptid;
> + return *this;
> + }
> +
> target_waitstatus &set_thread_created ()
> {
> this->reset ();
> @@ -369,8 +397,7 @@ struct target_waitstatus
>
> ptid_t child_ptid () const
> {
> - gdb_assert (m_kind == TARGET_WAITKIND_FORKED
> - || m_kind == TARGET_WAITKIND_VFORKED);
> + gdb_assert (is_new_child_status (m_kind));
> return m_value.child_ptid;
> }
>
> --
> 2.36.0
On 2023-02-04 3:38 p.m., Andrew Burgess wrote:
> Pedro Alves <pedro@palves.net> writes:
>> +/* Detach from LP. If SIGNO_P is non-NULL, then it points to the
>> + signal number that should be passed to the LWP when detaching.
>> + Otherwise pass any pending signal the LWP may have, if any. */
>> +
>> +static void
>> +detach_one_lwp (struct lwp_info *lp, int *signo_p)
>> +{
>> + int lwpid = lp->ptid.lwp ();
>> + int signo;
>> +
>> + /* If the lwp/thread we are about to detach has a pending fork/clone
>> + event, there is a process/thread GDB is attached to that the core
>> + of GDB doesn't know about. Detach from it. */
>> +
>> + if (gdb::optional<target_waitstatus> ws = get_pending_child_status (lp))
>
> I know this is just a style issue, but I'm really not a fan of this
> assignment in the if condition. I know it probably seems silly, but I'd
> find it much clearer if we did:
>
> gdb::optional<target_waitstatus> ws = get_pending_child_status (lp)
> if (ws.has_value ())
> ...
>
> The first time I read the initial line, my first thought was s/=/==/,
> then I had to read it again and figure out what was going on...
>
Ah, I think I was asked to switch to this style in a previous review, so
clearly it's a matter of taste. :-D
I was using this originally:
target_waitstatus ws;
if (get_pending_child_status (lp, &ws))
{
I used to think that "if (auto opt = foo ())" might be confusing too, to be honest,
but I've convinced myself that it is OK (but please see further below) for a couple
reasons:
#1 - these aren't really equivalent:
a) if (type var = func ())
b) itype var = func ();
if (var)
because in a), the scope of the variable is just the if then/else
scopes. So you can for example reuse the name like:
if (type var = func ())
{
}
if (type var = bar ())
{
}
and also you're guaranteed that the variable's dtor is run at the end of the
if block. So it'd be more equivalent to writing an explicit scope, like:
{
itype var = func ();
if (var)
{
...
}
}
#2 - the presence of the type makes it visually distinctive from the problematic case
of writing to a variable by accident:
if (var = func ()) // bad
if (var == func ()) // ok
if (auto var = func ()) // ok
if (auto var == func ()) // not valid
and, it's not different from initializing a variable in a for, like:
for (auto var = func (); ...; ...)
and we don't bat an eye when we see that.
in fact, C++17 let's you write an if with a for-like init-statement, like so:
if (auto var = func (); var)
so I think this is a case of being surprised at first, but then we just
get used to it quickly.
However, I've still converted to use the style you suggested, because
there might be a different reason for wanting to avoid that style, which is that
this:
if (gdb::optional<target_waitstatus> ws = get_pending_child_status (lp))
begs the question of whether we want to allow:
if (foo *ptr = get_foo ())
and this runs counter to our style of doing explicit nullptr checks... So
we can leave that to a seperate discussion and I don't want to be blocked
by it. :-)
>> diff --git a/gdb/target.h b/gdb/target.h
>> index 28aa9273893..aab390aec57 100644
>> --- a/gdb/target.h
>> +++ b/gdb/target.h
>> @@ -637,6 +637,8 @@ struct target_ops
>> TARGET_DEFAULT_RETURN (1);
>> virtual void follow_fork (inferior *, ptid_t, target_waitkind, bool, bool)
>> TARGET_DEFAULT_FUNC (default_follow_fork);
>> + virtual void follow_clone (ptid_t)
>> + TARGET_DEFAULT_FUNC (default_follow_clone);
>
> One thing that sucks about some of the older parts of the target API is
> just how undocumented it is. The only way to figure out what things
> like follow_fork do is to look at the code.
I don't disagree completely, but note that the comments for the target_ops methods
tend to be in the target_foo wrapper method. For example, target_follow_fork.
In this case, there's no such wrapper method, so I have no good excuse. :-)
I've added a comment, like:
--- c/gdb/target.h
+++ w/gdb/target.h
@@ -637,8 +637,13 @@ struct target_ops
TARGET_DEFAULT_RETURN (1);
virtual void follow_fork (inferior *, ptid_t, target_waitkind, bool, bool)
TARGET_DEFAULT_FUNC (default_follow_fork);
- virtual void follow_clone (ptid_t)
+
+ /* Add CHILD_PTID to the thread list, after handling a
+ TARGET_WAITKIND_THREAD_CLONE event for the clone parent. The
+ parent is inferior_ptid. */
+ virtual void follow_clone (ptid_t child_ptid)
TARGET_DEFAULT_FUNC (default_follow_clone);
+
virtual int insert_exec_catchpoint (int)
TARGET_DEFAULT_RETURN (1);
virtual int remove_exec_catchpoint (int)
>
> Some of the newer methods do have good comments. Or at least comments
> that give a good hint to what the method does.
>
> I'd be really grateful if new target API methods could be given a good
> comment in target.h.
Here's the full patch. Let me know what you think.
From 3b2638cf7507146385e16473c883b7d0d4a75277 Mon Sep 17 00:00:00 2001
From: Pedro Alves <pedro@palves.net>
Date: Fri, 12 Nov 2021 20:50:29 +0000
Subject: [PATCH] Step over clone syscall w/ breakpoint,
TARGET_WAITKIND_THREAD_CLONED
(A good chunk of the problem statement in the commit log below is
Andrew's, adjusted for a different solution, and for covering
displaced stepping too.)
This commit addresses bugs gdb/19675 and gdb/27830, which are about
stepping over a breakpoint set at a clone syscall instruction, one is
about displaced stepping, and the other about in-line stepping.
Currently, when a new thread is created through a clone syscall, GDB
sets the new thread running. With 'continue' this makes sense
(assuming no schedlock):
- all-stop mode, user issues 'continue', all threads are set running,
a newly created thread should also be set running.
- non-stop mode, user issues 'continue', other pre-existing threads
are not affected, but as the new thread is (sort-of) a child of the
thread the user asked to run, it makes sense that the new threads
should be created in the running state.
Similarly, if we are stopped at the clone syscall, and there's no
software breakpoint at this address, then the current behaviour is
fine:
- all-stop mode, user issues 'stepi', stepping will be done in place
(as there's no breakpoint to step over). While stepping the thread
of interest all the other threads will be allowed to continue. A
newly created thread will be set running, and then stopped once the
thread of interest has completed its step.
- non-stop mode, user issues 'stepi', stepping will be done in place
(as there's no breakpoint to step over). Other threads might be
running or stopped, but as with the continue case above, the new
thread will be created running. The only possible issue here is
that the new thread will be left running after the initial thread
has completed its stepi. The user would need to manually select
the thread and interrupt it, this might not be what the user
expects. However, this is not something this commit tries to
change.
The problem then is what happens when we try to step over a clone
syscall if there is a breakpoint at the syscall address.
- For both all-stop and non-stop modes, with in-line stepping:
+ user issues 'stepi',
+ [non-stop mode only] GDB stops all threads. In all-stop mode all
threads are already stopped.
+ GDB removes s/w breakpoint at syscall address,
+ GDB single steps just the thread of interest, all other threads
are left stopped,
+ New thread is created running,
+ Initial thread completes its step,
+ [non-stop mode only] GDB resumes all threads that it previously
stopped.
There are two problems in the in-line stepping scenario above:
1. The new thread might pass through the same code that the initial
thread is in (i.e. the clone syscall code), in which case it will
fail to hit the breakpoint in clone as this was removed so the
first thread can single step,
2. The new thread might trigger some other stop event before the
initial thread reports its step completion. If this happens we
end up triggering an assertion as GDB assumes that only the
thread being stepped should stop. The assert looks like this:
infrun.c:5899: internal-error: int finish_step_over(execution_control_state*): Assertion `ecs->event_thread->control.trap_expected' failed.
- For both all-stop and non-stop modes, with displaced stepping:
+ user issues 'stepi',
+ GDB starts the displaced step, moves thread's PC to the
out-of-line scratch pad, maybe adjusts registers,
+ GDB single steps the thread of interest, [non-stop mode only] all
other threads are left as they were, either running or stopped.
In all-stop, all other threads are left stopped.
+ New thread is created running,
+ Initial thread completes its step, GDB re-adjusts its PC,
restores/releases scratchpad,
+ [non-stop mode only] GDB resumes the thread, now past its
breakpoint.
+ [all-stop mode only] GDB resumes all threads.
There is one problem with the displaced stepping scenario above:
3. When the parent thread completed its step, GDB adjusted its PC,
but did not adjust the child's PC, thus that new child thread
will continue execution in the scratch pad, invoking undefined
behavior. If you're lucky, you see a crash. If unlucky, the
inferior gets silently corrupted.
What is needed is for GDB to have more control over whether the new
thread is created running or not. Issue #1 above requires that the
new thread not be allowed to run until the breakpoint has been
reinserted. The only way to guarantee this is if the new thread is
held in a stopped state until the single step has completed. Issue #3
above requires that GDB is informed of when a thread clones itself,
and of what is the child's ptid, so that GDB can fixup both the parent
and the child.
When looking for solutions to this problem I considered how GDB
handles fork/vfork as these have some of the same issues. The main
difference between fork/vfork and clone is that the clone events are
not reported back to core GDB. Instead, the clone event is handled
automatically in the target code and the child thread is immediately
set running.
Note we have support for requesting thread creation events out of the
target (TARGET_WAITKIND_THREAD_CREATED). However, those are reported
for the new/child thread. That would be sufficient to address in-line
stepping (issue #1), but not for displaced-stepping (issue #3). To
handle displaced-stepping, we need an event that is reported to the
_parent_ of the clone, as the information about the displaced step is
associated with the clone parent. TARGET_WAITKIND_THREAD_CREATED
includes no indication of which thread is the parent that spawned the
new child. In fact, for some targets, like e.g., Windows, it would be
impossible to know which thread that was, as thread creation there
doesn't work by "cloning".
The solution implemented here is to model clone on fork/vfork, and
introduce a new TARGET_WAITKIND_THREAD_CLONED event. This event is
similar to TARGET_WAITKIND_FORKED and TARGET_WAITKIND_VFORKED, except
that we end up with a new thread in the same process, instead of a new
thread of a new process. Like FORKED and VFORKED, THREAD_CLONED
waitstatuses have a child_ptid property, and the child is held stopped
until GDB explicitly resumes it. This addresses the in-line stepping
case (issues #1 and #2).
The infrun code that handles displaced stepping fixup for the child
after a fork/vfork event is thus reused for THREAD_CLONE, with some
minimal conditions added, addressing the displaced stepping case
(issue #3).
The native Linux backend is adjusted to unconditionally report
TARGET_WAITKIND_THREAD_CLONED events to the core.
Following the follow_fork model in core GDB, we introduce a
target_follow_clone target method, which is responsible for making the
new clone child visible to the rest of GDB.
Subsequent patches will add clone events support to the remote
protocol and gdbserver.
A testcase will be added by a later patch.
displaced_step_in_progress_thread becomes unused with this patch, but
a new use will reappear later in the series. To avoid deleting it and
readding it back, this patch marks it with attribute unused, and the
latter patch removes the attribute again. We need to do this because
the function is static, and with no callers, the compiler would warn,
(error with -Werror), breaking the build.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=19675
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=27830
Change-Id: I474e9a7015dd3d33469e322a5764ae83f8a32787
---
gdb/infrun.c | 158 ++++++++++++++-----------
gdb/linux-nat.c | 250 +++++++++++++++++++++-------------------
gdb/linux-nat.h | 2 +
gdb/target-delegates.c | 24 ++++
gdb/target.c | 7 ++
gdb/target.h | 2 +
gdb/target/waitstatus.c | 1 +
gdb/target/waitstatus.h | 31 ++++-
8 files changed, 283 insertions(+), 192 deletions(-)
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 99f2a8e039d..d1e6233591c 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -1584,6 +1584,7 @@ step_over_info_valid_p (void)
/* Return true if THREAD is doing a displaced step. */
static bool
+ATTRIBUTE_UNUSED
displaced_step_in_progress_thread (thread_info *thread)
{
gdb_assert (thread != nullptr);
@@ -1898,6 +1899,31 @@ static displaced_step_finish_status
displaced_step_finish (thread_info *event_thread,
const target_waitstatus &event_status)
{
+ /* Check whether the parent is displaced stepping. */
+ struct regcache *regcache = get_thread_regcache (event_thread);
+ struct gdbarch *gdbarch = regcache->arch ();
+ inferior *parent_inf = event_thread->inf;
+
+ /* If this was a fork/vfork/clone, this event indicates that the
+ displaced stepping of the syscall instruction has been done, so
+ we perform cleanup for parent here. Also note that this
+ operation also cleans up the child for vfork, because their pages
+ are shared. */
+
+ /* If this is a fork (child gets its own address space copy) and
+ some displaced step buffers were in use at the time of the fork,
+ restore the displaced step buffer bytes in the child process.
+
+ Architectures which support displaced stepping and fork events
+ must supply an implementation of
+ gdbarch_displaced_step_restore_all_in_ptid. This is not enforced
+ during gdbarch validation to support architectures which support
+ displaced stepping but not forks. */
+ if (event_status.kind () == TARGET_WAITKIND_FORKED
+ && gdbarch_supports_displaced_stepping (gdbarch))
+ gdbarch_displaced_step_restore_all_in_ptid
+ (gdbarch, parent_inf, event_status.child_ptid ());
+
displaced_step_thread_state *displaced = &event_thread->displaced_step_state;
/* Was this thread performing a displaced step? */
@@ -1917,8 +1943,39 @@ displaced_step_finish (thread_info *event_thread,
/* Do the fixup, and release the resources acquired to do the displaced
step. */
- return gdbarch_displaced_step_finish (displaced->get_original_gdbarch (),
- event_thread, event_status);
+ displaced_step_finish_status status
+ = gdbarch_displaced_step_finish (displaced->get_original_gdbarch (),
+ event_thread, event_status);
+
+ if (event_status.kind () == TARGET_WAITKIND_FORKED
+ || event_status.kind () == TARGET_WAITKIND_VFORKED
+ || event_status.kind () == TARGET_WAITKIND_THREAD_CLONED)
+ {
+ /* Since the vfork/fork/clone syscall instruction was executed
+ in the scratchpad, the child's PC is also within the
+ scratchpad. Set the child's PC to the parent's PC value,
+ which has already been fixed up. Note: we use the parent's
+ aspace here, although we're touching the child, because the
+ child hasn't been added to the inferior list yet at this
+ point. */
+
+ struct regcache *child_regcache
+ = get_thread_arch_aspace_regcache (parent_inf->process_target (),
+ event_status.child_ptid (),
+ gdbarch,
+ parent_inf->aspace);
+ /* Read PC value of parent. */
+ CORE_ADDR parent_pc = regcache_read_pc (regcache);
+
+ displaced_debug_printf ("write child pc from %s to %s",
+ paddress (gdbarch,
+ regcache_read_pc (child_regcache)),
+ paddress (gdbarch, parent_pc));
+
+ regcache_write_pc (child_regcache, parent_pc);
+ }
+
+ return status;
}
/* Data to be passed around while handling an event. This data is
@@ -5717,67 +5774,13 @@ handle_inferior_event (struct execution_control_state *ecs)
case TARGET_WAITKIND_FORKED:
case TARGET_WAITKIND_VFORKED:
- /* Check whether the inferior is displaced stepping. */
- {
- struct regcache *regcache = get_thread_regcache (ecs->event_thread);
- struct gdbarch *gdbarch = regcache->arch ();
- inferior *parent_inf = find_inferior_ptid (ecs->target, ecs->ptid);
-
- /* If this is a fork (child gets its own address space copy)
- and some displaced step buffers were in use at the time of
- the fork, restore the displaced step buffer bytes in the
- child process.
-
- Architectures which support displaced stepping and fork
- events must supply an implementation of
- gdbarch_displaced_step_restore_all_in_ptid. This is not
- enforced during gdbarch validation to support architectures
- which support displaced stepping but not forks. */
- if (ecs->ws.kind () == TARGET_WAITKIND_FORKED
- && gdbarch_supports_displaced_stepping (gdbarch))
- gdbarch_displaced_step_restore_all_in_ptid
- (gdbarch, parent_inf, ecs->ws.child_ptid ());
-
- /* If displaced stepping is supported, and thread ecs->ptid is
- displaced stepping. */
- if (displaced_step_in_progress_thread (ecs->event_thread))
- {
- struct regcache *child_regcache;
- CORE_ADDR parent_pc;
-
- /* GDB has got TARGET_WAITKIND_FORKED or TARGET_WAITKIND_VFORKED,
- indicating that the displaced stepping of syscall instruction
- has been done. Perform cleanup for parent process here. Note
- that this operation also cleans up the child process for vfork,
- because their pages are shared. */
- displaced_step_finish (ecs->event_thread, ecs->ws);
- /* Start a new step-over in another thread if there's one
- that needs it. */
- start_step_over ();
-
- /* Since the vfork/fork syscall instruction was executed in the scratchpad,
- the child's PC is also within the scratchpad. Set the child's PC
- to the parent's PC value, which has already been fixed up.
- FIXME: we use the parent's aspace here, although we're touching
- the child, because the child hasn't been added to the inferior
- list yet at this point. */
-
- child_regcache
- = get_thread_arch_aspace_regcache (parent_inf->process_target (),
- ecs->ws.child_ptid (),
- gdbarch,
- parent_inf->aspace);
- /* Read PC value of parent process. */
- parent_pc = regcache_read_pc (regcache);
-
- displaced_debug_printf ("write child pc from %s to %s",
- paddress (gdbarch,
- regcache_read_pc (child_regcache)),
- paddress (gdbarch, parent_pc));
-
- regcache_write_pc (child_regcache, parent_pc);
- }
- }
+ case TARGET_WAITKIND_THREAD_CLONED:
+
+ displaced_step_finish (ecs->event_thread, ecs->ws);
+
+ /* Start a new step-over in another thread if there's one that
+ needs it. */
+ start_step_over ();
context_switch (ecs);
@@ -5793,7 +5796,7 @@ handle_inferior_event (struct execution_control_state *ecs)
need to unpatch at follow/detach time instead to be certain
that new breakpoints added between catchpoint hit time and
vfork follow are detached. */
- if (ecs->ws.kind () != TARGET_WAITKIND_VFORKED)
+ if (ecs->ws.kind () == TARGET_WAITKIND_FORKED)
{
/* This won't actually modify the breakpoint list, but will
physically remove the breakpoints from the child. */
@@ -5825,14 +5828,24 @@ handle_inferior_event (struct execution_control_state *ecs)
if (!bpstat_causes_stop (ecs->event_thread->control.stop_bpstat))
{
bool follow_child
- = (follow_fork_mode_string == follow_fork_mode_child);
+ = (ecs->ws.kind () != TARGET_WAITKIND_THREAD_CLONED
+ && follow_fork_mode_string == follow_fork_mode_child);
ecs->event_thread->set_stop_signal (GDB_SIGNAL_0);
process_stratum_target *targ
= ecs->event_thread->inf->process_target ();
- bool should_resume = follow_fork ();
+ bool should_resume;
+ if (ecs->ws.kind () != TARGET_WAITKIND_THREAD_CLONED)
+ should_resume = follow_fork ();
+ else
+ {
+ should_resume = true;
+ inferior *inf = ecs->event_thread->inf;
+ inf->top_target ()->follow_clone (ecs->ws.child_ptid ());
+ ecs->event_thread->pending_follow.set_spurious ();
+ }
/* Note that one of these may be an invalid pointer,
depending on detach_fork. */
@@ -5843,16 +5856,21 @@ handle_inferior_event (struct execution_control_state *ecs)
child is marked stopped. */
/* If not resuming the parent, mark it stopped. */
- if (follow_child && !detach_fork && !non_stop && !sched_multi)
+ if (ecs->ws.kind () != TARGET_WAITKIND_THREAD_CLONED
+ && follow_child && !detach_fork && !non_stop && !sched_multi)
parent->set_running (false);
/* If resuming the child, mark it running. */
- if (follow_child || (!detach_fork && (non_stop || sched_multi)))
+ if (ecs->ws.kind () == TARGET_WAITKIND_THREAD_CLONED
+ || (follow_child || (!detach_fork && (non_stop || sched_multi))))
child->set_running (true);
/* In non-stop mode, also resume the other branch. */
- if (!detach_fork && (non_stop
- || (sched_multi && target_is_non_stop_p ())))
+ if ((ecs->ws.kind () == TARGET_WAITKIND_THREAD_CLONED
+ && target_is_non_stop_p ())
+ || (!detach_fork && (non_stop
+ || (sched_multi
+ && target_is_non_stop_p ()))))
{
if (follow_child)
switch_to_thread (parent);
diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index 5f67bcbcb4f..6db21e809c5 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -1286,64 +1286,80 @@ get_detach_signal (struct lwp_info *lp)
return 0;
}
-/* Detach from LP. If SIGNO_P is non-NULL, then it points to the
- signal number that should be passed to the LWP when detaching.
- Otherwise pass any pending signal the LWP may have, if any. */
+/* If LP has a pending fork/vfork/clone status, return it. */
-static void
-detach_one_lwp (struct lwp_info *lp, int *signo_p)
+static gdb::optional<target_waitstatus>
+get_pending_child_status (lwp_info *lp)
{
- int lwpid = lp->ptid.lwp ();
- int signo;
-
- gdb_assert (lp->status == 0 || WIFSTOPPED (lp->status));
-
- /* If the lwp/thread we are about to detach has a pending fork event,
- there is a process GDB is attached to that the core of GDB doesn't know
- about. Detach from it. */
-
/* Check in lwp_info::status. */
if (WIFSTOPPED (lp->status) && linux_is_extended_waitstatus (lp->status))
{
int event = linux_ptrace_get_extended_event (lp->status);
- if (event == PTRACE_EVENT_FORK || event == PTRACE_EVENT_VFORK)
+ if (event == PTRACE_EVENT_FORK
+ || event == PTRACE_EVENT_VFORK
+ || event == PTRACE_EVENT_CLONE)
{
unsigned long child_pid;
int ret = ptrace (PTRACE_GETEVENTMSG, lp->ptid.lwp (), 0, &child_pid);
if (ret == 0)
- detach_one_pid (child_pid, 0);
+ {
+ target_waitstatus ws;
+
+ if (event == PTRACE_EVENT_FORK)
+ ws.set_forked (ptid_t (child_pid, child_pid));
+ else if (event == PTRACE_EVENT_VFORK)
+ ws.set_vforked (ptid_t (child_pid, child_pid));
+ else if (event == PTRACE_EVENT_CLONE)
+ ws.set_thread_cloned (ptid_t (lp->ptid.pid (), child_pid));
+ else
+ gdb_assert_not_reached ("unhandled");
+
+ return ws;
+ }
else
- perror_warning_with_name (_("Failed to detach fork child"));
+ {
+ perror_warning_with_name (_("Failed to retrieve event msg"));
+ return {};
+ }
}
}
/* Check in lwp_info::waitstatus. */
- if (lp->waitstatus.kind () == TARGET_WAITKIND_VFORKED
- || lp->waitstatus.kind () == TARGET_WAITKIND_FORKED)
- detach_one_pid (lp->waitstatus.child_ptid ().pid (), 0);
-
+ if (is_new_child_status (lp->waitstatus.kind ()))
+ return lp->waitstatus;
- /* Check in thread_info::pending_waitstatus. */
thread_info *tp = find_thread_ptid (linux_target, lp->ptid);
- if (tp->has_pending_waitstatus ())
- {
- const target_waitstatus &ws = tp->pending_waitstatus ();
- if (ws.kind () == TARGET_WAITKIND_VFORKED
- || ws.kind () == TARGET_WAITKIND_FORKED)
- detach_one_pid (ws.child_ptid ().pid (), 0);
- }
+ /* Check in thread_info::pending_waitstatus. */
+ if (tp->has_pending_waitstatus ()
+ && is_new_child_status (tp->pending_waitstatus ().kind ()))
+ return tp->pending_waitstatus ();
/* Check in thread_info::pending_follow. */
- if (tp->pending_follow.kind () == TARGET_WAITKIND_VFORKED
- || tp->pending_follow.kind () == TARGET_WAITKIND_FORKED)
- detach_one_pid (tp->pending_follow.child_ptid ().pid (), 0);
+ if (is_new_child_status (tp->pending_follow.kind ()))
+ return tp->pending_follow;
- if (lp->status != 0)
- linux_nat_debug_printf ("Pending %s for %s on detach.",
- strsignal (WSTOPSIG (lp->status)),
- lp->ptid.to_string ().c_str ());
+ return {};
+}
+
+/* Detach from LP. If SIGNO_P is non-NULL, then it points to the
+ signal number that should be passed to the LWP when detaching.
+ Otherwise pass any pending signal the LWP may have, if any. */
+
+static void
+detach_one_lwp (struct lwp_info *lp, int *signo_p)
+{
+ int lwpid = lp->ptid.lwp ();
+ int signo;
+
+ /* If the lwp/thread we are about to detach has a pending fork/clone
+ event, there is a process/thread GDB is attached to that the core
+ of GDB doesn't know about. Detach from it. */
+
+ gdb::optional<target_waitstatus> ws = get_pending_child_status (lp);
+ if (ws.has_value ())
+ detach_one_pid (ws->child_ptid ().lwp (), 0);
/* If there is a pending SIGSTOP, get rid of it. */
if (lp->signalled)
@@ -1821,6 +1837,53 @@ linux_handle_syscall_trap (struct lwp_info *lp, int stopping)
return 1;
}
+void
+linux_nat_target::follow_clone (ptid_t child_ptid)
+{
+ lwp_info *new_lp = add_lwp (child_ptid);
+ new_lp->stopped = 1;
+
+ /* If the thread_db layer is active, let it record the user
+ level thread id and status, and add the thread to GDB's
+ list. */
+ if (!thread_db_notice_clone (inferior_ptid, new_lp->ptid))
+ {
+ /* The process is not using thread_db. Add the LWP to
+ GDB's list. */
+ add_thread (linux_target, new_lp->ptid);
+ }
+
+ /* We just created NEW_LP so it cannot yet contain STATUS. */
+ gdb_assert (new_lp->status == 0);
+
+ if (!pull_pid_from_list (&stopped_pids, child_ptid.lwp (), &new_lp->status))
+ internal_error (_("no saved status for clone lwp"));
+
+ if (WSTOPSIG (new_lp->status) != SIGSTOP)
+ {
+ /* This can happen if someone starts sending signals to
+ the new thread before it gets a chance to run, which
+ have a lower number than SIGSTOP (e.g. SIGUSR1).
+ This is an unlikely case, and harder to handle for
+ fork / vfork than for clone, so we do not try - but
+ we handle it for clone events here. */
+
+ new_lp->signalled = 1;
+
+ /* Save the wait status to report later. */
+ linux_nat_debug_printf
+ ("waitpid of new LWP %ld, saving status %s",
+ (long) new_lp->ptid.lwp (), status_to_str (new_lp->status).c_str ());
+ }
+ else
+ {
+ new_lp->status = 0;
+
+ if (report_thread_events)
+ new_lp->waitstatus.set_thread_created ();
+ }
+}
+
/* Handle a GNU/Linux extended wait response. If we see a clone
event, we need to add the new LWP to our list (and not report the
trap to higher layers). This function returns non-zero if the
@@ -1861,11 +1924,9 @@ linux_handle_extended_wait (struct lwp_info *lp, int status)
internal_error (_("wait returned unexpected status 0x%x"), status);
}
- ptid_t child_ptid (new_pid, new_pid);
-
if (event == PTRACE_EVENT_FORK || event == PTRACE_EVENT_VFORK)
{
- open_proc_mem_file (child_ptid);
+ open_proc_mem_file (ptid_t (new_pid, new_pid));
/* The arch-specific native code may need to know about new
forks even if those end up never mapped to an
@@ -1902,66 +1963,18 @@ linux_handle_extended_wait (struct lwp_info *lp, int status)
}
if (event == PTRACE_EVENT_FORK)
- ourstatus->set_forked (child_ptid);
+ ourstatus->set_forked (ptid_t (new_pid, new_pid));
else if (event == PTRACE_EVENT_VFORK)
- ourstatus->set_vforked (child_ptid);
+ ourstatus->set_vforked (ptid_t (new_pid, new_pid));
else if (event == PTRACE_EVENT_CLONE)
{
- struct lwp_info *new_lp;
-
- ourstatus->set_ignore ();
-
linux_nat_debug_printf
("Got clone event from LWP %d, new child is LWP %ld", pid, new_pid);
- new_lp = add_lwp (ptid_t (lp->ptid.pid (), new_pid));
- new_lp->stopped = 1;
- new_lp->resumed = 1;
+ /* Save the status again, we'll use it in follow_clone. */
+ add_to_pid_list (&stopped_pids, new_pid, status);
- /* If the thread_db layer is active, let it record the user
- level thread id and status, and add the thread to GDB's
- list. */
- if (!thread_db_notice_clone (lp->ptid, new_lp->ptid))
- {
- /* The process is not using thread_db. Add the LWP to
- GDB's list. */
- add_thread (linux_target, new_lp->ptid);
- }
-
- /* Even if we're stopping the thread for some reason
- internal to this module, from the perspective of infrun
- and the user/frontend, this new thread is running until
- it next reports a stop. */
- set_running (linux_target, new_lp->ptid, true);
- set_executing (linux_target, new_lp->ptid, true);
-
- if (WSTOPSIG (status) != SIGSTOP)
- {
- /* This can happen if someone starts sending signals to
- the new thread before it gets a chance to run, which
- have a lower number than SIGSTOP (e.g. SIGUSR1).
- This is an unlikely case, and harder to handle for
- fork / vfork than for clone, so we do not try - but
- we handle it for clone events here. */
-
- new_lp->signalled = 1;
-
- /* We created NEW_LP so it cannot yet contain STATUS. */
- gdb_assert (new_lp->status == 0);
-
- /* Save the wait status to report later. */
- linux_nat_debug_printf
- ("waitpid of new LWP %ld, saving status %s",
- (long) new_lp->ptid.lwp (), status_to_str (status).c_str ());
- new_lp->status = status;
- }
- else if (report_thread_events)
- {
- new_lp->waitstatus.set_thread_created ();
- new_lp->status = status;
- }
-
- return 1;
+ ourstatus->set_thread_cloned (ptid_t (lp->ptid.pid (), new_pid));
}
return 0;
@@ -3536,59 +3549,56 @@ kill_wait_callback (struct lwp_info *lp)
return 0;
}
-/* Kill the fork children of any threads of inferior INF that are
- stopped at a fork event. */
+/* Kill the fork/clone child of LP if it has an unfollowed child. */
-static void
-kill_unfollowed_fork_children (struct inferior *inf)
+static int
+kill_unfollowed_child_callback (lwp_info *lp)
{
- for (thread_info *thread : inf->non_exited_threads ())
+ gdb::optional<target_waitstatus> ws = get_pending_child_status (lp);
+ if (ws.has_value ())
{
- struct target_waitstatus *ws = &thread->pending_follow;
-
- if (ws->kind () == TARGET_WAITKIND_FORKED
- || ws->kind () == TARGET_WAITKIND_VFORKED)
- {
- ptid_t child_ptid = ws->child_ptid ();
- int child_pid = child_ptid.pid ();
- int child_lwp = child_ptid.lwp ();
+ ptid_t child_ptid = ws->child_ptid ();
+ int child_pid = child_ptid.pid ();
+ int child_lwp = child_ptid.lwp ();
- kill_one_lwp (child_lwp);
- kill_wait_one_lwp (child_lwp);
+ kill_one_lwp (child_lwp);
+ kill_wait_one_lwp (child_lwp);
- /* Let the arch-specific native code know this process is
- gone. */
- linux_target->low_forget_process (child_pid);
- }
+ /* Let the arch-specific native code know this process is
+ gone. */
+ if (ws->kind () != TARGET_WAITKIND_THREAD_CLONED)
+ linux_target->low_forget_process (child_pid);
}
+
+ return 0;
}
void
linux_nat_target::kill ()
{
- /* If we're stopped while forking and we haven't followed yet,
- kill the other task. We need to do this first because the
+ ptid_t pid_ptid (inferior_ptid.pid ());
+
+ /* If we're stopped while forking/cloning and we haven't followed
+ yet, kill the child task. We need to do this first because the
parent will be sleeping if this is a vfork. */
- kill_unfollowed_fork_children (current_inferior ());
+ iterate_over_lwps (pid_ptid, kill_unfollowed_child_callback);
if (forks_exist_p ())
linux_fork_killall ();
else
{
- ptid_t ptid = ptid_t (inferior_ptid.pid ());
-
/* Stop all threads before killing them, since ptrace requires
that the thread is stopped to successfully PTRACE_KILL. */
- iterate_over_lwps (ptid, stop_callback);
+ iterate_over_lwps (pid_ptid, stop_callback);
/* ... and wait until all of them have reported back that
they're no longer running. */
- iterate_over_lwps (ptid, stop_wait_callback);
+ iterate_over_lwps (pid_ptid, stop_wait_callback);
/* Kill all LWP's ... */
- iterate_over_lwps (ptid, kill_callback);
+ iterate_over_lwps (pid_ptid, kill_callback);
/* ... and wait until we've flushed all events. */
- iterate_over_lwps (ptid, kill_wait_callback);
+ iterate_over_lwps (pid_ptid, kill_wait_callback);
}
target_mourn_inferior (inferior_ptid);
diff --git a/gdb/linux-nat.h b/gdb/linux-nat.h
index 770fe924427..1cdbeafd4f3 100644
--- a/gdb/linux-nat.h
+++ b/gdb/linux-nat.h
@@ -129,6 +129,8 @@ class linux_nat_target : public inf_ptrace_target
void follow_fork (inferior *, ptid_t, target_waitkind, bool, bool) override;
+ void follow_clone (ptid_t) override;
+
std::vector<static_tracepoint_marker>
static_tracepoint_markers_by_strid (const char *id) override;
diff --git a/gdb/target-delegates.c b/gdb/target-delegates.c
index 57b66ce87b1..7a4ef05b4e1 100644
--- a/gdb/target-delegates.c
+++ b/gdb/target-delegates.c
@@ -76,6 +76,7 @@ struct dummy_target : public target_ops
int insert_vfork_catchpoint (int arg0) override;
int remove_vfork_catchpoint (int arg0) override;
void follow_fork (inferior *arg0, ptid_t arg1, target_waitkind arg2, bool arg3, bool arg4) override;
+ void follow_clone (ptid_t arg0) override;
int insert_exec_catchpoint (int arg0) override;
int remove_exec_catchpoint (int arg0) override;
void follow_exec (inferior *arg0, ptid_t arg1, const char *arg2) override;
@@ -250,6 +251,7 @@ struct debug_target : public target_ops
int insert_vfork_catchpoint (int arg0) override;
int remove_vfork_catchpoint (int arg0) override;
void follow_fork (inferior *arg0, ptid_t arg1, target_waitkind arg2, bool arg3, bool arg4) override;
+ void follow_clone (ptid_t arg0) override;
int insert_exec_catchpoint (int arg0) override;
int remove_exec_catchpoint (int arg0) override;
void follow_exec (inferior *arg0, ptid_t arg1, const char *arg2) override;
@@ -1545,6 +1547,28 @@ debug_target::follow_fork (inferior *arg0, ptid_t arg1, target_waitkind arg2, bo
gdb_puts (")\n", gdb_stdlog);
}
+void
+target_ops::follow_clone (ptid_t arg0)
+{
+ this->beneath ()->follow_clone (arg0);
+}
+
+void
+dummy_target::follow_clone (ptid_t arg0)
+{
+ default_follow_clone (this, arg0);
+}
+
+void
+debug_target::follow_clone (ptid_t arg0)
+{
+ gdb_printf (gdb_stdlog, "-> %s->follow_clone (...)\n", this->beneath ()->shortname ());
+ this->beneath ()->follow_clone (arg0);
+ gdb_printf (gdb_stdlog, "<- %s->follow_clone (", this->beneath ()->shortname ());
+ target_debug_print_ptid_t (arg0);
+ gdb_puts (")\n", gdb_stdlog);
+}
+
int
target_ops::insert_exec_catchpoint (int arg0)
{
diff --git a/gdb/target.c b/gdb/target.c
index 0cebecfafc3..9835222e5da 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -2701,6 +2701,13 @@ default_follow_fork (struct target_ops *self, inferior *child_inf,
internal_error (_("could not find a target to follow fork"));
}
+static void
+default_follow_clone (struct target_ops *self, ptid_t child_ptid)
+{
+ /* Some target returned a clone event, but did not know how to follow it. */
+ internal_error (_("could not find a target to follow clone"));
+}
+
/* See target.h. */
void
diff --git a/gdb/target.h b/gdb/target.h
index 2dac86c394d..43ab71093d9 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -637,6 +637,8 @@ struct target_ops
TARGET_DEFAULT_RETURN (1);
virtual void follow_fork (inferior *, ptid_t, target_waitkind, bool, bool)
TARGET_DEFAULT_FUNC (default_follow_fork);
+ virtual void follow_clone (ptid_t)
+ TARGET_DEFAULT_FUNC (default_follow_clone);
virtual int insert_exec_catchpoint (int)
TARGET_DEFAULT_RETURN (1);
virtual int remove_exec_catchpoint (int)
diff --git a/gdb/target/waitstatus.c b/gdb/target/waitstatus.c
index 2b8404fb75b..a8edbb17d60 100644
--- a/gdb/target/waitstatus.c
+++ b/gdb/target/waitstatus.c
@@ -45,6 +45,7 @@ DIAGNOSTIC_ERROR_SWITCH
case TARGET_WAITKIND_FORKED:
case TARGET_WAITKIND_VFORKED:
+ case TARGET_WAITKIND_THREAD_CLONED:
return string_appendf (str, ", child_ptid = %s",
this->child_ptid ().to_string ().c_str ());
diff --git a/gdb/target/waitstatus.h b/gdb/target/waitstatus.h
index 0a88b869044..4c3de005315 100644
--- a/gdb/target/waitstatus.h
+++ b/gdb/target/waitstatus.h
@@ -95,6 +95,13 @@ enum target_waitkind
/* There are no resumed children left in the program. */
TARGET_WAITKIND_NO_RESUMED,
+ /* The thread was cloned. The event's ptid corresponds to the
+ cloned parent. The cloned child is held stopped at its entry
+ point, and its ptid is in the event's m_child_ptid. The target
+ must not add the cloned child to GDB's thread list until
+ target_ops::follow_clone() is called. */
+ TARGET_WAITKIND_THREAD_CLONED,
+
/* The thread was created. */
TARGET_WAITKIND_THREAD_CREATED,
@@ -102,6 +109,17 @@ enum target_waitkind
TARGET_WAITKIND_THREAD_EXITED,
};
+/* Determine if KIND represents an event with a new child - a fork,
+ vfork, or clone. */
+
+static inline bool
+is_new_child_status (target_waitkind kind)
+{
+ return (kind == TARGET_WAITKIND_FORKED
+ || kind == TARGET_WAITKIND_VFORKED
+ || kind == TARGET_WAITKIND_THREAD_CLONED);
+}
+
/* Return KIND as a string. */
static inline const char *
@@ -125,6 +143,8 @@ DIAGNOSTIC_ERROR_SWITCH
return "FORKED";
case TARGET_WAITKIND_VFORKED:
return "VFORKED";
+ case TARGET_WAITKIND_THREAD_CLONED:
+ return "THREAD_CLONED";
case TARGET_WAITKIND_EXECD:
return "EXECD";
case TARGET_WAITKIND_VFORK_DONE:
@@ -325,6 +345,14 @@ struct target_waitstatus
return *this;
}
+ target_waitstatus &set_thread_cloned (ptid_t child_ptid)
+ {
+ this->reset ();
+ m_kind = TARGET_WAITKIND_THREAD_CLONED;
+ m_value.child_ptid = child_ptid;
+ return *this;
+ }
+
target_waitstatus &set_thread_created ()
{
this->reset ();
@@ -369,8 +397,7 @@ struct target_waitstatus
ptid_t child_ptid () const
{
- gdb_assert (m_kind == TARGET_WAITKIND_FORKED
- || m_kind == TARGET_WAITKIND_VFORKED);
+ gdb_assert (is_new_child_status (m_kind));
return m_value.child_ptid;
}
base-commit: 2562954ede66f32bff7d985e752b8052c2ae5775
prerequisite-patch-id: bbc9918ac5f79de07a29f34ec072794d270f942d
prerequisite-patch-id: c0bc5b4f99193bb50cb31f551673de1808dcda35
prerequisite-patch-id: ab0838f2bc02931d7e830abe23833e7a8224442c
Pedro Alves <pedro@palves.net> writes:
> On 2023-02-04 3:38 p.m., Andrew Burgess wrote:
>> Pedro Alves <pedro@palves.net> writes:
>
>>> +/* Detach from LP. If SIGNO_P is non-NULL, then it points to the
>>> + signal number that should be passed to the LWP when detaching.
>>> + Otherwise pass any pending signal the LWP may have, if any. */
>>> +
>>> +static void
>>> +detach_one_lwp (struct lwp_info *lp, int *signo_p)
>>> +{
>>> + int lwpid = lp->ptid.lwp ();
>>> + int signo;
>>> +
>>> + /* If the lwp/thread we are about to detach has a pending fork/clone
>>> + event, there is a process/thread GDB is attached to that the core
>>> + of GDB doesn't know about. Detach from it. */
>>> +
>>> + if (gdb::optional<target_waitstatus> ws = get_pending_child_status (lp))
>>
>> I know this is just a style issue, but I'm really not a fan of this
>> assignment in the if condition. I know it probably seems silly, but I'd
>> find it much clearer if we did:
>>
>> gdb::optional<target_waitstatus> ws = get_pending_child_status (lp)
>> if (ws.has_value ())
>> ...
>>
>> The first time I read the initial line, my first thought was s/=/==/,
>> then I had to read it again and figure out what was going on...
>>
>
> Ah, I think I was asked to switch to this style in a previous review, so
> clearly it's a matter of taste. :-D
>
> I was using this originally:
>
> target_waitstatus ws;
> if (get_pending_child_status (lp, &ws))
> {
>
>
> I used to think that "if (auto opt = foo ())" might be confusing too, to be honest,
> but I've convinced myself that it is OK (but please see further below) for a couple
> reasons:
>
> #1 - these aren't really equivalent:
>
> a) if (type var = func ())
>
> b) itype var = func ();
> if (var)
>
> because in a), the scope of the variable is just the if then/else
> scopes. So you can for example reuse the name like:
>
> if (type var = func ())
> {
> }
>
> if (type var = bar ())
> {
> }
>
> and also you're guaranteed that the variable's dtor is run at the end of the
> if block. So it'd be more equivalent to writing an explicit scope, like:
>
> {
> itype var = func ();
> if (var)
> {
> ...
> }
> }
>
> #2 - the presence of the type makes it visually distinctive from the problematic case
> of writing to a variable by accident:
>
> if (var = func ()) // bad
>
> if (var == func ()) // ok
>
> if (auto var = func ()) // ok
>
> if (auto var == func ()) // not valid
>
>
> and, it's not different from initializing a variable in a for, like:
>
> for (auto var = func (); ...; ...)
>
> and we don't bat an eye when we see that.
>
> in fact, C++17 let's you write an if with a for-like init-statement, like so:
>
> if (auto var = func (); var)
>
> so I think this is a case of being surprised at first, but then we just
> get used to it quickly.
>
>
> However, I've still converted to use the style you suggested, because
> there might be a different reason for wanting to avoid that style, which is that
> this:
>
> if (gdb::optional<target_waitstatus> ws = get_pending_child_status (lp))
>
> begs the question of whether we want to allow:
>
> if (foo *ptr = get_foo ())
>
> and this runs counter to our style of doing explicit nullptr checks... So
> we can leave that to a seperate discussion and I don't want to be blocked
> by it. :-)
>
>
>>> diff --git a/gdb/target.h b/gdb/target.h
>>> index 28aa9273893..aab390aec57 100644
>>> --- a/gdb/target.h
>>> +++ b/gdb/target.h
>>> @@ -637,6 +637,8 @@ struct target_ops
>>> TARGET_DEFAULT_RETURN (1);
>>> virtual void follow_fork (inferior *, ptid_t, target_waitkind, bool, bool)
>>> TARGET_DEFAULT_FUNC (default_follow_fork);
>>> + virtual void follow_clone (ptid_t)
>>> + TARGET_DEFAULT_FUNC (default_follow_clone);
>>
>> One thing that sucks about some of the older parts of the target API is
>> just how undocumented it is. The only way to figure out what things
>> like follow_fork do is to look at the code.
>
> I don't disagree completely, but note that the comments for the target_ops methods
> tend to be in the target_foo wrapper method. For example, target_follow_fork.
> In this case, there's no such wrapper method, so I have no good excuse. :-)
>
> I've added a comment, like:
>
> --- c/gdb/target.h
> +++ w/gdb/target.h
> @@ -637,8 +637,13 @@ struct target_ops
> TARGET_DEFAULT_RETURN (1);
> virtual void follow_fork (inferior *, ptid_t, target_waitkind, bool, bool)
> TARGET_DEFAULT_FUNC (default_follow_fork);
> - virtual void follow_clone (ptid_t)
> +
> + /* Add CHILD_PTID to the thread list, after handling a
> + TARGET_WAITKIND_THREAD_CLONE event for the clone parent. The
> + parent is inferior_ptid. */
> + virtual void follow_clone (ptid_t child_ptid)
> TARGET_DEFAULT_FUNC (default_follow_clone);
> +
> virtual int insert_exec_catchpoint (int)
> TARGET_DEFAULT_RETURN (1);
> virtual int remove_exec_catchpoint (int)
>
>>
>> Some of the newer methods do have good comments. Or at least comments
>> that give a good hint to what the method does.
>>
>> I'd be really grateful if new target API methods could be given a good
>> comment in target.h.
>
> Here's the full patch. Let me know what you think.
>
> From 3b2638cf7507146385e16473c883b7d0d4a75277 Mon Sep 17 00:00:00 2001
> From: Pedro Alves <pedro@palves.net>
> Date: Fri, 12 Nov 2021 20:50:29 +0000
> Subject: [PATCH] Step over clone syscall w/ breakpoint,
> TARGET_WAITKIND_THREAD_CLONED
>
> (A good chunk of the problem statement in the commit log below is
> Andrew's, adjusted for a different solution, and for covering
> displaced stepping too.)
>
> This commit addresses bugs gdb/19675 and gdb/27830, which are about
> stepping over a breakpoint set at a clone syscall instruction, one is
> about displaced stepping, and the other about in-line stepping.
>
> Currently, when a new thread is created through a clone syscall, GDB
> sets the new thread running. With 'continue' this makes sense
> (assuming no schedlock):
>
> - all-stop mode, user issues 'continue', all threads are set running,
> a newly created thread should also be set running.
>
> - non-stop mode, user issues 'continue', other pre-existing threads
> are not affected, but as the new thread is (sort-of) a child of the
> thread the user asked to run, it makes sense that the new threads
> should be created in the running state.
>
> Similarly, if we are stopped at the clone syscall, and there's no
> software breakpoint at this address, then the current behaviour is
> fine:
>
> - all-stop mode, user issues 'stepi', stepping will be done in place
> (as there's no breakpoint to step over). While stepping the thread
> of interest all the other threads will be allowed to continue. A
> newly created thread will be set running, and then stopped once the
> thread of interest has completed its step.
>
> - non-stop mode, user issues 'stepi', stepping will be done in place
> (as there's no breakpoint to step over). Other threads might be
> running or stopped, but as with the continue case above, the new
> thread will be created running. The only possible issue here is
> that the new thread will be left running after the initial thread
> has completed its stepi. The user would need to manually select
> the thread and interrupt it, this might not be what the user
> expects. However, this is not something this commit tries to
> change.
>
> The problem then is what happens when we try to step over a clone
> syscall if there is a breakpoint at the syscall address.
>
> - For both all-stop and non-stop modes, with in-line stepping:
>
> + user issues 'stepi',
> + [non-stop mode only] GDB stops all threads. In all-stop mode all
> threads are already stopped.
> + GDB removes s/w breakpoint at syscall address,
> + GDB single steps just the thread of interest, all other threads
> are left stopped,
> + New thread is created running,
> + Initial thread completes its step,
> + [non-stop mode only] GDB resumes all threads that it previously
> stopped.
>
> There are two problems in the in-line stepping scenario above:
>
> 1. The new thread might pass through the same code that the initial
> thread is in (i.e. the clone syscall code), in which case it will
> fail to hit the breakpoint in clone as this was removed so the
> first thread can single step,
>
> 2. The new thread might trigger some other stop event before the
> initial thread reports its step completion. If this happens we
> end up triggering an assertion as GDB assumes that only the
> thread being stepped should stop. The assert looks like this:
>
> infrun.c:5899: internal-error: int finish_step_over(execution_control_state*): Assertion `ecs->event_thread->control.trap_expected' failed.
>
> - For both all-stop and non-stop modes, with displaced stepping:
>
> + user issues 'stepi',
> + GDB starts the displaced step, moves thread's PC to the
> out-of-line scratch pad, maybe adjusts registers,
> + GDB single steps the thread of interest, [non-stop mode only] all
> other threads are left as they were, either running or stopped.
> In all-stop, all other threads are left stopped.
> + New thread is created running,
> + Initial thread completes its step, GDB re-adjusts its PC,
> restores/releases scratchpad,
> + [non-stop mode only] GDB resumes the thread, now past its
> breakpoint.
> + [all-stop mode only] GDB resumes all threads.
>
> There is one problem with the displaced stepping scenario above:
>
> 3. When the parent thread completed its step, GDB adjusted its PC,
> but did not adjust the child's PC, thus that new child thread
> will continue execution in the scratch pad, invoking undefined
> behavior. If you're lucky, you see a crash. If unlucky, the
> inferior gets silently corrupted.
>
> What is needed is for GDB to have more control over whether the new
> thread is created running or not. Issue #1 above requires that the
> new thread not be allowed to run until the breakpoint has been
> reinserted. The only way to guarantee this is if the new thread is
> held in a stopped state until the single step has completed. Issue #3
> above requires that GDB is informed of when a thread clones itself,
> and of what is the child's ptid, so that GDB can fixup both the parent
> and the child.
>
> When looking for solutions to this problem I considered how GDB
> handles fork/vfork as these have some of the same issues. The main
> difference between fork/vfork and clone is that the clone events are
> not reported back to core GDB. Instead, the clone event is handled
> automatically in the target code and the child thread is immediately
> set running.
>
> Note we have support for requesting thread creation events out of the
> target (TARGET_WAITKIND_THREAD_CREATED). However, those are reported
> for the new/child thread. That would be sufficient to address in-line
> stepping (issue #1), but not for displaced-stepping (issue #3). To
> handle displaced-stepping, we need an event that is reported to the
> _parent_ of the clone, as the information about the displaced step is
> associated with the clone parent. TARGET_WAITKIND_THREAD_CREATED
> includes no indication of which thread is the parent that spawned the
> new child. In fact, for some targets, like e.g., Windows, it would be
> impossible to know which thread that was, as thread creation there
> doesn't work by "cloning".
>
> The solution implemented here is to model clone on fork/vfork, and
> introduce a new TARGET_WAITKIND_THREAD_CLONED event. This event is
> similar to TARGET_WAITKIND_FORKED and TARGET_WAITKIND_VFORKED, except
> that we end up with a new thread in the same process, instead of a new
> thread of a new process. Like FORKED and VFORKED, THREAD_CLONED
> waitstatuses have a child_ptid property, and the child is held stopped
> until GDB explicitly resumes it. This addresses the in-line stepping
> case (issues #1 and #2).
>
> The infrun code that handles displaced stepping fixup for the child
> after a fork/vfork event is thus reused for THREAD_CLONE, with some
> minimal conditions added, addressing the displaced stepping case
> (issue #3).
>
> The native Linux backend is adjusted to unconditionally report
> TARGET_WAITKIND_THREAD_CLONED events to the core.
>
> Following the follow_fork model in core GDB, we introduce a
> target_follow_clone target method, which is responsible for making the
> new clone child visible to the rest of GDB.
>
> Subsequent patches will add clone events support to the remote
> protocol and gdbserver.
>
> A testcase will be added by a later patch.
What's the motivation for splitting the implementation from the
associated tests?
Though your implementation may be different (better) than mine, the
original test I wrote[1] still passes just fine with this commit.
When digging back through old patches it's much nicer to easier to find
the GDB changes and the tests in a single commit IMHO. Do feel free to
take the tests from that commit, if that helps at all (though I notice I
left some debug logging in which would need cleaning up, and the test
fails when using a remote target, but that's easily fixed by skipping
the test in that case).
[1] https://sourceware.org/pipermail/gdb-patches/2021-June/180520.html
I've looked through the code and it all looks good to me.
Reviewed-By: Andrew Burgess <aburgess@redhat.com>
Thanks,
Andrew
>
> displaced_step_in_progress_thread becomes unused with this patch, but
> a new use will reappear later in the series. To avoid deleting it and
> readding it back, this patch marks it with attribute unused, and the
> latter patch removes the attribute again. We need to do this because
> the function is static, and with no callers, the compiler would warn,
> (error with -Werror), breaking the build.
>
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=19675
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=27830
>
> Change-Id: I474e9a7015dd3d33469e322a5764ae83f8a32787
> ---
> gdb/infrun.c | 158 ++++++++++++++-----------
> gdb/linux-nat.c | 250 +++++++++++++++++++++-------------------
> gdb/linux-nat.h | 2 +
> gdb/target-delegates.c | 24 ++++
> gdb/target.c | 7 ++
> gdb/target.h | 2 +
> gdb/target/waitstatus.c | 1 +
> gdb/target/waitstatus.h | 31 ++++-
> 8 files changed, 283 insertions(+), 192 deletions(-)
>
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index 99f2a8e039d..d1e6233591c 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -1584,6 +1584,7 @@ step_over_info_valid_p (void)
> /* Return true if THREAD is doing a displaced step. */
>
> static bool
> +ATTRIBUTE_UNUSED
> displaced_step_in_progress_thread (thread_info *thread)
> {
> gdb_assert (thread != nullptr);
> @@ -1898,6 +1899,31 @@ static displaced_step_finish_status
> displaced_step_finish (thread_info *event_thread,
> const target_waitstatus &event_status)
> {
> + /* Check whether the parent is displaced stepping. */
> + struct regcache *regcache = get_thread_regcache (event_thread);
> + struct gdbarch *gdbarch = regcache->arch ();
> + inferior *parent_inf = event_thread->inf;
> +
> + /* If this was a fork/vfork/clone, this event indicates that the
> + displaced stepping of the syscall instruction has been done, so
> + we perform cleanup for parent here. Also note that this
> + operation also cleans up the child for vfork, because their pages
> + are shared. */
> +
> + /* If this is a fork (child gets its own address space copy) and
> + some displaced step buffers were in use at the time of the fork,
> + restore the displaced step buffer bytes in the child process.
> +
> + Architectures which support displaced stepping and fork events
> + must supply an implementation of
> + gdbarch_displaced_step_restore_all_in_ptid. This is not enforced
> + during gdbarch validation to support architectures which support
> + displaced stepping but not forks. */
> + if (event_status.kind () == TARGET_WAITKIND_FORKED
> + && gdbarch_supports_displaced_stepping (gdbarch))
> + gdbarch_displaced_step_restore_all_in_ptid
> + (gdbarch, parent_inf, event_status.child_ptid ());
> +
> displaced_step_thread_state *displaced = &event_thread->displaced_step_state;
>
> /* Was this thread performing a displaced step? */
> @@ -1917,8 +1943,39 @@ displaced_step_finish (thread_info *event_thread,
>
> /* Do the fixup, and release the resources acquired to do the displaced
> step. */
> - return gdbarch_displaced_step_finish (displaced->get_original_gdbarch (),
> - event_thread, event_status);
> + displaced_step_finish_status status
> + = gdbarch_displaced_step_finish (displaced->get_original_gdbarch (),
> + event_thread, event_status);
> +
> + if (event_status.kind () == TARGET_WAITKIND_FORKED
> + || event_status.kind () == TARGET_WAITKIND_VFORKED
> + || event_status.kind () == TARGET_WAITKIND_THREAD_CLONED)
> + {
> + /* Since the vfork/fork/clone syscall instruction was executed
> + in the scratchpad, the child's PC is also within the
> + scratchpad. Set the child's PC to the parent's PC value,
> + which has already been fixed up. Note: we use the parent's
> + aspace here, although we're touching the child, because the
> + child hasn't been added to the inferior list yet at this
> + point. */
> +
> + struct regcache *child_regcache
> + = get_thread_arch_aspace_regcache (parent_inf->process_target (),
> + event_status.child_ptid (),
> + gdbarch,
> + parent_inf->aspace);
> + /* Read PC value of parent. */
> + CORE_ADDR parent_pc = regcache_read_pc (regcache);
> +
> + displaced_debug_printf ("write child pc from %s to %s",
> + paddress (gdbarch,
> + regcache_read_pc (child_regcache)),
> + paddress (gdbarch, parent_pc));
> +
> + regcache_write_pc (child_regcache, parent_pc);
> + }
> +
> + return status;
> }
>
> /* Data to be passed around while handling an event. This data is
> @@ -5717,67 +5774,13 @@ handle_inferior_event (struct execution_control_state *ecs)
>
> case TARGET_WAITKIND_FORKED:
> case TARGET_WAITKIND_VFORKED:
> - /* Check whether the inferior is displaced stepping. */
> - {
> - struct regcache *regcache = get_thread_regcache (ecs->event_thread);
> - struct gdbarch *gdbarch = regcache->arch ();
> - inferior *parent_inf = find_inferior_ptid (ecs->target, ecs->ptid);
> -
> - /* If this is a fork (child gets its own address space copy)
> - and some displaced step buffers were in use at the time of
> - the fork, restore the displaced step buffer bytes in the
> - child process.
> -
> - Architectures which support displaced stepping and fork
> - events must supply an implementation of
> - gdbarch_displaced_step_restore_all_in_ptid. This is not
> - enforced during gdbarch validation to support architectures
> - which support displaced stepping but not forks. */
> - if (ecs->ws.kind () == TARGET_WAITKIND_FORKED
> - && gdbarch_supports_displaced_stepping (gdbarch))
> - gdbarch_displaced_step_restore_all_in_ptid
> - (gdbarch, parent_inf, ecs->ws.child_ptid ());
> -
> - /* If displaced stepping is supported, and thread ecs->ptid is
> - displaced stepping. */
> - if (displaced_step_in_progress_thread (ecs->event_thread))
> - {
> - struct regcache *child_regcache;
> - CORE_ADDR parent_pc;
> -
> - /* GDB has got TARGET_WAITKIND_FORKED or TARGET_WAITKIND_VFORKED,
> - indicating that the displaced stepping of syscall instruction
> - has been done. Perform cleanup for parent process here. Note
> - that this operation also cleans up the child process for vfork,
> - because their pages are shared. */
> - displaced_step_finish (ecs->event_thread, ecs->ws);
> - /* Start a new step-over in another thread if there's one
> - that needs it. */
> - start_step_over ();
> -
> - /* Since the vfork/fork syscall instruction was executed in the scratchpad,
> - the child's PC is also within the scratchpad. Set the child's PC
> - to the parent's PC value, which has already been fixed up.
> - FIXME: we use the parent's aspace here, although we're touching
> - the child, because the child hasn't been added to the inferior
> - list yet at this point. */
> -
> - child_regcache
> - = get_thread_arch_aspace_regcache (parent_inf->process_target (),
> - ecs->ws.child_ptid (),
> - gdbarch,
> - parent_inf->aspace);
> - /* Read PC value of parent process. */
> - parent_pc = regcache_read_pc (regcache);
> -
> - displaced_debug_printf ("write child pc from %s to %s",
> - paddress (gdbarch,
> - regcache_read_pc (child_regcache)),
> - paddress (gdbarch, parent_pc));
> -
> - regcache_write_pc (child_regcache, parent_pc);
> - }
> - }
> + case TARGET_WAITKIND_THREAD_CLONED:
> +
> + displaced_step_finish (ecs->event_thread, ecs->ws);
> +
> + /* Start a new step-over in another thread if there's one that
> + needs it. */
> + start_step_over ();
>
> context_switch (ecs);
>
> @@ -5793,7 +5796,7 @@ handle_inferior_event (struct execution_control_state *ecs)
> need to unpatch at follow/detach time instead to be certain
> that new breakpoints added between catchpoint hit time and
> vfork follow are detached. */
> - if (ecs->ws.kind () != TARGET_WAITKIND_VFORKED)
> + if (ecs->ws.kind () == TARGET_WAITKIND_FORKED)
> {
> /* This won't actually modify the breakpoint list, but will
> physically remove the breakpoints from the child. */
> @@ -5825,14 +5828,24 @@ handle_inferior_event (struct execution_control_state *ecs)
> if (!bpstat_causes_stop (ecs->event_thread->control.stop_bpstat))
> {
> bool follow_child
> - = (follow_fork_mode_string == follow_fork_mode_child);
> + = (ecs->ws.kind () != TARGET_WAITKIND_THREAD_CLONED
> + && follow_fork_mode_string == follow_fork_mode_child);
>
> ecs->event_thread->set_stop_signal (GDB_SIGNAL_0);
>
> process_stratum_target *targ
> = ecs->event_thread->inf->process_target ();
>
> - bool should_resume = follow_fork ();
> + bool should_resume;
> + if (ecs->ws.kind () != TARGET_WAITKIND_THREAD_CLONED)
> + should_resume = follow_fork ();
> + else
> + {
> + should_resume = true;
> + inferior *inf = ecs->event_thread->inf;
> + inf->top_target ()->follow_clone (ecs->ws.child_ptid ());
> + ecs->event_thread->pending_follow.set_spurious ();
> + }
>
> /* Note that one of these may be an invalid pointer,
> depending on detach_fork. */
> @@ -5843,16 +5856,21 @@ handle_inferior_event (struct execution_control_state *ecs)
> child is marked stopped. */
>
> /* If not resuming the parent, mark it stopped. */
> - if (follow_child && !detach_fork && !non_stop && !sched_multi)
> + if (ecs->ws.kind () != TARGET_WAITKIND_THREAD_CLONED
> + && follow_child && !detach_fork && !non_stop && !sched_multi)
> parent->set_running (false);
>
> /* If resuming the child, mark it running. */
> - if (follow_child || (!detach_fork && (non_stop || sched_multi)))
> + if (ecs->ws.kind () == TARGET_WAITKIND_THREAD_CLONED
> + || (follow_child || (!detach_fork && (non_stop || sched_multi))))
> child->set_running (true);
>
> /* In non-stop mode, also resume the other branch. */
> - if (!detach_fork && (non_stop
> - || (sched_multi && target_is_non_stop_p ())))
> + if ((ecs->ws.kind () == TARGET_WAITKIND_THREAD_CLONED
> + && target_is_non_stop_p ())
> + || (!detach_fork && (non_stop
> + || (sched_multi
> + && target_is_non_stop_p ()))))
> {
> if (follow_child)
> switch_to_thread (parent);
> diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
> index 5f67bcbcb4f..6db21e809c5 100644
> --- a/gdb/linux-nat.c
> +++ b/gdb/linux-nat.c
> @@ -1286,64 +1286,80 @@ get_detach_signal (struct lwp_info *lp)
> return 0;
> }
>
> -/* Detach from LP. If SIGNO_P is non-NULL, then it points to the
> - signal number that should be passed to the LWP when detaching.
> - Otherwise pass any pending signal the LWP may have, if any. */
> +/* If LP has a pending fork/vfork/clone status, return it. */
>
> -static void
> -detach_one_lwp (struct lwp_info *lp, int *signo_p)
> +static gdb::optional<target_waitstatus>
> +get_pending_child_status (lwp_info *lp)
> {
> - int lwpid = lp->ptid.lwp ();
> - int signo;
> -
> - gdb_assert (lp->status == 0 || WIFSTOPPED (lp->status));
> -
> - /* If the lwp/thread we are about to detach has a pending fork event,
> - there is a process GDB is attached to that the core of GDB doesn't know
> - about. Detach from it. */
> -
> /* Check in lwp_info::status. */
> if (WIFSTOPPED (lp->status) && linux_is_extended_waitstatus (lp->status))
> {
> int event = linux_ptrace_get_extended_event (lp->status);
>
> - if (event == PTRACE_EVENT_FORK || event == PTRACE_EVENT_VFORK)
> + if (event == PTRACE_EVENT_FORK
> + || event == PTRACE_EVENT_VFORK
> + || event == PTRACE_EVENT_CLONE)
> {
> unsigned long child_pid;
> int ret = ptrace (PTRACE_GETEVENTMSG, lp->ptid.lwp (), 0, &child_pid);
> if (ret == 0)
> - detach_one_pid (child_pid, 0);
> + {
> + target_waitstatus ws;
> +
> + if (event == PTRACE_EVENT_FORK)
> + ws.set_forked (ptid_t (child_pid, child_pid));
> + else if (event == PTRACE_EVENT_VFORK)
> + ws.set_vforked (ptid_t (child_pid, child_pid));
> + else if (event == PTRACE_EVENT_CLONE)
> + ws.set_thread_cloned (ptid_t (lp->ptid.pid (), child_pid));
> + else
> + gdb_assert_not_reached ("unhandled");
> +
> + return ws;
> + }
> else
> - perror_warning_with_name (_("Failed to detach fork child"));
> + {
> + perror_warning_with_name (_("Failed to retrieve event msg"));
> + return {};
> + }
> }
> }
>
> /* Check in lwp_info::waitstatus. */
> - if (lp->waitstatus.kind () == TARGET_WAITKIND_VFORKED
> - || lp->waitstatus.kind () == TARGET_WAITKIND_FORKED)
> - detach_one_pid (lp->waitstatus.child_ptid ().pid (), 0);
> -
> + if (is_new_child_status (lp->waitstatus.kind ()))
> + return lp->waitstatus;
>
> - /* Check in thread_info::pending_waitstatus. */
> thread_info *tp = find_thread_ptid (linux_target, lp->ptid);
> - if (tp->has_pending_waitstatus ())
> - {
> - const target_waitstatus &ws = tp->pending_waitstatus ();
>
> - if (ws.kind () == TARGET_WAITKIND_VFORKED
> - || ws.kind () == TARGET_WAITKIND_FORKED)
> - detach_one_pid (ws.child_ptid ().pid (), 0);
> - }
> + /* Check in thread_info::pending_waitstatus. */
> + if (tp->has_pending_waitstatus ()
> + && is_new_child_status (tp->pending_waitstatus ().kind ()))
> + return tp->pending_waitstatus ();
>
> /* Check in thread_info::pending_follow. */
> - if (tp->pending_follow.kind () == TARGET_WAITKIND_VFORKED
> - || tp->pending_follow.kind () == TARGET_WAITKIND_FORKED)
> - detach_one_pid (tp->pending_follow.child_ptid ().pid (), 0);
> + if (is_new_child_status (tp->pending_follow.kind ()))
> + return tp->pending_follow;
>
> - if (lp->status != 0)
> - linux_nat_debug_printf ("Pending %s for %s on detach.",
> - strsignal (WSTOPSIG (lp->status)),
> - lp->ptid.to_string ().c_str ());
> + return {};
> +}
> +
> +/* Detach from LP. If SIGNO_P is non-NULL, then it points to the
> + signal number that should be passed to the LWP when detaching.
> + Otherwise pass any pending signal the LWP may have, if any. */
> +
> +static void
> +detach_one_lwp (struct lwp_info *lp, int *signo_p)
> +{
> + int lwpid = lp->ptid.lwp ();
> + int signo;
> +
> + /* If the lwp/thread we are about to detach has a pending fork/clone
> + event, there is a process/thread GDB is attached to that the core
> + of GDB doesn't know about. Detach from it. */
> +
> + gdb::optional<target_waitstatus> ws = get_pending_child_status (lp);
> + if (ws.has_value ())
> + detach_one_pid (ws->child_ptid ().lwp (), 0);
>
> /* If there is a pending SIGSTOP, get rid of it. */
> if (lp->signalled)
> @@ -1821,6 +1837,53 @@ linux_handle_syscall_trap (struct lwp_info *lp, int stopping)
> return 1;
> }
>
> +void
> +linux_nat_target::follow_clone (ptid_t child_ptid)
> +{
> + lwp_info *new_lp = add_lwp (child_ptid);
> + new_lp->stopped = 1;
> +
> + /* If the thread_db layer is active, let it record the user
> + level thread id and status, and add the thread to GDB's
> + list. */
> + if (!thread_db_notice_clone (inferior_ptid, new_lp->ptid))
> + {
> + /* The process is not using thread_db. Add the LWP to
> + GDB's list. */
> + add_thread (linux_target, new_lp->ptid);
> + }
> +
> + /* We just created NEW_LP so it cannot yet contain STATUS. */
> + gdb_assert (new_lp->status == 0);
> +
> + if (!pull_pid_from_list (&stopped_pids, child_ptid.lwp (), &new_lp->status))
> + internal_error (_("no saved status for clone lwp"));
> +
> + if (WSTOPSIG (new_lp->status) != SIGSTOP)
> + {
> + /* This can happen if someone starts sending signals to
> + the new thread before it gets a chance to run, which
> + have a lower number than SIGSTOP (e.g. SIGUSR1).
> + This is an unlikely case, and harder to handle for
> + fork / vfork than for clone, so we do not try - but
> + we handle it for clone events here. */
> +
> + new_lp->signalled = 1;
> +
> + /* Save the wait status to report later. */
> + linux_nat_debug_printf
> + ("waitpid of new LWP %ld, saving status %s",
> + (long) new_lp->ptid.lwp (), status_to_str (new_lp->status).c_str ());
> + }
> + else
> + {
> + new_lp->status = 0;
> +
> + if (report_thread_events)
> + new_lp->waitstatus.set_thread_created ();
> + }
> +}
> +
> /* Handle a GNU/Linux extended wait response. If we see a clone
> event, we need to add the new LWP to our list (and not report the
> trap to higher layers). This function returns non-zero if the
> @@ -1861,11 +1924,9 @@ linux_handle_extended_wait (struct lwp_info *lp, int status)
> internal_error (_("wait returned unexpected status 0x%x"), status);
> }
>
> - ptid_t child_ptid (new_pid, new_pid);
> -
> if (event == PTRACE_EVENT_FORK || event == PTRACE_EVENT_VFORK)
> {
> - open_proc_mem_file (child_ptid);
> + open_proc_mem_file (ptid_t (new_pid, new_pid));
>
> /* The arch-specific native code may need to know about new
> forks even if those end up never mapped to an
> @@ -1902,66 +1963,18 @@ linux_handle_extended_wait (struct lwp_info *lp, int status)
> }
>
> if (event == PTRACE_EVENT_FORK)
> - ourstatus->set_forked (child_ptid);
> + ourstatus->set_forked (ptid_t (new_pid, new_pid));
> else if (event == PTRACE_EVENT_VFORK)
> - ourstatus->set_vforked (child_ptid);
> + ourstatus->set_vforked (ptid_t (new_pid, new_pid));
> else if (event == PTRACE_EVENT_CLONE)
> {
> - struct lwp_info *new_lp;
> -
> - ourstatus->set_ignore ();
> -
> linux_nat_debug_printf
> ("Got clone event from LWP %d, new child is LWP %ld", pid, new_pid);
>
> - new_lp = add_lwp (ptid_t (lp->ptid.pid (), new_pid));
> - new_lp->stopped = 1;
> - new_lp->resumed = 1;
> + /* Save the status again, we'll use it in follow_clone. */
> + add_to_pid_list (&stopped_pids, new_pid, status);
>
> - /* If the thread_db layer is active, let it record the user
> - level thread id and status, and add the thread to GDB's
> - list. */
> - if (!thread_db_notice_clone (lp->ptid, new_lp->ptid))
> - {
> - /* The process is not using thread_db. Add the LWP to
> - GDB's list. */
> - add_thread (linux_target, new_lp->ptid);
> - }
> -
> - /* Even if we're stopping the thread for some reason
> - internal to this module, from the perspective of infrun
> - and the user/frontend, this new thread is running until
> - it next reports a stop. */
> - set_running (linux_target, new_lp->ptid, true);
> - set_executing (linux_target, new_lp->ptid, true);
> -
> - if (WSTOPSIG (status) != SIGSTOP)
> - {
> - /* This can happen if someone starts sending signals to
> - the new thread before it gets a chance to run, which
> - have a lower number than SIGSTOP (e.g. SIGUSR1).
> - This is an unlikely case, and harder to handle for
> - fork / vfork than for clone, so we do not try - but
> - we handle it for clone events here. */
> -
> - new_lp->signalled = 1;
> -
> - /* We created NEW_LP so it cannot yet contain STATUS. */
> - gdb_assert (new_lp->status == 0);
> -
> - /* Save the wait status to report later. */
> - linux_nat_debug_printf
> - ("waitpid of new LWP %ld, saving status %s",
> - (long) new_lp->ptid.lwp (), status_to_str (status).c_str ());
> - new_lp->status = status;
> - }
> - else if (report_thread_events)
> - {
> - new_lp->waitstatus.set_thread_created ();
> - new_lp->status = status;
> - }
> -
> - return 1;
> + ourstatus->set_thread_cloned (ptid_t (lp->ptid.pid (), new_pid));
> }
>
> return 0;
> @@ -3536,59 +3549,56 @@ kill_wait_callback (struct lwp_info *lp)
> return 0;
> }
>
> -/* Kill the fork children of any threads of inferior INF that are
> - stopped at a fork event. */
> +/* Kill the fork/clone child of LP if it has an unfollowed child. */
>
> -static void
> -kill_unfollowed_fork_children (struct inferior *inf)
> +static int
> +kill_unfollowed_child_callback (lwp_info *lp)
> {
> - for (thread_info *thread : inf->non_exited_threads ())
> + gdb::optional<target_waitstatus> ws = get_pending_child_status (lp);
> + if (ws.has_value ())
> {
> - struct target_waitstatus *ws = &thread->pending_follow;
> -
> - if (ws->kind () == TARGET_WAITKIND_FORKED
> - || ws->kind () == TARGET_WAITKIND_VFORKED)
> - {
> - ptid_t child_ptid = ws->child_ptid ();
> - int child_pid = child_ptid.pid ();
> - int child_lwp = child_ptid.lwp ();
> + ptid_t child_ptid = ws->child_ptid ();
> + int child_pid = child_ptid.pid ();
> + int child_lwp = child_ptid.lwp ();
>
> - kill_one_lwp (child_lwp);
> - kill_wait_one_lwp (child_lwp);
> + kill_one_lwp (child_lwp);
> + kill_wait_one_lwp (child_lwp);
>
> - /* Let the arch-specific native code know this process is
> - gone. */
> - linux_target->low_forget_process (child_pid);
> - }
> + /* Let the arch-specific native code know this process is
> + gone. */
> + if (ws->kind () != TARGET_WAITKIND_THREAD_CLONED)
> + linux_target->low_forget_process (child_pid);
> }
> +
> + return 0;
> }
>
> void
> linux_nat_target::kill ()
> {
> - /* If we're stopped while forking and we haven't followed yet,
> - kill the other task. We need to do this first because the
> + ptid_t pid_ptid (inferior_ptid.pid ());
> +
> + /* If we're stopped while forking/cloning and we haven't followed
> + yet, kill the child task. We need to do this first because the
> parent will be sleeping if this is a vfork. */
> - kill_unfollowed_fork_children (current_inferior ());
> + iterate_over_lwps (pid_ptid, kill_unfollowed_child_callback);
>
> if (forks_exist_p ())
> linux_fork_killall ();
> else
> {
> - ptid_t ptid = ptid_t (inferior_ptid.pid ());
> -
> /* Stop all threads before killing them, since ptrace requires
> that the thread is stopped to successfully PTRACE_KILL. */
> - iterate_over_lwps (ptid, stop_callback);
> + iterate_over_lwps (pid_ptid, stop_callback);
> /* ... and wait until all of them have reported back that
> they're no longer running. */
> - iterate_over_lwps (ptid, stop_wait_callback);
> + iterate_over_lwps (pid_ptid, stop_wait_callback);
>
> /* Kill all LWP's ... */
> - iterate_over_lwps (ptid, kill_callback);
> + iterate_over_lwps (pid_ptid, kill_callback);
>
> /* ... and wait until we've flushed all events. */
> - iterate_over_lwps (ptid, kill_wait_callback);
> + iterate_over_lwps (pid_ptid, kill_wait_callback);
> }
>
> target_mourn_inferior (inferior_ptid);
> diff --git a/gdb/linux-nat.h b/gdb/linux-nat.h
> index 770fe924427..1cdbeafd4f3 100644
> --- a/gdb/linux-nat.h
> +++ b/gdb/linux-nat.h
> @@ -129,6 +129,8 @@ class linux_nat_target : public inf_ptrace_target
>
> void follow_fork (inferior *, ptid_t, target_waitkind, bool, bool) override;
>
> + void follow_clone (ptid_t) override;
> +
> std::vector<static_tracepoint_marker>
> static_tracepoint_markers_by_strid (const char *id) override;
>
> diff --git a/gdb/target-delegates.c b/gdb/target-delegates.c
> index 57b66ce87b1..7a4ef05b4e1 100644
> --- a/gdb/target-delegates.c
> +++ b/gdb/target-delegates.c
> @@ -76,6 +76,7 @@ struct dummy_target : public target_ops
> int insert_vfork_catchpoint (int arg0) override;
> int remove_vfork_catchpoint (int arg0) override;
> void follow_fork (inferior *arg0, ptid_t arg1, target_waitkind arg2, bool arg3, bool arg4) override;
> + void follow_clone (ptid_t arg0) override;
> int insert_exec_catchpoint (int arg0) override;
> int remove_exec_catchpoint (int arg0) override;
> void follow_exec (inferior *arg0, ptid_t arg1, const char *arg2) override;
> @@ -250,6 +251,7 @@ struct debug_target : public target_ops
> int insert_vfork_catchpoint (int arg0) override;
> int remove_vfork_catchpoint (int arg0) override;
> void follow_fork (inferior *arg0, ptid_t arg1, target_waitkind arg2, bool arg3, bool arg4) override;
> + void follow_clone (ptid_t arg0) override;
> int insert_exec_catchpoint (int arg0) override;
> int remove_exec_catchpoint (int arg0) override;
> void follow_exec (inferior *arg0, ptid_t arg1, const char *arg2) override;
> @@ -1545,6 +1547,28 @@ debug_target::follow_fork (inferior *arg0, ptid_t arg1, target_waitkind arg2, bo
> gdb_puts (")\n", gdb_stdlog);
> }
>
> +void
> +target_ops::follow_clone (ptid_t arg0)
> +{
> + this->beneath ()->follow_clone (arg0);
> +}
> +
> +void
> +dummy_target::follow_clone (ptid_t arg0)
> +{
> + default_follow_clone (this, arg0);
> +}
> +
> +void
> +debug_target::follow_clone (ptid_t arg0)
> +{
> + gdb_printf (gdb_stdlog, "-> %s->follow_clone (...)\n", this->beneath ()->shortname ());
> + this->beneath ()->follow_clone (arg0);
> + gdb_printf (gdb_stdlog, "<- %s->follow_clone (", this->beneath ()->shortname ());
> + target_debug_print_ptid_t (arg0);
> + gdb_puts (")\n", gdb_stdlog);
> +}
> +
> int
> target_ops::insert_exec_catchpoint (int arg0)
> {
> diff --git a/gdb/target.c b/gdb/target.c
> index 0cebecfafc3..9835222e5da 100644
> --- a/gdb/target.c
> +++ b/gdb/target.c
> @@ -2701,6 +2701,13 @@ default_follow_fork (struct target_ops *self, inferior *child_inf,
> internal_error (_("could not find a target to follow fork"));
> }
>
> +static void
> +default_follow_clone (struct target_ops *self, ptid_t child_ptid)
> +{
> + /* Some target returned a clone event, but did not know how to follow it. */
> + internal_error (_("could not find a target to follow clone"));
> +}
> +
> /* See target.h. */
>
> void
> diff --git a/gdb/target.h b/gdb/target.h
> index 2dac86c394d..43ab71093d9 100644
> --- a/gdb/target.h
> +++ b/gdb/target.h
> @@ -637,6 +637,8 @@ struct target_ops
> TARGET_DEFAULT_RETURN (1);
> virtual void follow_fork (inferior *, ptid_t, target_waitkind, bool, bool)
> TARGET_DEFAULT_FUNC (default_follow_fork);
> + virtual void follow_clone (ptid_t)
> + TARGET_DEFAULT_FUNC (default_follow_clone);
> virtual int insert_exec_catchpoint (int)
> TARGET_DEFAULT_RETURN (1);
> virtual int remove_exec_catchpoint (int)
> diff --git a/gdb/target/waitstatus.c b/gdb/target/waitstatus.c
> index 2b8404fb75b..a8edbb17d60 100644
> --- a/gdb/target/waitstatus.c
> +++ b/gdb/target/waitstatus.c
> @@ -45,6 +45,7 @@ DIAGNOSTIC_ERROR_SWITCH
>
> case TARGET_WAITKIND_FORKED:
> case TARGET_WAITKIND_VFORKED:
> + case TARGET_WAITKIND_THREAD_CLONED:
> return string_appendf (str, ", child_ptid = %s",
> this->child_ptid ().to_string ().c_str ());
>
> diff --git a/gdb/target/waitstatus.h b/gdb/target/waitstatus.h
> index 0a88b869044..4c3de005315 100644
> --- a/gdb/target/waitstatus.h
> +++ b/gdb/target/waitstatus.h
> @@ -95,6 +95,13 @@ enum target_waitkind
> /* There are no resumed children left in the program. */
> TARGET_WAITKIND_NO_RESUMED,
>
> + /* The thread was cloned. The event's ptid corresponds to the
> + cloned parent. The cloned child is held stopped at its entry
> + point, and its ptid is in the event's m_child_ptid. The target
> + must not add the cloned child to GDB's thread list until
> + target_ops::follow_clone() is called. */
> + TARGET_WAITKIND_THREAD_CLONED,
> +
> /* The thread was created. */
> TARGET_WAITKIND_THREAD_CREATED,
>
> @@ -102,6 +109,17 @@ enum target_waitkind
> TARGET_WAITKIND_THREAD_EXITED,
> };
>
> +/* Determine if KIND represents an event with a new child - a fork,
> + vfork, or clone. */
> +
> +static inline bool
> +is_new_child_status (target_waitkind kind)
> +{
> + return (kind == TARGET_WAITKIND_FORKED
> + || kind == TARGET_WAITKIND_VFORKED
> + || kind == TARGET_WAITKIND_THREAD_CLONED);
> +}
> +
> /* Return KIND as a string. */
>
> static inline const char *
> @@ -125,6 +143,8 @@ DIAGNOSTIC_ERROR_SWITCH
> return "FORKED";
> case TARGET_WAITKIND_VFORKED:
> return "VFORKED";
> + case TARGET_WAITKIND_THREAD_CLONED:
> + return "THREAD_CLONED";
> case TARGET_WAITKIND_EXECD:
> return "EXECD";
> case TARGET_WAITKIND_VFORK_DONE:
> @@ -325,6 +345,14 @@ struct target_waitstatus
> return *this;
> }
>
> + target_waitstatus &set_thread_cloned (ptid_t child_ptid)
> + {
> + this->reset ();
> + m_kind = TARGET_WAITKIND_THREAD_CLONED;
> + m_value.child_ptid = child_ptid;
> + return *this;
> + }
> +
> target_waitstatus &set_thread_created ()
> {
> this->reset ();
> @@ -369,8 +397,7 @@ struct target_waitstatus
>
> ptid_t child_ptid () const
> {
> - gdb_assert (m_kind == TARGET_WAITKIND_FORKED
> - || m_kind == TARGET_WAITKIND_VFORKED);
> + gdb_assert (is_new_child_status (m_kind));
> return m_value.child_ptid;
> }
>
>
> base-commit: 2562954ede66f32bff7d985e752b8052c2ae5775
> prerequisite-patch-id: bbc9918ac5f79de07a29f34ec072794d270f942d
> prerequisite-patch-id: c0bc5b4f99193bb50cb31f551673de1808dcda35
> prerequisite-patch-id: ab0838f2bc02931d7e830abe23833e7a8224442c
> --
> 2.36.0
On 2023-03-21 16:06, Andrew Burgess wrote:
> Pedro Alves <pedro@palves.net> writes:
>
>> On 2023-02-04 3:38 p.m., Andrew Burgess wrote:
>> The solution implemented here is to model clone on fork/vfork, and
>> introduce a new TARGET_WAITKIND_THREAD_CLONED event. This event is
>> similar to TARGET_WAITKIND_FORKED and TARGET_WAITKIND_VFORKED, except
>> that we end up with a new thread in the same process, instead of a new
>> thread of a new process. Like FORKED and VFORKED, THREAD_CLONED
>> waitstatuses have a child_ptid property, and the child is held stopped
>> until GDB explicitly resumes it. This addresses the in-line stepping
>> case (issues #1 and #2).
>>
>> The infrun code that handles displaced stepping fixup for the child
>> after a fork/vfork event is thus reused for THREAD_CLONE, with some
>> minimal conditions added, addressing the displaced stepping case
>> (issue #3).
>>
>> The native Linux backend is adjusted to unconditionally report
>> TARGET_WAITKIND_THREAD_CLONED events to the core.
>>
>> Following the follow_fork model in core GDB, we introduce a
>> target_follow_clone target method, which is responsible for making the
>> new clone child visible to the rest of GDB.
>>
>> Subsequent patches will add clone events support to the remote
>> protocol and gdbserver.
>>
>> A testcase will be added by a later patch.
>
> What's the motivation for splitting the implementation from the
> associated tests?
It's just that while developing this, I kept reordering patches, while
fixing different aspects, different modes (displaced step, in-line step), different
targets (native, remote). It was so much easier to keep the testcase in a
separate patch, and fix it / adjust it as I went along. Keeping the testcase separate
also allowed for keeping you listed as the main git commit author for that patch.
I've now merged the testcase into this patch, and added a note giving you credit
(along with git tag, of course). The test fails for remote at this point, so I added
a "require" call to skip it on anything other than native at this point.
>
> Though your implementation may be different (better) than mine, the
> original test I wrote[1] still passes just fine with this commit.
>
> When digging back through old patches it's much nicer to easier to find
> the GDB changes and the tests in a single commit IMHO. Do feel free to
> take the tests from that commit, if that helps at all (though I notice I
> left some debug logging in which would need cleaning up, and the test
> fails when using a remote target, but that's easily fixed by skipping
> the test in that case).
>
> [1] https://sourceware.org/pipermail/gdb-patches/2021-June/180520.html
>
I guess you noticed later while reading the rest of the series, but I did
already take the tests from that commit, and preserved them as patch #13
in this series:
https://inbox.sourceware.org/gdb-patches/8735331856.fsf@redhat.com/
I had kept the "New in vX" notes in there so you could more easily tell what
had changed from yours. (I've now dropped them in the new version.)
> I've looked through the code and it all looks good to me.
>
> Reviewed-By: Andrew Burgess <aburgess@redhat.com>
>
Thanks!
@@ -1583,6 +1583,7 @@ step_over_info_valid_p (void)
/* Return true if THREAD is doing a displaced step. */
static bool
+ATTRIBUTE_UNUSED
displaced_step_in_progress_thread (thread_info *thread)
{
gdb_assert (thread != nullptr);
@@ -1897,6 +1898,31 @@ static displaced_step_finish_status
displaced_step_finish (thread_info *event_thread,
const target_waitstatus &event_status)
{
+ /* Check whether the parent is displaced stepping. */
+ struct regcache *regcache = get_thread_regcache (event_thread);
+ struct gdbarch *gdbarch = regcache->arch ();
+ inferior *parent_inf = event_thread->inf;
+
+ /* If this was a fork/vfork/clone, this event indicates that the
+ displaced stepping of the syscall instruction has been done, so
+ we perform cleanup for parent here. Also note that this
+ operation also cleans up the child for vfork, because their pages
+ are shared. */
+
+ /* If this is a fork (child gets its own address space copy) and
+ some displaced step buffers were in use at the time of the fork,
+ restore the displaced step buffer bytes in the child process.
+
+ Architectures which support displaced stepping and fork events
+ must supply an implementation of
+ gdbarch_displaced_step_restore_all_in_ptid. This is not enforced
+ during gdbarch validation to support architectures which support
+ displaced stepping but not forks. */
+ if (event_status.kind () == TARGET_WAITKIND_FORKED
+ && gdbarch_supports_displaced_stepping (gdbarch))
+ gdbarch_displaced_step_restore_all_in_ptid
+ (gdbarch, parent_inf, event_status.child_ptid ());
+
displaced_step_thread_state *displaced = &event_thread->displaced_step_state;
/* Was this thread performing a displaced step? */
@@ -1916,8 +1942,39 @@ displaced_step_finish (thread_info *event_thread,
/* Do the fixup, and release the resources acquired to do the displaced
step. */
- return gdbarch_displaced_step_finish (displaced->get_original_gdbarch (),
- event_thread, event_status);
+ displaced_step_finish_status status
+ = gdbarch_displaced_step_finish (displaced->get_original_gdbarch (),
+ event_thread, event_status);
+
+ if (event_status.kind () == TARGET_WAITKIND_FORKED
+ || event_status.kind () == TARGET_WAITKIND_VFORKED
+ || event_status.kind () == TARGET_WAITKIND_THREAD_CLONED)
+ {
+ /* Since the vfork/fork/clone syscall instruction was executed
+ in the scratchpad, the child's PC is also within the
+ scratchpad. Set the child's PC to the parent's PC value,
+ which has already been fixed up. Note: we use the parent's
+ aspace here, although we're touching the child, because the
+ child hasn't been added to the inferior list yet at this
+ point. */
+
+ struct regcache *child_regcache
+ = get_thread_arch_aspace_regcache (parent_inf->process_target (),
+ event_status.child_ptid (),
+ gdbarch,
+ parent_inf->aspace);
+ /* Read PC value of parent. */
+ CORE_ADDR parent_pc = regcache_read_pc (regcache);
+
+ displaced_debug_printf ("write child pc from %s to %s",
+ paddress (gdbarch,
+ regcache_read_pc (child_regcache)),
+ paddress (gdbarch, parent_pc));
+
+ regcache_write_pc (child_regcache, parent_pc);
+ }
+
+ return status;
}
/* Data to be passed around while handling an event. This data is
@@ -5663,67 +5720,13 @@ handle_inferior_event (struct execution_control_state *ecs)
case TARGET_WAITKIND_FORKED:
case TARGET_WAITKIND_VFORKED:
- /* Check whether the inferior is displaced stepping. */
- {
- struct regcache *regcache = get_thread_regcache (ecs->event_thread);
- struct gdbarch *gdbarch = regcache->arch ();
- inferior *parent_inf = find_inferior_ptid (ecs->target, ecs->ptid);
-
- /* If this is a fork (child gets its own address space copy)
- and some displaced step buffers were in use at the time of
- the fork, restore the displaced step buffer bytes in the
- child process.
-
- Architectures which support displaced stepping and fork
- events must supply an implementation of
- gdbarch_displaced_step_restore_all_in_ptid. This is not
- enforced during gdbarch validation to support architectures
- which support displaced stepping but not forks. */
- if (ecs->ws.kind () == TARGET_WAITKIND_FORKED
- && gdbarch_supports_displaced_stepping (gdbarch))
- gdbarch_displaced_step_restore_all_in_ptid
- (gdbarch, parent_inf, ecs->ws.child_ptid ());
-
- /* If displaced stepping is supported, and thread ecs->ptid is
- displaced stepping. */
- if (displaced_step_in_progress_thread (ecs->event_thread))
- {
- struct regcache *child_regcache;
- CORE_ADDR parent_pc;
-
- /* GDB has got TARGET_WAITKIND_FORKED or TARGET_WAITKIND_VFORKED,
- indicating that the displaced stepping of syscall instruction
- has been done. Perform cleanup for parent process here. Note
- that this operation also cleans up the child process for vfork,
- because their pages are shared. */
- displaced_step_finish (ecs->event_thread, ecs->ws);
- /* Start a new step-over in another thread if there's one
- that needs it. */
- start_step_over ();
-
- /* Since the vfork/fork syscall instruction was executed in the scratchpad,
- the child's PC is also within the scratchpad. Set the child's PC
- to the parent's PC value, which has already been fixed up.
- FIXME: we use the parent's aspace here, although we're touching
- the child, because the child hasn't been added to the inferior
- list yet at this point. */
-
- child_regcache
- = get_thread_arch_aspace_regcache (parent_inf->process_target (),
- ecs->ws.child_ptid (),
- gdbarch,
- parent_inf->aspace);
- /* Read PC value of parent process. */
- parent_pc = regcache_read_pc (regcache);
-
- displaced_debug_printf ("write child pc from %s to %s",
- paddress (gdbarch,
- regcache_read_pc (child_regcache)),
- paddress (gdbarch, parent_pc));
-
- regcache_write_pc (child_regcache, parent_pc);
- }
- }
+ case TARGET_WAITKIND_THREAD_CLONED:
+
+ displaced_step_finish (ecs->event_thread, ecs->ws);
+
+ /* Start a new step-over in another thread if there's one that
+ needs it. */
+ start_step_over ();
context_switch (ecs);
@@ -5739,7 +5742,7 @@ handle_inferior_event (struct execution_control_state *ecs)
need to unpatch at follow/detach time instead to be certain
that new breakpoints added between catchpoint hit time and
vfork follow are detached. */
- if (ecs->ws.kind () != TARGET_WAITKIND_VFORKED)
+ if (ecs->ws.kind () == TARGET_WAITKIND_FORKED)
{
/* This won't actually modify the breakpoint list, but will
physically remove the breakpoints from the child. */
@@ -5771,14 +5774,24 @@ handle_inferior_event (struct execution_control_state *ecs)
if (!bpstat_causes_stop (ecs->event_thread->control.stop_bpstat))
{
bool follow_child
- = (follow_fork_mode_string == follow_fork_mode_child);
+ = (ecs->ws.kind () != TARGET_WAITKIND_THREAD_CLONED
+ && follow_fork_mode_string == follow_fork_mode_child);
ecs->event_thread->set_stop_signal (GDB_SIGNAL_0);
process_stratum_target *targ
= ecs->event_thread->inf->process_target ();
- bool should_resume = follow_fork ();
+ bool should_resume;
+ if (ecs->ws.kind () != TARGET_WAITKIND_THREAD_CLONED)
+ should_resume = follow_fork ();
+ else
+ {
+ should_resume = true;
+ inferior *inf = ecs->event_thread->inf;
+ inf->top_target ()->follow_clone (ecs->ws.child_ptid ());
+ ecs->event_thread->pending_follow.set_spurious ();
+ }
/* Note that one of these may be an invalid pointer,
depending on detach_fork. */
@@ -5789,16 +5802,21 @@ handle_inferior_event (struct execution_control_state *ecs)
child is marked stopped. */
/* If not resuming the parent, mark it stopped. */
- if (follow_child && !detach_fork && !non_stop && !sched_multi)
+ if (ecs->ws.kind () != TARGET_WAITKIND_THREAD_CLONED
+ && follow_child && !detach_fork && !non_stop && !sched_multi)
parent->set_running (false);
/* If resuming the child, mark it running. */
- if (follow_child || (!detach_fork && (non_stop || sched_multi)))
+ if (ecs->ws.kind () == TARGET_WAITKIND_THREAD_CLONED
+ || (follow_child || (!detach_fork && (non_stop || sched_multi))))
child->set_running (true);
/* In non-stop mode, also resume the other branch. */
- if (!detach_fork && (non_stop
- || (sched_multi && target_is_non_stop_p ())))
+ if ((ecs->ws.kind () == TARGET_WAITKIND_THREAD_CLONED
+ && target_is_non_stop_p ())
+ || (!detach_fork && (non_stop
+ || (sched_multi
+ && target_is_non_stop_p ()))))
{
if (follow_child)
switch_to_thread (parent);
@@ -1286,64 +1286,79 @@ get_detach_signal (struct lwp_info *lp)
return 0;
}
-/* Detach from LP. If SIGNO_P is non-NULL, then it points to the
- signal number that should be passed to the LWP when detaching.
- Otherwise pass any pending signal the LWP may have, if any. */
+/* If LP has a pending fork/vfork/clone status, return it. */
-static void
-detach_one_lwp (struct lwp_info *lp, int *signo_p)
+static gdb::optional<target_waitstatus>
+get_pending_child_status (lwp_info *lp)
{
- int lwpid = lp->ptid.lwp ();
- int signo;
-
- gdb_assert (lp->status == 0 || WIFSTOPPED (lp->status));
-
- /* If the lwp/thread we are about to detach has a pending fork event,
- there is a process GDB is attached to that the core of GDB doesn't know
- about. Detach from it. */
-
/* Check in lwp_info::status. */
if (WIFSTOPPED (lp->status) && linux_is_extended_waitstatus (lp->status))
{
int event = linux_ptrace_get_extended_event (lp->status);
- if (event == PTRACE_EVENT_FORK || event == PTRACE_EVENT_VFORK)
+ if (event == PTRACE_EVENT_FORK
+ || event == PTRACE_EVENT_VFORK
+ || event == PTRACE_EVENT_CLONE)
{
unsigned long child_pid;
int ret = ptrace (PTRACE_GETEVENTMSG, lp->ptid.lwp (), 0, &child_pid);
if (ret == 0)
- detach_one_pid (child_pid, 0);
+ {
+ target_waitstatus ws;
+
+ if (event == PTRACE_EVENT_FORK)
+ ws.set_forked (ptid_t (child_pid, child_pid));
+ else if (event == PTRACE_EVENT_VFORK)
+ ws.set_vforked (ptid_t (child_pid, child_pid));
+ else if (event == PTRACE_EVENT_CLONE)
+ ws.set_thread_cloned (ptid_t (lp->ptid.pid (), child_pid));
+ else
+ gdb_assert_not_reached ("unhandled");
+
+ return ws;
+ }
else
- perror_warning_with_name (_("Failed to detach fork child"));
+ {
+ perror_warning_with_name (_("Failed to retrieve event msg"));
+ return {};
+ }
}
}
/* Check in lwp_info::waitstatus. */
- if (lp->waitstatus.kind () == TARGET_WAITKIND_VFORKED
- || lp->waitstatus.kind () == TARGET_WAITKIND_FORKED)
- detach_one_pid (lp->waitstatus.child_ptid ().pid (), 0);
-
+ if (is_new_child_status (lp->waitstatus.kind ()))
+ return lp->waitstatus;
- /* Check in thread_info::pending_waitstatus. */
thread_info *tp = find_thread_ptid (linux_target, lp->ptid);
- if (tp->has_pending_waitstatus ())
- {
- const target_waitstatus &ws = tp->pending_waitstatus ();
- if (ws.kind () == TARGET_WAITKIND_VFORKED
- || ws.kind () == TARGET_WAITKIND_FORKED)
- detach_one_pid (ws.child_ptid ().pid (), 0);
- }
+ /* Check in thread_info::pending_waitstatus. */
+ if (tp->has_pending_waitstatus ()
+ && is_new_child_status (tp->pending_waitstatus ().kind ()))
+ return tp->pending_waitstatus ();
/* Check in thread_info::pending_follow. */
- if (tp->pending_follow.kind () == TARGET_WAITKIND_VFORKED
- || tp->pending_follow.kind () == TARGET_WAITKIND_FORKED)
- detach_one_pid (tp->pending_follow.child_ptid ().pid (), 0);
+ if (is_new_child_status (tp->pending_follow.kind ()))
+ return tp->pending_follow;
- if (lp->status != 0)
- linux_nat_debug_printf ("Pending %s for %s on detach.",
- strsignal (WSTOPSIG (lp->status)),
- lp->ptid.to_string ().c_str ());
+ return {};
+}
+
+/* Detach from LP. If SIGNO_P is non-NULL, then it points to the
+ signal number that should be passed to the LWP when detaching.
+ Otherwise pass any pending signal the LWP may have, if any. */
+
+static void
+detach_one_lwp (struct lwp_info *lp, int *signo_p)
+{
+ int lwpid = lp->ptid.lwp ();
+ int signo;
+
+ /* If the lwp/thread we are about to detach has a pending fork/clone
+ event, there is a process/thread GDB is attached to that the core
+ of GDB doesn't know about. Detach from it. */
+
+ if (gdb::optional<target_waitstatus> ws = get_pending_child_status (lp))
+ detach_one_pid (ws->child_ptid ().lwp (), 0);
/* If there is a pending SIGSTOP, get rid of it. */
if (lp->signalled)
@@ -1821,6 +1836,53 @@ linux_handle_syscall_trap (struct lwp_info *lp, int stopping)
return 1;
}
+void
+linux_nat_target::follow_clone (ptid_t child_ptid)
+{
+ lwp_info *new_lp = add_lwp (child_ptid);
+ new_lp->stopped = 1;
+
+ /* If the thread_db layer is active, let it record the user
+ level thread id and status, and add the thread to GDB's
+ list. */
+ if (!thread_db_notice_clone (inferior_ptid, new_lp->ptid))
+ {
+ /* The process is not using thread_db. Add the LWP to
+ GDB's list. */
+ add_thread (linux_target, new_lp->ptid);
+ }
+
+ /* We just created NEW_LP so it cannot yet contain STATUS. */
+ gdb_assert (new_lp->status == 0);
+
+ if (!pull_pid_from_list (&stopped_pids, child_ptid.lwp (), &new_lp->status))
+ internal_error (_("no saved status for clone lwp"));
+
+ if (WSTOPSIG (new_lp->status) != SIGSTOP)
+ {
+ /* This can happen if someone starts sending signals to
+ the new thread before it gets a chance to run, which
+ have a lower number than SIGSTOP (e.g. SIGUSR1).
+ This is an unlikely case, and harder to handle for
+ fork / vfork than for clone, so we do not try - but
+ we handle it for clone events here. */
+
+ new_lp->signalled = 1;
+
+ /* Save the wait status to report later. */
+ linux_nat_debug_printf
+ ("waitpid of new LWP %ld, saving status %s",
+ (long) new_lp->ptid.lwp (), status_to_str (new_lp->status).c_str ());
+ }
+ else
+ {
+ new_lp->status = 0;
+
+ if (report_thread_events)
+ new_lp->waitstatus.set_thread_created ();
+ }
+}
+
/* Handle a GNU/Linux extended wait response. If we see a clone
event, we need to add the new LWP to our list (and not report the
trap to higher layers). This function returns non-zero if the
@@ -1861,11 +1923,9 @@ linux_handle_extended_wait (struct lwp_info *lp, int status)
internal_error (_("wait returned unexpected status 0x%x"), status);
}
- ptid_t child_ptid (new_pid, new_pid);
-
if (event == PTRACE_EVENT_FORK || event == PTRACE_EVENT_VFORK)
{
- open_proc_mem_file (child_ptid);
+ open_proc_mem_file (ptid_t (new_pid, new_pid));
/* The arch-specific native code may need to know about new
forks even if those end up never mapped to an
@@ -1902,66 +1962,18 @@ linux_handle_extended_wait (struct lwp_info *lp, int status)
}
if (event == PTRACE_EVENT_FORK)
- ourstatus->set_forked (child_ptid);
+ ourstatus->set_forked (ptid_t (new_pid, new_pid));
else if (event == PTRACE_EVENT_VFORK)
- ourstatus->set_vforked (child_ptid);
+ ourstatus->set_vforked (ptid_t (new_pid, new_pid));
else if (event == PTRACE_EVENT_CLONE)
{
- struct lwp_info *new_lp;
-
- ourstatus->set_ignore ();
-
linux_nat_debug_printf
("Got clone event from LWP %d, new child is LWP %ld", pid, new_pid);
- new_lp = add_lwp (ptid_t (lp->ptid.pid (), new_pid));
- new_lp->stopped = 1;
- new_lp->resumed = 1;
+ /* Save the status again, we'll use it in follow_clone. */
+ add_to_pid_list (&stopped_pids, new_pid, status);
- /* If the thread_db layer is active, let it record the user
- level thread id and status, and add the thread to GDB's
- list. */
- if (!thread_db_notice_clone (lp->ptid, new_lp->ptid))
- {
- /* The process is not using thread_db. Add the LWP to
- GDB's list. */
- add_thread (linux_target, new_lp->ptid);
- }
-
- /* Even if we're stopping the thread for some reason
- internal to this module, from the perspective of infrun
- and the user/frontend, this new thread is running until
- it next reports a stop. */
- set_running (linux_target, new_lp->ptid, true);
- set_executing (linux_target, new_lp->ptid, true);
-
- if (WSTOPSIG (status) != SIGSTOP)
- {
- /* This can happen if someone starts sending signals to
- the new thread before it gets a chance to run, which
- have a lower number than SIGSTOP (e.g. SIGUSR1).
- This is an unlikely case, and harder to handle for
- fork / vfork than for clone, so we do not try - but
- we handle it for clone events here. */
-
- new_lp->signalled = 1;
-
- /* We created NEW_LP so it cannot yet contain STATUS. */
- gdb_assert (new_lp->status == 0);
-
- /* Save the wait status to report later. */
- linux_nat_debug_printf
- ("waitpid of new LWP %ld, saving status %s",
- (long) new_lp->ptid.lwp (), status_to_str (status).c_str ());
- new_lp->status = status;
- }
- else if (report_thread_events)
- {
- new_lp->waitstatus.set_thread_created ();
- new_lp->status = status;
- }
-
- return 1;
+ ourstatus->set_thread_cloned (ptid_t (lp->ptid.pid (), new_pid));
}
return 0;
@@ -3536,59 +3548,55 @@ kill_wait_callback (struct lwp_info *lp)
return 0;
}
-/* Kill the fork children of any threads of inferior INF that are
- stopped at a fork event. */
+/* Kill the fork/clone child of LP if it has an unfollowed child. */
-static void
-kill_unfollowed_fork_children (struct inferior *inf)
+static int
+kill_unfollowed_child_callback (lwp_info *lp)
{
- for (thread_info *thread : inf->non_exited_threads ())
+ if (gdb::optional<target_waitstatus> ws = get_pending_child_status (lp))
{
- struct target_waitstatus *ws = &thread->pending_follow;
-
- if (ws->kind () == TARGET_WAITKIND_FORKED
- || ws->kind () == TARGET_WAITKIND_VFORKED)
- {
- ptid_t child_ptid = ws->child_ptid ();
- int child_pid = child_ptid.pid ();
- int child_lwp = child_ptid.lwp ();
+ ptid_t child_ptid = ws->child_ptid ();
+ int child_pid = child_ptid.pid ();
+ int child_lwp = child_ptid.lwp ();
- kill_one_lwp (child_lwp);
- kill_wait_one_lwp (child_lwp);
+ kill_one_lwp (child_lwp);
+ kill_wait_one_lwp (child_lwp);
- /* Let the arch-specific native code know this process is
- gone. */
- linux_target->low_forget_process (child_pid);
- }
+ /* Let the arch-specific native code know this process is
+ gone. */
+ if (ws->kind () != TARGET_WAITKIND_THREAD_CLONED)
+ linux_target->low_forget_process (child_pid);
}
+
+ return 0;
}
void
linux_nat_target::kill ()
{
- /* If we're stopped while forking and we haven't followed yet,
- kill the other task. We need to do this first because the
+ ptid_t pid_ptid (inferior_ptid.pid ());
+
+ /* If we're stopped while forking/cloning and we haven't followed
+ yet, kill the child task. We need to do this first because the
parent will be sleeping if this is a vfork. */
- kill_unfollowed_fork_children (current_inferior ());
+ iterate_over_lwps (pid_ptid, kill_unfollowed_child_callback);
if (forks_exist_p ())
linux_fork_killall ();
else
{
- ptid_t ptid = ptid_t (inferior_ptid.pid ());
-
/* Stop all threads before killing them, since ptrace requires
that the thread is stopped to successfully PTRACE_KILL. */
- iterate_over_lwps (ptid, stop_callback);
+ iterate_over_lwps (pid_ptid, stop_callback);
/* ... and wait until all of them have reported back that
they're no longer running. */
- iterate_over_lwps (ptid, stop_wait_callback);
+ iterate_over_lwps (pid_ptid, stop_wait_callback);
/* Kill all LWP's ... */
- iterate_over_lwps (ptid, kill_callback);
+ iterate_over_lwps (pid_ptid, kill_callback);
/* ... and wait until we've flushed all events. */
- iterate_over_lwps (ptid, kill_wait_callback);
+ iterate_over_lwps (pid_ptid, kill_wait_callback);
}
target_mourn_inferior (inferior_ptid);
@@ -129,6 +129,8 @@ class linux_nat_target : public inf_ptrace_target
void follow_fork (inferior *, ptid_t, target_waitkind, bool, bool) override;
+ void follow_clone (ptid_t) override;
+
std::vector<static_tracepoint_marker>
static_tracepoint_markers_by_strid (const char *id) override;
@@ -76,6 +76,7 @@ struct dummy_target : public target_ops
int insert_vfork_catchpoint (int arg0) override;
int remove_vfork_catchpoint (int arg0) override;
void follow_fork (inferior *arg0, ptid_t arg1, target_waitkind arg2, bool arg3, bool arg4) override;
+ void follow_clone (ptid_t arg0) override;
int insert_exec_catchpoint (int arg0) override;
int remove_exec_catchpoint (int arg0) override;
void follow_exec (inferior *arg0, ptid_t arg1, const char *arg2) override;
@@ -250,6 +251,7 @@ struct debug_target : public target_ops
int insert_vfork_catchpoint (int arg0) override;
int remove_vfork_catchpoint (int arg0) override;
void follow_fork (inferior *arg0, ptid_t arg1, target_waitkind arg2, bool arg3, bool arg4) override;
+ void follow_clone (ptid_t arg0) override;
int insert_exec_catchpoint (int arg0) override;
int remove_exec_catchpoint (int arg0) override;
void follow_exec (inferior *arg0, ptid_t arg1, const char *arg2) override;
@@ -1545,6 +1547,28 @@ debug_target::follow_fork (inferior *arg0, ptid_t arg1, target_waitkind arg2, bo
gdb_puts (")\n", gdb_stdlog);
}
+void
+target_ops::follow_clone (ptid_t arg0)
+{
+ this->beneath ()->follow_clone (arg0);
+}
+
+void
+dummy_target::follow_clone (ptid_t arg0)
+{
+ default_follow_clone (this, arg0);
+}
+
+void
+debug_target::follow_clone (ptid_t arg0)
+{
+ gdb_printf (gdb_stdlog, "-> %s->follow_clone (...)\n", this->beneath ()->shortname ());
+ this->beneath ()->follow_clone (arg0);
+ gdb_printf (gdb_stdlog, "<- %s->follow_clone (", this->beneath ()->shortname ());
+ target_debug_print_ptid_t (arg0);
+ gdb_puts (")\n", gdb_stdlog);
+}
+
int
target_ops::insert_exec_catchpoint (int arg0)
{
@@ -2732,6 +2732,13 @@ default_follow_fork (struct target_ops *self, inferior *child_inf,
internal_error (_("could not find a target to follow fork"));
}
+static void
+default_follow_clone (struct target_ops *self, ptid_t child_ptid)
+{
+ /* Some target returned a clone event, but did not know how to follow it. */
+ internal_error (_("could not find a target to follow clone"));
+}
+
/* See target.h. */
void
@@ -637,6 +637,8 @@ struct target_ops
TARGET_DEFAULT_RETURN (1);
virtual void follow_fork (inferior *, ptid_t, target_waitkind, bool, bool)
TARGET_DEFAULT_FUNC (default_follow_fork);
+ virtual void follow_clone (ptid_t)
+ TARGET_DEFAULT_FUNC (default_follow_clone);
virtual int insert_exec_catchpoint (int)
TARGET_DEFAULT_RETURN (1);
virtual int remove_exec_catchpoint (int)
@@ -45,6 +45,7 @@ DIAGNOSTIC_ERROR_SWITCH
case TARGET_WAITKIND_FORKED:
case TARGET_WAITKIND_VFORKED:
+ case TARGET_WAITKIND_THREAD_CLONED:
return string_appendf (str, ", child_ptid = %s",
this->child_ptid ().to_string ().c_str ());
@@ -95,6 +95,13 @@ enum target_waitkind
/* There are no resumed children left in the program. */
TARGET_WAITKIND_NO_RESUMED,
+ /* The thread was cloned. The event's ptid corresponds to the
+ cloned parent. The cloned child is held stopped at its entry
+ point, and its ptid is in the event's m_child_ptid. The target
+ must not add the cloned child to GDB's thread list until
+ target_ops::follow_clone() is called. */
+ TARGET_WAITKIND_THREAD_CLONED,
+
/* The thread was created. */
TARGET_WAITKIND_THREAD_CREATED,
@@ -102,6 +109,17 @@ enum target_waitkind
TARGET_WAITKIND_THREAD_EXITED,
};
+/* Determine if KIND represents an event with a new child - a fork,
+ vfork, or clone. */
+
+static inline bool
+is_new_child_status (target_waitkind kind)
+{
+ return (kind == TARGET_WAITKIND_FORKED
+ || kind == TARGET_WAITKIND_VFORKED
+ || kind == TARGET_WAITKIND_THREAD_CLONED);
+}
+
/* Return KIND as a string. */
static inline const char *
@@ -125,6 +143,8 @@ DIAGNOSTIC_ERROR_SWITCH
return "FORKED";
case TARGET_WAITKIND_VFORKED:
return "VFORKED";
+ case TARGET_WAITKIND_THREAD_CLONED:
+ return "THREAD_CLONED";
case TARGET_WAITKIND_EXECD:
return "EXECD";
case TARGET_WAITKIND_VFORK_DONE:
@@ -325,6 +345,14 @@ struct target_waitstatus
return *this;
}
+ target_waitstatus &set_thread_cloned (ptid_t child_ptid)
+ {
+ this->reset ();
+ m_kind = TARGET_WAITKIND_THREAD_CLONED;
+ m_value.child_ptid = child_ptid;
+ return *this;
+ }
+
target_waitstatus &set_thread_created ()
{
this->reset ();
@@ -369,8 +397,7 @@ struct target_waitstatus
ptid_t child_ptid () const
{
- gdb_assert (m_kind == TARGET_WAITKIND_FORKED
- || m_kind == TARGET_WAITKIND_VFORKED);
+ gdb_assert (is_new_child_status (m_kind));
return m_value.child_ptid;
}