Re: [PATCH 04/31] Step over clone syscall w/ breakpoint, TARGET_WAITKIND_THREAD_CLONED

Andrew Burgess via Gdb-patches
2023-03-21T16:06:44Z

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