From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 17219 invoked by alias); 28 Sep 2014 20:16:25 -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 17208 invoked by uid 89); 28 Sep 2014 20:16:24 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.4 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-f180.google.com Received: from mail-vc0-f180.google.com (HELO mail-vc0-f180.google.com) (209.85.220.180) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Sun, 28 Sep 2014 20:16:23 +0000 Received: by mail-vc0-f180.google.com with SMTP id hq12so3584291vcb.25 for ; Sun, 28 Sep 2014 13:16:20 -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=5+RT4hC0QRVZ8gxRObRZDJoxPVBTF9aJ0gNxrxSfFLo=; b=a98JgQQCQdZ09lg5NKi23CIrdzqrdyUW2fqYymPtVWDXIPIISDj8NXoTrxOZQG1TUL lqbSkmuxsY9LRe30T3h/1ZUoXB4TBeN5wTwk6Yow/ko6jM33ube51FqGbgMc62+LA08/ 3RuAhNtJDXEIWHOjvUbiO6hrkTH/P5zMNChspkrXzCHl24+snurDEuMbb1P/k6WR1lhw FehKzE6O5TCBPsVM+tDLP15RnRUwkIdbpZFC7khKH/1go/hyR7pZTfV92vld7Dc60bxi Gai7Dwv7wPfGiI+qrxp+vifkPadMgQV8XlQ5t/iE5Tv3myd8/3HxLJQJVU1ILYZQ37JR 9+6A== X-Gm-Message-State: ALoCoQmYEjRigrTfsbcmkk1CZYwopmpING+Rw1akOg69b/PQYqL6rgfVxp/GyudAAynFetK0rhiZ MIME-Version: 1.0 X-Received: by 10.52.146.17 with SMTP id sy17mr21981906vdb.29.1411935380716; Sun, 28 Sep 2014 13:16:20 -0700 (PDT) Received: by 10.52.181.65 with HTTP; Sun, 28 Sep 2014 13:16:20 -0700 (PDT) In-Reply-To: <54242FBD.7030408@ericsson.com> References: <1411593539-6507-1-git-send-email-simon.marchi@ericsson.com> <54242FBD.7030408@ericsson.com> Date: Sun, 28 Sep 2014 20:16: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/msg00812.txt.bz2 On Thu, Sep 25, 2014 at 8:07 AM, Simon Marchi 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.