From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 21887 invoked by alias); 25 Sep 2014 15:07:46 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 21871 invoked by uid 89); 25 Sep 2014 15:07:45 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.3 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=ham version=3.3.2 X-HELO: usevmg21.ericsson.net Received: from usevmg21.ericsson.net (HELO usevmg21.ericsson.net) (198.24.6.65) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-SHA encrypted) ESMTPS; Thu, 25 Sep 2014 15:07:44 +0000 Received: from EUSAAHC004.ericsson.se (Unknown_Domain [147.117.188.84]) by usevmg21.ericsson.net (Symantec Mail Security) with SMTP id E4.E3.25146.217D3245; Thu, 25 Sep 2014 10:49:22 +0200 (CEST) Received: from [142.133.110.254] (147.117.188.8) by smtps-am.internal.ericsson.com (147.117.188.84) with Microsoft SMTP Server (TLS) id 14.3.174.1; Thu, 25 Sep 2014 11:07:42 -0400 Message-ID: <54242FBD.7030408@ericsson.com> Date: Thu, 25 Sep 2014 15:07:00 -0000 From: Simon Marchi User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.1.2 MIME-Version: 1.0 To: Doug Evans CC: gdb-patches Subject: Re: [PATCH] Add call to prune_program_spaces in mi_cmd_remove_inferior References: <1411593539-6507-1-git-send-email-simon.marchi@ericsson.com> In-Reply-To: Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2014-09/txt/msg00737.txt.bz2 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