[RFA_v3 0/8] Implement 'frame apply COMMAND', enhance 'thread apply COMMAND'

[RFA_v3 0/8] Implement 'frame apply COMMAND', enhance 'thread apply COMMAND'

[Philippe Waroquiers]

This is the fourth iteration of the patch series that:
 * implements a new command
     'frame apply [all | COUNT | -COUNT | id ID...] [FLAG]... COMMAND'.
 * enhance 'thread apply COMMAND' by adding FLAG arguments.
 * adds some shortcuts commands.
 * documents the above in gdb.texinfo and NEWS.
 * adds a unit test for cli-utils.c.
 * adds test for 'frame apply'.
 * modify gdb.threads/pthreads.exp to test 'thread apply' FLAG arguments.

The fourth version is RFA v3. It handles the comments given by Pedro.
The main changes compared to RFA v2 are:
  * The -q (quiet) flag replaces the verbosity/-v/-q concept.
  * Addition of 'frame apply id ID... [FLAG]... COMMAND' to allow
    applying a command on a selected list of frames.
  * [FLAG]... arguments are now parsed iteratively.
  * Documentation and on-line help updated accordingly.
  * tests updated accordingly.

The third version was RFA v2, handling the comments of Pedro about giving
the flags individually.
The changes compared to RFA v1 is:
  * The flags must be given individually, such as -v -v -c.
  * The documentation and on-line help was updated accordingly.
  * ChangeLog information is distributed in each commit log message.

The second version was RFA v1, changes compared to RFC are:
  * Replied to all comments of Eli and Simon.
  * Tests and ChangeLog entries added.
  * Comments received from Pedro

The first version was an RFC
  * Code and doc was complete, but was lacking ChangeLog and tests.
  * Comments received from Eli and Simon.

[RFA_v3 5/8] Announce the user visible changes for frame/thread apply in NEWS.

[Philippe Waroquiers]

'frame apply', faas, taas, tfaas commands and [FLAG]... arg for thread apply.

gdb/ChangeLog
2018-06-24  Philippe Waroquiers  <philippe.waroquiers@skynet.be>

        * NEWS: Mention new commands. Mention change to 'thread apply'.
---
 gdb/NEWS | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/gdb/NEWS b/gdb/NEWS
index 13da2f1d4e..85cd63c7c5 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -23,6 +23,24 @@
 
 * New commands
 
+frame apply [all | COUNT | -COUNT | id ID...] [FLAG]... COMMAND
+  Apply a command to some frames.
+  FLAG arguments allow to control what output to produce and how to handle
+  errors raised when applying COMMAND to a frame.
+
+taas COMMAND
+  Apply a command to all threads (ignoring errors and empty output).
+  Shortcut for 'thread apply all -s COMMAND'.
+
+faas COMMAND
+  Apply a command to all frames (ignoring errors and empty output).
+  Shortcut for 'frame apply all -s COMMAND'.
+
+tfaas COMMAND
+  Apply a command to all frames of all threads (ignoring errors and empty
+  output).
+  Shortcut for 'thread apply all -s frame apply all -s COMMAND'.
+
 set debug fbsd-nat
 show debug fbsd-nat
   Control display of debugging info regarding the FreeBSD native target.
@@ -46,6 +64,13 @@ maint show check-libthread-db
   debugging libraries as they are loaded.  The default is not to
   perform such checks.
 
+* Changed commands
+
+thread apply [all | COUNT | -COUNT] [FLAG]... COMMAND
+  The 'thread apply' command accepts new FLAG arguments.
+  FLAG arguments allow to control what output to produce and how to handle
+  errors raised when applying COMMAND to a thread.
+
 * Python API
 
   ** Type alignment is now exposed via the "align" attribute of a gdb.Type.
-- 
2.17.1

[RFA_v3 3/8] Add [FLAG]... arguments to 'thread apply'.

[Philippe Waroquiers]

Enhance 'thread apply' command to also accept [FLAG]... arguments.

An example usage for this new argument:
   thread apply all -s frame apply all -s p some_local_var_somewhere
      Prints the thread id, frame location and some_local_var_somewhere
      value in frames of threads that have such local var.

To make the life of the user easier, the most typical use cases
have shortcuts :
   taas  : shortcut for 'thread apply all -s'
   tfaas : shortcut for 'thread apply all -s frame apply all -s"

An example usage :
   tfaas p some_local_var_somewhere
     same as the longer:
        'thread apply all -s frame apply all -s p some_local_var_somewhere'

gdb/ChangeLog
2018-06-24  Philippe Waroquiers  <philippe.waroquiers@skynet.be>

	* thread.c (thr_try_catch_cmd): New function.
	(thread_apply_all_command): Handle qcs flags.
	(thread_apply_command): Handle qcs flags.
	(taas_command): New function.
	(tfaas_command): New function.
	(_initialize_thread): Update to setup the new commands 'taas
	and 'tfaas'. Change doc string for 'thread apply'.
---
 gdb/thread.c | 141 ++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 117 insertions(+), 24 deletions(-)

diff --git a/gdb/thread.c b/gdb/thread.c
index 2b471cd292..8249fdd127 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -1559,6 +1559,44 @@ tp_array_compar (const thread_info *a, const thread_info *b)
     return (a->per_inf_num > b->per_inf_num);
 }
 
+/* Switch to thread THR and execute CMD.
+   QUIET controls the printing of the thread information.
+   CONT and SILENT control how to handle errors.  */
+
+static void
+thr_try_catch_cmd (thread_info *thr, const char *cmd, int from_tty,
+		   bool quiet,  bool cont, bool silent)
+{
+  switch_to_thread (thr);
+  TRY
+    {
+      std::string cmd_result = execute_command_to_string (cmd, from_tty);
+      if (!silent || cmd_result.length () > 0)
+	{
+	  if (!quiet)
+	    printf_filtered (_("\nThread %s (%s):\n"),
+			     print_thread_id (thr),
+			     target_pid_to_str (inferior_ptid));
+	  printf_filtered ("%s", cmd_result.c_str ());
+	}
+    }
+  CATCH (ex, RETURN_MASK_ERROR)
+    {
+      if (!silent)
+	{
+	  if (!quiet)
+	    printf_filtered (_("\nThread %s (%s):\n"),
+			     print_thread_id (thr),
+			     target_pid_to_str (inferior_ptid));
+	  if (cont)
+	    printf_filtered ("%s\n", ex.message);
+	  else
+	    throw_exception (ex);
+	}
+    }
+  END_CATCH;
+}
+
 /* Apply a GDB command to a list of threads.  List syntax is a whitespace
    separated list of numbers, or ranges, or the keyword `all'.  Ranges consist
    of two numbers separated by a hyphen.  Examples:
@@ -1570,16 +1608,30 @@ tp_array_compar (const thread_info *a, const thread_info *b)
 static void
 thread_apply_all_command (const char *cmd, int from_tty)
 {
+  bool quiet = false;
+  bool cont = false;
+  bool silent = false;
+
   tp_array_compar_ascending = false;
-  if (cmd != NULL
-      && check_for_argument (&cmd, "-ascending", strlen ("-ascending")))
+
+  while (cmd != NULL)
     {
-      cmd = skip_spaces (cmd);
-      tp_array_compar_ascending = true;
+      if (check_for_argument (&cmd, "-ascending", strlen ("-ascending")))
+	{
+	  cmd = skip_spaces (cmd);
+	  tp_array_compar_ascending = true;
+	  continue;
+	}
+
+      if (parse_flags_qcs ("thread apply all", &cmd,
+			   &quiet, &cont, &silent))
+	continue;
+
+      break;
     }
 
   if (cmd == NULL || *cmd == '\000')
-    error (_("Please specify a command following the thread ID list"));
+    error (_("Please specify a command at the end of 'thread apply all'"));
 
   update_thread_list ();
 
@@ -1615,14 +1667,8 @@ thread_apply_all_command (const char *cmd, int from_tty)
 
       for (thread_info *thr : thr_list_cpy)
 	if (thread_alive (thr))
-	  {
-	    switch_to_thread (thr);
-	    printf_filtered (_("\nThread %s (%s):\n"),
-			     print_thread_id (thr),
-			     target_pid_to_str (inferior_ptid));
-
-	    execute_command (cmd, from_tty);
-	  }
+	  thr_try_catch_cmd (thr, cmd, from_tty,
+			     quiet, cont, silent);
     }
 }
 
@@ -1631,7 +1677,11 @@ thread_apply_all_command (const char *cmd, int from_tty)
 static void
 thread_apply_command (const char *tidlist, int from_tty)
 {
+  bool quiet = false;
+  bool cont = false;
+  bool silent = false;
   const char *cmd = NULL;
+  const char *cmd_or_flags;
   tid_range_parser parser;
 
   if (tidlist == NULL || *tidlist == '\000')
@@ -1649,6 +1699,11 @@ thread_apply_command (const char *tidlist, int from_tty)
 	}
     }
 
+  cmd_or_flags = cmd;
+  while (cmd != NULL && parse_flags_qcs ("thread apply", &cmd,
+					 &quiet, &cont, &silent))
+    ;
+
   if (cmd == NULL)
     error (_("Please specify a command following the thread ID list"));
 
@@ -1658,7 +1713,7 @@ thread_apply_command (const char *tidlist, int from_tty)
   scoped_restore_current_thread restore_thread;
 
   parser.init (tidlist, current_inferior ()->num);
-  while (!parser.finished () && parser.cur_tok () < cmd)
+  while (!parser.finished () && parser.cur_tok () < cmd_or_flags)
     {
       struct thread_info *tp = NULL;
       struct inferior *inf;
@@ -1703,14 +1758,31 @@ thread_apply_command (const char *tidlist, int from_tty)
 	  continue;
 	}
 
-      switch_to_thread (tp);
-
-      printf_filtered (_("\nThread %s (%s):\n"), print_thread_id (tp),
-		       target_pid_to_str (inferior_ptid));
-      execute_command (cmd, from_tty);
+      thr_try_catch_cmd (tp, cmd, from_tty,
+			 quiet, cont, silent);
     }
 }
 
+
+/* Implementation of the "taas" command.  */
+
+static void
+taas_command (const char *cmd, int from_tty)
+{
+  std::string expanded = std::string ("thread apply all -s ") + cmd;
+  execute_command (expanded.c_str (), from_tty);
+}
+
+/* Implementation of the "tfaas" command.  */
+
+static void
+tfaas_command (const char *cmd, int from_tty)
+{
+  std::string expanded
+    = std::string ("thread apply all -s frame apply all -s ") + cmd;
+  execute_command (expanded.c_str (), from_tty);
+}
+
 /* Switch to the specified thread, or print the current thread.  */
 
 void
@@ -2008,22 +2080,43 @@ Use this command to switch between threads.\n\
 The new thread ID must be currently known."),
 		  &thread_cmd_list, "thread ", 1, &cmdlist);
 
+#define THREAD_APPLY_FLAGS_HELP "\
+Prints per-inferior thread number and target system's thread id\n\
+followed by COMMAND output.\n\
+FLAG arguments are -q (quiet), -c (continue), -s (silent).\n\
+Flag -q disables printing the thread information.\n\
+By default, if a COMMAND raises an error, thread apply is aborted.\n\
+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,
 		  _("Apply a command to a list of threads.\n\
-Usage: thread apply ID... COMMAND\n\
-ID is a space-separated list of IDs of threads to apply COMMAND on."),
+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);
 
   add_cmd ("all", class_run, thread_apply_all_command,
 	   _("\
 Apply a command to all threads.\n\
 \n\
-Usage: thread apply all [-ascending] COMMAND\n\
+Usage: thread apply all [-ascending] [FLAG]... COMMAND\n\
 -ascending: Call COMMAND for all threads in ascending order.\n\
-            The default is descending order.\
-"),
+            The default is descending order.\n"
+THREAD_APPLY_FLAGS_HELP),
 	   &thread_apply_list);
 
