From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 17502 invoked by alias); 16 Dec 2011 17:20:09 -0000 Received: (qmail 17471 invoked by uid 22791); 16 Dec 2011 17:20:06 -0000 X-SWARE-Spam-Status: No, hits=-1.7 required=5.0 tests=AWL,BAYES_00,TW_BJ X-Spam-Check-By: sourceware.org Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 16 Dec 2011 17:19:51 +0000 Received: from nat-ies.mentorg.com ([192.94.31.2] helo=EU1-MAIL.mgc.mentorg.com) by relay1.mentorg.com with esmtp id 1RbbRm-0005lP-2H from pedro_alves@mentor.com ; Fri, 16 Dec 2011 09:19:50 -0800 Received: from scottsdale.localnet ([172.16.63.104]) by EU1-MAIL.mgc.mentorg.com with Microsoft SMTPSVC(6.0.3790.1830); Fri, 16 Dec 2011 17:19:48 +0000 From: Pedro Alves To: gdb-patches@sourceware.org Subject: Re: [RFC/WIP PATCH 06/14] Add base itsets support Date: Fri, 16 Dec 2011 17:26:00 -0000 User-Agent: KMail/1.13.6 (Linux/2.6.38-13-generic; KDE/4.7.2; x86_64; ; ) Cc: Tom Tromey References: <20111128153742.17761.21459.stgit@localhost6.localdomain6> <20111128153926.17761.97553.stgit@localhost6.localdomain6> In-Reply-To: MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-15" Content-Transfer-Encoding: 7bit Message-Id: <201112161719.42209.pedro@codesourcery.com> X-IsSubscribed: yes 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-12/txt/msg00518.txt.bz2 On Wednesday 30 November 2011 18:53:52, Tom Tromey wrote: > >>>>> "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. Thing is I have been focused much more on making the inferior control infrustructure actually work, and leaving the actual syntax etc. without that much attention. I have adjusted everything to not need the []'s, and I've rewriten itsets parsing and structures to support the idea that each object kind needs its own prefix letter, and dot is an intersection operator (e.g., i1.t2.c3), and I like the result. But I think it will need a bit more experience and thought. It's missing some important predicates. And I'm still wondering if we can / how can we unify the selected thread/inferior and current focus concepts more tightly into a single entity. > 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/ > Thanks, fixed. > 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. Yeah, though I think this would be a separate predicate. I mean, "do something whenever the current thread is in DSO with objfile named FOO" is a different test to "do something whenever the current thread is part of any inferior running program "EXEC". > > 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". To be honest, I really haven't really played much with 'exec' sets. I think I only tried it once. :-) I was even thinking of removing it from the first iteration, but ended up leaving it in place, since it was already there. > > 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. I don't think so, because the callers are expected to do: save_current_itset (); current_itset = itset; That is, the reference was transfered to the cleanup. But I did the change just in case anyway. Thanks. > > 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..? Most probably. MI support is a TODO yet. > > 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. Yeah, done. > > 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? Looks like it. In any case, this is gone. `[' is no longer used. > > 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. It actually needed. You missed the cli-decode.c change. :-) --- a/gdb/cli/cli-decode.c +++ b/gdb/cli/cli-decode.c @@ -1132,6 +1132,9 @@ find_command_name_length (const char *text) if (*p == '!') return 1; + if (*p == '[') + return 1; /* The temporary focus command. */ + But in any case, this is gone. > > Pedro> + add_com ("undefset", no_class, undefset_command, _("\ > > How about "delete itset ..." instead? Yeah, it's more gdb-ish. I added these because they are what HPD specifies. I'm going to post out a WIP v2 series with that not done yet though. Sorry about that. > > Pedro> + add_com ("whichsets", no_class, whichsets_command, _("\ > [...] > Pedro> + add_com ("viewset", no_class, viewset_command, _("\ > [...] > > How about "info" subcommands for these instead? Makes sense too. > > > 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. Indeed. Heck, most of the manual could be online. I'll have to leave this for v3 though. > > 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. Yeah. It's actually dynamic, and the comment is misleading. I've adjusted it. > > 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. itset_member is actually a relic of the old itsets implementation. It doesn't exist anymore, but the declaration was left behind. Removed now. > > 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. That's what WIP means. :-) There are a lot of comments missing throughout the whole series. Fixed this instance now. Thanks! -- Pedro Alves