Add call to prune_program_spaces in mi_cmd_remove_inferior

  ...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(+)
  

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.