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

[Pedro Alves <pedro at codesourcery dot com>]

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