From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id 9533A3858D3C for ; Tue, 21 Mar 2023 16:06:15 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 9533A3858D3C Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1679414775; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=lH7FJn6M1bqb1rOSo5GC3FmElotxgxJhLjLZbqfgTPM=; b=EGSH5eKJv+Q6B5B0DEPsaA7nx8Sd3DIiDKVwpgmZQr0KOZrE5BeFE0xeOZ12YnJPw9aPPJ 5OMnXcqIgu07GZ22W7ASTE1rFWIzZGbmONy4e5gu4ZtUk7z4CkVcRwQJIelSThygiJEUyV XV2e2OSk1JoOF0A5cAwRCyrlPQTHq6o= Received: from mail-wm1-f69.google.com (mail-wm1-f69.google.com [209.85.128.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-562-bnwR4GSVP9KfYtnTAUdvZQ-1; Tue, 21 Mar 2023 12:06:13 -0400 X-MC-Unique: bnwR4GSVP9KfYtnTAUdvZQ-1 Received: by mail-wm1-f69.google.com with SMTP id bi7-20020a05600c3d8700b003edecc610abso3537078wmb.7 for ; Tue, 21 Mar 2023 09:06:13 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1679414772; h=mime-version:message-id:date:references:in-reply-to:subject:to:from :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=lH7FJn6M1bqb1rOSo5GC3FmElotxgxJhLjLZbqfgTPM=; b=QhPShHLrvqpvLerkadXr06vWEKWLQtmrymxsmxxRp1tF8lXvatB1evD7KF3S/9WtT1 hWBxv5zufH1alq/bplu/DPIXWqlPRTfGv+PpHA+PGEEu9C+N6sXLIR63w8vYkQcCGdCx kYfAe47QDpR4i3/S8qvQQnR+O0tW5QXqKPXqDFZdU5kg4Mvk5uSaGep0e9pPmw5Goxv/ s2IwideN1tvOfi4LFLti1zuwcOl2lXLkIINwONG8fwXbg1pCjopV1TAu7XgFdSnMIqJA eaARTqPV1keCuSvE+T1PSiVsoRyF5TLljKmuyNEpKAb4w3ehiV1pW+cHRML+EkIegOLo og4Q== X-Gm-Message-State: AO0yUKUgdzLJatAcuADfOCXLC5MBbT5OX/cl0nhR1R+129Q9LGrUS4q2 P2RGwIFwU3mNiqFLcqLOgB/pRn/Hb+cykBrqF/JuDpkOSi7DtqB9OvtwAUpipAwrAzgdwZJE/bP UQM3dO9NaPp53uWtLdKuF29+XP2lVwQ== X-Received: by 2002:a1c:7c0b:0:b0:3eb:2e19:ff3a with SMTP id x11-20020a1c7c0b000000b003eb2e19ff3amr2756518wmc.7.1679414771779; Tue, 21 Mar 2023 09:06:11 -0700 (PDT) X-Google-Smtp-Source: AK7set9aRzwyPP45lnAIaEJNZzCiIVY35E5dTLrywW+2p+Qvm17y0TIkboch5TkX86Lfm3B/QNlhhw== X-Received: by 2002:a1c:7c0b:0:b0:3eb:2e19:ff3a with SMTP id x11-20020a1c7c0b000000b003eb2e19ff3amr2756473wmc.7.1679414771251; Tue, 21 Mar 2023 09:06:11 -0700 (PDT) Received: from localhost (95.72.115.87.dyn.plus.net. [87.115.72.95]) by smtp.gmail.com with ESMTPSA id l22-20020a05600c1d1600b003ee17ff73d3sm4274224wms.46.2023.03.21.09.06.10 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 21 Mar 2023 09:06:10 -0700 (PDT) From: Andrew Burgess To: Pedro Alves , gdb-patches@sourceware.org Subject: Re: [PATCH 04/31] Step over clone syscall w/ breakpoint, TARGET_WAITKIND_THREAD_CLONED In-Reply-To: References: <20221212203101.1034916-1-pedro@palves.net> <20221212203101.1034916-5-pedro@palves.net> <87bkm9qw4i.fsf@redhat.com> Date: Tue, 21 Mar 2023 16:06:09 +0000 Message-ID: <87fs9yccni.fsf@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain X-Spam-Status: No, score=-11.6 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,KAM_LOTSOFHASH,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Pedro Alves writes: > On 2023-02-04 3:38 p.m., Andrew Burgess wrote: >> Pedro Alves 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 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 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 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 > 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 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 > +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 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 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_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