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

[Pedro Alves <palves@redhat.com>]

Hi Philippe,

I really wish I had time to play a bit more with the series
(I really like the idea of "frame apply") and do a more in-depth
review today, but I probably won't, so here are some quick comments.

On 05/21/2018 12:06 PM, Philippe Waroquiers wrote:
> Implement 'frame apply COMMAND', enhance 'thread apply COMMAND'
> 
> Compared to RFC, this handles all comments received from Eli and Simon,
> and completes the changes so that it is (should be) ready for RFA.
> 
> This patch series :
>  * implements a new command
>      'frame apply [all | COUNT | -COUNT] [-FLAGS...] COMMAND'.
>  * enhance 'thread apply COMMAND' by adding a -FLAGS argument
>  * 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' -FLAGS argument

I'm not sure the idea of using "-" for flags is a good one,
because that conflicts with GDB's usual use of "-" for long
options, which can be abbreviated, and cannot be combined.

For example, "watch -location", "watch -l".

A while ago I was playing with adding a generic framework for
command options, which also handled TAB completion
automatically, and I was thinking about how gdb doesn't use "--"
for long options unlike getopt, and how single-"-" for long options
prevent combining options with a single "-", like you can
with "ls --all --size" -> "ls -as".  Then I realized something that I
had haven't seen written down, but I thought made some sense.  That
is, that we do have at least one command that allows combining short
options, "x/FMT", and it just uses "/" instead of "-"  I.e., we
could make that the way to handle short vs long options throughout.
I.e., comparing gdb's options to getopt-like options yields this:

        | getopt | gdb |
  long  | --     | -   |
  short | -      | /   |

So I'm wondering about using / for these new flags too.

Like, long form "-verbose -continue", short form "/vc".

Or you could sidestep the issue by ditching support for
combining flags, i.e., require "-v -v -c" instead of "-vvc".

> 
> Th new command 'frame apply' allows to apply a COMMAND to a number of frames,
> or to all frames.
> The optional -FLAGS... argument allows 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.
> 
> 'thread apply' command has been enhanced to also accepts a -FLAGS...
> argument.
> 
> Some examples usages 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 :
>    faas  : shortcut for 'frame apply all -s'
>    taas  : shortcut for 'thread apply all -s'
>    tfaas : shortcut for 'thread apply all -s frame apply all -s"

I'm not particularly sold on adding aliases, since you can
abbreviate and tab-complete.  Users are used to "t a a",
for example, so I think "f a a" will come naturally, and
users can add aliases themselves with the "alias" command.
But that may be because I haven't played with the patches much yet.

> 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-05-21  Philippe Waroquiers  <philippe.waroquiers@skynet.be>

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

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

Is the idea that the patches should be merged as a single bigger
patch?  Otherwise, the ChangeLogs should be split into the
individual patches.

Thanks,
Pedro Alves