Re: [PATCH] Add call to prune_program_spaces in mi_cmd_remove_inferior

[Doug Evans <dje at google dot com>]

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.