From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 6149 invoked by alias); 30 Nov 2011 18:54:19 -0000 Received: (qmail 6104 invoked by uid 22791); 30 Nov 2011 18:54:16 -0000 X-SWARE-Spam-Status: No, hits=-7.5 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI,RP_MATCHES_RCVD,SPF_HELO_PASS,TW_BJ X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 30 Nov 2011 18:53:58 +0000 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id pAUIrtqQ003150 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Wed, 30 Nov 2011 13:53:55 -0500 Received: from ns3.rdu.redhat.com (ns3.rdu.redhat.com [10.11.255.199]) by int-mx02.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id pAUIrsr3011446; Wed, 30 Nov 2011 13:53:54 -0500 Received: from barimba (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by ns3.rdu.redhat.com (8.13.8/8.13.8) with ESMTP id pAUIrq36030598; Wed, 30 Nov 2011 13:53:53 -0500 From: Tom Tromey To: Pedro Alves Cc: gdb-patches@sourceware.org Subject: Re: [RFC/WIP PATCH 06/14] Add base itsets support References: <20111128153742.17761.21459.stgit@localhost6.localdomain6> <20111128153926.17761.97553.stgit@localhost6.localdomain6> Date: Wed, 30 Nov 2011 18:54:00 -0000 In-Reply-To: <20111128153926.17761.97553.stgit@localhost6.localdomain6> (Pedro Alves's message of "Mon, 28 Nov 2011 15:39:26 +0000") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.0.90 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain 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 X-SW-Source: 2011-11/txt/msg00853.txt.bz2 >>>>> "Pedro" == Pedro Alves writes: Pedro> This started out by reusing the itsets parsing code in the old Pedro> multi-process branch in CVS, but was actually quite limited, and in Pedro> need of a full rewrite. I found that Tromey's struct itset & co from Pedro> the archer-tromey-multi-inferior arch branch was a good clean start Pedro> design-wise (thanks Tom! :-) ). My pleasure. Pedro> - A `[' command. This is a prefix command, used to execute a command Pedro> with a given focus, without having to flip the current focus with Pedro> itfocus. Cute implementation approach :) I don't think completion will work right for this, even if you add a completer for the new command. It would be good to have a way do "command" completion but then further delegate to the command's completer as appropriate. Pedro> Having played with this a lot, I'm not sure anymore requiring [] Pedro> everywhere is a good idea. It's a bit hard to type. We can get rid Pedro> of the need for '[]' if we forbid spaces in specs. Then the [] prefix Pedro> command could be replaced with the "itfocus" command taking an Pedro> optional command to execute, much like TotalView's dfocus command. Pedro> We could add an 'f' alias for itfocus. E.g., It would be fine by me. Actually, since you have used it, I think you should present it exactly as you would like it, then see what happens. It is generally better, I think, to have a concrete proposal, as worked out as possible. Otherwise the discussion can be speculative and veer into the bikesheddy. Pedro> + /* Add this itset to the end of the chain so that a list of Pedro> + breakpoints will come out in order of increasing numbers. */ s/breakpoints/itsets/ Pedro> +static int Pedro> +exec_contains_inferior (struct itset_elt *base, struct inferior *inf) Pedro> +{ Pedro> + struct itset_elt_exec *exec = (struct itset_elt_exec *) base; Pedro> + Pedro> + /* FIXME: smarter compare. */ Pedro> + return (inf->pspace->ebfd != NULL Pedro> + && strcmp (exec->exec_name, Pedro> + bfd_get_filename (inf->pspace->ebfd)) == 0); The FIXME is my fault; but still should be fixed. I wonder whether the 'exec' style should be extended to: 1. Allow matching any objfile in the inferior, not just the main executable. If not, it seems like a useful new sort of itset to provide. 2. Match just the path components provided by the user, so that the BFD /usr/bin/gdb will match user inputs "gdb" and "bin/gdb". Pedro> +static void Pedro> +restore_itset (void *arg) Pedro> +{ Pedro> + struct itset *saved_itset = arg; Pedro> + Pedro> + current_itset = saved_itset; Pedro> +} Pedro> + Pedro> +/* Save the current itset so that it may be restored by a later call Pedro> + to do_cleanups. Returns the struct cleanup pointer needed for Pedro> + later doing the cleanup. */ Pedro> + Pedro> +static struct cleanup * Pedro> +save_current_itset (void) Pedro> +{ Pedro> + struct cleanup *old_chain = make_cleanup (restore_itset, Pedro> + current_itset); Pedro> + Pedro> + return old_chain; Pedro> +} I think save_current_itset should acquire a reference, and restore_itset should release it. Otherwise I think the right call to temp_itfocus_command can free current_itset, causing a crash. Pedro> +static int Pedro> +exec_contains_thread (struct itset_elt *base, struct thread_info *thr) I think this one should delegate to exec_contains_inferior for better maintenance. Pedro> +void Pedro> +dump_itset (struct itset *itset) Pedro> +{ Pedro> +} :-) Pedro> +static void Pedro> +itfocus_command (char *spec, int from_tty) I wonder if it would be better to put the guts of this into a utility function and then call that from MI as well. Also I suspect we'd want some kind of MI async notification for changes to the current itset..? Pedro> +static void Pedro> +itsets_info (char *arg, int from_tty) Pedro> +{ [...] Pedro> + if (e->number > 0) It would be nice to have a "maint info" variant that skips this check. Pedro> +static void Pedro> +temp_itfocus_command (char *spec, int from_tty) Pedro> +{ Pedro> + struct itset *itset; Pedro> + struct cleanup *old_chain; Pedro> + Pedro> + if (spec == NULL) Pedro> + error_no_arg (_("i/t set")); Pedro> + Pedro> + /* Evil hack. We know the command is `['. */ Pedro> + spec--; I forget, are leading spaces skipped by the CLI command processor? Pedro> + add_com ("[", no_class, temp_itfocus_command, _("\ Pedro> +Change the set of current inferiors/threads.")); I'm a little surprised that this didn't need some change in the CLI code, like "!" did. Pedro> + add_com ("undefset", no_class, undefset_command, _("\ How about "delete itset ..." instead? Pedro> + add_com ("whichsets", no_class, whichsets_command, _("\ [...] Pedro> + add_com ("viewset", no_class, viewset_command, _("\ [...] How about "info" subcommands for these instead? As an aside, I have been thinking that some important (non-command) concepts in gdb should be in the online help. For example, I think it would be nice if "help linespec" described the possible linespecs; then other things like "help break" could refer to that. Anyway, "help itset" might be nice to have. Pedro> +/* Create a new I/T set which represents the current inferior, at the Pedro> + time that this call is made. */ Pedro> + Pedro> +struct itset *itset_create_current (void); Reading this again, I realize that the comment should specify whether a static or dynamic set is returned. Pedro> +/* Return true if the inferior is contained in the I/T set. */ Pedro> + Pedro> +int itset_contains_inferior (struct itset *itset, struct inferior *inf); Pedro> +/* Return true if the inferior is contained in the I/T set. */ Pedro> + Pedro> +int itset_member (struct itset *itset, int inf_id, int thread_id); Perhaps the names should be closer to indicate that these are just overloads. Pedro> +const char *itset_name (const struct itset *itset); Pedro> +const char *itset_spec (const struct itset *itset); Pedro> + Pedro> +int itset_is_empty (const struct itset *itset); These need comments. For this module I took the API comments in the header file approach... Pedro> +extern struct itset *current_itset; Ditto. Tom