+  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'"));
+
+  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'"));
+
   add_cmd ("name", class_run, thread_name_command,
 	   _("Set the current thread's name.\n\
 Usage: thread name [NAME]\n\
-- 
2.17.1

Re: [RFA_v3 3/8] Add [FLAG]... arguments to 'thread apply'.

[Pedro Alves]

On 06/24/2018 07:37 PM, Philippe Waroquiers wrote:
> +
> +static void
> +thr_try_catch_cmd (thread_info *thr, const char *cmd, int from_tty,
> +		   bool quiet,  bool cont, bool silent)

Spurious double-space after "quiet, ".

Otherwise looks fine.

Thanks,
Pedro Alves

[RFA_v3 4/8] Documents the new commands 'frame apply', faas, taas, tfaas

[Philippe Waroquiers]

Documents the new commands 'frame apply', faas, taas, tfaas.
Documents the new arguments [FLAG]... added to 'thread apply'.

gdb/doc/ChangeLog
2018-06-24  Philippe Waroquiers  <philippe.waroquiers@skynet.be>

        * gdb.texinfo (Debugging Programs with Multiple Threads):
        Document changes to 'thread apply'. Document 'taas'.
        Document 'tfaas'.
        (Examining the Stack): Document 'frame apply'. Document 'faas'.
---
 gdb/doc/gdb.texinfo | 190 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 187 insertions(+), 3 deletions(-)

diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index a6bad13d9d..a9cef2fd9c 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -2936,7 +2936,7 @@ programs:
 @item automatic notification of new threads
 @item @samp{thread @var{thread-id}}, a command to switch among threads
 @item @samp{info threads}, a command to inquire about existing threads
-@item @samp{thread apply [@var{thread-id-list}] [@var{all}] @var{args}},
+@item @samp{thread apply [@var{thread-id-list} | all] @var{args}},
 a command to apply a command to a list of threads
 @item thread-specific breakpoints
 @item @samp{set print thread-events}, which controls printing of 
@@ -3173,7 +3173,7 @@ threads.
 
 @kindex thread apply
 @cindex apply command to several threads
-@item thread apply [@var{thread-id-list} | all [-ascending]] @var{command}
+@item thread apply [@var{thread-id-list} | all [-ascending]] [@var{flag}]@dots{} @var{command}
 The @code{thread apply} command allows you to apply the named
 @var{command} to one or more threads.  Specify the threads that you
 want affected using the thread ID list syntax (@pxref{thread ID
@@ -3182,6 +3182,59 @@ command to all threads in descending order, type @kbd{thread apply all
 @var{command}}.  To apply a command to all threads in ascending order,
 type @kbd{thread apply all -ascending @var{command}}.
 
+The @var{flag} arguments control what output to produce and how to handle
+errors raised when applying @var{command} to a thread.  @var{flag}
+must start with a @code{-} directly followed by one letter in
+@code{qcs}.  If several flags are provided, they must be given
+individually, such as @code{-c -q}.
+
+By default, @value{GDBN} displays some thread information before the
+output produced by @var{command}, and an error raised during the
+execution of a @var{command} will abort @code{thread apply}.  The
+following flags can be used to fine-tune this behavior:
+
+@table @code
+@item -c
+The flag @code{-c}, which stands for @samp{continue}, causes any
+errors in @var{command} to be displayed, and the execution of
+@code{thread apply} then continues.
+@item -s
+The flag @code{-s}, which stands for @samp{silent}, causes any errors
+or empty output produced by a @var{command} to be silently ignored.
+That is, the execution continues, but the thread information and errors
+are not printed.
+@item -q
+The flag @code{-q} (@samp{quiet}) disables printing the thread
+information.
+@end table
+
+Flags @code{-c} and @code{-s} cannot be used together.
+
+@kindex taas
+@cindex apply command to all threads (ignoring errors and empty output)
+@item taas @var{command}
+Shortcut for @code{thread apply all -s @var{command}}.
+Applies @var{command} on all threads, ignoring errors and empty output.
+
+@kindex tfaas
+@cindex apply a command to all frames of all threads (ignoring errors and empty output)
+@item tfaas @var{command}
+Shortcut for @code{thread apply all -s frame apply all -s @var{command}}.
+Applies @var{command} on all frames of all threads, ignoring errors
+and empty output.  Note that the flag @code{-s} is specified twice:
+The first @code{-s} ensures that @code{thread apply} only shows the thread
+information of the threads for which @code{frame apply} produces
+some output.  The second @code{-s} is needed to ensure that @code{frame
+apply} shows the frame information of a frame only if the
+@var{command} successfully produced some output.
+
+It can for example be used to print a local variable or a function
+argument without knowing the thread or frame where this variable or argument
+is, using:
+@smallexample
+(@value{GDBP}) tfaas p some_local_var_i_do_not_remember_where_it_is
+@end smallexample
+
 
 @kindex thread name
 @cindex name a thread
@@ -7304,6 +7357,7 @@ currently executing frame and describes it briefly, similar to the
 * Backtrace::                   Backtraces
 * Selection::                   Selecting a frame
 * Frame Info::                  Information on a frame
+* Frame Apply::                 Applying a command to several frames
 * Frame Filter Management::     Managing frame filters
 
 @end menu
@@ -7716,6 +7770,136 @@ accessible at the point of execution of the selected frame.
 
 @end table
 
+@node Frame Apply
+@section Applying a Command to Several Frames.
+@kindex frame apply
+@cindex apply command to several frames
+@table @code
+@item frame apply [all | @var{count} | @var{-count} | id @var{id}@dots{}] [@var{flag}]@dots{} @var{command}
+The @code{frame apply} command allows you to apply the named
+@var{command} to one or more frames.
+
+@table @code
+@item @code{all}
+Specify @code{all} to apply @var{command} to all frames.
+
+@item @var{count}
+Use @var{count} to apply @var{command} to the innermost @var{count}
+frames, where @var{count} is a positive number.
+
+@item @var{-count}
+Use @var{-count} to apply @var{command} to the outermost @var{count}
+frames, where @var{count} is a positive number.
+
+@item @code{id}
+Use @code{id} to apply @var{command} to the set of frames identified
+by the @var{id} list.  @var{id} is a frame id or a range of frame ids
+as @var{id1}-@var{id2}.  The frame id is the number shown in the first field
+of the @samp{backtrace} command output.
+E.g., @samp{2-4 6-8 3} indicates to apply @var{command} for the frames
+2, 3, 4, 6, 7, 8, and then again on frame 3.
+
+@end table
+
+@end table
+
+Note that the frames on which @code{frame apply} applies a command are
+also influenced by the @code{set backtrace} settings such as @code{set
+backtrace past-main} and @code{set backtrace limit N}.  See
+@xref{Backtrace,,Backtraces}.
+
+The @var{flag} arguments control what output to produce and how to handle
+errors raised when applying @var{command} to a frame.  @var{flag}
+must start with a @code{-} directly followed by one letter in
+@code{qcs}.  If several flags are provided, they must be given
+individually, such as @code{-c -q}.
+
+By default, @value{GDBN} displays some frame information before the
+output produced by @var{command}, and an error raised during the
+execution of a @var{command} will abort @code{frame apply}.  The
+following flags can be used to fine-tune this behavior:
+
+@table @code
+@item -c
+The flag @code{-c}, which stands for @samp{continue}, causes any
+errors in @var{command} to be displayed, and the execution of
+@code{frame apply} then continues.
+@item -s
+The flag @code{-s}, which stands for @samp{silent}, causes any errors
+or empty output produced by a @var{command} to be silently ignored.
+That is, the execution continues, but the frame information and errors
+are not printed.
+@item -q
+The flag @code{-q} (@samp{quiet}) disables printing the frame
+information.
+@end table
+
+The following example shows how the flags @code{-c} and @code{-s} are
+working when applying the command @code{p j} to all frames, where
+variable @code{j} can only be successfully printed in the outermost
+@code{#1 main} frame.
+
+@smallexample
+@group
+(gdb) frame apply all p j
+#0  some_function (i=5) at fun.c:4
+No symbol "j" in current context.
+(gdb) frame apply all -c p j
+#0  some_function (i=5) at fun.c:4
+No symbol "j" in current context.
+#1  0x565555fb in main (argc=1, argv=0xffffd2c4) at fun.c:11
+$1 = 5
+(gdb) frame apply all -s p j
+#1  0x565555fb in main (argc=1, argv=0xffffd2c4) at fun.c:11
+$2 = 5
+(gdb)
+@end group
+@end smallexample
+
+By default, @samp{frame apply}, prints the frame location
+information before the command output:
+
+@smallexample
+@group
+(gdb) frame apply all p $sp
+#0  some_function (i=5) at fun.c:4
+$4 = (void *) 0xffffd1e0
+#1  0x565555fb in main (argc=1, argv=0xffffd2c4) at fun.c:11
+$5 = (void *) 0xffffd1f0
+(gdb)
+@end group
+@end smallexample
+
+If flag @code{-q} is given, no frame information is printed:
+@smallexample
+@group
+(gdb) frame apply all -q p $sp
+$12 = (void *) 0xffffd1e0
+$13 = (void *) 0xffffd1f0
+(gdb)
+@end group
+@end smallexample
+
+@table @code
+
+@kindex faas
+@cindex apply a command to all frames (ignoring errors and empty output)
+@item faas @var{command}
+Shortcut for @code{frame apply all -s @var{command}}.
+Applies @var{command} on all frames, ignoring errors and empty output.
+
+It can for example be used to print a local variable or a function
+argument without knowing the frame where this variable or argument
+is, using:
+@smallexample
+(@value{GDBP}) faas p some_local_var_i_do_not_remember_where_it_is
+@end smallexample
+
+Note that the command @code{tfaas @var{command}} applies @var{command}
+on all frames of all threads.  See @xref{Threads,,Threads}.
+@end table
+
+
 @node Frame Filter Management
 @section Management of Frame Filters.
 @cindex managing frame filters
@@ -11094,7 +11278,7 @@ Visiting node of type NODE_INTEGER
 convenience functions.
 
 @table @code
-@item help function
+@item help functionu
 @kindex help function
 @cindex show all convenience functions
 Print a list of all convenience functions.
-- 
2.17.1

Re: [RFA_v3 4/8] Documents the new commands 'frame apply', faas, taas, tfaas

[Pedro Alves]

On 06/24/2018 07:37 PM, Philippe Waroquiers wrote:

> @@ -11094,7 +11278,7 @@ Visiting node of type NODE_INTEGER
>  convenience functions.
>  
>  @table @code
> -@item help function
> +@item help functionu

Is that intended?

Thanks,
Pedro Alves

[RFA_v3 8/8] Add a self-test for cli-utils.c

[Philippe Waroquiers]

tests added for:
* number_or_range_parser
  In particular, it tests the cur_tok when parsing is finished.

* parse_flags

* parse_flags_qcs

gdb/ChangeLog
2018-06-05  Philippe Waroquiers  <philippe.waroquiers@skynet.be>

	* Makefile.in (SUBDIR_UNITTESTS_SRCS): Add
	unittests/cli-utils-selftests.c
	* unittests/cli-utils-selftests.c: New file.
---
 gdb/Makefile.in                     |   1 +
 gdb/unittests/cli-utils-selftests.c | 220 ++++++++++++++++++++++++++++
 2 files changed, 221 insertions(+)
 create mode 100644 gdb/unittests/cli-utils-selftests.c

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index 354a6361b7..f8cdf9a560 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -416,6 +416,7 @@ SUBDIR_PYTHON_CFLAGS =
 
 SUBDIR_UNITTESTS_SRCS = \
 	unittests/array-view-selftests.c \
+	unittests/cli-utils-selftests.c \
 	unittests/common-utils-selftests.c \
 	unittests/environ-selftests.c \
 	unittests/format_pieces-selftests.c \
diff --git a/gdb/unittests/cli-utils-selftests.c b/gdb/unittests/cli-utils-selftests.c
new file mode 100644
index 0000000000..99414f097e
--- /dev/null
+++ b/gdb/unittests/cli-utils-selftests.c
@@ -0,0 +1,220 @@
+/* Unit tests for the cli-utils.c file.
+
+   Copyright (C) 2018 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include "defs.h"
+#include "cli/cli-utils.h"
+#include "selftest.h"
+
+namespace selftests {
+namespace cli_utils {
+
+static void
+test_number_or_range_parser ()
+{
+  number_or_range_parser one ("1");
+
+  SELF_CHECK (one.finished () == false);
+  SELF_CHECK (one.get_number () == 1);
+  SELF_CHECK (one.finished () == true);
+  SELF_CHECK (strcmp (one.cur_tok (), "") == 0);
+
+  number_or_range_parser one_after ("1 after");
+
+  SELF_CHECK (one_after.finished () == false);
+  SELF_CHECK (one_after.get_number () == 1);
+  SELF_CHECK (one_after.finished () == true);
+  SELF_CHECK (strcmp (one_after.cur_tok (), "after") == 0);
+
+  number_or_range_parser one_three ("1-3");
+
+  for (int i = 1; i < 4; i++) {
+    SELF_CHECK (one_three.finished () == false);
+    SELF_CHECK (one_three.get_number () == i);
+  }
+  SELF_CHECK (one_three.finished () == true);
+  SELF_CHECK (strcmp (one_three.cur_tok (), "") == 0);
+
+  number_or_range_parser one_three_after ("1-3 after");
+
+  for (int i = 1; i < 4; i++) {
+    SELF_CHECK (one_three_after.finished () == false);
+    SELF_CHECK (one_three_after.get_number () == i);
+  }
+  SELF_CHECK (one_three_after.finished () == true);
+  SELF_CHECK (strcmp (one_three_after.cur_tok (), "after") == 0);
+
+  number_or_range_parser minus_one ("-1");
+
+  SELF_CHECK (minus_one.finished () == false);
+  TRY
+    {
+      minus_one.get_number ();
+      SELF_CHECK (false);
+    }
+  CATCH (ex, RETURN_MASK_ERROR)
+    {
+      SELF_CHECK (ex.reason == RETURN_ERROR);
+      SELF_CHECK (ex.error == GENERIC_ERROR);
+      SELF_CHECK (strcmp (ex.message, "negative value") == 0);
+      SELF_CHECK (strcmp (minus_one.cur_tok (), "-1") == 0);
+    }
+  END_CATCH;
+
+  number_or_range_parser nan ("-whatever");
+
+  SELF_CHECK (nan.finished () == true);
+  SELF_CHECK (one_three_after.get_number () == 0);
+  SELF_CHECK (strcmp (nan.cur_tok (), "-whatever") == 0);
+}
+
+static void
+test_parse_flags ()
+{
+  const char *flags = "abc";
+  const char *non_flags_args = "non flags args";
+  int res;
+
+  const char *t1 = "-a -a non flags args";
+
+  SELF_CHECK (parse_flags (&t1, flags) == 1);
+  SELF_CHECK (parse_flags (&t1, flags) == 1);
+  SELF_CHECK (strcmp (t1, non_flags_args) == 0);
+
+  const char *t2 = "-c     -b -c  -b -c    non flags args";
+
+  SELF_CHECK (parse_flags (&t2, flags) == 3);
+  SELF_CHECK (parse_flags (&t2, flags) == 2);
+  SELF_CHECK (parse_flags (&t2, flags) == 3);
+  SELF_CHECK (parse_flags (&t2, flags) == 2);
+  SELF_CHECK (parse_flags (&t2, flags) == 3);
+  SELF_CHECK (strcmp (t2, non_flags_args) == 0);
+
+  const char *t3 = non_flags_args;
+
+  SELF_CHECK (parse_flags (&t3, flags) == 0);
+  SELF_CHECK (strcmp (t3, non_flags_args) == 0);
+
+  const char *t4 = "-c -b -x -y -z -c";
+  const char *orig_t4 = t4;
+
+  SELF_CHECK (parse_flags (&t4, flags) == 3);
+  SELF_CHECK (parse_flags (&t4, flags) == 2);
+  SELF_CHECK (strcmp (t4, "-x -y -z -c") == 0);
+
+  const char *t5 = "-c -cb -c";
+  const char *orig_t5 = t5;
+
+  SELF_CHECK (parse_flags (&t5, flags) == 3);
+  SELF_CHECK (parse_flags (&t5, flags) == 0);
+  SELF_CHECK (strcmp (t5, "-cb -c") == 0);
+}
+
+static void
+test_parse_flags_qcs ()
+{
+  bool quiet;
+  bool cont;
+  bool silent;
+
+  const char *non_flags_args = "non flags args";
+
+  const char *t1 = "-q -s    non flags args";
+  quiet = false;
+  cont = false;
+  silent = false;
+
+  SELF_CHECK (parse_flags_qcs ("test_parse_flags_qcs.t1.q",
+			       &t1,
+			       &quiet, &cont, &silent) == 1);
+  SELF_CHECK (quiet == true && cont == false && silent == false);
+  SELF_CHECK (parse_flags_qcs ("test_parse_flags_qcs.t1.s",
+			       &t1,
+			       &quiet, &cont, &silent) == 1);
+  SELF_CHECK (quiet == true && cont == false && silent == true);
+  SELF_CHECK (strcmp (t1, non_flags_args) == 0);
+
+  const char *t2 = "non flags args";
+  quiet = false;
+  cont = false;
+  silent = false;
+
+  SELF_CHECK (parse_flags_qcs ("test_parse_flags_qcs.t2",
+			       &t2,
+			       &quiet, &cont, &silent) == 0);
+  SELF_CHECK (quiet == false && cont == false && silent == false);
+  SELF_CHECK (strcmp (t2, non_flags_args) == 0);
+
+  const char *t3 = "-123 non flags args";
+  const char *orig_t3 = t3;
+  quiet = false;
+  cont = false;
+  silent = false;
+
+  SELF_CHECK (parse_flags_qcs ("test_parse_flags_qcs.t3",
+			       &t3,
+			       &quiet, &cont, &silent) == 0);
+  SELF_CHECK (quiet == false && cont == false && silent == false);
+  SELF_CHECK (strcmp (t3, orig_t3) == 0);
+
+  const char *t4 = "-c -s non flags args";
+  const char *orig_t4 = t4;
+  quiet = false;
+  cont = false;
+  silent = false;
+  TRY
+    {
+      SELF_CHECK (parse_flags_qcs ("test_parse_flags_qcs.t4.cs",
+				   &t4,
+				   &quiet, &cont, &silent) == 1);
+
+      (void) parse_flags_qcs ("test_parse_flags_qcs.t4.cs",
+			      &t4,
+			      &quiet, &cont, &silent);
+      SELF_CHECK (false);
+    }
+  CATCH (ex, RETURN_MASK_ERROR)
+    {
+      SELF_CHECK (ex.reason == RETURN_ERROR);
+      SELF_CHECK (ex.error == GENERIC_ERROR);
+      SELF_CHECK
+	(strcmp (ex.message,
+		 "test_parse_flags_qcs.t4.cs: "
+		 "-c and -s are mutually exclusive") == 0);
+    }
+  END_CATCH;
+
+}
+
+static void
+test_cli_utils ()
+{
+  selftests::cli_utils::test_number_or_range_parser ();
+  selftests::cli_utils::test_parse_flags ();
+  selftests::cli_utils::test_parse_flags_qcs ();
+}
+
+}
+}
+
+void
+_initialize_cli_utils_selftests ()
+{
+  selftests::register_test ("cli_utils",
+			    selftests::cli_utils::test_cli_utils);
+}
-- 
2.17.1

Re: [RFA_v3 8/8] Add a self-test for cli-utils.c

[Pedro Alves]

On 06/24/2018 07:37 PM, Philippe Waroquiers wrote:
> tests added for:
> * number_or_range_parser
>   In particular, it tests the cur_tok when parsing is finished.
> 
> * parse_flags
> 
> * parse_flags_qcs

Thanks much for adding unit tests.  That's really great.

A small request below, but this looks good to me otherwise.

> 
> gdb/ChangeLog
> 2018-06-05  Philippe Waroquiers  <philippe.waroquiers@skynet.be>
> 
> 	* Makefile.in (SUBDIR_UNITTESTS_SRCS): Add
> 	unittests/cli-utils-selftests.c
> 	* unittests/cli-utils-selftests.c: New file.
> ---
>  gdb/Makefile.in                     |   1 +
>  gdb/unittests/cli-utils-selftests.c | 220 ++++++++++++++++++++++++++++
>  2 files changed, 221 insertions(+)
>  create mode 100644 gdb/unittests/cli-utils-selftests.c
> 
> diff --git a/gdb/Makefile.in b/gdb/Makefile.in
> index 354a6361b7..f8cdf9a560 100644
> --- a/gdb/Makefile.in
> +++ b/gdb/Makefile.in
> @@ -416,6 +416,7 @@ SUBDIR_PYTHON_CFLAGS =
>  
>  SUBDIR_UNITTESTS_SRCS = \
>  	unittests/array-view-selftests.c \
> +	unittests/cli-utils-selftests.c \
>  	unittests/common-utils-selftests.c \
>  	unittests/environ-selftests.c \
>  	unittests/format_pieces-selftests.c \
> diff --git a/gdb/unittests/cli-utils-selftests.c b/gdb/unittests/cli-utils-selftests.c
> new file mode 100644
> index 0000000000..99414f097e
> --- /dev/null
> +++ b/gdb/unittests/cli-utils-selftests.c
> @@ -0,0 +1,220 @@
> +/* Unit tests for the cli-utils.c file.
> +
> +   Copyright (C) 2018 Free Software Foundation, Inc.
> +
> +   This file is part of GDB.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +#include "defs.h"
> +#include "cli/cli-utils.h"
> +#include "selftest.h"
> +
> +namespace selftests {
> +namespace cli_utils {
> +
> +static void
> +test_number_or_range_parser ()
> +{
> +  number_or_range_parser one ("1");
> +
> +  SELF_CHECK (one.finished () == false);
> +  SELF_CHECK (one.get_number () == 1);
> +  SELF_CHECK (one.finished () == true);

Curious that you write "== false", "== true" throughout
instead of:

  SELF_CHECK (!one.finished ());
  SELF_CHECK (one.finished ());

I don't mind here, it just stood out.

> +  SELF_CHECK (strcmp (one.cur_tok (), "") == 0);
> +
> +  number_or_range_parser one_after ("1 after");
> +
> +  SELF_CHECK (one_after.finished () == false);
> +  SELF_CHECK (one_after.get_number () == 1);
> +  SELF_CHECK (one_after.finished () == true);
> +  SELF_CHECK (strcmp (one_after.cur_tok (), "after") == 0);
> +
> +  number_or_range_parser one_three ("1-3");
> +
> +  for (int i = 1; i < 4; i++) {

"{" goes on next line.  Appears in several places.

> +static void
> +test_parse_flags ()
> +{
> +  const char *flags = "abc";
> +  const char *non_flags_args = "non flags args";
> +  int res;
> +
> +  const char *t1 = "-a -a non flags args";
> +
> +  SELF_CHECK (parse_flags (&t1, flags) == 1);
> +  SELF_CHECK (parse_flags (&t1, flags) == 1);
> +  SELF_CHECK (strcmp (t1, non_flags_args) == 0);
> +
> +  const char *t2 = "-c     -b -c  -b -c    non flags args";
> +
> +  SELF_CHECK (parse_flags (&t2, flags) == 3);
> +  SELF_CHECK (parse_flags (&t2, flags) == 2);
> +  SELF_CHECK (parse_flags (&t2, flags) == 3);
> +  SELF_CHECK (parse_flags (&t2, flags) == 2);
> +  SELF_CHECK (parse_flags (&t2, flags) == 3);
> +  SELF_CHECK (strcmp (t2, non_flags_args) == 0);
> +
> +  const char *t3 = non_flags_args;
> +
> +  SELF_CHECK (parse_flags (&t3, flags) == 0);
> +  SELF_CHECK (strcmp (t3, non_flags_args) == 0);
> +
> +  const char *t4 = "-c -b -x -y -z -c";
> +  const char *orig_t4 = t4;
> +

orig_t4 appears unused.

> +  SELF_CHECK (parse_flags (&t4, flags) == 3);
> +  SELF_CHECK (parse_flags (&t4, flags) == 2);
> +  SELF_CHECK (strcmp (t4, "-x -y -z -c") == 0);
> +
> +  const char *t5 = "-c -cb -c";
> +  const char *orig_t5 = t5;

orig_t5 appears unused.

> +
> +  SELF_CHECK (parse_flags (&t5, flags) == 3);
> +  SELF_CHECK (parse_flags (&t5, flags) == 0);
> +  SELF_CHECK (strcmp (t5, "-cb -c") == 0);

Could you add short single-line comments indicating
what the different parts of the function are testing?
I.e., some guidance into what detail the t1 section is
testing, what detail the t2 section is testing, etc.  I think
that likely helps whoever needs to extend the testcase in future.

I'd suggest wrapping such sections of these functions
in their own scope, which both helps visually distinguish the
sections, and avoids variable spillage between the sections
too.  See e.g., run_tests in array-view-selftests.c.

Thanks,
Pedro Alves

[RFA_v3 6/8] Add a test for 'frame apply'

[Philippe Waroquiers]

Add a test for 'frame apply'

gdb/testsuite/ChangeLog
2018-06-24  Philippe Waroquiers  <philippe.waroquiers@skynet.be>

	* gdb.base/frameapply.c: New file.
	* gdb.base/frameapply.exp: New file.
---
 gdb/testsuite/gdb.base/frameapply.c   |  71 ++++++++++
 gdb/testsuite/gdb.base/frameapply.exp | 179 ++++++++++++++++++++++++++
 2 files changed, 250 insertions(+)
 create mode 100644 gdb/testsuite/gdb.base/frameapply.c
 create mode 100644 gdb/testsuite/gdb.base/frameapply.exp

diff --git a/gdb/testsuite/gdb.base/frameapply.c b/gdb/testsuite/gdb.base/frameapply.c
new file mode 100644
index 0000000000..dccf4031ed
--- /dev/null
+++ b/gdb/testsuite/gdb.base/frameapply.c
@@ -0,0 +1,71 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2018 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+static void
+setup_done (void)
+{
+}
+
+static int
+f1 (int f1arg)
+{
+  int f1loc;
+
+  f1loc = f1arg - 1;
+
+  setup_done ();
+  return f1loc;
+}
+
+static int
+f2 (int f2arg)
+{
+  int f2loc;
+
+  f2loc = f1 (f2arg - 1);
+
+  return f2loc;
+}
+
+static int
+f3 (int f3arg)
+{
+  int f3loc;
+
+  f3loc = f2 (f3arg - 1);
+
+  return f3loc;
+}
+
+static int
+f4 (int f4arg)
+{
+  int f4loc;
+
+  f4loc = f3 (f4arg - 1);
+
+  return f4loc;
+}
+
+int
+main (void)
+{
+  int result;
+
+  result = f4 (4);
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.base/frameapply.exp b/gdb/testsuite/gdb.base/frameapply.exp
new file mode 100644
index 0000000000..1911e15b72
--- /dev/null
+++ b/gdb/testsuite/gdb.base/frameapply.exp
@@ -0,0 +1,179 @@
+# This testcase is part of GDB, the GNU debugger.
+
+# Copyright 2018 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+
+# Test 'frame apply [all | COUNT | -COUNT | id ID...] [FLAG]... COMMAND'
+
+standard_testfile
+
+if { [prepare_for_testing "failed to prepare" ${testfile}] } {
+    return -1
+}
+
+clean_restart ${binfile}
+
+
+if ![runto setup_done] then {
+    gdb_suppress_tests
+}
+
+set any "\[^\r\n\]*"
+set ws "\[ \t\]\+"
+set number "\[0-9]\+"
+
+
+# check all | COUNT | -COUNT | id ID... with a simple command
+foreach cmd {
+    "frame apply all p /x 20"
+    "frame apply id 0-5 p /x 20"
+    "frame apply id 0-3 4-5 p /x 20"
+    "frame apply id 0 1-2 3-5 p /x 20"
+    "frame apply id 0 1 2 3 4 5 p /x 20"
+    "frame apply id 0-0 1-1 2-2 3-3 4-4 5-5 p /x 20" } {
+    gdb_test $cmd \
+    [multi_line \
+	 "#0${ws}setup_done ${any}" \
+	 "\\\$\[0-9]+ = 0x14${any}" \
+	 "#1${ws}${any} f1 ${any}" \
+	 "\\\$\[0-9]+ = 0x14${any}" \
+	 "#2${ws}${any} f2 ${any}" \
+	 "\\\$\[0-9]+ = 0x14${any}" \
+	 "#3${ws}${any} f3 ${any}" \
+	 "\\\$\[0-9]+ = 0x14${any}" \
+	 "#4${ws}${any} f4 ${any}" \
+	 "\\\$\[0-9]+ = 0x14${any}" \
+	 "#5${ws}${any} main ${any}" \
+	 "\\\$\[0-9]+ = 0x14${any}" \
+	] \
+	"Run a simple command on all frames"
+}
+
+foreach cmd {"frame apply 3 p /x 20" "frame apply id 0-2 p /x 20"} {
+    gdb_test $cmd \
+	[multi_line \
+	     "#0${ws}setup_done ${any}" \
+	     "\\\$\[0-9]+ = 0x14${any}" \
+	     "#1${ws}${any} f1 ${any}" \
+	     "\\\$\[0-9]+ = 0x14${any}" \
+	     "#2${ws}${any} f2 ${any}" \
+	     "\\\$\[0-9]+ = 0x14${any}" \
+	    ] \
+	"Run a simple command on the 3 innermost frames"
+}
+
+gdb_test "frame apply -3 p /x 20" \
+    [multi_line \
+	 "#3${ws}${any} f3 ${any}" \
+	 "\\\$\[0-9]+ = 0x14${any}" \
+	 "#4${ws}${any} f4 ${any}" \
+	 "\\\$\[0-9]+ = 0x14${any}" \
+	 "#5${ws}${any} main ${any}" \
+	 "\\\$\[0-9]+ = 0x14${any}" \
+	] \
+    "Run a simple command on the 3 outermost frames"
+
+# check -c (continue) and -s (silently continue) flags
+foreach cmd {
+    "frame apply all p f3arg"
+    "frame apply id 0-5 p f3arg"} {
+    gdb_test $cmd \
+	[multi_line \
+	     "#0${ws}setup_done ${any}" \
+	     "No symbol \\\"f3arg\\\" in current context." \
+	    ] \
+	"Run a failing command that aborts frame apply"
+}
+
+foreach cmd {
+    "frame apply all -c p f3arg"
+    "frame apply id 0-5 -c p f3arg"} {
+    gdb_test $cmd \
+	[multi_line \
+	     "#0${ws}setup_done ${any}" \
+	     "No symbol \\\"f3arg\\\" in current context." \
+	     "#1${ws}${any} f1 ${any}" \
+	     "No symbol \\\"f3arg\\\" in current context." \
+	     "#2${ws}${any} f2 ${any}" \
+	     "No symbol \\\"f3arg\\\" in current context." \
+	     "#3${ws}${any} f3 ${any}" \
+	     "\\\$\[0-9]+ = 3${any}" \
+	     "#4${ws}${any} f4 ${any}" \
+	     "No symbol \\\"f3arg\\\" in current context." \
+	     "#5${ws}${any} main ${any}" \
+	     "No symbol \\\"f3arg\\\" in current context." \
+	    ] \
+	"Run a command failing in all frames except #3, -c to continue"
+}
+
+foreach cmd {
+    "frame apply all -s p f3arg"
+    "faas p f3arg"
+    "frame apply id 0-5 -s p f3arg"} {
+    gdb_test $cmd \
+	[multi_line \
+	     "#3${ws}${any} f3 ${any}" \
+	     "\\\$\[0-9]+ = 3${any}" \
+	    ] \
+	"Run a command failing in all frames except #3, -s to silently continue"
+}
+
+# check quietness
+foreach cmd {
+    "frame apply 2 p /x 20"
+    "frame apply id 0-1 p /x 20"} {
+    gdb_test $cmd \
+	[multi_line \
+	     "#0${ws}setup_done ${any}" \
+	     "\\\$\[0-9]+ = 0x14${any}" \
+	     "#1${ws}${any} f1 ${any}" \
+	     "\\\$\[0-9]+ = 0x14${any}" \
+	    ] \
+	"Run a command, printing location"
+}
+
+foreach cmd {
+    "frame apply 2 -q p /x 20"
+    "frame apply id 0-1 -q p /x 20"} {
+    gdb_test $cmd \
+	[multi_line \
+	     "\\\$\[0-9]+ = 0x14${any}" \
+	     "\\\$\[0-9]+ = 0x14${any}" \
+	    ] \
+	"Run a command with -q quiet flag, printing only command results"
+}
+
+# check multiple flags together
+foreach cmd {
+    "frame apply all -q -s -q p f3arg"
+    "frame apply id 0-5 -q -s -q p f3arg"} {
+    gdb_test $cmd \
+	"\\\$\[0-9]+ = 3${any}" \
+	"Run a command failing in all frames except #3, -s to silently continue, quiet"
+}
+
+# check invalid flag combinations
+gdb_test "frame apply all -c -s p f3arg" \
+    "frame apply all: -c and -s are mutually exclusive" \
+    "Check -c and -s cannot be used simultaneously"
+
+# check some cases of missing or wrong args
+gdb_test "frame apply" "Missing COUNT argument." "missing COUNT"
+gdb_test "frame apply -c" "Invalid COUNT argument." "invalid COUNT arg"
+gdb_test "frame apply id 4-2 p 1" "inverted range" "inverted range"
+gdb_test "frame apply id 0-3" \
+    "Please specify a command to apply on the selected frames" \
+    "missing command"
-- 
2.17.1

Re: [RFA_v3 6/8] Add a test for 'frame apply'

[Pedro Alves]

On 06/24/2018 07:37 PM, Philippe Waroquiers wrote:
> Add a test for 'frame apply'

Very nice.  This looks mostly OK, though there's a couple details
that need sorting out.  See below.

> diff --git a/gdb/testsuite/gdb.base/frameapply.exp b/gdb/testsuite/gdb.base/frameapply.exp
> new file mode 100644
> index 0000000000..1911e15b72
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/frameapply.exp
> @@ -0,0 +1,179 @@
> +# This testcase is part of GDB, the GNU debugger.
> +
> +# Copyright 2018 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +
> +# Test 'frame apply [all | COUNT | -COUNT | id ID...] [FLAG]... COMMAND'
> +
> +standard_testfile
> +
> +if { [prepare_for_testing "failed to prepare" ${testfile}] } {
> +    return -1
> +}
> +
> +clean_restart ${binfile}
> +
> +
> +if ![runto setup_done] then {
> +    gdb_suppress_tests
> +}

gdb_suppress_tests is a really old mechanism that we shouldn't
be adding new uses of.  Make that call fail and return immediately,
instead of continuing.

> +
> +set any "\[^\r\n\]*"
> +set ws "\[ \t\]\+"
> +set number "\[0-9]\+"
> +
> +
> +# check all | COUNT | -COUNT | id ID... with a simple command
> +foreach cmd {
> +    "frame apply all p /x 20"
> +    "frame apply id 0-5 p /x 20"
> +    "frame apply id 0-3 4-5 p /x 20"
> +    "frame apply id 0 1-2 3-5 p /x 20"
> +    "frame apply id 0 1 2 3 4 5 p /x 20"
> +    "frame apply id 0-0 1-1 2-2 3-3 4-4 5-5 p /x 20" } {
> +    gdb_test $cmd \
> +    [multi_line \
> +	 "#0${ws}setup_done ${any}" \
> +	 "\\\$\[0-9]+ = 0x14${any}" \
> +	 "#1${ws}${any} f1 ${any}" \
> +	 "\\\$\[0-9]+ = 0x14${any}" \
> +	 "#2${ws}${any} f2 ${any}" \
> +	 "\\\$\[0-9]+ = 0x14${any}" \
> +	 "#3${ws}${any} f3 ${any}" \
> +	 "\\\$\[0-9]+ = 0x14${any}" \
> +	 "#4${ws}${any} f4 ${any}" \
> +	 "\\\$\[0-9]+ = 0x14${any}" \
> +	 "#5${ws}${any} main ${any}" \
> +	 "\\\$\[0-9]+ = 0x14${any}" \
> +	] \
> +	"Run a simple command on all frames"

lowercase "Run" -> "run".  Likewise throughout.

> +# check quietness
> +foreach cmd {
> +    "frame apply 2 p /x 20"
> +    "frame apply id 0-1 p /x 20"} {

I noticed that the testcase produces duplicated test names in gdb.sum.
Take a look here:

  https://sourceware.org/gdb/wiki/GDBTestcaseCookbook#Make_sure_test_messages_are_unique

You can fix this with e.g., foreach_with_prefix.  I'd suggest
wrapping the different stages of the testcase with
with_test_prefix as well, for visual guidance when browsing
gdb.sum results though that's less important.

Something like this:

# Check quietness.
with_test_prefix "quietness" {
  foreach_with_prefix frames {"2" "id 0-1"} {

    set cmd "frame apply $frames p /x 20"

    gdb_test $cmd \
        [multi_line \
	     "#0${ws}setup_done ${any}" \
	     "\\\$\[0-9]+ = 0x14${any}" \
	     "#1${ws}${any} f1 ${any}" \
	     "\\\$\[0-9]+ = 0x14${any}" \
	    ] \
	"run a command, printing location"
...

  }
}

(Note, "Check quietness." with uppercase and period.  Might as
well write complete sentences while at it.)

The same comments apply to patch #7, so I won't repeat them there.

Thanks,
Pedro Alves

[RFA_v3 1/8] Add helper functions parse_flags and parse_flags_qcs

[Philippe Waroquiers]

Add helper functions parse_flags and parse_flags_qcs.
parse_flags helper function allows to look for a set of flags at
the start of a string.
A flag must be given individually.

parse_flags_qcs is a specialised helper function to handle
the flags -q, -c and -s, that are used in the new command 'frame apply'
and in the command 'thread apply.

Modify number_or_range_parser::get_number to differentiate a
- followed by digits from a - followed by an alpha (i.e. a flag or an option).
That is needed for the addition of the [FLAG]... arguments to
thread apply ID... [FLAG]... COMMAND

Remove bool number_or_range_parser::m_finished, rather
implement the 'finished' logic inside number_or_range_parser::finished.
The new logic properly detects the end of parsing even if not at
end of the string. This ensures that number_or_range_parser::cur_tok
really points past the last parsed token when parsing is finished.
Before, it was always pointing at the end of the string.
As parsing now is finished directly when not positioned on a number,
number_is_in_list must do an error check before the loop getting all
numbers.

The error message for 'thread apply -$unknownconvvar p 1'
is now the more clear:
  Convenience variable must have integer value.
  Invalid thread ID: -$unknownconvvar p 1
instead of previously:
  negative value

gdb/ChangeLog
2018-06-24  Philippe Waroquiers  <philippe.waroquiers@skynet.be>

	* cli-utils.c (number_or_range_parser::get_number): Only handle
	numbers or convenience var as numbers.
	(parse_flags): New function.
	(parse_flags_qcs): New function.
        (number_or_range_parser::finished): ensure parsing end is detected
        before end of string.
	* cli-utils.h (parse_flags): New function.
	(parse_flags_qcs): New function.
        (number_or_range_parser): Remove m_finished bool.
        (number_or_range_parser::skip_range): set m_in_range to false.
---
 gdb/cli/cli-utils.c | 98 +++++++++++++++++++++++++++++++++++++++++++--
 gdb/cli/cli-utils.h | 35 +++++++++++++---
 2 files changed, 124 insertions(+), 9 deletions(-)

diff --git a/gdb/cli/cli-utils.c b/gdb/cli/cli-utils.c
index c55b5035e4..6fef419216 100644
--- a/gdb/cli/cli-utils.c
+++ b/gdb/cli/cli-utils.c
@@ -137,7 +137,6 @@ number_or_range_parser::number_or_range_parser (const char *string)
 void
 number_or_range_parser::init (const char *string)
 {
-  m_finished = false;
   m_cur_tok = string;
   m_last_retval = 0;
   m_end_value = 0;
@@ -169,7 +168,10 @@ number_or_range_parser::get_number ()
       /* Default case: state->m_cur_tok is pointing either to a solo
 	 number, or to the first number of a range.  */
       m_last_retval = get_number_trailer (&m_cur_tok, '-');
-      if (*m_cur_tok == '-')
+      /* If get_number_trailer has found a -, it might be the start
+	 of a command option.  So, do not parse a range if the - is
+	 followed by an alpha.  */
+      if (*m_cur_tok == '-' && !isalpha (*(m_cur_tok + 1)))
 	{
 	  const char **temp;
 
@@ -196,8 +198,17 @@ number_or_range_parser::get_number ()
 	}
     }
   else
-    error (_("negative value"));
-  m_finished = *m_cur_tok == '\0';
+    {
+      if (isdigit (*(m_cur_tok + 1)))
+	error (_("negative value"));
+      if (*(m_cur_tok + 1) == '$')
+	{
+	  /* Convenience variable.  */
+	  m_last_retval = ::get_number (&m_cur_tok);
+	  if (m_last_retval < 0)
+	    error (_("negative value"));
+	}
+    }
   return m_last_retval;
 }
 
@@ -215,6 +226,22 @@ number_or_range_parser::setup_range (int start_value, int end_value,
   m_end_value = end_value;
 }
 
+/* See documentation in cli-utils.h.  */
+
+bool
+number_or_range_parser::finished () const
+{
+  /* Parsing is finished when at end of string or null string,
+     or
+     we are not in a range and not in front of an integer,
+     negative integer, convenience var or negative convenience var.  */
+  return (m_cur_tok == NULL || *m_cur_tok == '\0')
+    || (!m_in_range
+	&& !(isdigit (*m_cur_tok) || *m_cur_tok == '$')
+	&& !(*m_cur_tok == '-'
+	     && (isdigit (*(m_cur_tok + 1)) || *(m_cur_tok + 1) == '$')));
+}
+
 /* Accept a number and a string-form list of numbers such as is 
    accepted by get_number_or_range.  Return TRUE if the number is
    in the list.
@@ -230,6 +257,9 @@ number_is_in_list (const char *list, int number)
     return 1;
 
   number_or_range_parser parser (list);
+
+  if (parser.finished ())
+    error (_("Args must be numbers or '$' variables."));
   while (!parser.finished ())
     {
       int gotnum = parser.get_number ();
@@ -304,3 +334,63 @@ check_for_argument (const char **str, const char *arg, int arg_len)
     }
   return 0;
 }
+
+/* See documentation in cli-utils.h.  */
+
+int
+parse_flags (const char **str, const char *flags)
+{
+  const char *p = skip_spaces (*str);
+
+  if (p[0] == '-'
+      && isalpha (p[1])
+      && (p[2] == '\0' || isspace (p[2])))
+    {
+      const char pf = p[1];
+      const char *f = flags;
+
+      while (*f != '\0')
+	{
+	  if (*f == pf)
+	    {
+	      *str = skip_spaces (p + 2);
+	      return f - flags + 1;
+	    }
+	  f++;
+	}
+    }
+
+  return 0;
+}
+
+/* See documentation in cli-utils.h.  */
+
+int
+parse_flags_qcs (const char *which_command, const char **str,
+		 bool *quiet, bool *cont, bool *silent)
+{
+  const char *flags = "qcs";
+  int res;
+
+  switch (parse_flags (str, flags))
+    {
+    case 0:
+      return 0;
+    case 1:
+      *quiet = true;
+      break;
+    case 2:
+      *cont = true;
+      break;
+    case 3:
+      *silent = true;
+      break;
+    default:
+      gdb_assert (0);
+    }
+
+  if (*cont && *silent)
+    error (_("%s: -c and -s are mutually exclusive"), which_command);
+
+  return 1;
+}
diff --git a/gdb/cli/cli-utils.h b/gdb/cli/cli-utils.h
index e34ee0df37..56be58e955 100644
--- a/gdb/cli/cli-utils.h
+++ b/gdb/cli/cli-utils.h
@@ -75,8 +75,7 @@ public:
 		    const char *end_ptr);
 
   /* Returns true if parsing has completed.  */
-  bool finished () const
-  { return m_finished; }
+  bool finished () const;
 
   /* Return the string being parsed.  When parsing has finished, this
      points past the last parsed token.  */
@@ -96,6 +95,7 @@ public:
   {
     gdb_assert (m_in_range);
     m_cur_tok = m_end_ptr;
+    m_in_range = false;
   }
 
 private:
@@ -103,9 +103,6 @@ private:
   number_or_range_parser (const number_or_range_parser &);
   number_or_range_parser &operator= (const number_or_range_parser &);
 
-  /* True if parsing has completed.  */
-  bool m_finished;
-
   /* The string being parsed.  When parsing has finished, this points
      past the last parsed token.  */
   const char *m_cur_tok;
@@ -173,4 +170,32 @@ check_for_argument (char **str, const char *arg, int arg_len)
 			     arg, arg_len);
 }
 
