From: Simon Marchi <simon.marchi@ericsson.com>
To: Doug Evans <dje@google.com>
Cc: gdb-patches <gdb-patches@sourceware.org>
Subject: Re: [PATCH] Add call to prune_program_spaces in mi_cmd_remove_inferior
Date: Thu, 25 Sep 2014 15:07:00 -0000 [thread overview]
Message-ID: <54242FBD.7030408@ericsson.com> (raw)
In-Reply-To: <CADPb22RUCDTyQd0qtJBcJX56mpk4C_RjZn3pRobKXFHBCnc42w@mail.gmail.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
next prev parent reply other threads:[~2014-09-25 15:07 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-09-24 21:19 Simon Marchi
2014-09-24 22:43 ` Doug Evans
2014-09-25 15:07 ` Simon Marchi [this message]
2014-09-28 20:16 ` Doug Evans
2014-09-28 20:54 ` Doug Evans
2014-09-29 17:48 ` Simon Marchi
2014-09-29 23:16 ` Doug Evans
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=54242FBD.7030408@ericsson.com \
--to=simon.marchi@ericsson.com \
--cc=dje@google.com \
--cc=gdb-patches@sourceware.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).