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.129.124]) by sourceware.org (Postfix) with ESMTPS id A354C3858281 for ; Tue, 6 Jun 2023 14:12:16 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org A354C3858281 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=1686060736; 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=QiiIEh/gpNKudc6gXp8f8qLI2ZzeS8ydwU5c4Se8ptQ=; b=T1fYx6r9d6Mo0ExS6FZoeDihq+jd9d/xfqTnV2vOd9h9W9vGYmmU9myto82/x4etVoChh8 VfuYHt2QgDHw3E86uLsrA8Ww76cx2+PRaKI0/DRPHx18PE4QpPjBAgDEdfJy4kpN+crq7s pzg8XHMfJ8JCTblWS9rpADQ2M+qPpuk= Received: from mail-wr1-f70.google.com (mail-wr1-f70.google.com [209.85.221.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-345-EoyihDzqNIOowK1Wmt-byw-1; Tue, 06 Jun 2023 10:12:14 -0400 X-MC-Unique: EoyihDzqNIOowK1Wmt-byw-1 Received: by mail-wr1-f70.google.com with SMTP id ffacd0b85a97d-30ae18b11bfso2685534f8f.0 for ; Tue, 06 Jun 2023 07:12:14 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1686060733; x=1688652733; 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=QiiIEh/gpNKudc6gXp8f8qLI2ZzeS8ydwU5c4Se8ptQ=; b=Puy+vv5eXU1U1m0C9HqgwFQxezDjwtZav0utKDbMJFybDS5/YFn20NgLWyjWKhbwLV FKnsuD8LBWydGmNb1HZZJWZ219UXSsf3nFkBAtjjZ/qSIY/TuaL7Sl8xR9EF+Y8MZ3WK msMty8Poj0MluqK7+Foi07yrj1C5kVmMXEXXE9Z6IopGFRLNkOS27YfitDgBM2ni0tRv czwtVfEJboAYUBDhCCUJuXhv/bgHFShTobRny0rjIy+zMF5YoWPsUJhLndmU8Ew09NjD 2M70rekwtZ35zEzYMo9nI1Km2dpHdDr/7TZ0XvphCEqhjI6FvFKQtxpx1SbgxQWbtME2 ccmg== X-Gm-Message-State: AC+VfDxPj7bumtM6MJhNPQuw9XXv3HCloi7WmlZaUesWdBSPCApnUmyK vivFcsg+lu9Ke+IUdOqiHoTn4yY/KC9iWcuJ5aCVPVdom42h9TM0OiC5G7hIu3kW7BYzUYIobjQ t5YCJ/7ErW54Rps2ufRaNc4STrknzQQ== X-Received: by 2002:adf:ed49:0:b0:30a:f0a0:90ad with SMTP id u9-20020adfed49000000b0030af0a090admr8078827wro.25.1686060733027; Tue, 06 Jun 2023 07:12:13 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ4vrbP27Lzb1f5zAmJmhi4nWSPGUM+cgkVDlQQNeDMjrPvI4trel/Mm9rXf0owCKheX9sVbYQ== X-Received: by 2002:adf:ed49:0:b0:30a:f0a0:90ad with SMTP id u9-20020adfed49000000b0030af0a090admr8078806wro.25.1686060732619; Tue, 06 Jun 2023 07:12:12 -0700 (PDT) Received: from localhost (11.72.115.87.dyn.plus.net. [87.115.72.11]) by smtp.gmail.com with ESMTPSA id n10-20020a5d420a000000b003062b6a522bsm12631660wrq.96.2023.06.06.07.12.12 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 06 Jun 2023 07:12:12 -0700 (PDT) From: Andrew Burgess To: Pedro Alves , gdb-patches@sourceware.org Subject: Re: [PATCH 10/31] Thread options & clone events (Linux GDBserver) In-Reply-To: <20221212203101.1034916-11-pedro@palves.net> References: <20221212203101.1034916-1-pedro@palves.net> <20221212203101.1034916-11-pedro@palves.net> Date: Tue, 06 Jun 2023 15:12:11 +0100 Message-ID: <87h6rk3cjo.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.8 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_NONE,TXREP,T_SCC_BODY_TEXT_LINE 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: > This patch teaches the Linux GDBserver backend to report clone events > to GDB, when GDB has requested them with the GDB_THREAD_OPTION_CLONE > thread option, via the new QThreadOptions packet. > > This shuffles code in linux_process_target::handle_extended_wait > around to a more logical order when we now have to handle and > potentially report all of fork/vfork/clone. > > Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=19675 > Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=27830 > Change-Id: I3a19bc98801ec31e5c6fdbe1ebe17df855142bb2 > --- > gdbserver/linux-low.cc | 235 ++++++++++++++++++++++------------------- > gdbserver/linux-low.h | 2 + > 2 files changed, 129 insertions(+), 108 deletions(-) > > diff --git a/gdbserver/linux-low.cc b/gdbserver/linux-low.cc > index 6f96e16d6f0..d755cda0e44 100644 > --- a/gdbserver/linux-low.cc > +++ b/gdbserver/linux-low.cc > @@ -491,7 +491,6 @@ linux_process_target::handle_extended_wait (lwp_info **orig_event_lwp, > struct lwp_info *event_lwp = *orig_event_lwp; > int event = linux_ptrace_get_extended_event (wstat); > struct thread_info *event_thr = get_lwp_thread (event_lwp); > - struct lwp_info *new_lwp; > > gdb_assert (event_lwp->waitstatus.kind () == TARGET_WAITKIND_IGNORE); > > @@ -503,7 +502,6 @@ linux_process_target::handle_extended_wait (lwp_info **orig_event_lwp, > if ((event == PTRACE_EVENT_FORK) || (event == PTRACE_EVENT_VFORK) > || (event == PTRACE_EVENT_CLONE)) > { > - ptid_t ptid; > unsigned long new_pid; > int ret, status; > > @@ -527,61 +525,65 @@ linux_process_target::handle_extended_wait (lwp_info **orig_event_lwp, > warning ("wait returned unexpected status 0x%x", status); > } > > - if (event == PTRACE_EVENT_FORK || event == PTRACE_EVENT_VFORK) > + if (debug_threads) > { > - struct process_info *parent_proc; > - struct process_info *child_proc; > - struct lwp_info *child_lwp; > - struct thread_info *child_thr; > + debug_printf ("HEW: Got %s event from LWP %ld, new child is %ld\n", > + (event == PTRACE_EVENT_FORK ? "fork" > + : event == PTRACE_EVENT_VFORK ? "vfork" > + : event == PTRACE_EVENT_CLONE ? "clone" > + : "???"), > + ptid_of (event_thr).lwp (), > + new_pid); > + } > + > + ptid_t child_ptid = (event != PTRACE_EVENT_CLONE > + ? ptid_t (new_pid, new_pid) > + : ptid_t (ptid_of (event_thr).pid (), new_pid)); > > - ptid = ptid_t (new_pid, new_pid); > + lwp_info *child_lwp = add_lwp (child_ptid); > + gdb_assert (child_lwp != NULL); > + child_lwp->stopped = 1; > + if (event != PTRACE_EVENT_CLONE) > + child_lwp->must_set_ptrace_flags = 1; > + child_lwp->status_pending_p = 0; > > - threads_debug_printf ("Got fork event from LWP %ld, " > - "new child is %d", > - ptid_of (event_thr).lwp (), > - ptid.pid ()); > + thread_info *child_thr = get_lwp_thread (child_lwp); > > + /* If we're suspending all threads, leave this one suspended > + too. If the fork/clone parent is stepping over a breakpoint, > + all other threads have been suspended already. Leave the > + child suspended too. */ > + if (stopping_threads == STOPPING_AND_SUSPENDING_THREADS > + || event_lwp->bp_reinsert != 0) > + { > + threads_debug_printf ("leaving child suspended"); > + child_lwp->suspended = 1; > + } > + > + if (event_lwp->bp_reinsert != 0 > + && supports_software_single_step () > + && event == PTRACE_EVENT_VFORK) > + { > + /* If we leave single-step breakpoints there, child will > + hit it, so uninsert single-step breakpoints from parent > + (and child). Once vfork child is done, reinsert > + them back to parent. */ > + uninsert_single_step_breakpoints (event_thr); > + } > + > + if (event != PTRACE_EVENT_CLONE) > + { > /* Add the new process to the tables and clone the breakpoint > lists of the parent. We need to do this even if the new process > will be detached, since we will need the process object and the > breakpoints to remove any breakpoints from memory when we > detach, and the client side will access registers. */ > - child_proc = add_linux_process (new_pid, 0); > + process_info *child_proc = add_linux_process (new_pid, 0); > gdb_assert (child_proc != NULL); > - child_lwp = add_lwp (ptid); > - gdb_assert (child_lwp != NULL); > - child_lwp->stopped = 1; > - child_lwp->must_set_ptrace_flags = 1; > - child_lwp->status_pending_p = 0; > - child_thr = get_lwp_thread (child_lwp); > - child_thr->last_resume_kind = resume_stop; > - child_thr->last_status.set_stopped (GDB_SIGNAL_0); > - > - /* If we're suspending all threads, leave this one suspended > - too. If the fork/clone parent is stepping over a breakpoint, > - all other threads have been suspended already. Leave the > - child suspended too. */ > - if (stopping_threads == STOPPING_AND_SUSPENDING_THREADS > - || event_lwp->bp_reinsert != 0) > - { > - threads_debug_printf ("leaving child suspended"); > - child_lwp->suspended = 1; > - } > > - parent_proc = get_thread_process (event_thr); > + process_info *parent_proc = get_thread_process (event_thr); > child_proc->attached = parent_proc->attached; > > - if (event_lwp->bp_reinsert != 0 > - && supports_software_single_step () > - && event == PTRACE_EVENT_VFORK) > - { > - /* If we leave single-step breakpoints there, child will > - hit it, so uninsert single-step breakpoints from parent > - (and child). Once vfork child is done, reinsert > - them back to parent. */ > - uninsert_single_step_breakpoints (event_thr); > - } > - > clone_all_breakpoints (child_thr, event_thr); > > target_desc_up tdesc = allocate_target_description (); > @@ -590,88 +592,97 @@ linux_process_target::handle_extended_wait (lwp_info **orig_event_lwp, > > /* Clone arch-specific process data. */ > low_new_fork (parent_proc, child_proc); > + } > > - /* Save fork info in the parent thread. */ > - if (event == PTRACE_EVENT_FORK) > - event_lwp->waitstatus.set_forked (ptid); > - else if (event == PTRACE_EVENT_VFORK) > - event_lwp->waitstatus.set_vforked (ptid); > - > + /* Save fork/clone info in the parent thread. */ > + if (event == PTRACE_EVENT_FORK) > + event_lwp->waitstatus.set_forked (child_ptid); > + else if (event == PTRACE_EVENT_VFORK) > + event_lwp->waitstatus.set_vforked (child_ptid); > + else if (event == PTRACE_EVENT_CLONE > + && (event_thr->thread_options & GDB_THREAD_OPTION_CLONE) != 0) > + event_lwp->waitstatus.set_thread_cloned (child_ptid); > + > + if (event != PTRACE_EVENT_CLONE > + || (event_thr->thread_options & GDB_THREAD_OPTION_CLONE) != 0) > + { > /* The status_pending field contains bits denoting the > - extended event, so when the pending event is handled, > - the handler will look at lwp->waitstatus. */ > + extended event, so when the pending event is handled, the > + handler will look at lwp->waitstatus. */ > event_lwp->status_pending_p = 1; > event_lwp->status_pending = wstat; > > - /* Link the threads until the parent event is passed on to > - higher layers. */ > + /* Link the threads until the parent's event is passed on to > + GDB. */ > event_lwp->fork_relative = child_lwp; > child_lwp->fork_relative = event_lwp; > - > - /* If the parent thread is doing step-over with single-step > - breakpoints, the list of single-step breakpoints are cloned > - from the parent's. Remove them from the child process. > - In case of vfork, we'll reinsert them back once vforked > - child is done. */ > - if (event_lwp->bp_reinsert != 0 > - && supports_software_single_step ()) > - { > - /* The child process is forked and stopped, so it is safe > - to access its memory without stopping all other threads > - from other processes. */ > - delete_single_step_breakpoints (child_thr); > - > - gdb_assert (has_single_step_breakpoints (event_thr)); > - gdb_assert (!has_single_step_breakpoints (child_thr)); > - } > - > - /* Report the event. */ > - return 0; > } > > - threads_debug_printf > - ("Got clone event from LWP %ld, new child is LWP %ld", > - lwpid_of (event_thr), new_pid); > - > - ptid = ptid_t (pid_of (event_thr), new_pid); > - new_lwp = add_lwp (ptid); > - > - /* Either we're going to immediately resume the new thread > - or leave it stopped. resume_one_lwp is a nop if it > - thinks the thread is currently running, so set this first > - before calling resume_one_lwp. */ > - new_lwp->stopped = 1; > + /* If the parent thread is doing step-over with single-step > + breakpoints, the list of single-step breakpoints are cloned > + from the parent's. Remove them from the child process. > + In case of vfork, we'll reinsert them back once vforked > + child is done. */ > + if (event_lwp->bp_reinsert != 0 > + && supports_software_single_step ()) > + { > + /* The child process is forked and stopped, so it is safe > + to access its memory without stopping all other threads > + from other processes. */ > + delete_single_step_breakpoints (child_thr); > > - /* If we're suspending all threads, leave this one suspended > - too. If the fork/clone parent is stepping over a breakpoint, > - all other threads have been suspended already. Leave the > - child suspended too. */ > - if (stopping_threads == STOPPING_AND_SUSPENDING_THREADS > - || event_lwp->bp_reinsert != 0) > - new_lwp->suspended = 1; > + gdb_assert (has_single_step_breakpoints (event_thr)); > + gdb_assert (!has_single_step_breakpoints (child_thr)); > + } > > /* Normally we will get the pending SIGSTOP. But in some cases > we might get another signal delivered to the group first. > If we do get another signal, be sure not to lose it. */ > if (WSTOPSIG (status) != SIGSTOP) > { > - new_lwp->stop_expected = 1; > - new_lwp->status_pending_p = 1; > - new_lwp->status_pending = status; > + child_lwp->stop_expected = 1; > + child_lwp->status_pending_p = 1; > + child_lwp->status_pending = status; > } > - else if (cs.report_thread_events) > + else if (event == PTRACE_EVENT_CLONE && cs.report_thread_events) > { > - new_lwp->waitstatus.set_thread_created (); > - new_lwp->status_pending_p = 1; > - new_lwp->status_pending = status; > + child_lwp->waitstatus.set_thread_created (); > + child_lwp->status_pending_p = 1; > + child_lwp->status_pending = status; > } > > + if (event == PTRACE_EVENT_CLONE) > + { > #ifdef USE_THREAD_DB > - thread_db_notice_clone (event_thr, ptid); > + thread_db_notice_clone (event_thr, child_ptid); > #endif > + } > > - /* Don't report the event. */ > - return 1; > + if (event == PTRACE_EVENT_CLONE > + && (event_thr->thread_options & GDB_THREAD_OPTION_CLONE) == 0) > + { > + threads_debug_printf > + ("not reporting clone event from LWP %ld, new child is %ld\n", > + ptid_of (event_thr).lwp (), > + new_pid); > + return 1; > + } > + > + /* Leave the child stopped until GDB processes the parent > + event. */ > + child_thr->last_resume_kind = resume_stop; > + child_thr->last_status.set_stopped (GDB_SIGNAL_0); > + > + /* Report the event. */ > + threads_debug_printf > + ("reporting %s event from LWP %ld, new child is %ld\n", > + (event == PTRACE_EVENT_FORK ? "fork" > + : event == PTRACE_EVENT_VFORK ? "vfork" > + : event == PTRACE_EVENT_CLONE ? "clone" > + : "???"), > + ptid_of (event_thr).lwp (), > + new_pid); > + return 0; > } > else if (event == PTRACE_EVENT_VFORK_DONE) > { > @@ -3527,7 +3538,8 @@ linux_process_target::wait_1 (ptid_t ptid, target_waitstatus *ourstatus, > > /* Break the unreported fork relationship chain. */ > if (event_child->waitstatus.kind () == TARGET_WAITKIND_FORKED > - || event_child->waitstatus.kind () == TARGET_WAITKIND_VFORKED) > + || event_child->waitstatus.kind () == TARGET_WAITKIND_VFORKED > + || event_child->waitstatus.kind () == TARGET_WAITKIND_THREAD_CLONED) Can we not use: if (is_new_child_status (event_child->waitstatus.kind ())) here? I almost asked here: do we really need to check for THREAD_CLONED here, given the body of the `if` only changes the `fork_relative` field, which surely is only related to forks, right? ... but then I dug deeper and spotted that the field also used for clone events now, so: I wonder if we should rename `fork_relative` to something better, maybe `fork_clone_relative`, or maybe go generic with something like `related_lwp`? It just seemed a little confusing (to me). But otherwise, this patch looks good. Reviewed-By: Andrew Burgess Thanks, Andrew > { > event_child->fork_relative->fork_relative = NULL; > event_child->fork_relative = NULL; > @@ -4263,15 +4275,14 @@ linux_set_resume_request (thread_info *thread, thread_resume *resume, size_t n) > continue; > } > > - /* Don't let wildcard resumes resume fork children that GDB > - does not yet know are new fork children. */ > + /* Don't let wildcard resumes resume fork/vfork/clone > + children that GDB does not yet know are new children. */ > if (lwp->fork_relative != NULL) > { > struct lwp_info *rel = lwp->fork_relative; > > if (rel->status_pending_p > - && (rel->waitstatus.kind () == TARGET_WAITKIND_FORKED > - || rel->waitstatus.kind () == TARGET_WAITKIND_VFORKED)) > + && is_new_child_status (rel->waitstatus.kind ())) > { > threads_debug_printf > ("not resuming LWP %ld: has queued stop reply", > @@ -5894,6 +5905,14 @@ linux_process_target::supports_vfork_events () > return true; > } > > +/* Return the set of supported thread options. */ > + > +gdb_thread_options > +linux_process_target::supported_thread_options () > +{ > + return GDB_THREAD_OPTION_CLONE; > +} > + > /* Check if exec events are supported. */ > > bool > diff --git a/gdbserver/linux-low.h b/gdbserver/linux-low.h > index 1594f063f47..69a34fb96fc 100644 > --- a/gdbserver/linux-low.h > +++ b/gdbserver/linux-low.h > @@ -234,6 +234,8 @@ class linux_process_target : public process_stratum_target > > bool supports_vfork_events () override; > > + gdb_thread_options supported_thread_options () override; > + > bool supports_exec_events () override; > > void handle_new_gdb_connection () override; > -- > 2.36.0