From patchwork Thu Oct 6 11:14:22 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andrew Burgess X-Patchwork-Id: 58730 From: aburgess@redhat.com (Andrew Burgess) Date: Thu, 06 Oct 2022 12:14:22 +0100 List-Id: gdb-patches mailing list Subject: [PATCHv2 4/7] gdb: remove the pop_all_targets (and friends) global functions In-Reply-To: <20221005204950.v6gendcr5rlous7b@ubuntu.lan> References: <20220921131200.3983844-1-aburgess@redhat.com> <20221005204950.v6gendcr5rlous7b@ubuntu.lan> Message-ID: <878rltfbld.fsf@redhat.com> Lancelot SIX writes: > 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? No, I just copied over the existing code. I've removed the cast in the updated patch below. Thanks, Andrew --- commit 290023b39717030586fca501aa4c80af578b2bb0 Author: Andrew Burgess Date: Thu Sep 22 12:22:22 2022 +0100 List-Id: gdb-patches mailing list gdb: remove the pop_all_targets (and friends) global functions 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. 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..676b37ce6d5 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 (top_target ()->stratum () > stratum) + 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 (top_target ()->stratum () >= stratum) + 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 27765304ece..ab6a209a448 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) {