From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id mIecEXrK5GD4TAAAWB0awg (envelope-from ) for ; Tue, 06 Jul 2021 17:26:18 -0400 Received: by simark.ca (Postfix, from userid 112) id 445961F1F4; Tue, 6 Jul 2021 17:26:17 -0400 (EDT) X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-1.1 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,MAILING_LIST_MULTI,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.2 Received: from sourceware.org (server2.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id 08EB11E54D for ; Tue, 6 Jul 2021 17:26:16 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 57B1138930D9 for ; Tue, 6 Jul 2021 21:26:15 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 57B1138930D9 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1625606775; bh=fwLNTIHbyh4V55sxE5IpbQ1Y9qs4eKVdDGhunIF/Rdc=; h=Subject:To:References:Date:In-Reply-To:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To: From; b=ao8Yvomp6az0pAHDUMNAK0FszaUTn7tQqFImeOQ7pwzZZyr2+TwGmuvt62hoOADFH g+Rlu1GXSCYhvR4cXZAECeXXLSn46/Qi+AdLClxmElnaTU09/88MWIo0As2C3nDt6o gfeRltodMCeR9gkJNjnY8sPMFtqQRst8gdvwtWn0= Received: from smtp.polymtl.ca (smtp.polymtl.ca [132.207.4.11]) by sourceware.org (Postfix) with ESMTPS id 6B2143861035 for ; Tue, 6 Jul 2021 21:25:55 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 6B2143861035 Received: from simark.ca (simark.ca [158.69.221.121]) (authenticated bits=0) by smtp.polymtl.ca (8.14.7/8.14.7) with ESMTP id 166LPmen005778 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Tue, 6 Jul 2021 17:25:53 -0400 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp.polymtl.ca 166LPmen005778 Received: from [10.0.0.11] (192-222-157-6.qc.cable.ebox.net [192.222.157.6]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 2E21E1E54D; Tue, 6 Jul 2021 17:25:48 -0400 (EDT) Subject: Re: [PATCH 07/11] gdb: maintain per-process-target list of resumed threads with pending wait status To: Pedro Alves , gdb-patches@sourceware.org References: <20210622165704.2404007-1-simon.marchi@polymtl.ca> <20210622165704.2404007-8-simon.marchi@polymtl.ca> <52c8488e-fef2-a1fe-d150-50d510bedbe1@palves.net> Message-ID: <252f2a84-c7b4-d07a-fecc-c57685a89ee1@polymtl.ca> Date: Tue, 6 Jul 2021 17:25:47 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 MIME-Version: 1.0 In-Reply-To: <52c8488e-fef2-a1fe-d150-50d510bedbe1@palves.net> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Tue, 6 Jul 2021 21:25:48 +0000 X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Simon Marchi via Gdb-patches Reply-To: Simon Marchi Errors-To: gdb-patches-bounces+public-inbox=simark.ca@sourceware.org Sender: "Gdb-patches" >> In set_thread_exited, we try to remove the thread from the list, because >> keeping an exited thread in that list would make no sense (especially if >> the thread is freed). My first implementation assumed that a process >> stratum target was always present when set_thread_exited is called. >> That's however, not the case: in some cases, targets unpush themselves >> from an inferior and then call "exit_inferior", which exits all the >> threads. If the target is unpushed before set_thread_exited is called >> on the threads, it means we could mistakenly leave some threads in the >> list. I tried to see how hard it would be to make it such that targets >> have to exit all threads before unpushing themselves from the inferior >> (that would seem logical to me, we don't want threads belonging to an >> inferior that has no process target). That seem quite difficult and not >> worth the time. Instead, I changed inferior::unpush_target to remove an >> threads of that inferior from the list. > > "remove an threads" -> "remove all threads" ? Yes, fixed. >> @@ -470,6 +469,10 @@ class thread_info : public refcounted_object, >> linked. */ >> intrusive_list_node step_over_list_node; >> >> + /* Node for list of threads that are resumed and have a pending wait >> + status. */ > > Maybe mention that all threads in list list belong to the same > process_stratum_target ? Sounds good, changed to: /* Node for list of threads that are resumed and have a pending wait status. The list head for this is in process_stratum_target, hence all threads in this list belong to that process target. */ intrusive_list_node resumed_with_pending_wait_status_node; >> @@ -89,6 +89,25 @@ inferior::inferior (int pid_) >> m_target_stack.push (get_dummy_target ()); >> } >> >> +/* See inferior.h. */ >> + >> +int >> +inferior::unpush_target (struct target_ops *t) >> +{ >> + /* If unpushing the process stratum target while threads exists, ensure that > > "threads exists" -> "threads exist" Done. >> + we don't leave any threads of this inferior in the target's "resumed with >> + pending wait status" list. */ >> + if (t->stratum () == process_stratum) >> + { >> + process_stratum_target *proc_target = as_process_stratum_target (t); >> + >> + for (thread_info *thread : this->non_exited_threads ()) >> + proc_target->maybe_remove_resumed_with_pending_wait_status (thread); > > Note the target_pid_to_str call inside maybe_remove_resumed_with_pending_wait_status > adds back a dependency on current_inferior. Arggg, we don't want that. Since that is in a process_stratum_target method, I'll change the message to call the pid_to_str method of the current object instead: infrun_debug_printf ("removing from resumed threads with event list: %s", this->pid_to_str (thread->ptid).c_str ()); Does that sound good? >> --- a/gdb/process-stratum-target.c >> +++ b/gdb/process-stratum-target.c >> @@ -106,6 +106,40 @@ process_stratum_target::follow_exec (inferior *follow_inf, ptid_t ptid, >> >> /* See process-stratum-target.h. */ >> >> +void >> +process_stratum_target::maybe_add_resumed_with_pending_wait_status >> + (thread_info *thread) >> +{ >> + gdb_assert (!thread->resumed_with_pending_wait_status_node.is_linked ()); >> + >> + if (thread->resumed () && thread->has_pending_waitstatus ()) >> + { >> + infrun_debug_printf ("adding to resumed threads with event list: %s", >> + target_pid_to_str (thread->ptid).c_str ()); > > This here too. Not 100% sure this target call is always done > with the right target stack selected. I'd make the same change to use this->pid_to_str here too. > >> + m_resumed_with_pending_wait_status.push_back (*thread); >> + } >> +} >> + >> +/* See process-stratum-target.h. */ >> + > >> + >> +private: >> + /* List of threads managed by this target which simultaneously are resumed >> + and have a pending wait status. */ > > I'd suggest expanding this comment a little to mention this > is done for optimization reasons, to avoid walking > thread lists, something like that. Or maybe say that in > the thread_info node. Or both places. Done: /* List of threads managed by this target which simultaneously are resumed and have a pending wait status. This is done for optimization reasons, it would be possible to walk the inferior thread lists to find these threads. But since this is something we need to do quite frequently in the hot path, maintaining this list avoids walking the thread lists repeatedly. */ I would prefer to avoid repeating the same thing at two places, because it's a recipe for the two places getting out of sync. Since the node comment in thread_info now points to here (says the list head is in process_stratum_target), people looking for more information about this list should have no problem finding the comment here. > >> + thread_info_resumed_with_pending_wait_status_list >> + m_resumed_with_pending_wait_status; >> }; >> >> /* Downcast TARGET to process_stratum_target. */ >> diff --git a/gdb/thread.c b/gdb/thread.c >> index 289d33c74c3b..26974e1b8cbc 100644 >> --- a/gdb/thread.c >> +++ b/gdb/thread.c >> @@ -188,6 +188,10 @@ set_thread_exited (thread_info *tp, bool silent) >> >> if (tp->state != THREAD_EXITED) >> { >> + process_stratum_target *proc_target = tp->inf->process_target (); >> + if (proc_target != nullptr) > > I think this check needs a comment. Done: /* Some targets unpush themselves from the inferior's target stack before clearing the inferior's thread list (which marks all threads as exited, and therefore leads to this function). In this case, the inferior's process target will be nullptr when we arrive here. See also the comment in inferior::unpush_target. */ And I also added a cross-reference to here from the comment in inferior::unpush_target. >> + /* If we transition from not resumed to resumed, we might need to add >> + the thread to the resumed threads with pending statuses list. */ >> + if (resumed) >> + proc_target->maybe_add_resumed_with_pending_wait_status (this); > > Longest function name award goes to... ;-) Indeed! If you have a name that is shorter but just as clear, I'm open for suggestion. But I prefer names that are non-ambiguous and use the right terminology over names that are short just for convenience's sake. Thanks! Simon