+/* A helper function that looks for a set of flags at the start of a
+   string.  The possible flags are given as a null terminated string.
+   A flag in STR must either be at the end of the string,
+   or be followed by whitespace.
+   Returns 0 if no valid flag is found at the start of STR.
+   Otherwise updates *STR, and returns N (which is > 0),
+   such that FLAGS [N - 1] is the valid found flag.  */
+extern int parse_flags (const char **str, const char *flags);
+
+/* A helper function that uses parse_flags to handle the flags qcs :
+     A flag -q sets *QUIET to true.
+     A flag -c sets *CONT to true.
+     A flag -s sets *SILENT to true.
+
+   The caller is responsible to initialize *QUIET, *CONT, *SILENT to
+   false before the (first) call to parse_flags_qcs.
+   parse_flags_qcs can then be called iteratively to search for more
+   valid flags, as part of a 'main parsing loop' searching for -q/-c/-s
+   flags together with other flags and options.
+
+   Returns 1 and updates *STR and one of *QUIET, *CONT, *SILENT
+   if it finds a valid flag.
+   Returns 0 if no valid flag is found at the beginning of STR.
+
+   Throws an error if a flag is found such that both *CONT and *SILENT
+   are true.  */
+extern int parse_flags_qcs (const char *which_command, const char **str,
+			    bool *quiet, bool *cont, bool *silent);
 #endif /* CLI_UTILS_H */
