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 DC5763857B9B for ; Wed, 7 Jun 2023 16:11:09 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org DC5763857B9B 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=1686154269; 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=hUrsSLaklFcSRNg73/bwqZXGFTaQ5Bk7Pye8sxUP96o=; b=glbOcZbecOxJnGrfp/sNvQnk3aVjUK9twDzcM3cDW4WISik5MMRkf2xYjq3l//PwacfqDh 53d/7CUWmVyA/x1f7P/Pr2grOAhduBra5IqzYudqyJ9YiPaX9xxE5bagXmN1Ku+TMtaIR7 B1cW85MXCVjaMBXsLPeQFQ9bXBuCH3U= 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-158-wVSaF5vQP9WsRRZwD6sx1w-1; Wed, 07 Jun 2023 12:11:08 -0400 X-MC-Unique: wVSaF5vQP9WsRRZwD6sx1w-1 Received: by mail-wr1-f70.google.com with SMTP id ffacd0b85a97d-30793c16c78so10339137f8f.3 for ; Wed, 07 Jun 2023 09:11:03 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1686154260; x=1688746260; 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=hUrsSLaklFcSRNg73/bwqZXGFTaQ5Bk7Pye8sxUP96o=; b=gRk5TY/BYWW7X/aHSw77VuU3H2wIQBLrBEv2/9FRZEA7mJFRhhudgcpiJ9YzNAhuML oaz9spIfF8IKp0D5pgUbor+P07iFYJ4e2My4FSUO/Hy43YR0JhPH3EnomDzVkOJ8B/ka Nxzfzlj0MKeZQtoUWpv6wYHRy9avrrzpe3sd3jSpESgmPhk4JwGGPWOd7mAD1FbAECwV 2O0MCRN0LELlysz2ASjYf+QbHcd5Ij1DIA9+p9IRCZjK7/Lc+1YyVDXmACpoqK+mYBE7 Iu5Dv0mV19qF7ZQh89vcCu9/dZvNjVcp7V2p3Trp1IpH2kZ7F3XQH7pc82QbYXunf4+t wzMw== X-Gm-Message-State: AC+VfDzZvZ6DkaFMY+r9rddhPVDJWQrwVR9SQ1ryEKmM7dzedpXswCRj vwntZzUQU2PPU+mqxG9zAtClcGIKE2QqruvDkAuA5CTeaPmzW6kkHIYHb+fAFfzFiFlCSrNqsKL GcaTmVc6NexAo+bchz2wKeiXd0SjaAg== X-Received: by 2002:a5d:6801:0:b0:30a:e619:3a71 with SMTP id w1-20020a5d6801000000b0030ae6193a71mr4660206wru.23.1686154260419; Wed, 07 Jun 2023 09:11:00 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ4pFTVoN6YdaKfrsj6r4eGomsVKB/LIT8QeZB0eTh+FVnz0NrS8Bq6kxI+a52b44TMRvZmucA== X-Received: by 2002:a5d:6801:0:b0:30a:e619:3a71 with SMTP id w1-20020a5d6801000000b0030ae6193a71mr4660187wru.23.1686154260027; Wed, 07 Jun 2023 09:11:00 -0700 (PDT) Received: from localhost (11.72.115.87.dyn.plus.net. [87.115.72.11]) by smtp.gmail.com with ESMTPSA id f9-20020a056000036900b0030c2e3c7fb3sm15781262wrf.101.2023.06.07.09.10.59 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 07 Jun 2023 09:10:59 -0700 (PDT) From: Andrew Burgess To: Pedro Alves , gdb-patches@sourceware.org Subject: Re: [PATCH 11/31] gdbserver: Hide and don't detach pending clone children In-Reply-To: <20221212203101.1034916-12-pedro@palves.net> References: <20221212203101.1034916-1-pedro@palves.net> <20221212203101.1034916-12-pedro@palves.net> Date: Wed, 07 Jun 2023 17:10:58 +0100 Message-ID: <878rcv1cdp.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 commit extends the logic added by these two commits from a while > ago: > > #1 7b961964f866 (gdbserver: hide fork child threads from GDB), > #2 df5ad102009c (gdb, gdbserver: detach fork child when detaching from fork parent) > > ... to handle thread clone events, which are very similar to (v)fork > events. > > For #1, we want to hide clone children as well, so just update the > comments. > > For #2, unlike (v)fork children, pending clone children aren't full > processes, they're just threads, so don't detach them in > handle_detach. linux-low.cc will take care of detaching them along > with all other threads of the process, there's nothing special that > needs to be done. > > Change-Id: I7f5901d07efda576a2522d03e183994e071b8ffc > --- > gdbserver/linux-low.cc | 5 +++-- > gdbserver/linux-low.h | 36 ++++++++++++++++++++---------------- > gdbserver/server.cc | 12 +++++++----- > gdbserver/target.cc | 3 ++- > gdbserver/target.h | 15 ++++++++------- > 5 files changed, 40 insertions(+), 31 deletions(-) > > diff --git a/gdbserver/linux-low.cc b/gdbserver/linux-low.cc > index d755cda0e44..a7c310260ca 100644 > --- a/gdbserver/linux-low.cc > +++ b/gdbserver/linux-low.cc > @@ -6940,9 +6940,10 @@ linux_process_target::thread_pending_parent (thread_info *thread) > } > > thread_info * > -linux_process_target::thread_pending_child (thread_info *thread) > +linux_process_target::thread_pending_child (thread_info *thread, > + target_waitkind *kind) > { > - lwp_info *child = get_thread_lwp (thread)->pending_child (); > + lwp_info *child = get_thread_lwp (thread)->pending_child (kind); > > if (child == nullptr) > return nullptr; > diff --git a/gdbserver/linux-low.h b/gdbserver/linux-low.h > index 69a34fb96fc..c9f9db71e09 100644 > --- a/gdbserver/linux-low.h > +++ b/gdbserver/linux-low.h > @@ -315,7 +315,8 @@ class linux_process_target : public process_stratum_target > #endif > > thread_info *thread_pending_parent (thread_info *thread) override; > - thread_info *thread_pending_child (thread_info *thread) override; > + thread_info *thread_pending_child (thread_info *thread, > + target_waitkind *kind) override; > > bool supports_catch_syscall () override; > > @@ -734,8 +735,8 @@ struct pending_signal > > struct lwp_info > { > - /* If this LWP is a fork child that wasn't reported to GDB yet, return > - its parent, else nullptr. */ > + /* If this LWP is a fork/vfork/clone child that wasn't reported to > + GDB yet, return its parent, else nullptr. */ > lwp_info *pending_parent () const > { > if (this->fork_relative == nullptr) > @@ -743,10 +744,10 @@ struct lwp_info > > gdb_assert (this->fork_relative->fork_relative == this); > > - /* In a fork parent/child relationship, the parent has a status pending and > - the child does not, and a thread can only be in one such relationship > - at most. So we can recognize who is the parent based on which one has > - a pending status. */ > + /* In a parent/child relationship, the parent has a status pending > + and the child does not, and a thread can only be in one such > + relationship at most. So we can recognize who is the parent > + based on which one has a pending status. */ > gdb_assert (!!this->status_pending_p > != !!this->fork_relative->status_pending_p); > > @@ -756,24 +757,25 @@ struct lwp_info > const target_waitstatus &ws > = this->fork_relative->waitstatus; > gdb_assert (ws.kind () == TARGET_WAITKIND_FORKED > - || ws.kind () == TARGET_WAITKIND_VFORKED); > + || ws.kind () == TARGET_WAITKIND_VFORKED > + || ws.kind () == TARGET_WAITKIND_THREAD_CLONED); > > return this->fork_relative; > } > > - /* If this LWP is the parent of a fork child we haven't reported to GDB yet, > - return that child, else nullptr. */ > - lwp_info *pending_child () const > + /* If this LWP is the parent of a fork/vfork/clone child we haven't > + reported to GDB yet, return that child, else nullptr. */ > + lwp_info *pending_child (target_waitkind *kind) const > { > if (this->fork_relative == nullptr) > return nullptr; > > gdb_assert (this->fork_relative->fork_relative == this); > > - /* In a fork parent/child relationship, the parent has a status pending and > - the child does not, and a thread can only be in one such relationship > - at most. So we can recognize who is the parent based on which one has > - a pending status. */ > + /* In a parent/child relationship, the parent has a status pending > + and the child does not, and a thread can only be in one such > + relationship at most. So we can recognize who is the parent > + based on which one has a pending status. */ > gdb_assert (!!this->status_pending_p > != !!this->fork_relative->status_pending_p); > > @@ -782,8 +784,10 @@ struct lwp_info > > const target_waitstatus &ws = this->waitstatus; > gdb_assert (ws.kind () == TARGET_WAITKIND_FORKED > - || ws.kind () == TARGET_WAITKIND_VFORKED); > + || ws.kind () == TARGET_WAITKIND_VFORKED > + || ws.kind () == TARGET_WAITKIND_THREAD_CLONED); > > + *kind = ws.kind (); > return this->fork_relative; > } > > diff --git a/gdbserver/server.cc b/gdbserver/server.cc > index 5b07c4e4388..07a3319d114 100644 > --- a/gdbserver/server.cc > +++ b/gdbserver/server.cc > @@ -1344,8 +1344,9 @@ handle_detach (char *own_buf) > continue; > > /* Only threads that have a pending fork event. */ > - thread_info *child = target_thread_pending_child (thread); > - if (child == nullptr) > + target_waitkind kind; > + thread_info *child = target_thread_pending_child (thread, &kind); > + if (child == nullptr || kind == TARGET_WAITKIND_THREAD_CLONED) > continue; > > process_info *fork_child_process = get_thread_process (child); > @@ -1765,9 +1766,10 @@ handle_qxfer_threads_worker (thread_info *thread, struct buffer *buffer) > gdb_byte *handle; > bool handle_status = target_thread_handle (ptid, &handle, &handle_len); > > - /* If this is a fork or vfork child (has a fork parent), GDB does not yet > - know about this process, and must not know about it until it gets the > - corresponding (v)fork event. Exclude this thread from the list. */ > + /* If this is a (v)fork/clone child (has a (v)fork/clone parent), > + GDB does not yet know about this thread, and must not know about > + it until it gets the corresponding (v)fork/clone event. Exclude > + this thread from the list. */ > if (target_thread_pending_parent (thread) != nullptr) > return; > > diff --git a/gdbserver/target.cc b/gdbserver/target.cc > index 168b843d2ec..4584e9b3a8e 100644 > --- a/gdbserver/target.cc > +++ b/gdbserver/target.cc > @@ -814,7 +814,8 @@ process_stratum_target::thread_pending_parent (thread_info *thread) > } > > thread_info * > -process_stratum_target::thread_pending_child (thread_info *thread) > +process_stratum_target::thread_pending_child (thread_info *thread, > + target_waitkind *kind) > { > return nullptr; > } > diff --git a/gdbserver/target.h b/gdbserver/target.h > index 8a0d9f42f7d..e2e818b130b 100644 > --- a/gdbserver/target.h > +++ b/gdbserver/target.h > @@ -479,13 +479,14 @@ class process_stratum_target > virtual bool thread_handle (ptid_t ptid, gdb_byte **handle, > int *handle_len); > > - /* If THREAD is a fork child that was not reported to GDB, return its parent > - else nullptr. */ > + /* If THREAD is a fork/vfork/clone child that was not reported to > + GDB, return its parent else nullptr. */ > virtual thread_info *thread_pending_parent (thread_info *thread); > > - /* If THREAD is the parent of a fork child that was not reported to GDB, > - return this child, else nullptr. */ > - virtual thread_info *thread_pending_child (thread_info *thread); > + /* If THREAD is the parent of a fork/vfork/clone child that was not > + reported to GDB, return this child, else nullptr. */ > + virtual thread_info *thread_pending_child (thread_info *thread, > + target_waitkind *kind); Should the comment not say what happens to KIND? I think this comment applies throughout this patch where KIND was added as an argument. Otherwise, this looks good. Reviewed-By: Andrew Burgess Thanks, Andrew > > /* Returns true if the target can software single step. */ > virtual bool supports_software_single_step (); > @@ -701,9 +702,9 @@ target_thread_pending_parent (thread_info *thread) > } > > static inline thread_info * > -target_thread_pending_child (thread_info *thread) > +target_thread_pending_child (thread_info *thread, target_waitkind *kind) > { > - return the_target->thread_pending_child (thread); > + return the_target->thread_pending_child (thread, kind); > } > > int read_inferior_memory (CORE_ADDR memaddr, unsigned char *myaddr, int len); > -- > 2.36.0