[RFA_v2,2/8] Implement frame apply [all | COUNT | -COUNT] [FLAGS...] COMMAND.

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

Note: the syntax of 'frame apply' to specify some innermost or outermost
frames is similar to 'backtrace' command.
An alternative could be to make 'frame apply' similar to
'thread apply'. This was not chosen as:
  * the typical use cases for frame apply are all/some innermost/some outermost
    frames.
  * a range based syntax would have obliged the user to use absolute numbers
    to specify the outermost N frames, which is cumbersone.
    Or an syntax different from the thread range would have been needed
    (e.g. -0-4 to specify the 5 outermost frames).
So, making frame apply similar to backtrace is found better.

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

Some examples 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 -qq p $sp
      Same as before, but -qq flags (q = quiet) indicate to only print
      the frames sp.
   frame apply all -vv p $sp
      Same as before, but -vv flags (v = verbose) indicate to print
      location and source line for each frame.
   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.

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

	* stack.c: (trailing_outermost_frame): New function, mostly
	extracted from backtrace_command_1.
	(backtrace_command_1): Update to call trailing_outermost_frame.
	(frame_apply_command_count): 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 | 246 ++++++++++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 223 insertions(+), 23 deletions(-)
  

On 06/05/2018 09:48 PM, Philippe Waroquiers wrote:
> Implement frame apply [all | COUNT | -COUNT] [FLAGS...] COMMAND.

coreutils puts the "..." after the [], and uses singular.
E.g., ls --help says "Usage: ls [OPTION]... [FILE]...".

I still think it's better to say OPTION, because future
options may not be flags, they may take arguments.  So:

  Implement frame apply [all | COUNT | -COUNT] [OPTION]... COMMAND.

Same comments apply to the "thread apply" patch too, of course.

> Also implement the command 'faas COMMAND', a shortcut for
> 'frame apply all -s COMMAND'.
> 
> Note: the syntax of 'frame apply' to specify some innermost or outermost
> frames is similar to 'backtrace' command.
> An alternative could be to make 'frame apply' similar to
> 'thread apply'. This was not chosen as:
>   * the typical use cases for frame apply are all/some innermost/some outermost
>     frames.
>   * a range based syntax would have obliged the user to use absolute numbers
>     to specify the outermost N frames, which is cumbersone.
>     Or an syntax different from the thread range would have been needed
>     (e.g. -0-4 to specify the 5 outermost frames).
> So, making frame apply similar to backtrace is found better.

Hmm, I played with this a bit now, AFAICT, this syntax forces the applicable
range to start at either the innermost or outermost?  How does one e.g., apply
the command to frames 10 to 20 (because e.g., they are the frames that
are running some library code), when there exist frames 0-50?  

> 
> Th new command 'frame apply' allows to apply a COMMAND to a number of frames,
> or to all frames.

This stands out as a limitation to me.

"Th" -> "The"

> The optional FLAGS... arguments allow to control what output to produce
> and how to handle errors raised when applying COMMAND to a frame.
> 
> Some examples 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 -qq p $sp
>       Same as before, but -qq flags (q = quiet) indicate to only print
>       the frames sp.
>    frame apply all -vv p $sp

This needs updating for non-combining options.

>       Same as before, but -vv flags (v = verbose) indicate to print
>       location and source line for each frame.
>    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.

I played with this a bit, and I have to admit that I found the
-v / -q combinations, and relativeness of the "quietness/verbosity"
level, a bit unintuitive and confusing.

E.g.,:

 (top-gdb) frame apply 2 echo
 #0  0x00007ffff54e0c6b in __GI___poll (fds=0x1a343d0, nfds=4, timeout=-1) at ../sysdeps/unix/sysv/linux/poll.c:29
 #1  0x00000000007083e4 in gdb_wait_for_event (block=1) at src/gdb/event-loop.c:771

 (top-gdb) frame apply 2 -q echo
 0x00007ffff54e0c6b      29        return SYSCALL_CANCEL (poll, fds, nfds, timeout);
 0x00000000007083e4      771           num_found = poll (gdb_notifier.poll_fds,
 (top-gdb) 

Here, "-q" didn't make the output really be quieter, only different.

Consider also if we add a knob to tweak the default verbosity.
Like "set frame-apply-default-format N" or whatever.
With that, "frame apply -q" does a different thing depending on
the value of the default setting.

So I'm wondering whether "frame apply -quiet/-source/-location/-etc."
options wouldn't be better.  

Or "frame apply -format [0-4]" and/or 
"frame apply -format quiet/source/location/location".

Maybe alternatively consider ditching the format ideas, and have
users combine "frame apply -q" with "list" if they want to see
sources.  OK, I expect resistance to that idea.  :-)