-- 
2.17.1

Re: [RFA_v3 1/8] Add helper functions parse_flags and parse_flags_qcs

[Pedro Alves]

Hi Philippe,

This looks mostly good to me.  A few minor comments below,
and a suggestion.

On 06/24/2018 07:37 PM, Philippe Waroquiers wrote:

> 
> The error message for 'thread apply -$unknownconvvar p 1'
> is now the more clear:
>   Convenience variable must have integer value.
>   Invalid thread ID: -$unknownconvvar p 1
> instead of previously:
>   negative value
> 
> gdb/ChangeLog
> 2018-06-24  Philippe Waroquiers  <philippe.waroquiers@skynet.be>
> 
> 	* cli-utils.c (number_or_range_parser::get_number): Only handle
> 	numbers or convenience var as numbers.
> 	(parse_flags): New function.
> 	(parse_flags_qcs): New function.
>         (number_or_range_parser::finished): ensure parsing end is detected
>         before end of string.
> 	* cli-utils.h (parse_flags): New function.
> 	(parse_flags_qcs): New function.
>         (number_or_range_parser): Remove m_finished bool.
>         (number_or_range_parser::skip_range): set m_in_range to false.

Note some tab vs spaces mixup above.

> ---
>  gdb/cli/cli-utils.c | 98 +++++++++++++++++++++++++++++++++++++++++++--
>  gdb/cli/cli-utils.h | 35 +++++++++++++---
>  2 files changed, 124 insertions(+), 9 deletions(-)
> 
> diff --git a/gdb/cli/cli-utils.c b/gdb/cli/cli-utils.c
> index c55b5035e4..6fef419216 100644
> --- a/gdb/cli/cli-utils.c
> +++ b/gdb/cli/cli-utils.c
> @@ -137,7 +137,6 @@ number_or_range_parser::number_or_range_parser (const char *string)
>  void
>  number_or_range_parser::init (const char *string)
>  {
> -  m_finished = false;
>    m_cur_tok = string;
>    m_last_retval = 0;
>    m_end_value = 0;
> @@ -169,7 +168,10 @@ number_or_range_parser::get_number ()
>        /* Default case: state->m_cur_tok is pointing either to a solo
>  	 number, or to the first number of a range.  */
>        m_last_retval = get_number_trailer (&m_cur_tok, '-');
> -      if (*m_cur_tok == '-')
> +      /* If get_number_trailer has found a -, it might be the start
> +	 of a command option.  So, do not parse a range if the - is
> +	 followed by an alpha.  */
> +      if (*m_cur_tok == '-' && !isalpha (*(m_cur_tok + 1)))
>  	{
>  	  const char **temp;
>  
> @@ -196,8 +198,17 @@ number_or_range_parser::get_number ()
>  	}
>      }
>    else
> -    error (_("negative value"));
> -  m_finished = *m_cur_tok == '\0';
> +    {
> +      if (isdigit (*(m_cur_tok + 1)))
> +	error (_("negative value"));
> +      if (*(m_cur_tok + 1) == '$')
> +	{
> +	  /* Convenience variable.  */
> +	  m_last_retval = ::get_number (&m_cur_tok);
> +	  if (m_last_retval < 0)
> +	    error (_("negative value"));
> +	}
> +    }
>    return m_last_retval;
>  }
>  
> @@ -215,6 +226,22 @@ number_or_range_parser::setup_range (int start_value, int end_value,
>    m_end_value = end_value;
>  }
>  
> +/* See documentation in cli-utils.h.  */
> +
> +bool
> +number_or_range_parser::finished () const
> +{
> +  /* Parsing is finished when at end of string or null string,
> +     or
> +     we are not in a range and not in front of an integer,
> +     negative integer, convenience var or negative convenience var.  */

Seems strange that "or" is in its own line.  Hit meta-q in emacs?

> +  return (m_cur_tok == NULL || *m_cur_tok == '\0')
> +    || (!m_in_range
> +	&& !(isdigit (*m_cur_tok) || *m_cur_tok == '$')
> +	&& !(*m_cur_tok == '-'
> +	     && (isdigit (*(m_cur_tok + 1)) || *(m_cur_tok + 1) == '$')));
> +}
> +

