[PATCHv2,4/7] gdb: remove the pop_all_targets (and friends) global functions

Message ID 878rltfbld.fsf@redhat.com
State New
Headers
Series None |

Commit Message

Andrew Burgess Oct. 6, 2022, 11:14 a.m. UTC
  Lancelot SIX <lsix@lancelotsix.com> 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 <aburgess@redhat.com>
Date:   Thu Sep 22 12:22:22 2022 +0100
List-Id: gdb-patches mailing list <gdb-patches.sourceware.org>

    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.
  

Patch

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<inferior> 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)
 	{