> 
> gdb/ChangeLog
> 2018-06-04  Philippe Waroquiers  <philippe.waroquiers@skynet.be>
> 
> 	* stack.c: (trailing_outermost_frame): New function, mostly
> 	extracted from backtrace_command_1.
> 	(backtrace_command_1): Update to call trailing_outermost_frame.
> 	(frame_apply_command_count): 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 | 246 ++++++++++++++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 223 insertions(+), 23 deletions(-)
> 
> diff --git a/gdb/stack.c b/gdb/stack.c
> index 97ebc8bc23..090483cd12 100644
> --- a/gdb/stack.c
> +++ b/gdb/stack.c
> @@ -1687,6 +1687,38 @@ info_frame_command (const char *addr_exp, int from_tty)
>    }
>  }
>  
> +/* trailing_outermost_frame returns the starting frame

Don't repeat function names:

 s/trailing_outermost_frame returns/Return/

> +   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 +1783,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 +2508,160 @@ func_command (const char *arg, int from_tty)
>      }
>  }
>  
> +/* Apply a GDB command to a all stack 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 -sqq p local_var_no_idea_in_which_frame
> +                Same as before, but only print the variable value.  */

Needs updating for non-combined options.

> +
> +/* 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* " => "const char *"

> +			   const char *cmd, int from_tty,
> +			   struct frame_info *trailing, int count)
> +{
> +  int print_what_v = 2; /* Corresponding to LOCATION.  */
> +  enum print_what print_what[5] =

No need for this explicit "5", I think.

> +    {
> +      LOC_AND_ADDRESS, /* Should never be used, this is verbosity 0.  */
> +      SRC_LINE,
> +      LOCATION,
> +      LOC_AND_ADDRESS,
> +      SRC_AND_LOC
> +    };
> +  bool cont;
> +  bool silent;
> +  struct frame_info *fi;
> +
> +  if (cmd != NULL)
> +    check_for_flags_vqcs (which_command, &cmd,
> +			  &print_what_v, 4,
> +			  &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))
> +    {
> +      struct frame_id frame_id = get_frame_id (fi);
> +
> +      QUIT;
> +
> +      select_frame (fi);
> +      TRY
> +	{
> +	  std::string cmd_result;
> +	  {
> +	    /* In case CMD switches of inferior/thread/frame, the below
> +	       restores the inferior/thread.  We can then re-initialise
> +	       FI based on FRAME_ID.  */
> +	    scoped_restore_current_thread restore_fi_current_frame;
> +
> +	    cmd_result = execute_command_to_string (cmd, from_tty);
> +	  }
> +	  fi = frame_find_by_id (frame_id);
> +	  if (fi == NULL)
> +	    {
> +	      warning (_("Unable to restore previously selected frame."));

scoped_restore_current_thread also restores the frame, and, also
warns.

> +	      break;

This "break" isn't really breaking out of the for loop, because
of the way TRY is implemented.

> +	    }
> +	  if (!silent || cmd_result.length () > 0)
> +	    {
> +	      if (print_what_v > 0)
> +		print_stack_frame (fi, 1, print_what[print_what_v], 0);
> +	      printf_filtered ("%s", cmd_result.c_str ());
> +	    }
> +	}
> +      CATCH (ex, RETURN_MASK_ERROR)
> +	{
> +	  fi = frame_find_by_id (frame_id);
> +	  if (fi == NULL)
> +	    {
> +	      warning (_("Unable to restore previously selected frame."));
> +	      break;
> +	    }

Ditto on the warning.

> +	  if (!silent)
> +	    {
> +	      if (print_what_v > 0)
> +		print_stack_frame (fi, 1, print_what [print_what_v], 0);
> +	      if (cont)
> +		printf_filtered ("%s\n", ex.message);
> +	      else
> +		throw_exception (ex);
> +	    }
> +	}
> +      END_CATCH;
> +    }
> +}
> +

Missing intro comment:

/* 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."));
> +
> +  count = get_number_trailer (&cmd, 0);

You can declare and initialize at the same time:

   int count = get_number_trailer (&cmd, 0);

> +
> +  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 ") + std::string (cmd);

 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 +2684,49 @@ 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 "\
> +FLAGS are -v (increase verbosity), -q (decrease verbosity)\n\
> +  -c (continue), -s (silent).\n\
> +Verbosity (default 2) controls what to print for a frame:\n\
> +  0 : no frame info is printed\n\
> +  1 : source line\n\
> +  2 : location\n\
> +  3 : location and address\n\
> +  4 : source line and location\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 [FLAGS...] 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 [FLAGS...] COMMAND\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\
> 
Thanks,
Pedro Alves
  

On Wed, 2018-06-13 at 20:52 +0100, Pedro Alves wrote:
>  Implement frame apply [all | COUNT | -COUNT] [OPTION]... COMMAND.
> 
> Same comments apply to the "thread apply" patch too, of course.
> 
> > Also implement the command 'faas COMMAND', a shortcut for
> > 'frame apply all -s COMMAND'.
> > 
> > Note: the syntax of 'frame apply' to specify some innermost or outermost
> > frames is similar to 'backtrace' command.
> > An alternative could be to make 'frame apply' similar to
> > 'thread apply'. This was not chosen as:
> >   * the typical use cases for frame apply are all/some innermost/some outermost
> >     frames.
> >   * a range based syntax would have obliged the user to use absolute numbers
> >     to specify the outermost N frames, which is cumbersone.
> >     Or an syntax different from the thread range would have been needed
> >     (e.g. -0-4 to specify the 5 outermost frames).
> > So, making frame apply similar to backtrace is found better.
> 
> Hmm, I played with this a bit now, AFAICT, this syntax forces the applicable
> range to start at either the innermost or outermost?  How does one e.g., apply
> the command to frames 10 to 20 (because e.g., they are the frames that
> are running some library code), when there exist frames 0-50?  
> 
> > 
> > Th new command 'frame apply' allows to apply a COMMAND to a number of frames,
> > or to all frames.
> 
> This stands out as a limitation to me.
Yes, it allows to (only) do what backtrace allows, and so, similarly
to backtrace, does not allow to 'cherry pick' frames (or range of frames).

I initially tried to find a reasonable syntax to allow a range based
selection of frames, but could not obtain anything intuitive to specify
the outermost N frames.

Assuming no better syntax is found, maybe we need to explain better
in the user manual that 'frame apply' selection of frame is similar/compatible
with backtrace, maybe something like:
    'frame apply' uses the same convention as 'backtrace' to specify the
    frames to apply COMMAND on : N innermost frames, or N outermost frames,
    or all frames.
    Note that 'thread apply' uses  a list of thread IDs or thread IDs range.

I understand that the differences is not ideal, but IMO, there
are inherent differences between thread and frames, making it difficult
to use a common syntax for 'frame lists' and 'thread lists'.
E.g. there is no concept of innermost/outermost thread list, while
I think we need a syntax for this concept in 'frame apply'.

So, in summary, would be fine for me to change 'frame apply'
for a better syntax, but which better syntax allows to specify
as intuitively as backtrace N outermost frames ?

And if we have another syntax than backtrace, then it should be
similar to thread apply THREADID ranges, otherwise that would be very
confusing.
=> I do not see a syntax consistent with thread apply ID ranges
or individual IDs, that would allow to specify N innermost or
outermost frames.

> 
> I played with this a bit, and I have to admit that I found the
> -v / -q combinations, and relativeness of the "quietness/verbosity"
> level, a bit unintuitive and confusing.
> 
> E.g.,:
> 
>  (top-gdb) frame apply 2 echo
>  #0  0x00007ffff54e0c6b in __GI___poll (fds=0x1a343d0, nfds=4, timeout=-1) at ../sysdeps/unix/sysv/linux/poll.c:29
>  #1  0x00000000007083e4 in gdb_wait_for_event (block=1) at src/gdb/event-loop.c:771
> 
>  (top-gdb) frame apply 2 -q echo
>  0x00007ffff54e0c6b      29        return SYSCALL_CANCEL (poll, fds, nfds, timeout);
>  0x00000000007083e4      771           num_found = poll (gdb_notifier.poll_fds,
>  (top-gdb) 
> 
> Here, "-q" didn't make the output really be quieter, only different.
Yes, the verbosity just controls the choice of the value of
frame.h enum print_what:
 enum print_what
  { 
    /* Print only the source line, like in stepi.  */
    SRC_LINE = -1, 
    /* Print only the location, i.e. level, address (sometimes)
       function, args, file, line, line num.  */
    LOCATION,
    /* Print both of the above.  */
    SRC_AND_LOC, 
    /* Print location only, but always include the address.  */
    LOC_AND_ADDRESS 
  };

and that is not really a 'monotonically increasing function'.