GNU's convention is to wrap the whole multi-line expression
in parens so that emacs indents the || in the second line under
"(m_curr_tok".  We don't really need the parens in the first
line, so that gives us:

bool
number_or_range_parser::finished () const
{
  /* Parsing is finished when at end of string or null string, or we
     are not in a range and not in front of an integer, negative
     integer, convenience var or negative convenience var.  */
  return (m_cur_tok == NULL || *m_cur_tok == '\0'
	  || (!m_in_range
	      && !(isdigit (*m_cur_tok) || *m_cur_tok == '$')
	      && !(*m_cur_tok == '-'
		   && (isdigit (*(m_cur_tok + 1)) || *(m_cur_tok + 1) == '$')));
}


You could also use m_cur_tok[1] instead of "*(m_cur_tok + 1)" if you
like to make it a little bit shorter.


>  /* Accept a number and a string-form list of numbers such as is 
>     accepted by get_number_or_range.  Return TRUE if the number is
>     in the list.
> @@ -230,6 +257,9 @@ number_is_in_list (const char *list, int number)
>      return 1;
>  
>    number_or_range_parser parser (list);
> +
> +  if (parser.finished ())
> +    error (_("Args must be numbers or '$' variables."));

I think it's better to spell out "Arguments".

> +
> +int
> +parse_flags_qcs (const char *which_command, const char **str,
> +		 bool *quiet, bool *cont, bool *silent)
> +{
> +  const char *flags = "qcs";
> +  int res;

"res" is not used.

> +
> +  switch (parse_flags (str, flags))
> +    {
> +    case 0:
> +      return 0;
> +    case 1:
> +      *quiet = true;
> +      break;
> +    case 2:
> +      *cont = true;
> +      break;
> +    case 3:
> +      *silent = true;
> +      break;
> +    default:
> +      gdb_assert (0);

Use gdb_assert_not_reached.

> +    }
> +
> +  if (*cont && *silent)
> +    error (_("%s: -c and -s are mutually exclusive"), which_command);
> +
> +  return 1;
> +}

> @@ -173,4 +170,32 @@ check_for_argument (char **str, const char *arg, int arg_len)
>  			     arg, arg_len);
>  }
>  
> +/* A helper function that looks for a set of flags at the start of a
> +   string.  The possible flags are given as a null terminated string.
> +   A flag in STR must either be at the end of the string,
> +   or be followed by whitespace.
> +   Returns 0 if no valid flag is found at the start of STR.
> +   Otherwise updates *STR, and returns N (which is > 0),
> +   such that FLAGS [N - 1] is the valid found flag.  */
> +extern int parse_flags (const char **str, const char *flags);
> +
> +/* A helper function that uses parse_flags to handle the flags qcs :
> +     A flag -q sets *QUIET to true.
> +     A flag -c sets *CONT to true.
> +     A flag -s sets *SILENT to true.
> +
> +   The caller is responsible to initialize *QUIET, *CONT, *SILENT to
> +   false before the (first) call to parse_flags_qcs.
> +   parse_flags_qcs can then be called iteratively to search for more
> +   valid flags, as part of a 'main parsing loop' searching for -q/-c/-s
> +   flags together with other flags and options.
> +
> +   Returns 1 and updates *STR and one of *QUIET, *CONT, *SILENT
> +   if it finds a valid flag.
> +   Returns 0 if no valid flag is found at the beginning of STR.

This can be bool and true/false, right?

> +
> +   Throws an error if a flag is found such that both *CONT and *SILENT
> +   are true.  */
> +extern int parse_flags_qcs (const char *which_command, const char **str,
> +			    bool *quiet, bool *cont, bool *silent);

I suspect that using a single structure instead of multiple output bools,
like this:

struct qcs_flags
{
  bool quiet = false;
  bool cont = false;
  bool silent = false;
};

extern bool parse_flags_qcs (const char *which_command, const char **str,
			    qcs_flags *flags);

would make the code a little clearer.  Note that that way the "= false"
default initializers are part of the type, no need to do that explicitly
at the callers.

Thanks,
Pedro Alves

[RFA_v3 7/8] Modify gdb.threads/pthreads.exp to test FLAG qcs arguments for thread apply

[Philippe Waroquiers]

gdb/testsuite/ChangeLog
2018-06-24  Philippe Waroquiers  <philippe.waroquiers@skynet.be>

	* gdb.threads/pthreads.exp: Test qcs FLAG arguments.
---
 gdb/testsuite/gdb.threads/pthreads.exp | 65 ++++++++++++++++++++++++++
 1 file changed, 65 insertions(+)

diff --git a/gdb/testsuite/gdb.threads/pthreads.exp b/gdb/testsuite/gdb.threads/pthreads.exp
index 830432b833..ccd02046f8 100644
--- a/gdb/testsuite/gdb.threads/pthreads.exp
+++ b/gdb/testsuite/gdb.threads/pthreads.exp
@@ -267,6 +267,70 @@ proc check_backtraces {} {
     }
 }
 
