From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id kB8KKHUq42AyYwAAWB0awg (envelope-from ) for ; Mon, 05 Jul 2021 11:51:17 -0400 Received: by simark.ca (Postfix, from userid 112) id 948711F1F2; Mon, 5 Jul 2021 11:51: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=-0.6 required=5.0 tests=MAILING_LIST_MULTI, RDNS_DYNAMIC autolearn=ham autolearn_force=no version=3.4.2 Received: from sourceware.org (ip-8-43-85-97.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 954B91E940 for ; Mon, 5 Jul 2021 11:51:16 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 4A95A383F421 for ; Mon, 5 Jul 2021 15:51:16 +0000 (GMT) Received: from mail-wr1-f46.google.com (mail-wr1-f46.google.com [209.85.221.46]) by sourceware.org (Postfix) with ESMTPS id 3DC50385700B for ; Mon, 5 Jul 2021 15:51:05 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 3DC50385700B Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=palves.net Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com Received: by mail-wr1-f46.google.com with SMTP id t6so12696586wrm.9 for ; Mon, 05 Jul 2021 08:51:05 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:from:to:references:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=rSq6RcqZbEOIUnKVq7kqsMpbeX77a68uDd12nQFiCzg=; b=MmkVA23W11M1CLzUan/kxPX2eJm+4g4ZanFvRJ1guUjEHt5KqQPi4xPXvsE99aD8bw GaQPOMM/DnypV1jBiS26MrBDJUu+Tw6iAqeg3gzeHaIyjsNkY6inm5N4WoPw6Q8CaIzD pYNE1uHi8n2w537t3PbmoPHk67+Xv1hUSWYjUb1ZK9RJOX1co+RT0ZegWLFK5eBUnSzW ldvYkj3zQkPdFdeVDpKGKIXPrbpRWKwjW9opttGbDX1I4qSroBAWn9jsZ/bQzSJv+lIG FPSZljeeks8JCcPawRlg4WRsM9QsBTT2NpGe3YpupCQHmqQRZ/bUabmmBkWyH/LGRjQ1 RITw== X-Gm-Message-State: AOAM530c+UfJZHMCRKmBFa/3LsgKoMae8kDLl7sAS2M1Hv4Mc4upLASp HqTnHXudVBAmR4y7pRvJLFAcR11B5Pih8g== X-Google-Smtp-Source: ABdhPJzfHvGBzXguttsWY2sEHo9po1M4Pbk7WqV6Zf4caHZ2Ns6Fzo+LitAH7+g22TWz9mwpbdRUzQ== X-Received: by 2002:a5d:59ad:: with SMTP id p13mr8956413wrr.25.1625500263717; Mon, 05 Jul 2021 08:51:03 -0700 (PDT) Received: from ?IPv6:2001:8a0:f932:6a00:46bc:d03b:7b3a:2227? ([2001:8a0:f932:6a00:46bc:d03b:7b3a:2227]) by smtp.gmail.com with ESMTPSA id z4sm12583900wmf.9.2021.07.05.08.51.02 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 05 Jul 2021 08:51:02 -0700 (PDT) Subject: Re: [PATCH 07/11] gdb: maintain per-process-target list of resumed threads with pending wait status From: Pedro Alves To: Simon Marchi , gdb-patches@sourceware.org References: <20210622165704.2404007-1-simon.marchi@polymtl.ca> <20210622165704.2404007-8-simon.marchi@polymtl.ca> Message-ID: <52c8488e-fef2-a1fe-d150-50d510bedbe1@palves.net> Date: Mon, 5 Jul 2021 16:51:01 +0100 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: <20210622165704.2404007-8-simon.marchi@polymtl.ca> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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: , Errors-To: gdb-patches-bounces+public-inbox=simark.ca@sourceware.org Sender: "Gdb-patches" Hi! This LGTM. Some comments, typos and minor things to address below. On 2021-06-22 5:57 p.m., Simon Marchi via Gdb-patches wrote: > 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" ? > > diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h > index 5ea08a13ee5f..47d7f40eaa08 100644 > --- a/gdb/gdbthread.h > +++ b/gdb/gdbthread.h > @@ -296,8 +296,7 @@ class thread_info : public refcounted_object, > bool resumed () const > { return m_resumed; } > > - void set_resumed (bool resumed) > - { m_resumed = resumed; } > + void set_resumed (bool resumed); > > /* Frontend view of the thread state. Note that the THREAD_RUNNING/ > THREAD_STOPPED states are different from EXECUTING. When the > @@ -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 ? > + intrusive_list_node resumed_with_pending_wait_status_node; > + > --- a/gdb/inferior.c > +++ b/gdb/inferior.c > @@ -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" > + 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. > + } > + > + return m_target_stack.unpush (t); > +} > + > --- 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. > + 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. > + 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. > + proc_target->maybe_remove_resumed_with_pending_wait_status (tp); > + > gdb::observers::thread_exit.notify (tp, silent); > > /* Tag it as exited. */ > @@ -295,6 +299,29 @@ thread_info::deletable () const > > /* See gdbthread.h. */ > > +void > +thread_info::set_resumed (bool resumed) > +{ > + if (resumed == m_resumed) > + return; > + > + process_stratum_target *proc_target = this->inf->process_target (); > + > + /* If we transition from resumed to not resumed, we might need to remove > + the thread from the resumed threads with pending statuses list. */ > + if (!resumed) > + proc_target->maybe_remove_resumed_with_pending_wait_status (this); > + > + m_resumed = resumed; > + > + /* 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... ;-)