Re: [PATCH] Add call to prune_program_spaces in mi_cmd_remove_inferior

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

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