> 
> Consider also if we add a knob to tweak the default verbosity.
> Like "set frame-apply-default-format N" or whatever.
> With that, "frame apply -q" does a different thing depending on
> the value of the default setting.
> 
> So I'm wondering whether "frame apply -quiet/-source/-location/-etc."
> options wouldn't be better.  
> 
> Or "frame apply -format [0-4]" and/or 
> "frame apply -format quiet/source/location/location".
> 
> Maybe alternatively consider ditching the format ideas, and have
> users combine "frame apply -q" with "list" if they want to see
> sources.  OK, I expect resistance to that idea.  :-)

Not too much resistance on my side to drop the format idea :).

On a second thought, this verbosity looks all too 'over-engineered' :
  thread apply verbosity currently only allows to print nothing
    or a fixed set of information per thread
  backtrace prints a fixed information per frame
  frame apply allows to choose whatever in enum print_what with
    non intuitive verbosity control.

=> I suggest to then just have the -q option : by default,
frame information is printed like backtrace, and if -q is given,
no information is printed.
And same for 'thread apply' : by default, it prints a fixed
set of info. And -q means do not print it.
(we for sure need at least the -q to allow only the thread/frame info
to be
printed for successful COMMAND).

(again, in the future, we can if we want add other options
to control what to print, if we really need this flexibility).

So, what do you think about the (simpler) idea of just having
  -q for both thread apply and frame apply ?

Philippe
  

On 06/15/2018 12:01 AM, Philippe Waroquiers wrote:
> On Wed, 2018-06-13 at 20:52 +0100, Pedro Alves wrote:
>>  Implement frame apply [all | COUNT | -COUNT] [OPTION]... COMMAND.
>>
>> Same comments apply to the "thread apply" patch too, of course.
>>
>>> Also implement the command 'faas COMMAND', a shortcut for
>>> 'frame apply all -s COMMAND'.
>>>
>>> Note: the syntax of 'frame apply' to specify some innermost or outermost
>>> frames is similar to 'backtrace' command.
>>> An alternative could be to make 'frame apply' similar to
>>> 'thread apply'. This was not chosen as:
>>>   * the typical use cases for frame apply are all/some innermost/some outermost
>>>     frames.
>>>   * a range based syntax would have obliged the user to use absolute numbers
>>>     to specify the outermost N frames, which is cumbersone.
>>>     Or an syntax different from the thread range would have been needed
>>>     (e.g. -0-4 to specify the 5 outermost frames).
>>> So, making frame apply similar to backtrace is found better.
>>
>> Hmm, I played with this a bit now, AFAICT, this syntax forces the applicable
>> range to start at either the innermost or outermost?  How does one e.g., apply
>> the command to frames 10 to 20 (because e.g., they are the frames that
>> are running some library code), when there exist frames 0-50?  
>>
>>>
>>> Th new command 'frame apply' allows to apply a COMMAND to a number of frames,
>>> or to all frames.
>>
>> This stands out as a limitation to me.
> Yes, it allows to (only) do what backtrace allows, and so, similarly
> to backtrace, does not allow to 'cherry pick' frames (or range of frames).
> 
> I initially tried to find a reasonable syntax to allow a range based
> selection of frames, but could not obtain anything intuitive to specify
> the outermost N frames.
> 
> Assuming no better syntax is found, maybe we need to explain better
> in the user manual that 'frame apply' selection of frame is similar/compatible
> with backtrace, maybe something like:
>     'frame apply' uses the same convention as 'backtrace' to specify the
>     frames to apply COMMAND on : N innermost frames, or N outermost frames,
>     or all frames.
>     Note that 'thread apply' uses  a list of thread IDs or thread IDs range.
> 
> I understand that the differences is not ideal, but IMO, there
> are inherent differences between thread and frames, making it difficult
> to use a common syntax for 'frame lists' and 'thread lists'.
> E.g. there is no concept of innermost/outermost thread list, while
> I think we need a syntax for this concept in 'frame apply'.
> 
> So, in summary, would be fine for me to change 'frame apply'
> for a better syntax, but which better syntax allows to specify
> as intuitively as backtrace N outermost frames ?
> 
> And if we have another syntax than backtrace, then it should be
> similar to thread apply THREADID ranges, otherwise that would be very
> confusing.
> => I do not see a syntax consistent with thread apply ID ranges
> or individual IDs, that would allow to specify N innermost or
> outermost frames.

Hmm, maybe the problem is with the trying to fit two concepts
into the same syntax.

"backtrace" suggests in its name a trace of calls going
back.  When you think of filtering the output, you're usually
interested in some number of innermost frames leading up to the
current frame, so an optional count argument there seems kind
of natural.

Note also that you can do "bt" without a COUNT argument,
again given that order is inherent to the command.

