From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 6420 invoked by alias); 8 Jul 2015 15:43:04 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 6368 invoked by uid 89); 8 Jul 2015 15:42:58 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-HELO: usevmg21.ericsson.net Received: from usevmg21.ericsson.net (HELO usevmg21.ericsson.net) (198.24.6.65) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-SHA encrypted) ESMTPS; Wed, 08 Jul 2015 15:42:58 +0000 Received: from EUSAAHC007.ericsson.se (Unknown_Domain [147.117.188.93]) by usevmg21.ericsson.net (Symantec Mail Security) with SMTP id 82.DF.07675.CADDC955; Wed, 8 Jul 2015 10:22:04 +0200 (CEST) Received: from [142.133.110.144] (147.117.188.8) by smtp-am.internal.ericsson.com (147.117.188.95) with Microsoft SMTP Server id 14.3.210.2; Wed, 8 Jul 2015 11:42:55 -0400 Message-ID: <559D44FF.60705@ericsson.com> Date: Wed, 08 Jul 2015 15:43:00 -0000 From: Simon Marchi User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 MIME-Version: 1.0 To: Pedro Alves , Subject: Re: [PATCH] Delete program spaces directly when removing inferiors References: <1412022790-21931-1-git-send-email-simon.marchi@ericsson.com> <559D1220.1060708@redhat.com> <559D4314.2000906@ericsson.com> In-Reply-To: <559D4314.2000906@ericsson.com> Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2015-07/txt/msg00227.txt.bz2 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 > 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 > + > + * 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 > > 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.