This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [RFC/WIP PATCH 06/14] Add base itsets support
>>>>> "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.
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