+proc check_qcs {} {
+    set any "\[^\r\n\]*"
+    set ws "\[ \t\]\+"
+    set number "\[0-9]\+"
+
+    # check -c (continue) and -s (silently continue) flags
+    gdb_test "thread apply 2-3 p notfound" \
+	[multi_line \
+	     "" \
+	     "Thread 2 ${any}" \
+	     "No symbol \\\"notfound\\\" in current context." \
+	    ] \
+	"Run a failing command that aborts thread apply"
+
+    gdb_test "thread apply 2-3 -c p notfound" \
+	[multi_line \
+	     "" \
+	     "Thread 2 ${any}" \
+	     "No symbol \\\"notfound\\\" in current context." \
+	     "" \
+	     "Thread 3 ${any}" \
+	     "No symbol \\\"notfound\\\" in current context." \
+	    ] \
+	"Run a failing command, -c to continue"
+
+    foreach cmd {"thread apply all -s frame apply all -s p i" "tfaas p i" "taas faas p i"} {
+	gdb_test $cmd \
+	    [multi_line \
+		 "" \
+		 "Thread 3 ${any}" \
+		 "#${number}${ws}${any} in thread2 ${any}" \
+		 "\\\$\[0-9]+ = ${number}${any}" \
+		 "" \
+		 "Thread 2 ${any}" \
+		 "#${number}${ws}${any} in thread1 ${any}" \
+		 "\\\$\[0-9]+ = ${number}${any}" \
+		] \
+	    "Run a failing command except in one frame of thread 2,3, -s to silently continue"
+    }
+
+    # Quietness tests
+    gdb_test "thread apply all -s -q frame apply all -s p i" \
+	[multi_line \
+	     "#${number}${ws}${any} in thread2 ${any}" \
+	     "\\\$\[0-9]+ = ${number}${any}" \
+	     "#${number}${ws}${any} in thread1 ${any}" \
+	     "\\\$\[0-9]+ = ${number}${any}" \
+	    ] \
+	"Run a failing command except in one frame of thread 2,3, -s to silently continue.  Do not show thread information"
+
+    gdb_test "thread apply all -s -q frame apply all -s -q p i" \
+	[multi_line \
+	     "\\\$\[0-9]+ = ${number}${any}" \
+	     "\\\$\[0-9]+ = ${number}${any}" \
+	    ] \
+	"Run a failing command except in one frame of thread 2,3, -s to silently continue.  Do not show thread and frame info"
+
+    # check invalid flag combinations
+    gdb_test "thread apply all -c -s p 1" \
+	"thread apply all: -c and -s are mutually exclusive" \
+	"Check -c and -s cannot be used simultaneously"
+
+}
+
 if [runto_main] then {
     if [test_startup] then {
 	if [check_control_c] then {
@@ -274,5 +338,6 @@ if [runto_main] then {
 	    return
 	}
 	check_backtraces
+	check_qcs
     }
 }
-- 
2.17.1

[RFA_v3 2/8] Implement frame apply [all | COUNT | -COUNT | id ID... ] [FLAG]... COMMAND.

[Philippe Waroquiers]

Implement frame apply [all | COUNT | -COUNT | id ID... ] [FLAG]... COMMAND.
Also implement the command 'faas COMMAND', a shortcut for
'frame apply all -s COMMAND'.

The syntax of 'frame apply' to specify some innermost or outermost
frames is similar to 'backtrace' command, using COUNT or -COUNT.

To apply a COMMAND to a more specific set of frames, the following
new command and syntax can be used:
frame apply id ID... [FLAG]... COMMAND
where ID is one or more frame ids or range of frame ids.

The new command 'frame apply' allows to apply a COMMAND to a number of frames,
or to all frames, or to a set of frames.
The optional [FLAG]... arguments allow to control what output to produce
and how to handle errors raised when applying COMMAND to a frame.

Some example usages for this new command:
   frame apply all info frame
      Produce info frame for all frames.
   frame apply all p $sp
      For each frame, print the location, followed by the frame sp.
   frame apply all -q p $sp
      Same as before, but -q flag (q = quiet) indicates to only print
      the frames sp.
   frame apply all p some_local_var_somewhere
      Print some_local_var_somewhere in all frames. 'frame apply'
      will abort as soon as the print command fails.
   frame apply all -c p some_local_var_somewhere
      Same as before, but -c flag (c = continue) means to
      print the error and continue applying command in case the
      print command fails.
   frame apply all -s p some_local_var_somewhere
      Same as before, but -s flag (s = silent) means to
      be silent for frames where the print command fails.
      In other words, this allows to 'search' the frame in which
      some_local_var_somewhere can be printed.
   frame apply all -s -q p some_local_var_somewhere
      Same as before, but does not print the frame info in which
      the variable is found.
   frame apply id 2-4 5 8-10 -s p i = i + 1
      Increments i in the identified frames.

gdb/ChangeLog
2018-06-24  Philippe Waroquiers  <philippe.waroquiers@skynet.be>

	* stack.c: (trailing_outermost_frame): New function, mostly
	extracted from backtrace_command_1.
	(leading_innermost_frame): New function.
	(backtrace_command_1): Update to call trailing_outermost_frame.
	(frame_apply_command_count): New function.
	(frame_apply_id_command): New function.
	(frame_apply_all_command): New function.
	(frame_apply_command): New function.
	(faas_command): New function.
	(frame_cmd_list): New variable.
	(_initialize_stack): Update to setup the new commands 'frame apply'
	and 'faas'.
---
 gdb/stack.c | 310 ++++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 287 insertions(+), 23 deletions(-)

diff --git a/gdb/stack.c b/gdb/stack.c
index 9426d4f159..6420ed023a 100644
--- a/gdb/stack.c
+++ b/gdb/stack.c
@@ -1687,6 +1687,58 @@ info_frame_command (const char *addr_exp, int from_tty)
   }
 }
 
