From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from lndn.lancelotsix.com (vps-42846194.vps.ovh.net [IPv6:2001:41d0:801:2000::2400]) by sourceware.org (Postfix) with ESMTPS id 079143858CDB for ; Wed, 5 Oct 2022 20:49:57 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 079143858CDB Received: from ubuntu.lan (unknown [IPv6:2a02:390:9086::635]) by lndn.lancelotsix.com (Postfix) with ESMTPSA id 9B24980F1F; Wed, 5 Oct 2022 20:49:55 +0000 (UTC) Date: Wed, 5 Oct 2022 20:49:50 +0000 From: Lancelot SIX To: Andrew Burgess Cc: gdb-patches@sourceware.org Subject: Re: [PATCHv2 4/7] gdb: remove the pop_all_targets (and friends) global functions Message-ID: <20221005204950.v6gendcr5rlous7b@ubuntu.lan> References: <20220921131200.3983844-1-aburgess@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.5.11 (lndn.lancelotsix.com [0.0.0.0]); Wed, 05 Oct 2022 20:49:55 +0000 (UTC) X-Spam-Status: No, score=-10.0 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_SBL_CSS, SPF_HELO_NONE, SPF_PASS, 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 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: , X-List-Received-Date: Wed, 05 Oct 2022 20:49:59 -0000 Hi, I have a minor comment inline below. On Sun, Oct 02, 2022 at 06:04:45PM +0100, Andrew Burgess via Gdb-patches wrote: > This commit removes the global functions pop_all_targets, > pop_all_targets_above, and pop_all_targets_at_and_above, and makes > them methods on the inferior class. > > As the pop_all_targets functions will unpush each target, which > decrements the targets reference count, it is possible that the target > might be closed. > > Right now, closing a target, in some cases, depends on the current > inferior being set correctly, that is, to the inferior from which the > target was popped. > > To facilitate this I have used switch_to_inferior_no_thread within the > new methods. Previously it was the responsibility of the caller to > ensure that the correct inferior was selected. > > In a couple of places (event-top.c and top.c) I have been able to > remove a previous switch_to_inferior_no_thread call. > > In remote_unpush_target (remote.c) I have left the > switch_to_inferior_no_thread call as it is required for the > generic_mourn_inferior call. > --- > gdb/event-top.c | 3 +-- > gdb/inferior.c | 40 ++++++++++++++++++++++++++++++++++++++ > gdb/inferior.h | 20 +++++++++++++++++++ > gdb/remote.c | 2 +- > gdb/scoped-mock-context.h | 2 +- > gdb/target.c | 41 +-------------------------------------- > gdb/target.h | 11 ----------- > gdb/top.c | 3 +-- > 8 files changed, 65 insertions(+), 57 deletions(-) > > diff --git a/gdb/event-top.c b/gdb/event-top.c > index 88c53b720a9..836e6b935e3 100644 > --- a/gdb/event-top.c > +++ b/gdb/event-top.c > @@ -1293,10 +1293,9 @@ async_disconnect (gdb_client_data arg) > > for (inferior *inf : all_inferiors ()) > { > - switch_to_inferior_no_thread (inf); > try > { > - pop_all_targets (); > + inf->pop_all_targets (); > } > catch (const gdb_exception &exception) > { > diff --git a/gdb/inferior.c b/gdb/inferior.c > index 7eb2bd97907..2014bf926b7 100644 > --- a/gdb/inferior.c > +++ b/gdb/inferior.c > @@ -103,6 +103,46 @@ inferior::unpush_target (struct target_ops *t) > return m_target_stack.unpush (t); > } > > +/* See inferior.h. */ > + > +void inferior::unpush_target_and_assert (struct target_ops *target) > +{ > + gdb_assert (current_inferior () == this); > + > + if (!unpush_target (target)) > + internal_error (__FILE__, __LINE__, > + "pop_all_targets couldn't find target %s\n", > + target->shortname ()); > +} > + > +/* See inferior.h. */ > + > +void inferior::pop_all_targets_above (enum strata stratum) > +{ > + /* Unpushing a target might cause it to close. Some targets currently > + rely on the current_inferior being set for their ::close method, so we > + temporarily switch inferior now. */ > + scoped_restore_current_pspace_and_thread restore_pspace_and_thread; > + switch_to_inferior_no_thread (this); > + > + while ((int) (top_target ()->stratum ()) > (int) stratum) Seems to me that the casts to int are not necessary here. Any reason to keep those? > + unpush_target_and_assert (top_target ()); > +} > + > +/* See inferior.h. */ > + > +void inferior::pop_all_targets_at_and_above (enum strata stratum) > +{ > + /* Unpushing a target might cause it to close. Some targets currently > + rely on the current_inferior being set for their ::close method, so we > + temporarily switch inferior now. */ > + scoped_restore_current_pspace_and_thread restore_pspace_and_thread; > + switch_to_inferior_no_thread (this); > + > + while ((int) (top_target ()->stratum ()) >= (int) stratum) Same here. Best, Lancelot. > + unpush_target_and_assert (top_target ()); > +} > + > void > inferior::set_tty (std::string terminal_name) > { > diff --git a/gdb/inferior.h b/gdb/inferior.h > index bb3aec1e8f8..344974c4811 100644 > --- a/gdb/inferior.h > +++ b/gdb/inferior.h > @@ -380,6 +380,22 @@ class inferior : public refcounted_object, > target_ops *top_target () > { return m_target_stack.top (); } > > + /* Unpush all targets except the dummy target from m_target_stack. As > + targets are removed from m_target_stack their reference count is > + decremented, which may cause a target to close. */ > + void pop_all_targets () > + { pop_all_targets_above (dummy_stratum); } > + > + /* Unpush all targets above STRATUM from m_target_stack. As targets are > + removed from m_target_stack their reference count is decremented, > + which may cause a target to close. */ > + void pop_all_targets_above (enum strata stratum); > + > + /* Unpush all targets at and above STRATUM from m_target_stack. As > + targets are removed from m_target_stack their reference count is > + decremented, which may cause a target to close. */ > + void pop_all_targets_at_and_above (enum strata stratum); > + > /* Return the target at process_stratum level in this inferior's > target stack. */ > struct process_stratum_target *process_target () > @@ -598,6 +614,10 @@ class inferior : public refcounted_object, > registry registry_fields; > > private: > + > + /* Unpush TARGET and assert that it worked. */ > + void unpush_target_and_assert (struct target_ops *target); > + > /* The inferior's target stack. */ > target_stack m_target_stack; > > diff --git a/gdb/remote.c b/gdb/remote.c > index 5a71c41d61e..4864e9a55c3 100644 > --- a/gdb/remote.c > +++ b/gdb/remote.c > @@ -5721,7 +5721,7 @@ remote_unpush_target (remote_target *target) > for (inferior *inf : all_inferiors (target)) > { > switch_to_inferior_no_thread (inf); > - pop_all_targets_at_and_above (process_stratum); > + inf->pop_all_targets_at_and_above (process_stratum); > generic_mourn_inferior (); > } > > diff --git a/gdb/scoped-mock-context.h b/gdb/scoped-mock-context.h > index a9895303015..87c1df0d206 100644 > --- a/gdb/scoped-mock-context.h > +++ b/gdb/scoped-mock-context.h > @@ -71,7 +71,7 @@ struct scoped_mock_context > ~scoped_mock_context () > { > inferior_list.erase (inferior_list.iterator_to (mock_inferior)); > - pop_all_targets_at_and_above (process_stratum); > + mock_inferior.pop_all_targets_at_and_above (process_stratum); > } > }; > > diff --git a/gdb/target.c b/gdb/target.c > index 0f4d6d01057..1e447f604d9 100644 > --- a/gdb/target.c > +++ b/gdb/target.c > @@ -1238,45 +1238,6 @@ target_stack::unpush (target_ops *t) > return true; > } > > -/* Unpush TARGET and assert that it worked. */ > - > -static void > -unpush_target_and_assert (struct target_ops *target) > -{ > - if (!current_inferior ()->unpush_target (target)) > - { > - gdb_printf (gdb_stderr, > - "pop_all_targets couldn't find target %s\n", > - target->shortname ()); > - internal_error (__FILE__, __LINE__, > - _("failed internal consistency check")); > - } > -} > - > -void > -pop_all_targets_above (enum strata above_stratum) > -{ > - while ((int) (current_inferior ()->top_target ()->stratum ()) > - > (int) above_stratum) > - unpush_target_and_assert (current_inferior ()->top_target ()); > -} > - > -/* See target.h. */ > - > -void > -pop_all_targets_at_and_above (enum strata stratum) > -{ > - while ((int) (current_inferior ()->top_target ()->stratum ()) > - >= (int) stratum) > - unpush_target_and_assert (current_inferior ()->top_target ()); > -} > - > -void > -pop_all_targets (void) > -{ > - pop_all_targets_above (dummy_stratum); > -} > - > void > target_unpusher::operator() (struct target_ops *ops) const > { > @@ -2533,7 +2494,7 @@ target_preopen (int from_tty) > it doesn't (which seems like a win for UDI), remove it now. */ > /* Leave the exec target, though. The user may be switching from a > live process to a core of the same program. */ > - pop_all_targets_above (file_stratum); > + current_inferior ()->pop_all_targets_above (file_stratum); > > target_pre_inferior (from_tty); > } > diff --git a/gdb/target.h b/gdb/target.h > index 68446a39c1b..547ee8a3bbd 100644 > --- a/gdb/target.h > +++ b/gdb/target.h > @@ -2389,17 +2389,6 @@ extern void target_pre_inferior (int); > > extern void target_preopen (int); > > -/* Does whatever cleanup is required to get rid of all pushed targets. */ > -extern void pop_all_targets (void); > - > -/* Like pop_all_targets, but pops only targets whose stratum is at or > - above STRATUM. */ > -extern void pop_all_targets_at_and_above (enum strata stratum); > - > -/* Like pop_all_targets, but pops only targets whose stratum is > - strictly above ABOVE_STRATUM. */ > -extern void pop_all_targets_above (enum strata above_stratum); > - > extern CORE_ADDR target_translate_tls_address (struct objfile *objfile, > CORE_ADDR offset); > > diff --git a/gdb/top.c b/gdb/top.c > index 54c7c922142..5f64c6bddf0 100644 > --- a/gdb/top.c > +++ b/gdb/top.c > @@ -1845,10 +1845,9 @@ quit_force (int *exit_arg, int from_tty) > them all out. */ > for (inferior *inf : all_inferiors ()) > { > - switch_to_inferior_no_thread (inf); > try > { > - pop_all_targets (); > + inf->pop_all_targets (); > } > catch (const gdb_exception &ex) > { > -- > 2.25.4 >