But you can't do that with "frame apply", like:
  (gdb) frame apply COMMAND
at least not in your implementation.

(Funnily you get no error if you try it.)

For "frame apply", a count doesn't seem to fit as
naturally to me.  But that might be because I was
drawing the parallel to "thread apply".  I do see
the argument for modeling on "backtrace".

Maybe we could have this (1):

 frame apply all COMMAND
 frame apply 1 3 5 7-9 100 COMMAND
 frame apply count [COUNT | -COUNT] COMMAND

Or if we go with number == count by default, this (2):

 frame apply all COMMAND
 frame apply [COUNT | -COUNT] COMMAND
 frame apply id 1 3 5 7-9 100 COMMAND

I think I'd mildly prefer option (1) above.  It'd avoid
having to decide whether to spell it "id" or "level"
too.  :-)  But both ways are fine with me.

Note, instead of "count", we could have for example:

 frame apply inner N COMMAND
 frame apply outer N COMMAND

dunno.

I could also see adding different kinds of filters
in the future, like, e.g., applying a command to
frames running function FUNCTION:

 frame apply function FUNCTION COMMAND

Or apply a command to frames running on a library or
executable FILE:

 frame apply objfile FILE COMMAND

Though it could also make sense to be able to combine
the filters, like:

 frame apply all -function foo -objfile FOO -whatnot BAR COMMAND

which might be a little to far into over-engineeringland.  Not sure.
Maybe not.

> 
>>
>> I played with this a bit, and I have to admit that I found the
>> -v / -q combinations, and relativeness of the "quietness/verbosity"
>> level, a bit unintuitive and confusing.
>>
>> E.g.,:
>>
>>  (top-gdb) frame apply 2 echo
>>  #0  0x00007ffff54e0c6b in __GI___poll (fds=0x1a343d0, nfds=4, timeout=-1) at ../sysdeps/unix/sysv/linux/poll.c:29
>>  #1  0x00000000007083e4 in gdb_wait_for_event (block=1) at src/gdb/event-loop.c:771
>>
>>  (top-gdb) frame apply 2 -q echo
>>  0x00007ffff54e0c6b      29        return SYSCALL_CANCEL (poll, fds, nfds, timeout);
>>  0x00000000007083e4      771           num_found = poll (gdb_notifier.poll_fds,
>>  (top-gdb) 
>>
>> Here, "-q" didn't make the output really be quieter, only different.
> Yes, the verbosity just controls the choice of the value of
> frame.h enum print_what:
>  enum print_what
>   { 
>     /* Print only the source line, like in stepi.  */
>     SRC_LINE = -1, 
>     /* Print only the location, i.e. level, address (sometimes)
>        function, args, file, line, line num.  */
>     LOCATION,
>     /* Print both of the above.  */
>     SRC_AND_LOC, 
>     /* Print location only, but always include the address.  */
>     LOC_AND_ADDRESS 
>   };
> 
> and that is not really a 'monotonically increasing function'.

Yeah.


>> Consider also if we add a knob to tweak the default verbosity.
>> Like "set frame-apply-default-format N" or whatever.
>> With that, "frame apply -q" does a different thing depending on
>> the value of the default setting.
>>
>> So I'm wondering whether "frame apply -quiet/-source/-location/-etc."
>> options wouldn't be better.  
>>
>> Or "frame apply -format [0-4]" and/or 
>> "frame apply -format quiet/source/location/location".
>>
>> Maybe alternatively consider ditching the format ideas, and have
>> users combine "frame apply -q" with "list" if they want to see
>> sources.  OK, I expect resistance to that idea.  :-)
> 
> Not too much resistance on my side to drop the format idea :).

Eh. :-)

> 
> On a second thought, this verbosity looks all too 'over-engineered' :
>   thread apply verbosity currently only allows to print nothing
>     or a fixed set of information per thread
>   backtrace prints a fixed information per frame
>   frame apply allows to choose whatever in enum print_what with
>     non intuitive verbosity control.
> 
> => I suggest to then just have the -q option : by default,
> frame information is printed like backtrace, and if -q is given,
> no information is printed.
> And same for 'thread apply' : by default, it prints a fixed
> set of info. And -q means do not print it.
> (we for sure need at least the -q to allow only the thread/frame info
> to be
> printed for successful COMMAND).
> 
> (again, in the future, we can if we want add other options
> to control what to print, if we really need this flexibility).
> 
> So, what do you think about the (simpler) idea of just having
>   -q for both thread apply and frame apply ?

I like it.  :-)  As you say, we can always think more about
this later and add ability to fine tune in the future.

Thanks,
Pedro Alves