+/* Return the innermost frame at level LEVEL.  */
+
+static struct frame_info *
+leading_innermost_frame (int level)
+{
+  struct frame_info *leading;
+
+  leading = get_current_frame ();
+
+  gdb_assert (level >= 0);
+
+  while (leading != nullptr && level)
+    {
+      QUIT;
+      leading = get_prev_frame (leading);
+      level--;
+    }
+
+  return leading;
+}
+
+/* Return the starting frame needed to handle COUNT outermost frames.  */
+
+static struct frame_info *
+trailing_outermost_frame (int count)
+{
+  struct frame_info *current;
+  struct frame_info *trailing;
+
+  trailing = get_current_frame ();
+
+  gdb_assert (count > 0);
+
+  current = trailing;
+  while (current != nullptr && count--)
+    {
+      QUIT;
+      current = get_prev_frame (current);
+    }
+
+  /* Will stop when CURRENT reaches the top of the stack.
+     TRAILING will be COUNT below it.  */
+  while (current != nullptr)
+    {
+      QUIT;
+      trailing = get_prev_frame (trailing);
+      current = get_prev_frame (current);
+    }
+
+  return trailing;
+}
+
 /* Print briefly all stack frames or just the innermost COUNT_EXP
    frames.  */
 
@@ -1751,32 +1803,14 @@ backtrace_command_1 (const char *count_exp, frame_filter_flags flags,
 	 variable TRAILING to the frame from which we should start
 	 printing.  Second, it must set the variable count to the number
 	 of frames which we should print, or -1 if all of them.  */
-      trailing = get_current_frame ();
 
       if (count_exp != NULL && count < 0)
 	{
-	  struct frame_info *current;
-
-	  count = -count;
-
-	  current = trailing;
-	  while (current && count--)
-	    {
-	      QUIT;
-	      current = get_prev_frame (current);
-	    }
-
-	  /* Will stop when CURRENT reaches the top of the stack.
-	     TRAILING will be COUNT below it.  */
-	  while (current)
-	    {
-	      QUIT;
-	      trailing = get_prev_frame (trailing);
-	      current = get_prev_frame (current);
-	    }
-
+	  trailing = trailing_outermost_frame (-count);
 	  count = -1;
 	}
+      else
+	trailing = get_current_frame ();
 
       for (fi = trailing; fi && count--; fi = get_prev_frame (fi))
 	{
@@ -2494,9 +2528,200 @@ func_command (const char *arg, int from_tty)
     }
 }
 
