[RFA 0/2] Add completion for COMMAND for frame|thread apply

[RFA 0/2] Add completion for COMMAND for frame|thread apply

[Philippe Waroquiers]

Add completion for COMMAND for frame|thread apply
This patch series adds completion for the COMMAND part of
frame and thread apply.
If the approach used here is approved, I will afterwards similarly
update the slash and pipe commands to complete.
See
https://sourceware.org/ml/gdb-patches/2019-04/msg00242.html
and
https://sourceware.org/ml/gdb-patches/2019-04/msg00396.html

[RFA 2/2] Add completion for COMMAND in 'frame apply all|level|COUNT... COMMAND'

[Philippe Waroquiers]

This patch adds logic to complete the COMMAND part of the
'frame apply all|level|COUNT...' command.

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

	* stack.c (frame_apply_level_command_completer,
	frame_apply_all_command_completer, frame_apply_command_completer):
	 New functions.
	(frame_apply_level_command): Add comment
	referring to the corresponding completer function.
	(_initialize_stack): Setup completers for frame apply all,
	frame apply COUNT, frame apply level, faas.

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

	* gdb.base/frameapply.exp: Test COMMAND completion.
---
 gdb/stack.c                           | 103 ++++++++++++++++++++++++--
 gdb/testsuite/gdb.base/frameapply.exp |  21 ++++++
 2 files changed, 119 insertions(+), 5 deletions(-)

diff --git a/gdb/stack.c b/gdb/stack.c
index f7fd9433b5..1a66de16a5 100644
--- a/gdb/stack.c
+++ b/gdb/stack.c
@@ -2733,6 +2733,9 @@ frame_apply_level_command (const char *cmd, int from_tty)
   const char *levels_str = cmd;
   number_or_range_parser levels (levels_str);
 
+  /* Changing this parsing logic probably implies to similarly update
+     frame_apply_level_command below.  */
+
   /* Skip the LEVEL list to find the flags and command args.  */
   while (!levels.finished ())
     {
@@ -2769,6 +2772,46 @@ frame_apply_level_command (const char *cmd, int from_tty)
     }
 }
 
+/* Skips the known arguments of frame apply level
+   and then invokes the usual command_completer.  */
+
+static void
+frame_apply_level_command_completer (struct cmd_list_element *cmd,
+				     completion_tracker &tracker,
+				     const char *text, const char *word)
+{
+  number_or_range_parser levels (text);
+  bool level_found = false;
+
+  while (!levels.finished ())
+    {
+      /* Call for effect.  */
+      levels.get_number ();
+
+      level_found = true;
+      if (levels.in_range ())
+	levels.skip_range ();
+    }
+
+  if (!level_found)
+    return;
+
+  text = levels.cur_tok ();
+
+  while (text != NULL)
+    {
+      qcs_flags dummy;
+
+      if (parse_flags_qcs ("frame apply level COMMAND completer",
+			   &text, &dummy))
+	continue;
+
+      break;
+    }
+
+  command_completer (cmd, tracker, text, word);
+}
+
 /* Implementation of the "frame apply all" command.  */
 
 static void
@@ -2781,6 +2824,28 @@ frame_apply_all_command (const char *cmd, int from_tty)
 			     get_current_frame (), INT_MAX);
 }
 
+/* Skips the known arguments of frame apply all
+   and then invokes the usual command_completer.  */
+
+static void
+frame_apply_all_command_completer (struct cmd_list_element *cmd,
+				   completion_tracker &tracker,
+				   const char *text, const char *word)
+{
+  while (text != NULL)
+    {
+      qcs_flags dummy;
+
+      if (parse_flags_qcs ("frame apply all COMMAND completer",
+			   &text, &dummy))
+	continue;
+
+      break;
+    }
+
+  command_completer (cmd, tracker, text, word);
+}
+
 /* Implementation of the "frame apply" command.  */
 
 static void
@@ -2810,6 +2875,31 @@ frame_apply_command (const char* cmd, int from_tty)
 			     trailing, count);
 }
 
+/* Skips the known arguments of frame apply COUNT
+   and then invokes the usual command_completer.  */
+
+static void
+frame_apply_command_completer (struct cmd_list_element *cmd,
+			       completion_tracker &tracker,
+			       const char *text, const char *word)
+{
+  if (get_number_trailer (&text, 0) == 0)
+    return;
+
+  while (text != NULL)
+    {
+      qcs_flags dummy;
+
+      if (parse_flags_qcs ("frame apply COUNT COMMAND completer",
+			   &text, &dummy))
+	continue;
+
+      break;
+    }
+
+  command_completer (cmd, tracker, text, word);
+}
+
 /* Implementation of the "faas" command.  */
 
 static void
