This is the mail archive of the
mailing list for the GDB project.
Re: [PATCH] Add call to prune_program_spaces in mi_cmd_remove_inferior
- From: Simon Marchi <simon dot marchi at ericsson dot com>
- To: Doug Evans <dje at google dot com>
- Cc: gdb-patches <gdb-patches at sourceware dot org>
- Date: Thu, 25 Sep 2014 11:07:41 -0400
- Subject: Re: [PATCH] Add call to prune_program_spaces in mi_cmd_remove_inferior
- Authentication-results: sourceware.org; auth=none
- References: <1411593539-6507-1-git-send-email-simon dot marchi at ericsson dot com> <CADPb22RUCDTyQd0qtJBcJX56mpk4C_RjZn3pRobKXFHBCnc42w at mail dot gmail dot com>
On 2014-09-24 06:43 PM, Doug Evans wrote:
> 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))
(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.
> 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.
> 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
> 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.