Message ID | 1411593539-6507-1-git-send-email-simon.marchi@ericsson.com |
---|---|
State | Superseded |
Headers |
Received: (qmail 21221 invoked by alias); 24 Sep 2014 21:19:12 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 21185 invoked by uid 89); 24 Sep 2014 21:19:09 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.3 required=5.0 tests=AWL, BAYES_00, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=ham version=3.3.2 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, 24 Sep 2014 21:19:07 +0000 Received: from EUSAAHC006.ericsson.se (Unknown_Domain [147.117.188.90]) by usevmg21.ericsson.net (Symantec Mail Security) with SMTP id E5.D5.25146.6ACD2245; Wed, 24 Sep 2014 17:00:54 +0200 (CEST) Received: from simark-hp.mo.ca.am.ericsson.se (147.117.188.8) by smtps-am.internal.ericsson.com (147.117.188.90) with Microsoft SMTP Server (TLS) id 14.3.174.1; Wed, 24 Sep 2014 17:19:04 -0400 From: Simon Marchi <simon.marchi@ericsson.com> To: <gdb-patches@sourceware.org> CC: Simon Marchi <simon.marchi@ericsson.com> Subject: [PATCH] Add call to prune_program_spaces in mi_cmd_remove_inferior Date: Wed, 24 Sep 2014 17:18:59 -0400 Message-ID: <1411593539-6507-1-git-send-email-simon.marchi@ericsson.com> MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes |
Commit Message
Simon Marchi
Sept. 24, 2014, 9:18 p.m. UTC
...so the -remove-inferior MI command behaves more like the remove-inferiors CLI command. The CLI version already calls prune_program_spaces. Currently, when removing an inferior with MI, the associated program space is not removed, even if it is not useful anyore. A visible consequence of that is that after doing -remove-inferior, you won't get the =library-unloaded messages yet. Only when prune_program_spaces is called later, for unrelated reasons (i.e. starting another inferior), gdb will realize that the program space is useless and will issue the library events. Another consequence of that is that breakpoints are not re-evaluated and "info breakpoints" will still show the locations in the old inferior/program space. I also noticed that in the =library-unloaded messages that you get when removing an inferior, 'thread-group' value is not good. This is because the code that emits the event uses current_inferior()->num to generate the value (mi-interp.c:1022). The inferior that is being removed can't be the current_inferior. I will try to look at it later, but if you have an idea on how to fix it, I am open to suggestions. No change in the test results (Ubuntu 14.10). gdb/Changelog: * mi/mi-main.c (mi_cmd_remove_inferior): Add call to prune_program_spaces. --- gdb/mi/mi-main.c | 2 ++ 1 file changed, 2 insertions(+)
Comments
On Wed, Sep 24, 2014 at 2:18 PM, Simon Marchi <simon.marchi@ericsson.com> wrote: > ...so the -remove-inferior MI command behaves more like the > remove-inferiors CLI command. The CLI version already calls > prune_program_spaces. > > Currently, when removing an inferior with MI, the associated program > space is not removed, even if it is not useful anyore. A visible > consequence of that is that after doing -remove-inferior, you won't get > the =library-unloaded messages yet. Only when prune_program_spaces is > called later, for unrelated reasons (i.e. starting another inferior), > gdb will realize that the program space is useless and will issue the > library events. > > Another consequence of that is that breakpoints are not re-evaluated and > "info breakpoints" will still show the locations in the old > inferior/program space. > > I also noticed that in the =library-unloaded messages that you get when > removing an inferior, 'thread-group' value is not good. This is because > the code that emits the event uses current_inferior()->num to generate > the value (mi-interp.c:1022). The inferior that is being removed can't be > the current_inferior. I will try to look at it later, but if you have an > idea on how to fix it, I am open to suggestions. > > No change in the test results (Ubuntu 14.10). > > gdb/Changelog: > > * mi/mi-main.c (mi_cmd_remove_inferior): Add call to > prune_program_spaces. > --- > gdb/mi/mi-main.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c > index 59717ca..ba710ff 100644 > --- a/gdb/mi/mi-main.c > +++ b/gdb/mi/mi-main.c > @@ -1951,6 +1951,8 @@ mi_cmd_remove_inferior (char *command, char **argv, int argc) > } > > delete_inferior_1 (inf, 1 /* silent */); > + > + prune_program_spaces (); > } Hi. One of my pet peeves of gdb is that too much implementation logic is spread throughout gdb. By that I mean random bits of gdb take on the job of maintaining random bits of internal gdb state, instead of calling one routine (or very few) whose job it is to encapsulate all that knowledge. It's not clear that that applies here, but I think it does. With that in mind the first question that comes to mind when reviewing this patch is: "Is there ever a time when deleting an inferior (from outside inferior.c) would ever *not* want to also prune program spaces (at least by default)?" I think the answer is "No" and thus I think it'd be preferable to have one call here instead of one call to delete_inferior_1 and another to prune_program_spaces. void delete_inferior_and_prune (struct inferior *todel) { delete_inferior_1 (todel, 1); prune_program_spaces (); } and then call it from mi_cmd_remove_inferior? I'm ok with that name, but perhaps there's a better name. There would then be the issue that delete_inferior_and_prune takes an inferior pointer whereas delete_inferior_silent takes a pid. void delete_inferior_silent (int pid) { struct inferior *inf = find_inferior_pid (pid); delete_inferior_1 (inf, 1); } delete_inferior_silent is only called from monitor.c:monitor_close. [And I see it doesn't also call prune_program_spaces. Is that another bug I wonder (or at least one waiting to happen).] I'd be ok with calling find_inferior_pid from monitor.c. That would leave delete_inferior_silent being just a simple wrapper of delete_inferior_1. And since in general we don't want to export functions with _1 in the name ... How about the following? 1) delete the existing delete_inferior and delete_inferior_silent functions - delete_inferior is unused 2) rename delete_inferior_1 to delete_inferior, and remove the "silent" argument - or keep the argument, but it'd only ever be "1" 3) write a new function delete_inferior_and_prune - and call it from mi_cmd_remove_inferior 4) have monitor_close call delete_inferior (find_inferior_pid (ptid_get_pid (monitor_ptid))); btw, as another cleanup (though not part of this patch), find_inferior_pid (ptid_get_pid (...)) seems to be a common idiom. I'd be ok with adding a find_inferior_ptid utility.
On 2014-09-24 06:43 PM, Doug Evans wrote: > Hi. > > One of my pet peeves of gdb is that too much implementation logic is > spread throughout gdb. > By that I mean random bits of gdb take on the job of maintaining > random bits of internal gdb state, > instead of calling one routine (or very few) whose job it is to > encapsulate all that knowledge. > > It's not clear that that applies here, but I think it does. > > With that in mind the first question that comes to mind when reviewing > this patch is: > "Is there ever a time when deleting an inferior (from outside inferior.c) > would ever *not* want to also prune program spaces (at least by default)?" I had the same thought. I actually have another patch in the pipeline that addresses this. I think that the pruning approach is a bit wasteful when deleting an inferior. The only possible program space that we could possibly delete is the one that is tied to the deleted inferior. I was thinking of adding something like this in delete_inferior: /* If this program space is rendered useless, remove it. */ if (pspace_empty_p (inf->pspace)) delete_program_space (inf->pspace); (this is done after "inf" has been removed from the inferiors list, such that pspace_empty_p returns true if inf was the last inferior tied to that pspace) I think this will allow to completely remove the prune_program_spaces function, since deleting an inferior is the only case where this is used. If you prefer, I can go directly with a patch like that and drop this one. I sent the current one first because I thought it would be a bit more obvious and require less discussion (and at least get the functionality right). > I think the answer is "No" and thus I think it'd be preferable to have > one call here > instead of one call to delete_inferior_1 and another to prune_program_spaces. > > void > delete_inferior_and_prune (struct inferior *todel) > { > delete_inferior_1 (todel, 1); > prune_program_spaces (); > } > > and then call it from mi_cmd_remove_inferior? > > I'm ok with that name, but perhaps there's a better name. > > There would then be the issue that delete_inferior_and_prune takes an > inferior pointer whereas > delete_inferior_silent takes a pid. > > void > delete_inferior_silent (int pid) > { > struct inferior *inf = find_inferior_pid (pid); > > delete_inferior_1 (inf, 1); > } > > delete_inferior_silent is only called from monitor.c:monitor_close. > [And I see it doesn't also call prune_program_spaces. > Is that another bug I wonder (or at least one waiting to happen).] > I'd be ok with calling find_inferior_pid from monitor.c. > > That would leave delete_inferior_silent being just a simple wrapper of > delete_inferior_1. > And since in general we don't want to export functions with _1 in the name ... > > How about the following? > > 1) delete the existing delete_inferior and delete_inferior_silent functions > - delete_inferior is unused > 2) rename delete_inferior_1 to delete_inferior, and remove the "silent" argument > - or keep the argument, but it'd only ever be "1" > 3) write a new function delete_inferior_and_prune > - and call it from mi_cmd_remove_inferior > 4) have monitor_close call delete_inferior (find_inferior_pid > (ptid_get_pid (monitor_ptid))); I'd be ok with removing unused functions (delete_inferior) and standardizing the interface (take a struct inferior* as argument, not a pid). However, considering my suggestion above, I think the delete_inferior_and_prune function would be unnecessary (delete_inferior, renamed from delete_inferior_1, would take care of cleaning up unused program spaces). > btw, as another cleanup (though not part of this patch), > find_inferior_pid (ptid_get_pid (...)) seems to be a common idiom. > I'd be ok with adding a find_inferior_ptid utility. That would be a good cleanup, I'll put it on my list. Thanks, Simon
On Thu, Sep 25, 2014 at 8:07 AM, Simon Marchi <simon.marchi@ericsson.com> wrote: > On 2014-09-24 06:43 PM, Doug Evans wrote: >> Hi. >> >> One of my pet peeves of gdb is that too much implementation logic is >> spread throughout gdb. >> By that I mean random bits of gdb take on the job of maintaining >> random bits of internal gdb state, >> instead of calling one routine (or very few) whose job it is to >> encapsulate all that knowledge. >> >> It's not clear that that applies here, but I think it does. >> >> With that in mind the first question that comes to mind when reviewing >> this patch is: >> "Is there ever a time when deleting an inferior (from outside inferior.c) >> would ever *not* want to also prune program spaces (at least by default)?" > > I had the same thought. > > I actually have another patch in the pipeline that addresses this. I think that > the pruning approach is a bit wasteful when deleting an inferior. The only possible > program space that we could possibly delete is the one that is tied to the deleted > inferior. I was thinking of adding something like this in delete_inferior: > > /* If this program space is rendered useless, remove it. */ > if (pspace_empty_p (inf->pspace)) > delete_program_space (inf->pspace); > > (this is done after "inf" has been removed from the inferiors list, such that > pspace_empty_p returns true if inf was the last inferior tied to that pspace) > > I think this will allow to completely remove the prune_program_spaces function, > since deleting an inferior is the only case where this is used. If you prefer, > I can go directly with a patch like that and drop this one. I sent the current > one first because I thought it would be a bit more obvious and require less > discussion (and at least get the functionality right). That would be nice. I only stopped where I did because I didn't want to impose vetting all callers of delete_inferior(_*) on this patch series. I don't have a preference, and I don't want to impose too much specifics on the patch since you're the one writing it. OTOH, I do want to make progress and do a bit more than your original patch. If you're willing to go directly to always deleting unused progspaces whenever we delete inferiors then let's do that. --- btw, it would be nice to have an assert that we're not deleting the last inferior. "There's always (at least) one inferior." This invariant should be readily visible to readers of delete_inferior(_*). btw #2, There's also the invariant "There's always (at least) one program space." One is left with the question of whether they could be unrelated. IOW could there be an inferior without a program space or a program space without an inferior? [At least in general. There are special cases where we do temporary hacks to get through forks and such.] Another cleanup could be to make this clearer.
On Sun, Sep 28, 2014 at 1:16 PM, Doug Evans <dje@google.com> wrote: >[...] > btw #2, There's also the invariant "There's always (at least) one > program space." > One is left with the question of whether they could be unrelated. > IOW could there be an inferior without a program space or a program > space without an inferior? > [At least in general. There are special cases where we do temporary > hacks to get through forks and such.] > Another cleanup could be to make this clearer. One thought I had, and I'm just thinking out loud here, is to rephrase this invariant as "An inferior always has a program space." Then, given that there is always an inferior, it falls out that there will also always be a program space. But, at least to this reader, program spaces can't ever be thought of as being created on their own, they are only created when an inferior is created, and deleted when the last inferior using it is deleted. That includes the initial program space, which leads to the thought of merging the creation of the initial program space and initial inferior. Doing that feels clearer to me than initializing them separately, given that we're going to be actively deleting program spaces when the last-using inferior is deleted.
On 2014-09-28 04:54 PM, Doug Evans wrote: > On Sun, Sep 28, 2014 at 1:16 PM, Doug Evans <dje@google.com> wrote: >> [...] >> btw #2, There's also the invariant "There's always (at least) one >> program space." >> One is left with the question of whether they could be unrelated. >> IOW could there be an inferior without a program space or a program >> space without an inferior? >> [At least in general. There are special cases where we do temporary >> hacks to get through forks and such.] >> Another cleanup could be to make this clearer. > > One thought I had, and I'm just thinking out loud here, is to rephrase > this invariant as "An inferior always has a program space." Then, > given that there is always an inferior, it falls out that there will > also always be a program space. > > But, at least to this reader, program spaces can't ever be thought of > as being created on their own, they are only created when an inferior > is created, and deleted when the last inferior using it is deleted. > That includes the initial program space, which leads to the thought of > merging the creation of the initial program space and initial > inferior. > Doing that feels clearer to me than initializing them separately, > given that we're going to be actively deleting program spaces when the > last-using inferior is deleted. It makes sense to me. If we tie the creation/deletion of program spaces with the creation/deletion of inferiors, could we go further and tie the concept of current program space and current inferior? I see very often a set_current_inferior (inf) followed by a set_current_program_space (inf->pspace). I don't really know when we would want a current program space that is not the program space of our current inferior. From what I can see, the only times set_current_program_space is called alone is in constructs like this: old_chain = save_current_program_space (); ALL_PSPACES (ss) { set_current_program_space (ss); clear_section_table (current_target_sections); exec_close (); } do_cleanups (old_chain); where exec_close accesses the global "current_program_space". So in reality, it is passing a parameter indirectly using a global variable. I suppose we should rather see: ALL_PSPACES (ss) { clear_section_table (current_target_sections); exec_close (ss); } I realize that there is a lot of such indirect parameter passing in gdb. It wouldn't be easy do to such a change, but I think it would help in many regards.
On Mon, Sep 29, 2014 at 10:48 AM, Simon Marchi <simon.marchi@ericsson.com> wrote: > [...] > If we tie the creation/deletion of program spaces with the creation/deletion of > inferiors, could we go further and tie the concept of current program space and > current inferior? I see very often a set_current_inferior (inf) followed by a > set_current_program_space (inf->pspace). I don't really know when we would want > a current program space that is not the program space of our current inferior. This would be nice alright. There are special cases, which we can handle in whatever way works, but IWBN if this was the normal way of maintaining such state - anytime we can remove unnecessary global state "works for me". > From what I can see, the only times set_current_program_space is called alone > is in constructs like this: > > old_chain = save_current_program_space (); > ALL_PSPACES (ss) > { > set_current_program_space (ss); > clear_section_table (current_target_sections); > exec_close (); > } > do_cleanups (old_chain); > > where exec_close accesses the global "current_program_space". So in reality, > it is passing a parameter indirectly using a global variable. I suppose we should > rather see: > > ALL_PSPACES (ss) > { > clear_section_table (current_target_sections); current_target_sections is actually a member of the current_program_space, so this could be improved even more (gotta love macros ...). > exec_close (ss); > } > > I realize that there is a lot of such indirect parameter passing in gdb. It > wouldn't be easy do to such a change, but I think it would help in many regards. Agreed! I think it's a longterm todo on everyone's list.
diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c index 59717ca..ba710ff 100644 --- a/gdb/mi/mi-main.c +++ b/gdb/mi/mi-main.c @@ -1951,6 +1951,8 @@ mi_cmd_remove_inferior (char *command, char **argv, int argc) } delete_inferior_1 (inf, 1 /* silent */); + + prune_program_spaces (); }