@@ -2916,22 +3006,24 @@ Flag -c indicates to print the error and continue.\n\
 Flag -s indicates to silently ignore a COMMAND that raises an error\n\
 or produces no output."
 
-  add_prefix_cmd ("apply", class_stack, frame_apply_command,
+  cmd = add_prefix_cmd ("apply", class_stack, frame_apply_command,
 		  _("Apply a command to a number of frames.\n\
 Usage: frame apply COUNT [FLAG]... COMMAND\n\
 With a negative COUNT argument, applies the command on outermost -COUNT frames.\n"
 FRAME_APPLY_FLAGS_HELP),
 		  &frame_apply_cmd_list, "frame apply ", 1, &frame_cmd_list);
+  set_cmd_completer (cmd, frame_apply_command_completer);
 
-  add_cmd ("all", class_stack, frame_apply_all_command,
+  cmd = add_cmd ("all", class_stack, frame_apply_all_command,
 	   _("\
 Apply a command to all frames.\n\
 \n\
 Usage: frame apply all [FLAG]... COMMAND\n"
 FRAME_APPLY_FLAGS_HELP),
 	   &frame_apply_cmd_list);
+  set_cmd_completer (cmd, frame_apply_all_command_completer);
 
-  add_cmd ("level", class_stack, frame_apply_level_command,
+  cmd = add_cmd ("level", class_stack, frame_apply_level_command,
 	   _("\
 Apply a command to a list of frames.\n\
 \n\
@@ -2939,12 +3031,13 @@ Usage: frame apply level LEVEL... [FLAG]... COMMAND\n\
 ID is a space-separated list of LEVELs of frames to apply COMMAND on.\n"
 FRAME_APPLY_FLAGS_HELP),
 	   &frame_apply_cmd_list);
+  set_cmd_completer (cmd, frame_apply_level_command_completer);
 
-  add_com ("faas", class_stack, faas_command, _("\
+  cmd = add_com ("faas", class_stack, faas_command, _("\
 Apply a command to all frames (ignoring errors and empty output).\n\
 Usage: faas COMMAND\n\
 shortcut for 'frame apply all -s COMMAND'"));
-
+  set_cmd_completer (cmd, command_completer);
 
   add_prefix_cmd ("frame", class_stack,
 		  &frame_cmd.base_command, _("\
diff --git a/gdb/testsuite/gdb.base/frameapply.exp b/gdb/testsuite/gdb.base/frameapply.exp
index ccf30f2079..6b23d42143 100644
--- a/gdb/testsuite/gdb.base/frameapply.exp
+++ b/gdb/testsuite/gdb.base/frameapply.exp
@@ -18,6 +18,8 @@
 
 # Test 'frame apply [all | COUNT | -COUNT | level LEVEL...] [FLAG]... COMMAND'.
 
+load_lib completion-support.exp
+
 standard_testfile
 
 if { [prepare_for_testing "failed to prepare" ${testfile}] } {
@@ -36,6 +38,25 @@ set any "\[^\r\n\]*"
 set ws "\[ \t\]\+"
 set number "\[0-9]\+"
 
+# Check completion.
+test_gdb_complete_cmd_unique "frame apply al" "frame apply all"
+test_gdb_complete_cmd_unique "frame apply all info local" \
+    "frame apply all info locals"
+test_gdb_complete_cmd_unique "frame apply all -q info local" \
+    "frame apply all -q info locals"
+
+test_gdb_complete_cmd_unique "frame apply 3 info local" \
+    "frame apply 3 info locals"
+test_gdb_complete_cmd_unique "frame apply 3 -q info local" \
+    "frame apply 3 -q info locals"
+
+test_gdb_complete_cmd_unique "frame apply level 3 info local" \
+    "frame apply level 3 info locals"
+test_gdb_complete_cmd_unique "frame apply level 3 -q info local" \
+    "frame apply level 3 -q info locals"
+
+test_gdb_complete_cmd_unique "faas info local" "faas info locals"
+
 
 # Check all | COUNT | -COUNT | level LEVEL... with a simple command.
 with_test_prefix "simple command" {
-- 
2.20.1

Re: [RFA 2/2] Add completion for COMMAND in 'frame apply all|level|COUNT... COMMAND'

[Tom Tromey]

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

Philippe> This patch adds logic to complete the COMMAND part of the
Philippe> 'frame apply all|level|COUNT...' command.

Thank you for doing this.

Philippe> +  while (text != NULL)
Philippe> +    {
Philippe> +      qcs_flags dummy;
Philippe> +
Philippe> +      if (parse_flags_qcs ("frame apply level COMMAND completer",
Philippe> +			   &text, &dummy))
Philippe> +	continue;

I think this has the same behavior as the earlier patch when completion
occurs just after a "-" or "-q".

Tom

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

[Philippe Waroquiers]

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(-)

diff --git a/gdb/testsuite/gdb.threads/pthreads.exp b/gdb/testsuite/gdb.threads/pthreads.exp
index 0bb9083f67..2cde3e64d5 100644
--- a/gdb/testsuite/gdb.threads/pthreads.exp
+++ b/gdb/testsuite/gdb.threads/pthreads.exp
@@ -15,6 +15,8 @@
 
 # This file was written by Fred Fish. (fnf@cygnus.com)
 
+load_lib completion-support.exp
+
 # This test requires sending ^C to interrupt the running target.
 if [target_info exists gdb,nointerrupts] {
     verbose "Skipping pthreads.exp because of nointerrupts."
@@ -341,6 +343,29 @@ proc check_qcs {} {
 
 }
 
+proc check_completion {} {
+    test_gdb_complete_cmd_unique "thread apply al" "thread apply all"
+    test_gdb_complete_cmd_unique "thread apply all info al" \
+	"thread apply all info all-registers"
+    test_gdb_complete_cmd_unique "thread apply all -ascending info al" \
+	"thread apply all -ascending info all-registers"
+    test_gdb_complete_cmd_unique "thread apply all -ascending -q info al" \
+	"thread apply all -ascending -q info all-registers"
+
+    test_gdb_complete_cmd_unique "thread apply" "thread apply"
+    test_gdb_complete_none "thread apply 1"
+    test_gdb_complete_none "thread apply 1 2"
+    test_gdb_complete_cmd_unique "thread apply 1 2 info al" \
+	"thread apply 1 2 info all-registers"
+    test_gdb_complete_cmd_unique "thread apply 1 2 -q -c info al" \
+	"thread apply 1 2 -q -c info all-registers"
+
+    test_gdb_complete_cmd_unique "taas info al" "taas info all-registers"
+    test_gdb_complete_cmd_unique "tfaas info al" "tfaas info all-registers"
+}
+
+check_completion
+
 if [runto_main] then {
     if [test_startup] then {
 	if [check_control_c] then {
diff --git a/gdb/thread.c b/gdb/thread.c
index dbcf8be0e1..c8ca49d54c 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -1502,6 +1502,8 @@ thread_apply_all_command (const char *cmd, int from_tty)
 
   tp_array_compar_ascending = false;
 
+  /* Changing this parsing logic probably implies to similarly update
+     thread_apply_all_completer below.  */
   while (cmd != NULL)
     {
       if (check_for_argument (&cmd, "-ascending", strlen ("-ascending")))
@@ -1551,6 +1553,33 @@ thread_apply_all_command (const char *cmd, int from_tty)
     }
 }
 
+/* Skips the known arguments of thread apply all
+   and then invokes the usual command_completer.  */
+
+static void
+thread_apply_all_command_completer (struct cmd_list_element *cmd,
+				    completion_tracker &tracker,
+				    const char *text, const char *word)
+{
+  while (text != NULL)
+    {
+      qcs_flags dummy;
+
+      if (check_for_argument (&text, "-ascending", strlen ("-ascending")))
+	{
+	  text = skip_spaces (text);
+	  continue;
+	}
+
+      if (parse_flags_qcs ("thread apply all COMMAND completer", &text, &dummy))
+	continue;
+
+      break;
+    }
+
+  command_completer (cmd, tracker, text, word);
+}
+
 /* Implementation of the "thread apply" command.  */
 
 static void
@@ -1588,6 +1617,8 @@ thread_apply_command (const char *tidlist, int from_tty)
 
   scoped_restore_current_thread restore_thread;
 
+  /* Changing this parsing logic probably implies to similarly update
+     thread_apply_id_completer below.  */
   parser.init (tidlist, current_inferior ()->num);
   while (!parser.finished () && parser.cur_tok () < cmd_or_flags)
     {
@@ -1638,6 +1669,40 @@ thread_apply_command (const char *tidlist, int from_tty)
     }
 }
 
+/* Skips the known arguments of thread apply ID...
+   and then invokes the usual command_completer.  */
+
+static void
+thread_apply_id_command_completer (struct cmd_list_element *cmd,
+				   completion_tracker &tracker,
+				   const char *text, const char *word)
+{
+  tid_range_parser parser;
+  qcs_flags dummy;
+
+  if (text == NULL)
+    return;  /* No ID yet.  */
+
+  parser.init (text, current_inferior ()->num);
+  while (!parser.finished ())
+    {
+      int inf_num, thr_start, thr_end;
+
+      if (!parser.get_tid_range (&inf_num, &thr_start, &thr_end))
+	{
+	  text = parser.cur_tok ();
+	  break;
+	}
+    }
+
+  while (text != NULL
+	 && parse_flags_qcs ("thread apply ID... COMMAND completer",
+			     &text, &dummy))
+    ;
+
+  command_completer (cmd, tracker, text, word);
+}
+
 
 /* Implementation of the "taas" command.  */
 
@@ -1939,6 +2004,7 @@ void
 _initialize_thread (void)
 {
   static struct cmd_list_element *thread_apply_list = NULL;
+  struct cmd_list_element *c;
 
   add_info ("threads", info_threads_command,
 	    _("Display currently known threads.\n\
@@ -1962,32 +2028,36 @@ Flag -c indicates to print the error and continue.\n\
 Flag -s indicates to silently ignore a COMMAND that raises an error\n\
 or produces no output."
 
-  add_prefix_cmd ("apply", class_run, thread_apply_command,
+  c = add_prefix_cmd ("apply", class_run, thread_apply_command,
 		  _("Apply a command to a list of threads.\n\
 Usage: thread apply ID... [FLAG]... COMMAND\n\
 ID is a space-separated list of IDs of threads to apply COMMAND on.\n"
 THREAD_APPLY_FLAGS_HELP),
-		  &thread_apply_list, "thread apply ", 1, &thread_cmd_list);
+		      &thread_apply_list, "thread apply ", 1, &thread_cmd_list);
+  set_cmd_completer (c, thread_apply_id_command_completer);
 
-  add_cmd ("all", class_run, thread_apply_all_command,
-	   _("\
+  c = add_cmd ("all", class_run, thread_apply_all_command,
+	       _("\
 Apply a command to all threads.\n\
 \n\
 Usage: thread apply all [-ascending] [FLAG]... COMMAND\n\
 -ascending: Call COMMAND for all threads in ascending order.\n\
             The default is descending order.\n"
 THREAD_APPLY_FLAGS_HELP),
-	   &thread_apply_list);
+	       &thread_apply_list);
+  set_cmd_completer (c, thread_apply_all_command_completer);
 
-  add_com ("taas", class_run, taas_command, _("\
+  c = add_com ("taas", class_run, taas_command, _("\
 Apply a command to all threads (ignoring errors and empty output).\n\
 Usage: taas COMMAND\n\
 shortcut for 'thread apply all -s COMMAND'"));
+  set_cmd_completer (c, command_completer);
 
-  add_com ("tfaas", class_run, tfaas_command, _("\
+  c = add_com ("tfaas", class_run, tfaas_command, _("\
 Apply a command to all frames of all threads (ignoring errors and empty output).\n\
 Usage: tfaas COMMAND\n\
 shortcut for 'thread apply all -s frame apply all -s COMMAND'"));
+  set_cmd_completer (c, command_completer);
 
   add_cmd ("name", class_run, thread_name_command,
 	   _("Set the current thread's name.\n\
-- 
2.20.1

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

[Tom Tromey]

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

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

[Philippe Waroquiers]

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

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

[Tom Tromey]

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

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

[Philippe Waroquiers]

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

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

[Pedro Alves]

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

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

[Philippe Waroquiers]

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

"with" command (alternative to the "/" command)

[Pedro Alves]

On 5/1/19 8:54 PM, Philippe Waroquiers wrote:
> 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).

Right.  I think TAB-completion is a must-have feature.

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

I think this is the most useful property of the command.

I'd like to explore other user interfaces for this.  I'm aware
that you've done a ton of work on the / command, which makes
it uncomfortable for me to suggest it...  I wish it was
discussed before that; if it was and I missed it, I'm truly
sorry.  :-(

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

Right, so today I'm kind of sick with fever so I decided to
prototype something, instead of working on what I should be
working on.  :-P.

So I tried quickly prototyping a "with" command, which is
just like "set", but sets the setting, runs the command
and then restores the setting.

 (gdb) help with 
 Temporarily set SETTING, run COMMAND, and restore SETTING.
 Usage: with SETTING -- COMMAND
 SETTING is any setting settable with the "set" command.
 E.g.:
   with language pascal -- print obj
   with print elements unlimited -- print obj

I integrated it with TAB-completion, which I think makes
it OK to not have shorter aliases like in the / command.
That's the part that I dislike about / -- that you're 
adding another "language" to GDB, something else that
users need to learn.

It actually works surprisingly nicely, even if verbose:

 (gdb) with print elements unlimited -- print 1
 $1 = 1
 (gdb) with non-stop on -- show non-stop 
 Controlling the inferior in non-stop mode is on.
 (gdb) show non-stop 
 Controlling the inferior in non-stop mode is off.
 (gdb) with language pascal -- print 1
 $2 = 1
 (gdb) with language pascal -- show language 
 The current source language is "pascal".
 (gdb) show language 
 The current source language is "auto; currently c".
 (gdb) with print elements 100 -- with print object off -- print 1
 $3 = 1

You can shorten things a bit though, as long as unambiguous:

So this:

 (gdb) with print elements 100 -- with print object off -- print 1

is the same as:

 (gdb) wit p el 100 -- wit p o 0 -- p 1

We could add a "w" alias for "with", as "w" is not taken.  Then we'd
have:

 (gdb) w p el 100 -- w p o 0 -- p 1

As mentioned, TAB completion works nicely:

 (gdb) with p[TAB]
 pagination  print       prompt      python      
 (gdb) with print [TAB]
 address                max-depth              static-members
 array                  max-symbolic-offset    symbol
 array-indexes          null-stop              symbol-filename
 asm-demangle           object                 symbol-loading
 demangle               pascal_static-members  thread-events
 elements               pretty                 type
 entry-values           raw                    union
 frame-arguments        repeats                vtbl
 inferior-events        sevenbit-strings       
 (gdb) with print [TAB]

The patch below applies on top of the users/palves/cli-options
branch, in order to make use of the complete_command function,
the one that allows recursing the completion to the nested
COMMAND:

 (gdb) with print elements unlimited -- thread apply all -
 -ascending  -c          -q          -s          

 (gdb) with print elements unlimited -- print -[TAB]
 -address         -max-depth       -repeats         -vtbl
 -array           -null-stop       -static-members  
 -array-indexes   -object          -symbol          
 -elements        -pretty          -union           

> 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 :).

:-)

Notes:

  - I'm requiring "--" to separate the setting from the command.
    I think it might be possible to do without that, but, well,
    this is a prototype.  :-)

Here's the patch.  It's smaller than one might think.  :-)

From 22805567c5ca8d008f2898083f2027e9d3f1ea43 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Fri, 24 May 2019 18:55:46 +0100
Subject: [PATCH] with_command

---
 gdb/printcmd.c | 109 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 109 insertions(+)

diff --git a/gdb/printcmd.c b/gdb/printcmd.c
index 0509360581e..6098dc57b33 100644
--- a/gdb/printcmd.c
+++ b/gdb/printcmd.c
@@ -48,6 +48,8 @@
 #include "cli/cli-option.h"
 #include "cli/cli-script.h"
 #include "cli/cli-style.h"
+#include "cli/cli-decode.h"
+#include "cli/cli-setshow.h"
 #include "common/format.h"
 #include "source.h"
 #include "common/byte-vector.h"
@@ -1289,6 +1291,103 @@ set_command (const char *exp, int from_tty)
   evaluate_expression (expr.get ());
 }
 
+/* The "with" command.  */
+
+static void
+with_command (const char *exp, int from_tty)
+{
+  const char *delim = strstr (exp, "--");
+
+  if (delim == exp)
+    error (_("Missing setting before '--' delimiter\n"));
+
+  if (delim == nullptr
+      || !isspace (delim[-1])
+      || !(isspace (delim[2]) || delim[2] == '\0'))
+    error (_("Missing '--' delimiter\n"));
+
+  cmd_list_element *set_cmd = lookup_cmd (&exp, setlist, "with", 0, 1);
+
+  if (set_cmd == nullptr)
+    error (_("Unknown setting %s\n"), exp);
+
+  std::string value = std::string (exp, delim - exp);
+  const char *nested_cmd = skip_spaces (delim + 2);
+
+  std::string org_value;
+
+  switch (set_cmd->var_type)
+    {
+    case var_boolean:
+    case var_auto_boolean:
+    case var_integer:
+    case var_zinteger:
+    case var_zuinteger_unlimited:
+      org_value = std::to_string (*(int *) set_cmd->var);
+      break;
+    case var_uinteger:
+    case var_zuinteger:
+      org_value = std::to_string (*(unsigned int *) set_cmd->var);
+      break;
+    case var_string:
+    case var_string_noescape:
+    case var_filename:
+    case var_optional_filename:
+    case var_enum:
+      org_value = *(const char **) set_cmd->var;
+      break;
+    default:
+      gdb_assert_not_reached ("unhandled var_type");
+    }
+
+  /* Tweak the setting to the new temporary value.  */
+  do_set_command (value.c_str (), from_tty, set_cmd);
+
+  try
+    {
+      /* Execute the nested command.  */
+      execute_command (nested_cmd, from_tty);
+    }
+  catch (const gdb_exception &ex)
+    {
+      /* Restore the setting and rethrow.  */
+      do_set_command (org_value.c_str (), from_tty, set_cmd);
+      throw;
+    }
+
+  /* Restore the setting.  */
+  do_set_command (org_value.c_str (), from_tty, set_cmd);
+}
+
+/* See valprint.h.  */
+
+void
+with_command_completer (struct cmd_list_element *ignore,
+			completion_tracker &tracker,
+			const char *text, const char * /*word*/)
+{
+  tracker.set_use_custom_word_point (true);
+
+  const char *delim = strstr (text, "--");
+
+  if (delim == text
+      || delim == nullptr
+      || !isspace (delim[-1])
+      || !(isspace (delim[2]) || delim[2] == '\0'))
+    {
+      std::string new_text = std::string ("set ") + text;
+      tracker.advance_custom_word_point_by (-(int) strlen ("with"));
+      complete_command (tracker, new_text.c_str ());
+      return;
+    }
+
+  /* We're past the "--" delimiter.  Complete on the sub command.  */
+  const char *nested_cmd = skip_spaces (delim + 2);
+  tracker.advance_custom_word_point_by (nested_cmd - text);
+  complete_command (tracker, nested_cmd);
+}
+
+
 static void
 info_symbol_command (const char *arg, int from_tty)
 {
@@ -2750,6 +2849,16 @@ Like \"print\" but don't put in value history and don't print newline.\n\
 Usage: output EXP\n\
 This is useful in user-defined commands."));
 
+  c = add_com ("with", class_vars, with_command, _("\
+Temporarily set SETTING, run COMMAND, and restore SETTING.\n\
+Usage: with SETTING -- COMMAND\n\
+SETTING is any setting settable with the \"set\" command.\n\
+E.g.:\n\
+  with language pascal -- print obj\n\
+  with print elements unlimited -- print obj\n\
+\n"));
+  set_cmd_completer_handle_brkchars (c, with_command_completer);
+
   add_prefix_cmd ("set", class_vars, set_command, _("\
 Evaluate expression EXP and assign result to variable VAR\n\
 Usage: set VAR = EXP\n\
-- 
2.14.5

Re: "with" command (alternative to the "/" command)

[Philippe Waroquiers]

On Fri, 2019-05-24 at 19:31 +0100, Pedro Alves wrote:
> I'd like to explore other user interfaces for this.  I'm aware
> that you've done a ton of work on the / command, which makes
> it uncomfortable for me to suggest it...  I wish it was
> discussed before that; if it was and I missed it, I'm truly
> sorry.  :-(
No problem, the work done on / was not that huge, and at the end,
if it was the trigger that led to finish the option work, it was time
well spent :).

> > 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.
> > 
> 
> Right, so today I'm kind of sick with fever so I decided to
> prototype something, instead of working on what I should be
> working on.  :-P.
> 
> So I tried quickly prototyping a "with" command, which is
> just like "set", but sets the setting, runs the command
> and then restores the setting.
Looks nice and good enough to replace the / command
(and a very often 'with print some_option_often_temporary_changed -- COMMAND'
can always be put in a user defined command with a short name).

Thanks for doing this,

Philippe

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

[Pedro Alves]

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

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

[Philippe Waroquiers]

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.

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

[Pedro Alves]

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