+/* Apply a GDB command to all stack frames, or a set of identified frames,
+   or innermost COUNT frames.
+   With a negative COUNT, apply command on outermost -COUNT frames.
+
+   frame apply 3 info frame     Apply 'info frame' to frames 0, 1, 2
+   frame apply -3 info frame    Apply 'info frame' to outermost 3 frames.
+   frame apply all x/i $pc      Apply 'x/i $pc' cmd to all frames.
+   frame apply all -s p local_var_no_idea_in_which_frame
+                If a frame has a local variable called
+                local_var_no_idea_in_which_frame, print frame
+                and value of local_var_no_idea_in_which_frame.
+   frame apply all -s -q p local_var_no_idea_in_which_frame
+                Same as before, but only print the variable value.
+   frame apply id 2-5 0 4-7 -s p i = i + 1
+                Adds 1 to the variable i in the specified frames.
+                Note that i will be incremented twice in
+                frames 4 and 5.  */
+
+/* Apply a GDB command to COUNT stack frames, starting at TRAILING.
+   COUNT -1 means all frames starting at TRAILING.  WHICH_COMMAND is used
+   for error messages.  */
+
+static void
+frame_apply_command_count (const char *which_command,
+			   const char *cmd, int from_tty,
+			   struct frame_info *trailing, int count)
+{
+  bool quiet = false;
+  bool cont = false;
+  bool silent = false;
+  struct frame_info *fi;
+
+  while (cmd != NULL && parse_flags_qcs (which_command, &cmd,
+					 &quiet, &cont, &silent))
+    ;
+
+  if (cmd == NULL || *cmd == '\0')
+    error (_("Please specify a command to apply on the selected frames"));
+
+  /* The below will restore the current inferior/thread/frame.
+     Usually, only the frame is effectively to be restored.
+     But in case CMD switches of inferior/thread, better restore
+     these also.  */
+  scoped_restore_current_thread restore_thread;
+
+  for (fi = trailing; fi && count--; fi = get_prev_frame (fi))
+    {
+      QUIT;
+
+      select_frame (fi);
+      TRY
+	{
+	  std::string cmd_result;
+	  {
+	    /* In case CMD switches of inferior/thread/frame, the below
+	       restores the inferior/thread/frame.  FI can then be
+	       set to the selected frame.  */
+	    scoped_restore_current_thread restore_fi_current_frame;
+
+	    cmd_result = execute_command_to_string (cmd, from_tty);
+	  }
+	  fi = get_selected_frame (_("frame apply "
+				     "unable to get selected frame."));
+	  if (!silent || cmd_result.length () > 0)
+	    {
+	      if (!quiet)
+		print_stack_frame (fi, 1, LOCATION, 0);
+	      printf_filtered ("%s", cmd_result.c_str ());
+	    }
+	}
+      CATCH (ex, RETURN_MASK_ERROR)
+	{
+	  fi = get_selected_frame (_("frame apply "
+				     "unable to get selected frame."));
+	  if (!silent)
+	    {
+	      if (!quiet)
+		print_stack_frame (fi, 1, LOCATION, 0);
+	      if (cont)
+		printf_filtered ("%s\n", ex.message);
+	      else
+		throw_exception (ex);
+	    }
+	}
+      END_CATCH;
+    }
+}
+
+/* Implementation of the "frame apply id" command.  */
+
+static void
+frame_apply_id_command (const char *cmd, int from_tty)
+{
+  if (!target_has_stack)
+    error (_("No stack."));
+
+  bool id_found = false;
+  const char *ids = cmd;
+  number_or_range_parser idls (ids);
+
+  /* Skip the ID list to find the flags and command args.  */
+  while (!idls.finished ())
+    {
+      const int id_beg = idls.get_number ();
+
+      id_found = true;
+      if (idls.in_range ())
+	idls.skip_range ();
+    }
+
+  if (!id_found)
+    error (_("Missing or invalid ID... argument"));
+
+  cmd = idls.cur_tok ();
+
+  /* Redo the IDLS parsing, but applying COMMAND.  */
+  idls.init (ids);
+  while (!idls.finished ())
+    {
+      const int id_beg = idls.get_number ();
+      int n_frames;
+
+      if (idls.in_range ())
+	{
+	  n_frames = idls.end_value () - id_beg + 1;
+	  idls.skip_range ();
+	}
+      else
+	n_frames = 1;
+
+      frame_apply_command_count ("frame apply id", cmd, from_tty,
+				 leading_innermost_frame (id_beg), n_frames);
+    }
+}
+
+/* Implementation of the "frame apply all" command.  */
+
+static void
+frame_apply_all_command (const char *cmd, int from_tty)
+{
+  if (!target_has_stack)
+    error (_("No stack."));
+
+  frame_apply_command_count ("frame apply all", cmd, from_tty,
+			     get_current_frame (), INT_MAX);
+}
+
+/* Implementation of the "frame apply" command.  */
+
+static void
+frame_apply_command (const char* cmd, int from_tty)
+{
+  int count;
+  struct frame_info *trailing;
+
+  if (!target_has_stack)
+    error (_("No stack."));
+
+  if (cmd == NULL)
+    error (_("Missing COUNT argument."));
+  count = get_number_trailer (&cmd, 0);
+  if (count == 0)
+    error (_("Invalid COUNT argument."));
+
+  if (count < 0)
+    {
+      trailing = trailing_outermost_frame (-count);
+      count = -1;
+    }
+  else
+    trailing = get_current_frame ();
+
+  frame_apply_command_count ("frame apply", cmd, from_tty,
+			     trailing, count);
+}
+
+/* Implementation of the "faas" command.  */
+
+static void
+faas_command (const char *cmd, int from_tty)
+{
+  std::string expanded = std::string ("frame apply all -s ") + cmd;
+  execute_command (expanded.c_str (), from_tty);
+}
+
+
+/* Commands with a prefix of `frame'.  */
+struct cmd_list_element *frame_cmd_list = NULL;
+
 void
 _initialize_stack (void)
 {
+  static struct cmd_list_element *frame_apply_list = NULL;
+
   add_com ("return", class_stack, return_command, _("\
 Make selected stack frame return to its caller.\n\
 Control remains in the debugger, but when you continue\n\
@@ -2519,14 +2744,53 @@ An argument says how many frames down to go."));
 Same as the `down' command, but does not print anything.\n\
 This is useful in command scripts."));
 
-  add_com ("frame", class_stack, frame_command, _("\
+  add_prefix_cmd ("frame", class_stack, frame_command, _("\
 Select and print a stack frame.\nWith no argument, \
 print the selected stack frame.  (See also \"info frame\").\n\
 An argument specifies the frame to select.\n\
-It can be a stack frame number or the address of the frame."));
+It can be a stack frame number or the address of the frame."),
+		  &frame_cmd_list, "frame ", 1, &cmdlist);
 
   add_com_alias ("f", "frame", class_stack, 1);
 
+#define FRAME_APPLY_FLAGS_HELP "\
+Prints the frame location information followed by COMMAND output.\n\
+FLAG arguments are -q (quiet), -c (continue), -s (silent).\n\
+Flag -q disables printing the frame location information.\n\
+By default, if a COMMAND raises an error, frame apply is aborted.\n\
+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,
+		  _("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_list, "frame apply ", 1, &frame_cmd_list);
+
+  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_list);
+
+  add_cmd ("id", class_stack, frame_apply_id_command,
+	   _("\
+Apply a command to a list of frames.\n\
+\n\
+Usage: frame apply id ID... [FLAG]... COMMAND\n\
+ID is a space-separated list of IDs of frames to apply COMMAND on.\n"
+FRAME_APPLY_FLAGS_HELP),
+	   &frame_apply_list);
+
+  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'"));
+
   add_com_suppress_notification ("select-frame", class_stack, select_frame_command, _("\
 Select a stack frame without printing anything.\n\
 An argument specifies the frame to select.\n\
-- 
2.17.1

Re: [RFA_v3 2/8] Implement frame apply [all | COUNT | -COUNT | id ID... ] [FLAG]... COMMAND.

[Pedro Alves]

On 06/24/2018 07:37 PM, Philippe Waroquiers wrote:
> Implement frame apply [all | COUNT | -COUNT | id ID... ] [FLAG]... COMMAND.
> Also implement the command 'faas COMMAND', a shortcut for
> 'frame apply all -s COMMAND'.
> 
> The syntax of 'frame apply' to specify some innermost or outermost
> frames is similar to 'backtrace' command, using COUNT or -COUNT.
> 
> To apply a COMMAND to a more specific set of frames, the following
> new command and syntax can be used:
> frame apply id ID... [FLAG]... COMMAND
> where ID is one or more frame ids or range of frame ids.

Thanks much for doing this.  Other than the "id" / "level"
thing, this looks mostly OK to me.  One minor detail below.

> +
> +/* Implementation of the "frame apply id" command.  */
> +
> +static void
> +frame_apply_id_command (const char *cmd, int from_tty)
> +{
> +  if (!target_has_stack)
> +    error (_("No stack."));
> +
> +  bool id_found = false;
> +  const char *ids = cmd;
> +  number_or_range_parser idls (ids);
> +
> +  /* Skip the ID list to find the flags and command args.  */
> +  while (!idls.finished ())
> +    {
> +      const int id_beg = idls.get_number ();
> +
> +      id_found = true;
> +      if (idls.in_range ())
> +	idls.skip_range ();
> +    }
> +
> +  if (!id_found)
> +    error (_("Missing or invalid ID... argument"));
> +
> +  cmd = idls.cur_tok ();
> +
> +  /* Redo the IDLS parsing, but applying COMMAND.  */
> +  idls.init (ids);
> +  while (!idls.finished ())
> +    {
> +      const int id_beg = idls.get_number ();
> +      int n_frames;
> +
> +      if (idls.in_range ())
> +	{
> +	  n_frames = idls.end_value () - id_beg + 1;
> +	  idls.skip_range ();
> +	}
> +      else
> +	n_frames = 1;
> +
> +      frame_apply_command_count ("frame apply id", cmd, from_tty,
> +				 leading_innermost_frame (id_beg), n_frames);

I noticed that as is, frame_apply_command_count is going to parse CMD
for parse_flags_qcs for every iteration.  I wonder whether it wouldn't
be clearer to split frame_apply_command_count after that CMD parsing
to a separate function that is called by both frame_apply_id_command
and frame_apply_command_count.

Thanks,
Pedro Alves

Re: [RFA_v3 2/8] Implement frame apply [all | COUNT | -COUNT | id ID... ] [FLAG]... COMMAND.

[Philippe Waroquiers]

On Mon, 2018-07-09 at 20:15 +0100, Pedro Alves wrote:
> I noticed that as is, frame_apply_command_count is going to parse CMD
> for parse_flags_qcs for every iteration.  I wonder whether it wouldn't
> be clearer to split frame_apply_command_count after that CMD parsing
> to a separate function that is called by both frame_apply_id_command
> and frame_apply_command_count.
Thanks for the comments.
I have just posted RFA_v4, which I think handles all the comments
as suggested, except the above :
currently, all 3 'frame apply' commands are directly
calling frame_apply_command_count after having established the
starting frame and count.
I think this is preferable, as we have the flag parsing and
error checking about missing cmd at only one place.
The only consequence I see is that flags will be parsed for each
range of frames, but that will for sure not be performance critical
(we parse already twice the LEVEL arg).

So, I have added a sentence in the frame_apply_command_count 
function to describe that CMD optionally starts with qcs flags.

Thanks

Philippe

Re: [RFA_v3 0/8] Implement 'frame apply COMMAND', enhance 'thread apply COMMAND'

[Andrew Burgess]

* Philippe Waroquiers <philippe.waroquiers@skynet.be> [2018-06-24 20:37:00 +0200]:

> This is the fourth iteration of the patch series that:
>  * implements a new command
>      'frame apply [all | COUNT | -COUNT | id ID...] [FLAG]... COMMAND'.

I still have this patch pending (which I need to ping) but I still
hope to get it merged:

  https://sourceware.org/ml/gdb-patches/2018-06/msg00170.html

This patch makes it much clearer (I think) that there are multiple was
to IDentify a frame, level, address, function-name.

I wonder if using the keyword 'id' might be confusing were my patch to
end up being merged?  Would a consistent 'level' be better?

Interested to hear your thoughts...

Thanks,
Andrew





>  * enhance 'thread apply COMMAND' by adding FLAG arguments.
>  * adds some shortcuts commands.
>  * documents the above in gdb.texinfo and NEWS.
>  * adds a unit test for cli-utils.c.
>  * adds test for 'frame apply'.
>  * modify gdb.threads/pthreads.exp to test 'thread apply' FLAG arguments.
> 
> The fourth version is RFA v3. It handles the comments given by Pedro.
> The main changes compared to RFA v2 are:
>   * The -q (quiet) flag replaces the verbosity/-v/-q concept.
>   * Addition of 'frame apply id ID... [FLAG]... COMMAND' to allow
>     applying a command on a selected list of frames.
>   * [FLAG]... arguments are now parsed iteratively.
>   * Documentation and on-line help updated accordingly.
>   * tests updated accordingly.
> 
> The third version was RFA v2, handling the comments of Pedro about giving
> the flags individually.
> The changes compared to RFA v1 is:
>   * The flags must be given individually, such as -v -v -c.
>   * The documentation and on-line help was updated accordingly.
>   * ChangeLog information is distributed in each commit log message.
> 
> The second version was RFA v1, changes compared to RFC are:
>   * Replied to all comments of Eli and Simon.
>   * Tests and ChangeLog entries added.
>   * Comments received from Pedro
> 
> The first version was an RFC
>   * Code and doc was complete, but was lacking ChangeLog and tests.
>   * Comments received from Eli and Simon.
> 
> 
> 
> 
> 

Re: [RFA_v3 0/8] Implement 'frame apply COMMAND', enhance 'thread apply COMMAND'

[Philippe Waroquiers]

On Fri, 2018-06-29 at 13:21 +0100, Andrew Burgess wrote:
> This patch makes it much clearer (I think) that there are multiple was
> to IDentify a frame, level, address, function-name.
> 
> I wonder if using the keyword 'id' might be confusing were my patch to
> end up being merged?  Would a consistent 'level' be better?
> 
> Interested to hear your thoughts...

Counting in gdb.texinfo, I see 5 references to regexp 'frame.id'
(but I am not at all sure that these are referencing the #integer
that backtrace shows).

I see one reference to 'frame.level'.

I see 8 references to 'frame.number', and some of these
references are explicitly explaining that backtrace shows
a frame number.

In the code, I see a bunch of occurrences of 'frame.level'
and much more of 'frame.id', but as far as I can see, frame id
is rather the internal 'struct frame_id' concept.
frame.number seems not used too much in the code, except
as traceframe_number

So, for me, fine to replace
   Usage: frame apply id ID... [FLAG]... COMMAND
   ID is a space-separated list of IDs of frames to apply COMMAND on.
by
   Usage: frame apply level LEVEL... [FLAG]... COMMAND
   LEVEL is a space-separated list of frames identified by their level to apply COMMAND on.

But if we want to keep the current use of frame number, it should rather be 
   Usage: frame apply number NUMBER... [FLAG]... COMMAND
   NUMBER is a space-separated list of frames identified by their number to apply COMMAND on.

Whatever we take, we have to ensure that gdb.texinfo systematically use 
it to identify a frame as shown by backtrace (#integer).


All the above change is quite mechanical, but that is better done once feedback
is given e.g. by Pedro over this patch RFA_v3 and about your patch.

And then, we need to see in which order which patch is applied.
(I slightly prefer my patch first, but I guess you slightly prefer
your patch first :).

So let e.g. Pedro decide about which one is ready to go in first.

In summary: review needed, and then a decision about the above and
   about which patch goes first ...

Thanks

Philippe

Re: [RFA_v3 0/8] Implement 'frame apply COMMAND', enhance 'thread apply COMMAND'

[Philippe Waroquiers]

On Fri, 2018-06-29 at 22:15 +0200, Philippe Waroquiers wrote:
> Whatever we take, we have to ensure that gdb.texinfo systematically use 
> it to identify a frame as shown by backtrace (#integer).
Note that in the online help, we have already some usages of level
speaking about frames.

So, all that taken into account, my preference would be to remove
the frame number wording,
and use frame level everywhere in the doc and online help : 
LEVEL seems more clear/intuitive than NUMBER or ID

But whatever choice is ok for me.

Philippe

Re: [RFA_v3 0/8] Implement 'frame apply COMMAND', enhance 'thread apply COMMAND'

[Pedro Alves]

On 06/29/2018 09:38 PM, Philippe Waroquiers wrote:
> On Fri, 2018-06-29 at 22:15 +0200, Philippe Waroquiers wrote:
>> Whatever we take, we have to ensure that gdb.texinfo systematically use 
>> it to identify a frame as shown by backtrace (#integer).
> Note that in the online help, we have already some usages of level
> speaking about frames.
> 
> So, all that taken into account, my preference would be to remove
> the frame number wording,
> and use frame level everywhere in the doc and online help : 
> LEVEL seems more clear/intuitive than NUMBER or ID
> 
> But whatever choice is ok for me.

Ahah, I did warn about having to decide this at
<https://sourceware.org/ml/gdb-patches/2018-06/msg00410.html>.

Thanks Andrew for bringing this up.

I agree, "level" seems like the best option to me.  I should have
suggested that one more strongly instead of "id" when I wrote
the above, but I was distracted thinking that we'd end up
with "count" instead.  :-)

Thanks,
Pedro Alves