[RFA,1/2] Add completion for COMMAND in 'thread apply all|ID... COMMAND'

  This patch adds logic to complete the COMMAND part of the
'thread apply all|ID...' command.

gdb/ChangeLog
2019-04-21  Philippe Waroquiers  <philippe.waroquiers@skynet.be>

	* thread.c (thread_apply_all_command_completer,
	thread_apply_id_command_completer): New functions.
	(thread_apply_all_command, thread_apply_command): Add comment
	referring to the corresponding completer function.
	(_initialize_thread): Setup completers for thread apply all,
	thread apply ID, taas, tfaas.

gdb/testsuite/ChangeLog
2019-04-21  Philippe Waroquiers  <philippe.waroquiers@skynet.be>

	* gdb.threads/pthreads.exp: Test COMMAND completion.
---
 gdb/testsuite/gdb.threads/pthreads.exp | 25 ++++++++
 gdb/thread.c                           | 84 +++++++++++++++++++++++---
 2 files changed, 102 insertions(+), 7 deletions(-)
  

>>>>> "Philippe" == Philippe Waroquiers <philippe.waroquiers@skynet.be> writes:

Philippe> This patch adds logic to complete the COMMAND part of the
Philippe> 'thread apply all|ID...' command.

Thanks for the patch.  I think this is a good idea.

