Re: [RFC/WIP PATCH 06/14] Add base itsets support

[Tom Tromey <tromey at redhat dot com>]

>>>>> "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