This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] Delete program spaces directly when removing inferiors
- From: Simon Marchi <simon dot marchi at ericsson dot com>
- To: Pedro Alves <palves at redhat dot com>, <gdb-patches at sourceware dot org>
- Date: Wed, 8 Jul 2015 11:42:55 -0400
- Subject: Re: [PATCH] Delete program spaces directly when removing inferiors
- Authentication-results: sourceware.org; auth=none
- References: <1412022790-21931-1-git-send-email-simon dot marchi at ericsson dot com> <559D1220 dot 1060708 at redhat dot com> <559D4314 dot 2000906 at ericsson dot com>
On 15-07-08 11:34 AM, Simon Marchi wrote:
>
> On 15-07-08 08:05 AM, Pedro Alves wrote:
>> On 09/29/2014 09:33 PM, Simon Marchi wrote:
>>
>>> -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);
>>
>> I think there's something odd with indentation here.
>
> Ok.
>
>>> - 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)
>>> + if (pspace == program_spaces)
>>> + program_spaces = pspace->next;
>>> + else
>>> {
>>> - if (ss == current || !pspace_empty_p (ss))
>>> + struct program_space *i = program_spaces;
>>> +
>>> + for (i = program_spaces; i != NULL; i = i->next)
>>> {
>>> - ss_link = &ss->next;
>>> - ss = *ss_link;
>>> - continue;
>>> + if (i->next == pspace)
>>> + {
>>> + i->next = i->next->next;
>>> + break;
>>> + }
>>> }
>>> -
>>> - *ss_link = ss->next;
>>> - release_program_space (ss);
>>> - ss = *ss_link;
>>> }
>>
>> I don't find this conversion from while to if+else+for an
>> improvement. You can instead write:
>>
>> ss = program_spaces;
>> ss_link = &program_spaces;
>> while (ss != NULL)
>> {
>> if (ss == pspace)
>> {
>> *ss_link = ss->next;
>> break;
>> }
>>
>> ss_link = &ss->next;
>> ss = *ss_link;
>> }
>> release_program_space (pspace);
>>
>> We use this _link pattern in several places.
>>
>> OK with that change.
>
> Ok, pushed with those changes:
>
> From 0560c645c02eba2828a053039dcfdf676cdd1d00 Mon Sep 17 00:00:00 2001
> From: Simon Marchi <simon.marchi@ericsson.com>
> Date: Mon, 29 Sep 2014 16:33:10 -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_silent and delete_inferior were unused, which allowed
> renaming delete_inferior_1 to delete_inferior. Also, since all calls to
> it were with silent=1, I removed that parameter completely.
>
> 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.
>
> This obsoletes my previous patch "Add call to prune_program_spaces in
> mi_cmd_remove_inferior" [1].
>
> [1] https://sourceware.org/ml/gdb-patches/2014-09/msg00717.html
>
> 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.
> ---
> gdb/ChangeLog | 26 ++++++++++++++++++++++++++
> gdb/inferior.c | 39 ++++++++-------------------------------
> gdb/inferior.h | 9 +--------
> gdb/mi/mi-main.c | 2 +-
> gdb/progspace.c | 27 ++++++++++++++-------------
> gdb/progspace.h | 11 +++++++----
> 6 files changed, 57 insertions(+), 57 deletions(-)
>
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index b565dde..0a0d50a 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,29 @@
> +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.
> +
> 2015-07-08 Jan Kratochvil <jan.kratochvil@redhat.com>
>
> PR compile/18484
> 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/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 have reverted the patch, since it causes a build failure:
http://gdb-build.sergiodj.net/builders/Debian-s390x-native-extended-gdbserver-m64/builds/77
delete_inferior_silent is actually used in monitor.c. What I don't understand is why
monitor.o doesn't get built when I build gdb locally. When I type "make monitor.o",
I can see the build error at least.