From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 24936 invoked by alias); 24 Sep 2014 22:43:41 -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 24923 invoked by uid 89); 24 Sep 2014 22:43:40 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.5 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_LOW,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-vc0-f181.google.com Received: from mail-vc0-f181.google.com (HELO mail-vc0-f181.google.com) (209.85.220.181) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Wed, 24 Sep 2014 22:43:38 +0000 Received: by mail-vc0-f181.google.com with SMTP id hq12so851858vcb.26 for ; Wed, 24 Sep 2014 15:43:36 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc:content-type; bh=4WH8gjY/t1V5l/avkPY697/EJOxAXH8eq0QipbdxpIA=; b=NcAMjHhlRHvdLFqhl91KkHlqkpHFjgak+UQtcDrFMRlhNaJNmIOVXYvQQrPRm1rItc T8xKJRXp40Xk5qS+8q8pSAAnlSBoTxx2l1DP+l9WQpQkZSf2F/U212E+hP6/eH50vI9b OXc8JEV5/XRxdev3f96yrKZG/6oR/lx6B7fsDdSop02IcZkrE/0K7/kyyqgSFbSgnjog W/braqSj9aRiX6f9AxHDPJcYoelxnZw+p30bY10/v4tS8zqDqyBSHNmbCdKujZ9aH4Jr ZgDmmJTfBvGX/CrdqWFVsBHhTUsHUHhAC0UPBWhPFttrqXyHnKw1sIgBi4Rf1Oi5hnCl YZ3Q== X-Gm-Message-State: ALoCoQkLm42vL3V5NqhGXqTl865hNMzvNg+wBgQWhrwxOhHlG9soofelv5LnEG/xHILK7oMgbYrN MIME-Version: 1.0 X-Received: by 10.52.165.97 with SMTP id yx1mr6743201vdb.15.1411598616450; Wed, 24 Sep 2014 15:43:36 -0700 (PDT) Received: by 10.52.181.65 with HTTP; Wed, 24 Sep 2014 15:43:36 -0700 (PDT) In-Reply-To: <1411593539-6507-1-git-send-email-simon.marchi@ericsson.com> References: <1411593539-6507-1-git-send-email-simon.marchi@ericsson.com> Date: Wed, 24 Sep 2014 22:43:00 -0000 Message-ID: Subject: Re: [PATCH] Add call to prune_program_spaces in mi_cmd_remove_inferior From: Doug Evans To: Simon Marchi Cc: gdb-patches Content-Type: text/plain; charset=UTF-8 X-IsSubscribed: yes X-SW-Source: 2014-09/txt/msg00719.txt.bz2 On Wed, Sep 24, 2014 at 2:18 PM, Simon Marchi 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.