[RFC/WIP PATCH 06/14] Add base itsets support
Pedro Alves
pedro@codesourcery.com
Fri Dec 16 17:26:00 GMT 2011
On Wednesday 30 November 2011 18:53:52, Tom Tromey wrote:
> >>>>> "Pedro" == Pedro Alves <pedro@codesourcery.com> 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
More information about the Gdb-patches
mailing list