Philippe> +/* Skips the known arguments of thread apply all
Philippe> +   and then invokes the usual command_completer.  */
Philippe> +
Philippe> +static void
Philippe> +thread_apply_all_command_completer (struct cmd_list_element *cmd,
Philippe> +				    completion_tracker &tracker,
Philippe> +				    const char *text, const char *word)
Philippe> +{
Philippe> +  while (text != NULL)
Philippe> +    {
Philippe> +      qcs_flags dummy;
Philippe> +
Philippe> +      if (check_for_argument (&text, "-ascending", strlen ("-ascending")))
Philippe> +	{
Philippe> +	  text = skip_spaces (text);
Philippe> +	  continue;
Philippe> +	}
Philippe> +
Philippe> +      if (parse_flags_qcs ("thread apply all COMMAND completer", &text, &dummy))
Philippe> +	continue;

There are some corner cases that aren't handled here, that maybe should
be.

One example is if the input text is "thread apply all -ascen".
It seems like this should complete to "-ascending " (with the trailing space).

If the input text is "thread apply all -ascending" (no trailing space),
then won't it start completing commands without adding a trailing space?
This would result in an invalid command.

A similar thing applies if the completion occurs just after an isolated "-".

These aren't all equally important, but I think at minimum completion
shouldn't result in something invalid.

thanks,
Tom
  

On Thu, 2019-04-25 at 07:30 -0600, Tom Tromey wrote:
> > > > > > "Philippe" == Philippe Waroquiers <philippe.waroquiers@skynet.be> writes:
> 
> Philippe> This patch adds logic to complete the COMMAND part of the
> Philippe> 'thread apply all|ID...' command.
> 
> Thanks for the patch.  I think this is a good idea.
> 
> Philippe> +/* Skips the known arguments of thread apply all
> Philippe> +   and then invokes the usual command_completer.  */
> Philippe> +
> Philippe> +static void
> Philippe> +thread_apply_all_command_completer (struct cmd_list_element *cmd,
> Philippe> +				    completion_tracker &tracker,
> Philippe> +				    const char *text, const char *word)
> Philippe> +{
> Philippe> +  while (text != NULL)
> Philippe> +    {
> Philippe> +      qcs_flags dummy;
> Philippe> +
> Philippe> +      if (check_for_argument (&text, "-ascending", strlen ("-ascending")))
> Philippe> +	{
> Philippe> +	  text = skip_spaces (text);
> Philippe> +	  continue;
> Philippe> +	}
> Philippe> +
> Philippe> +      if (parse_flags_qcs ("thread apply all COMMAND completer", &text, &dummy))
> Philippe> +	continue;
> 
> There are some corner cases that aren't handled here, that maybe should
> be.
> 
> One example is if the input text is "thread apply all -ascen".
> It seems like this should complete to "-ascending " (with the trailing space).
Currently, the unpatched GDB HEAD does not complete -ascen.
I agree it should complete but it does not :(.
(the GDB behaviour regarding command option syntax is wildly inconsistent).


> 
> If the input text is "thread apply all -ascending" (no trailing space),
> then won't it start completing commands without adding a trailing space?
> This would result in an invalid command.
I cannot produce this problem (or a bug).

In an unpatched GDB HEAD, typing tab after each letter of -ascending
does not do anything : in  'thread apply all -ascending',
the last thing that completes is 'all'.

With this patch, the behaviour is not very different, except
that the patched GDB 'helps' once the user has completely typed
-ascending: then TAB will insert a space, wonderful help :).

After that, it looks to me that correct command completion is happening.

It would be nice to have better completion for -ascending
and/or have thread apply all -asc <return>
working properly.  Today, it does:
   (gdb) thread apply all -asc

   Thread 4 (Thread 0x7ffff681a700 (LWP 23471)):
   Undefined command: "-asc".  Try "help".
   (gdb) 

To the contrary, 'bt full', 'bt fu' will work the same way !
But similarly, bt fu<TAB> does not complete.

I guess it should be possible with a slight change of this patch to
have at least -asc<TAB> complete.

> 
> A similar thing applies if the completion occurs just after an isolated "-".
Same as above: not clear to me how to create a problem.

> 
> These aren't all equally important, but I think at minimum completion
> shouldn't result in something invalid.
> 
> thanks,
> Tom
  

>>>>> "Philippe" == Philippe Waroquiers <philippe.waroquiers@skynet.be> writes:

>> One example is if the input text is "thread apply all -ascen".
>> It seems like this should complete to "-ascending " (with the trailing space).

Philippe> Currently, the unpatched GDB HEAD does not complete -ascen.
Philippe> I agree it should complete but it does not :(.
Philippe> (the GDB behaviour regarding command option syntax is wildly inconsistent).

Yeah, what I meant here is that it would be good if the new completion
code did this, not that this was something that currently occurred.

Tom
  

On Tue, 2019-04-30 at 09:13 -0600, Tom Tromey wrote:
> > > > > > "Philippe" == Philippe Waroquiers <philippe.waroquiers@skynet.be> writes:
> > > One example is if the input text is "thread apply all -ascen".
> > > It seems like this should complete to "-ascending " (with the trailing space).
> 
> Philippe> Currently, the unpatched GDB HEAD does not complete -ascen.
> Philippe> I agree it should complete but it does not :(.
> Philippe> (the GDB behaviour regarding command option syntax is wildly inconsistent).
> 
> Yeah, what I meant here is that it would be good if the new completion
> code did this, not that this was something that currently occurred.
Yes, effectively, the fact that GDB does not complete arguments is surprising
(while e.g. bash has completion for arguments of almost all commands, such as:
      ls --alm<TAB>
  =>  ls --almost-all
:).

I will take a look at how to have completions for command arguments,
but IMO that is better done as a separate patch to provide a more general
solution (i.e. not only for commands that are launching other commands,
which is what this patch is providing).

So, maybe this patch can be pushed as is (assuming no other comments) ?

Thanks

Philippe
  

On 5/1/19 5:53 AM, Philippe Waroquiers wrote:
> On Tue, 2019-04-30 at 09:13 -0600, Tom Tromey wrote:
>>>>>>> "Philippe" == Philippe Waroquiers <philippe.waroquiers@skynet.be> writes:
>>>> One example is if the input text is "thread apply all -ascen".
>>>> It seems like this should complete to "-ascending " (with the trailing space).
>>
>> Philippe> Currently, the unpatched GDB HEAD does not complete -ascen.
>> Philippe> I agree it should complete but it does not :(.
>> Philippe> (the GDB behaviour regarding command option syntax is wildly inconsistent).
>>
>> Yeah, what I meant here is that it would be good if the new completion
>> code did this, not that this was something that currently occurred.
> Yes, effectively, the fact that GDB does not complete arguments is surprising
> (while e.g. bash has completion for arguments of almost all commands, such as:
>       ls --alm<TAB>
>   =>  ls --almost-all
> :).
> 
> I will take a look at how to have completions for command arguments,
> but IMO that is better done as a separate patch to provide a more general
> solution (i.e. not only for commands that are launching other commands,
> which is what this patch is providing).

I'm going to post the "print -OPT" patch soon, which includes a generic
framework for command options, and integrates with completion.  It's the
patch that I mentioned to you a while ago when we discussed the "qvcs" flags
patches.  I've pushed it to the users/palves/cli-options branch on
sourceware last night, if you guys want to take a look meanwhile.  I'd
like to post it for discussion before we decide how to go forward on the
"/" patches.  I'm on holiday today, and likely OOO for most of it, so not
sure I'll be able to post it today.

Funny enough, "thread apply all -ascen[TAB]" is one of the options
that I converted to the new scheme in the branch.

Thanks,
Pedro Alves
  

On Wed, 2019-05-01 at 11:18 +0100, Pedro Alves wrote:
> > I will take a look at how to have completions for command arguments,
> > but IMO that is better done as a separate patch to provide a more general
> > solution (i.e. not only for commands that are launching other commands,
> > which is what this patch is providing).
> 
> I'm going to post the "print -OPT" patch soon, which includes a generic
> framework for command options, and integrates with completion.  It's the
> patch that I mentioned to you a while ago when we discussed the "qvcs" flags
> patches.  I've pushed it to the users/palves/cli-options branch on
> sourceware last night, if you guys want to take a look meanwhile.
I quickly tried it, it is really nice to have command arguments completion.

The "/" command aims at solving a problem (temporary change some option
for one command) that is at least partially covered by this.
=> possibly having both this patch and the "/" command is an overkill.

I first give some comments about the patch (by pasting/commenting the
commit log).  Then at the end, I will list a few points that "/" covers
and that this does not.  Then to be seen if the additional points provided
by "/" are worth keeping the "/" on top of this patch.

>commit log:   Introduce generic CLI options framework, support "print -option val --"
>commit log:    
>commit log:    And "bt -OPTS" too.  And probably others I no longer remember.
>commit log:    
>commit log:    TAB completion fully supported.  That's pretty neat I think.  Give it
>commit log:    a go!  Try:
>commit log:    
>commit log:      (gdb) p -[TAB]
>commit log:      -address         -array-indexes   -null-stop       -pretty          -
static-members  -union
>commit log:      -array           -elements        -object          -repeats         -
symbol          -vtbl
>commit log:    
>commit log:      (gdb) bt -[TAB]
>commit log:      -entry-values         -frame-arguments      -full                 -
hide                 -no-filters           -raw-frame-arguments
>commit log:    
>commit log:    Note that:
>commit log:    
>commit log:     - you can shorten option names, as long as unambiguous.
I effectively see the bt command detecting the ambiguity:
  (gdb) bt -f
  Ambiguous option at: -f
  (gdb) 
but the print command does not:
  (gdb) p -a<TAB>
  -address        -array          -array-indexes  
  (gdb) p -a 1
  $2 = 1
  (gdb)
So, unclear which of the 3 options was activated by -a.


>commit log:     - For boolean options, 0/1 stand for off/on.
>commit log:     - For boolean options, "true" is implied.
Good idea to not oblige to input 0/1.
Why not the same for the 'integer options' ?
When no integer value is provided, that could equally mean:
  'unlimited' (or UINT/INT_MAX).
(that is what "/" considers).

>commit log:    
>commit log:    So these are all equivalent:
>commit log:    
>commit log:     (gdb) print -object on -static-members off -pretty on foo
>commit log:     (gdb) print -object -static-members off -pretty foo
>commit log:     (gdb) print -object -static-members 0 -pretty foo
>commit log:     (gdb) print -o -st 0 -p foo
>commit log:    
>commit log:    And so are these:
>commit log:    
>commit log:     (gdb) bt -entry-values both
>commit log:     (gdb) bt -e b
>commit log:    
>commit log:    "--" explicitly terminates options.  This is necessary because some
>commit log:    expressions may start with "-".  E.g., if you have a variable called
>commit log:    "object" and you want to print its negative: "-object":
>commit log:    
>commit log:       (gdb) print -- -object
>commit log:    
>commit log:    That's a minor potential script breakage, but I think it's well worth
>commit log:    it.
Yes, that does not look to be a big problem.  If ever, it might be possible
to reduce the nr of breakage, e.g. : when an unrecognised option is at the end
of the args or is only followed by non options and/or non recognised options,
then the whole set of args starting at the unrecognised option might be considered
to be the expression.

I have however seen a regression in the print command
HEAD:
(gdb) p -1
$1 = -1
(gdb) 

With the patch:
(gdb) p -1
$3 = 1
(gdb) 

So, either print should tell that -1 is an unknown option, 
or (maybe better?) it should guess that -1 is the expression to print,
as suggested above ?
(there are a bunch of tests failing with the patch, I guess most
are due to the above breakage reason).

>commit log:    
>commit log:    Note that the code is organized such that some of the options and the
>commit log:    "set/show" commands code is shared.  In particular, the "print"
>commit log:    options and the corresponding "set print" commands are defined with
>commit log:    the same structures.  Same thing for some of the "bt" options.
>commit log:    
>commit log:    I think this is a better approach than trying to cram a bunch of
>commit log:    unrelated settings under a single global one-character namespace.
Note that the "/" command uses the upper case range as "prefix" characters,
so the namespace is one-character per level, with as many levels as desired.

>commit log:    
>commit log:    Still missing:
>commit log:    
>commit log:     - run it by upstream, as an RFC.
>commit log:     - comments.  mention why polymorphism isn't done with virtual methods.
>commit log:     - convert more commands.
>commit log:     - not integrated with the new "thread apply" "vqs" flags yet, which
>commit log:       had gone in after this was originally written.
>commit log:     - write tests.
>commit log:     - write docs.

>   I'd
> like to post it for discussion before we decide how to go forward on the
> "/" patches.  I'm on holiday today, and likely OOO for most of it, so not
> sure I'll be able to post it today.
> 
> Funny enough, "thread apply all -ascen[TAB]" is one of the options
> that I converted to the new scheme in the branch.
> 
> Thanks,
> Pedro Alves

There are some differences between what this patch provides
and the "/" command.  I am listing them below, and discuss
if these differences are worth keeping "/" or not.

"/" provides a quicker/faster to type way to set options,
but it is less 'discoverable' than the completion feature,
which probably all GDB users know already (and would like
to have for the options).
For very often options that share an initial word, we might
define an alternate option e.g.
   print [-A | -array] [-i | -array-indexes]
if we believe these options are so frequently used that
they must be fast to type.

Also, the idea is that t"/" command will allow to redo
the previous command with additional options, eg.
(gdb) p larger
$2 = <error reading variable: object size is larger than varsize-limit
(gdb) /v
$3 = "1234567890|1234567890|1234567890"
(gdb) 

This might also be implemented with this patch e.g.
(gdb) p larger
$2 = <error reading variable: object size is larger than varsize-limit
(gdb) -v
$3 = "1234567890|1234567890|1234567890"
(gdb)
where -v would be a shortcut for relaunching the previous command as:
(gdb) p -varsize-list unlimited Larger

This patch implies to add new options to existing commands
(such as the options added to
print) to have a quick way
to change them for one command.
It looks however easy to do (in
particular as some of
the code is shared with the 'set option' code).

"/" changes the global options, then launches the given command,
and then restore the global options.
The given command can e.g. be a user/python defined command
that itself launches a bunch of other commands, which should
be influenced by the global settings.
The user must then (like with GDB 8.3) type a bunch of
  'set this'
  'set that'
  launch the user defined command
  'reset this'
  'reset that'
Or define a new user command that automates the above.

Then if the user types C-c while the middle command runs,
the options will not be restored.
Possibly we could allow a command to be optionally given
after the existing 'set command' (which means:
 change the option, runs the given command, restore the option).
That is in fact the equivalent of the "/" command, but
for just one option.
(compare with the shell:
   export DISPLAY=xxxx
   some_command
  versus
   DISPLAY=xxxx some_command).
Then most of the "/" is still available (e.g. in user
defined commands), but without the alternate "/" letters
to activate.
Alternatively, the 'try/catch' in the CLI or the 'block
of command' patch I started and never finished some
years ago.

In summary: IMO, there is not a huge set of reasons to
have both the "/" and this patch, or at least there are
reasonable ways to do what "/" provides, maybe with
little additional features such as:
  * the optional COMMAND after 
      'set some_option [COMMAND]'
  * add a systematic way to relaunch the previous command
    by starting a line with a '-' option.

Now, of course, if hundreds of GDB users are suddenly wearing
a yellow jacket, and go in the street destroying everything
while shouting 'we want the "/" command', then we might have
to rediscuss :).

Philippe
  

On 5/1/19 11:18 AM, Pedro Alves wrote:
> On 5/1/19 5:53 AM, Philippe Waroquiers wrote:
>> On Tue, 2019-04-30 at 09:13 -0600, Tom Tromey wrote:
>>>>>>>> "Philippe" == Philippe Waroquiers <philippe.waroquiers@skynet.be> writes:
>>>>> One example is if the input text is "thread apply all -ascen".
>>>>> It seems like this should complete to "-ascending " (with the trailing space).
>>>
>>> Philippe> Currently, the unpatched GDB HEAD does not complete -ascen.
>>> Philippe> I agree it should complete but it does not :(.
>>> Philippe> (the GDB behaviour regarding command option syntax is wildly inconsistent).
>>>
>>> Yeah, what I meant here is that it would be good if the new completion
>>> code did this, not that this was something that currently occurred.
>> Yes, effectively, the fact that GDB does not complete arguments is surprising
>> (while e.g. bash has completion for arguments of almost all commands, such as:
>>       ls --alm<TAB>
>>   =>  ls --almost-all
>> :).
>>
>> I will take a look at how to have completions for command arguments,
>> but IMO that is better done as a separate patch to provide a more general
>> solution (i.e. not only for commands that are launching other commands,
>> which is what this patch is providing).
> 
> I'm going to post the "print -OPT" patch soon, which includes a generic
> framework for command options, and integrates with completion.  It's the
> patch that I mentioned to you a while ago when we discussed the "qvcs" flags
> patches.  I've pushed it to the users/palves/cli-options branch on
> sourceware last night, if you guys want to take a look meanwhile.  I'd
> like to post it for discussion before we decide how to go forward on the
> "/" patches.  I'm on holiday today, and likely OOO for most of it, so not
> sure I'll be able to post it today.
> 
> Funny enough, "thread apply all -ascen[TAB]" is one of the options
> that I converted to the new scheme in the branch.

A status update -- I've been working on this as much as I'm able.  I fixed
a ton of details on the branch, plus hooked in more options to the "frame apply"
commands, wrote a testcase (need to extend it some more), wrote some
docs.  And also, today I made a replacement for your patch (this thread) that
is quite neat.  Instead of only completing on the command name itself,
my version recurses into the completion machinery, and thus completes
on the command's options correctly.  I.e., you get this:

 (gdb) thread apply all -[TAB]
 -ascending  -c          -q          -s          
 (gdb) thread apply all print -[TAB]
 -address         -array-indexes   -null-stop       -pretty          -static-members  -union           
 -array           -elements        -object          -repeats         -symbol          -vtbl            
 (gdb) thread apply all print -pretty -- [TAB]
 __init_array_start                      global                                  pthread_self@got.plt                    
 __libc_csu_fini                         global_neg                              pthread_self@plt                        
 __libc_csu_init                         int                                     pthread_setname_np                      
 ...

I force-pushed what I have to the branch.  I have not re-run the testsuite yet,
there may be regressions, though the new test exercising the new options framework
passes, at least.

Oh, one change I made was that for "print", if you want to specify - options,
then you must end the options with "--".  There was one testcase that regressed
without this, and so I concluded that this was safer.  I also noticed
that lldb's expr command does the same thing.

Note, you can do "help print" to see the available options.  That bit is
auto-generated.

I'm pretty happy and excited with the result.

My TODO says:

  - comments throughout
  - finish docs
  - NEWS
  - split patch into chunks

  - add smoke tests for "frame apply -TAB", etc.

  - The new options.exp testcase should validate parsed options, instead of being mostly focused 
    exercising completion.
    E.g., make the new maint commands print them out.

  - Hook options to "frame apply level"

  - Fix "print -element 123[TAB]".  Shouldn't complete commands unless
    there's a space after the number.

Thanks,
Pedro Alves
  

On Fri, 2019-05-10 at 01:53 +0100, Pedro Alves wrote:
> I'm pretty happy and excited with the result.
Yes, really a (lot of) nice work.
Wondering if/where I can help.
In the meantime, I quickly read the (big :) patch and played
a little bit with it.

Some feedback below, it is not yet perfect, but it gets
closer :). 

* I run the regression tests.  I have not seen any regression,
  so that it all good.

* print /x does not combine with the new options e.g.
    (gdb) print /x 4
    $2 = 0x4
    (gdb) print -pretty -- 4
    $3 = 4
    (gdb) print -pretty -- /x 4
    A syntax error in expression, near `/x 4'.
    (gdb) print /x -pretty -- 4
    No symbol "pretty" in current context.
    (gdb) 
  The problem is because the function print_command_parse_format
  is called with *expp pointing at the space character following --.
  Wondering where this is better fixed.
  Maybe at the lowest level (i.e. check_for_argument) : if an argument
  is followed by spaces, I guess these spaces should be 'eaten' by
  check_for_argument.

* print usage should probably better mention /FMT e.g.
   Usage: print [[OPTION]... --] [/FMT] [EXP]
  instead of
   Usage: print [[OPTION]... --] [EXP]

* compile usage does not mention the new print options:
   Usage: compile print[/FMT] [EXPR]

* Some 'enum' values (on, off, unlimited, ...) are not accepted as
  abbreviations, while some others are.
  E.g.  bt -e b works
  while  p -e u -- 4 does not
   (but u will complete to unlimited)

* bt fu<TAB> has changed of behaviour.
  It now gives a lot more completion possibilities.
  This comment can probably just be ignored, as HEAD does in any case
  produce not very relevant completions.
  Alternatively, it might maybe be also be possible to complete on the
  'old option qualifier style'.

* bt accepts now 2 'counts' to  limit on the nr of frame to show.
  This is a little bit confusing, so maybe the interactions between
  the 2 might be explained in the help.
  E.g. explain that the below will only give 2 frames:
    bt -limit 2 3

* The 'print' examples in the commit log are missing the trailing --

* Now that we have nice command options completions, I wonder
  if the -q -c -s options should/could not be reworded as
    -quiet -continue -silent
  That will be backward compatible, and is better ergonomy.
  I can do that as a follow-up patch if that will help you.

* display and output commands have no OPTIONS yet. 
  For display, I guess this means the 'struct display' has to store
  the option values (or maybe the string options ?).

* (gdb) thread apply 1 3 -ascenTAB will complete
  but the resulting command fails:
   (gdb) thread apply 1 3 -ascending p $pc
   Invalid thread ID: -ascending p $pc
  (gdb) 


* frame apply 10 -pasTAB completes as
   frame apply 10 -passwd 

* the help frame apply  only shows the qcs options
  (same for frame apply all)

* help thread apply: the second line for the help for -ascending
  should probably better be indented to the right, like the first line.
  

On 5/11/19 4:01 PM, Philippe Waroquiers wrote:
> On Fri, 2019-05-10 at 01:53 +0100, Pedro Alves wrote:
>> I'm pretty happy and excited with the result.
> Yes, really a (lot of) nice work.
> Wondering if/where I can help.
> In the meantime, I quickly read the (big :) patch and played
> a little bit with it.
> 
> Some feedback below, it is not yet perfect, but it gets
> closer :). 
> 
> * I run the regression tests.  I have not seen any regression,
>   so that it all good.
> 
> * print /x does not combine with the new options e.g.
>     (gdb) print /x 4
>     $2 = 0x4
>     (gdb) print -pretty -- 4
>     $3 = 4
>     (gdb) print -pretty -- /x 4
>     A syntax error in expression, near `/x 4'.
>     (gdb) print /x -pretty -- 4
>     No symbol "pretty" in current context.
>     (gdb) 
>   The problem is because the function print_command_parse_format
>   is called with *expp pointing at the space character following --.
>   Wondering where this is better fixed.
>   Maybe at the lowest level (i.e. check_for_argument) : if an argument
>   is followed by spaces, I guess these spaces should be 'eaten' by
>   check_for_argument.
> 
> * print usage should probably better mention /FMT e.g.
>    Usage: print [[OPTION]... --] [/FMT] [EXP]
>   instead of
>    Usage: print [[OPTION]... --] [EXP]
> 
> * compile usage does not mention the new print options:
>    Usage: compile print[/FMT] [EXPR]
> 
> * Some 'enum' values (on, off, unlimited, ...) are not accepted as
>   abbreviations, while some others are.
>   E.g.  bt -e b works
>   while  p -e u -- 4 does not
>    (but u will complete to unlimited)
> 
> * bt fu<TAB> has changed of behaviour.
>   It now gives a lot more completion possibilities.
>   This comment can probably just be ignored, as HEAD does in any case
>   produce not very relevant completions.
>   Alternatively, it might maybe be also be possible to complete on the
>   'old option qualifier style'.
> 
> * bt accepts now 2 'counts' to  limit on the nr of frame to show.
>   This is a little bit confusing, so maybe the interactions between
>   the 2 might be explained in the help.
>   E.g. explain that the below will only give 2 frames:
>     bt -limit 2 3
> 
> * The 'print' examples in the commit log are missing the trailing --
> 
> * Now that we have nice command options completions, I wonder
>   if the -q -c -s options should/could not be reworded as
>     -quiet -continue -silent
>   That will be backward compatible, and is better ergonomy.
>   I can do that as a follow-up patch if that will help you.
> 
> * display and output commands have no OPTIONS yet. 
>   For display, I guess this means the 'struct display' has to store
>   the option values (or maybe the string options ?).
> 
> * (gdb) thread apply 1 3 -ascenTAB will complete
>   but the resulting command fails:
>    (gdb) thread apply 1 3 -ascending p $pc
>    Invalid thread ID: -ascending p $pc
>   (gdb) 
> 
> 
> * frame apply 10 -pasTAB completes as
>    frame apply 10 -passwd 
> 
> * the help frame apply  only shows the qcs options
>   (same for frame apply all)
> 
> * help thread apply: the second line for the help for -ascending
>   should probably better be indented to the right, like the first line.

Thanks so much for the feedback.  For some reason I only noticed the
email today, but I worked a ton on this over the weekend.  I may
have fixed some of the bugs mentioned above already, but I won't
have a chance to check today, I think.  Definitely did not fix all.

Off the top of my head:

 - integrated completion with tfaas and faas too now.
 - processing, completion, and building help strings now share
   a single function to build the options description array/structure.
 - a ton of tweaks to the completion code.  most of those came from ...
 - extending the testcase a lot to validate the result of
   the gdb::option:completion_option function in many different scenarios.
 - that uncovered more latent bugs.  patches fixing latent bugs are split
   from the main patch in the branch.
 - testsuite regression-free for me.

I force-pushed what I have to the branch now.

Thanks,
Pedro Alves