Re: [PATCH] Delete program spaces directly when removing inferiors

[Simon Marchi <simon dot marchi at ericsson dot com>]

On 15-07-08 03:44 PM, Simon Marchi wrote:
> On 15-07-08 02:50 PM, Pedro Alves wrote:
>> The patch is preapproved.  Meanwhile I sent a mail to gdb@ about
>> deleting monitor.c and friends.
> 
> Thanks for cleaning up the cruft.
> 
> I pushed this updated version:
> 
> 
> From 7a41607e01b505db895fcebcad618606cfab1ecf Mon Sep 17 00:00:00 2001
> From: Simon Marchi <simon.marchi@ericsson.com>
> Date: Wed, 8 Jul 2015 15:41:01 -0400
> Subject: [PATCH] Delete program spaces directly when removing inferiors
> 
> When deleting an inferior, delete the associated program space as well
> if it becomes unused. This replaces the "pruning" approach, with which
> you could forget to call prune_program_spaces (as seen, with the
> -remove-inferior command, see [1]).
> 
> This allows to remove the prune_program_spaces function. At the same
> time, I was able to clean up the delete_inferior* family:
> 
>  - delete_inferior is unused
>  - delete_inferior_silent is only used in monitor_close, but is replaced
>    with discard_all_inferiors [2], so it becomes unused
>  - All remaining calls to delete_inferior_1 are with silent=1, so the
>    parameter is removed
>  - delete_inferior_1 is renamed to delete_inferior
> 
> I renamed pspace_empty_p to program_space_empty_p. I prefer if the
> "exported" functions have a more explicit and standard name.
> 
> Tested on Ubuntu 14.10.
> 
> [1] https://sourceware.org/ml/gdb-patches/2014-09/msg00717.html
> [2] See https://sourceware.org/ml/gdb-patches/2015-07/msg00228.html and
>     follow-ups for details.
> 
> gdb/Changelog:
> 
> 	* inferior.c (delete_inferior_1): Rename to ...
> 	(delete_inferior): ..., remove 'silent' parameter, delete
> 	program space when unused and remove call to prune_program_spaces.
> 	Remove the old, unused, delete_inferior.
> 	(delete_inferior_silent): Remove.
> 	(prune_inferiors): Change call from delete_inferior_1 to
> 	delete_inferior and remove 'silent' parameter. Remove call to
> 	prune_program_spaces.
> 	(remove_inferior_command): Idem.
> 	* inferior.h (delete_inferior_1): Rename to...
> 	(delete_inferior): ..., remove 'silent' parameter and remove the
> 	original delete_inferior.
> 	(delete_inferior_silent): Remove.
> 	* mi/mi-main.c (mi_cmd_remove_inferior): Change call from
> 	delete_inferior_1 to delete_inferior and remove 'silent'
> 	parameter.
> 	* progspace.c (prune_program_spaces): Remove.
> 	(pspace_empty_p): Rename to...
> 	(program_space_empty_p): ... and make non-static.
> 	(delete_program_space): New.
> 	* progspace.h (prune_program_spaces): Remove declaration.
> 	(program_space_empty_p): New declaration.
> 	(delete_program_space): New declaration.
> 	* monitor.c (monitor_close): Replace call to
> 	delete_thread_silent and delete_inferior_silent with
> 	discard_all_inferiors.
> ---
>  gdb/ChangeLog    | 29 +++++++++++++++++++++++++++++
>  gdb/inferior.c   | 39 ++++++++-------------------------------
>  gdb/inferior.h   |  9 +--------
>  gdb/mi/mi-main.c |  2 +-
>  gdb/monitor.c    |  3 +--
>  gdb/progspace.c  | 27 ++++++++++++++-------------
>  gdb/progspace.h  | 11 +++++++----
>  7 files changed, 61 insertions(+), 59 deletions(-)
> 
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index 3bdb099..c35cbd7 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,32 @@
> +2015-07-08  Simon Marchi  <simon.marchi@ericsson.com>
> +
> +	* inferior.c (delete_inferior_1): Rename to ...
> +	(delete_inferior): ..., remove 'silent' parameter, delete
> +	program space when unused and remove call to prune_program_spaces.
> +	Remove the old, unused, delete_inferior.
> +	(delete_inferior_silent): Remove.
> +	(prune_inferiors): Change call from delete_inferior_1 to
> +	delete_inferior and remove 'silent' parameter. Remove call to
> +	prune_program_spaces.
> +	(remove_inferior_command): Idem.
> +	* inferior.h (delete_inferior_1): Rename to...
> +	(delete_inferior): ..., remove 'silent' parameter and remove the
> +	original delete_inferior.
> +	(delete_inferior_silent): Remove.
> +	* mi/mi-main.c (mi_cmd_remove_inferior): Change call from
> +	delete_inferior_1 to delete_inferior and remove 'silent'
> +	parameter.
> +	* progspace.c (prune_program_spaces): Remove.
> +	(pspace_empty_p): Rename to...
> +	(program_space_empty_p): ... and make non-static.
> +	(delete_program_space): New.
> +	* progspace.h (prune_program_spaces): Remove declaration.
> +	(program_space_empty_p): New declaration.
> +	(delete_program_space): New declaration.
> +	* monitor.c (monitor_close): Replace call to
> +	delete_thread_silent and delete_inferior_silent with
> +	discard_all_inferiors.
> +
>  2015-07-08  Patrick Palka  <patrick@parcs.ath.cx>
> 
>  	* defs.h (deprecated_register_changed_hook): Remove prototype.
> diff --git a/gdb/inferior.c b/gdb/inferior.c
> index d0783d3..5e98df5 100644
> --- a/gdb/inferior.c
> +++ b/gdb/inferior.c
> @@ -184,11 +184,8 @@ delete_thread_of_inferior (struct thread_info *tp, void *data)
>    return 0;
>  }
> 
> -/* If SILENT then be quiet -- don't announce a inferior death, or the
> -   exit of its threads.  */
> -
>  void
> -delete_inferior_1 (struct inferior *todel, int silent)
> +delete_inferior (struct inferior *todel)
>  {
>    struct inferior *inf, *infprev;
>    struct delete_thread_of_inferior_arg arg;
> @@ -203,7 +200,7 @@ delete_inferior_1 (struct inferior *todel, int silent)
>      return;
> 
>    arg.pid = inf->pid;
> -  arg.silent = silent;
> +  arg.silent = 1;
> 
>    iterate_over_threads (delete_thread_of_inferior, &arg);
> 
> @@ -214,29 +211,13 @@ delete_inferior_1 (struct inferior *todel, int silent)
> 
>    observer_notify_inferior_removed (inf);
> 
> -  free_inferior (inf);
> -}
> -
> -void
> -delete_inferior (int pid)
> -{
> -  struct inferior *inf = find_inferior_pid (pid);
> -
> -  delete_inferior_1 (inf, 0);
> -
> -  if (print_inferior_events)
> -    printf_unfiltered (_("[Inferior %d exited]\n"), pid);
> -}
> +  /* If this program space is rendered useless, remove it. */
> +  if (program_space_empty_p (inf->pspace))
> +    delete_program_space (inf->pspace);
> 
> -void
> -delete_inferior_silent (int pid)
> -{
> -  struct inferior *inf = find_inferior_pid (pid);
> -
> -  delete_inferior_1 (inf, 1);
> +  free_inferior (inf);
>  }
> 
> -
>  /* If SILENT then be quiet -- don't announce a inferior exit, or the
>     exit of its threads.  */
> 
> @@ -509,11 +490,9 @@ prune_inferiors (void)
>  	}
> 
>        *ss_link = ss->next;
> -      delete_inferior_1 (ss, 1);
> +      delete_inferior (ss);
>        ss = *ss_link;
>      }
> -
> -  prune_program_spaces ();
>  }
> 
>  /* Simply returns the count of inferiors.  */
> @@ -797,10 +776,8 @@ remove_inferior_command (char *args, int from_tty)
>  	  continue;
>  	}
> 
> -      delete_inferior_1 (inf, 1);
> +      delete_inferior (inf);
>      }
> -
> -  prune_program_spaces ();
>  }
> 
>  struct inferior *
> diff --git a/gdb/inferior.h b/gdb/inferior.h
> index 2054a2a..48cba45 100644
> --- a/gdb/inferior.h
> +++ b/gdb/inferior.h
> @@ -418,14 +418,7 @@ extern struct inferior *add_inferior (int pid);
>     the CLI.  */
>  extern struct inferior *add_inferior_silent (int pid);
> 
> -/* Delete an existing inferior list entry, due to inferior exit.  */
> -extern void delete_inferior (int pid);
> -
> -extern void delete_inferior_1 (struct inferior *todel, int silent);
> -
> -/* Same as delete_inferior, but don't print new inferior notifications
> -   to the CLI.  */
> -extern void delete_inferior_silent (int pid);
> +extern void delete_inferior (struct inferior *todel);
> 
>  /* Delete an existing inferior list entry, due to inferior detaching.  */
>  extern void detach_inferior (int pid);
> diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
> index ddfc9d9..66bcd88 100644
> --- a/gdb/mi/mi-main.c
> +++ b/gdb/mi/mi-main.c
> @@ -1964,7 +1964,7 @@ mi_cmd_remove_inferior (char *command, char **argv, int argc)
>        set_current_program_space (new_inferior->pspace);
>      }
> 
> -  delete_inferior_1 (inf, 1 /* silent */);
> +  delete_inferior (inf);
>  }
> 
>  
> diff --git a/gdb/monitor.c b/gdb/monitor.c
> index c7f5fc7..4657d73 100644
> --- a/gdb/monitor.c
> +++ b/gdb/monitor.c
> @@ -853,8 +853,7 @@ monitor_close (struct target_ops *self)
> 
>    monitor_desc = NULL;
> 
> -  delete_thread_silent (monitor_ptid);
> -  delete_inferior_silent (ptid_get_pid (monitor_ptid));
> +  discard_all_inferiors ();
>  }
> 
>  /* Terminate the open connection to the remote debugger.  Use this
> diff --git a/gdb/progspace.c b/gdb/progspace.c
> index 1c0a254..a8f5ea0 100644
> --- a/gdb/progspace.c
> +++ b/gdb/progspace.c
> @@ -235,8 +235,8 @@ save_current_program_space (void)
> 
>  /* Returns true iff there's no inferior bound to PSPACE.  */
> 
> -static int
> -pspace_empty_p (struct program_space *pspace)
> +int
> +program_space_empty_p (struct program_space *pspace)
>  {
>    if (find_inferior_for_program_space (pspace) != NULL)
>        return 0;
> @@ -244,30 +244,31 @@ pspace_empty_p (struct program_space *pspace)
>    return 1;
>  }
> 
> -/* Prune away automatically added program spaces that aren't required
> -   anymore.  */
> +/* Remove a program space from the program spaces list and release it.  It is
> +   an error to call this function while PSPACE is the current program space. */
> 
>  void
> -prune_program_spaces (void)
> +delete_program_space (struct program_space *pspace)
>  {
>    struct program_space *ss, **ss_link;
> -  struct program_space *current = current_program_space;
> +  gdb_assert(pspace != NULL);
> +  gdb_assert(pspace != current_program_space);
> 
>    ss = program_spaces;
>    ss_link = &program_spaces;
> -  while (ss)
> +  while (ss != NULL)
>      {
> -      if (ss == current || !pspace_empty_p (ss))
> +      if (ss == pspace)
>  	{
> -	  ss_link = &ss->next;
> -	  ss = *ss_link;
> -	  continue;
> +	  *ss_link = ss->next;
> +	  break;
>  	}
> 
> -      *ss_link = ss->next;
> -      release_program_space (ss);
> +      ss_link = &ss->next;
>        ss = *ss_link;
>      }
> +
> +  release_program_space (pspace);
>  }
> 
>  /* Prints the list of program spaces and their details on UIOUT.  If
> diff --git a/gdb/progspace.h b/gdb/progspace.h
> index f960093..48f206e 100644
> --- a/gdb/progspace.h
> +++ b/gdb/progspace.h
> @@ -236,9 +236,16 @@ extern struct program_space *current_program_space;
>     pointer to the new object.  */
>  extern struct program_space *add_program_space (struct address_space *aspace);
> 
> +/* Remove a program space from the program spaces list and release it.  It is
> +   an error to call this function while PSPACE is the current program space. */
> +extern void delete_program_space (struct program_space *pspace);
> +
>  /* Returns the number of program spaces listed.  */
>  extern int number_of_program_spaces (void);
> 
> +/* Returns true iff there's no inferior bound to PSPACE.  */
> +extern int program_space_empty_p (struct program_space *pspace);
> +
>  /* Copies program space SRC to DEST.  Copies the main executable file,
>     and the main symbol file.  Returns DEST.  */
>  extern struct program_space *clone_program_space (struct program_space *dest,
> @@ -289,10 +296,6 @@ extern int address_space_num (struct address_space *aspace);
>     mappings.  */
>  extern void update_address_spaces (void);
> 
> -/* Prune away automatically added program spaces that aren't required
> -   anymore.  */
> -extern void prune_program_spaces (void);
> -
>  /* Reset saved solib data at the start of an solib event.  This lets
>     us properly collect the data when calling solib_add, so it can then
>     later be printed.  */
> 

I had some formatting errors left, I pushed this right after:


>From 4ab31498e400c89ba9dcea2444c65aa020ab53fc Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@ericsson.com>
Date: Wed, 8 Jul 2015 15:48:02 -0400
Subject: [PATCH] Add missing spaces in previous patch

gdb/ChangeLog:

	* progspace.c (delete_program_space): Add missing spaces.
---
 gdb/ChangeLog   | 4 ++++
 gdb/progspace.c | 4 ++--
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index c35cbd7..8a971e5 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,9 @@
 2015-07-08  Simon Marchi  <simon.marchi@ericsson.com>

+	* progspace.c (delete_program_space): Add missing spaces.
+
+2015-07-08  Simon Marchi  <simon.marchi@ericsson.com>
+
 	* inferior.c (delete_inferior_1): Rename to ...
 	(delete_inferior): ..., remove 'silent' parameter, delete
 	program space when unused and remove call to prune_program_spaces.
diff --git a/gdb/progspace.c b/gdb/progspace.c
index a8f5ea0..ea2f8ec 100644
--- a/gdb/progspace.c
+++ b/gdb/progspace.c
@@ -251,8 +251,8 @@ void
 delete_program_space (struct program_space *pspace)
 {
   struct program_space *ss, **ss_link;
-  gdb_assert(pspace != NULL);
-  gdb_assert(pspace != current_program_space);
+  gdb_assert (pspace != NULL);
+  gdb_assert (pspace != current_program_space);

   ss = program_spaces;
   ss_link = &program_spaces;
-- 
2.1.4