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

[Lancelot SIX <lsix@lancelotsix.com>]

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