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

[Philippe Waroquiers]

On Fri, 2018-06-01 at 15:32 +0100, Pedro Alves wrote:
> 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.
Hello Pedro,
Thanks for these comments.
Note that this work is a re-implementation of an earlier prototype
(the 'block of commands' prototype) that a.o. added some flags and
an optional trailing COMMAND to:
   backtrace [QUALIFIERS]... [FLAGS] [COUNT] [COMMAND]
Doug Evans then suggested to rather have a new "frame apply",
which this patch provides. So, the "frame apply" elegance
is "copyrighted" Doug Evans :).

In this earlier prototype, the FLAGS were using /.
Below a tentative explanation (very long, sorry for that) why
this patch rather uses '-' flags.

> 
> 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".
When re-starting this work, I looked at the various rich different
ways GDB command parses 'flags/options/qualifiers/modifiers/...'
where "various rich different ways" is a politically correct synonym
of "mess" :).

We have at least:
  * some commands have options but without -, and these options can be
    abbreviated, e.g.    backtrace full  same as   backtrace f 
      or backtrace fu
    (this abbreviation feature seems not documented)
  * some commands have options such as -x, and use -- as option terminator
      e.g.       alias -a -- -myaliasfordownstartswithaminus = down
                 demangle [-l language] [--] name
  * some commands have long option that starts with a -, and cannot be
    abbreviated e.g.   thread apply all -ascending p 1
  * some commands have a short -a option, but accepts whatever after
    e.g.   (gdb) interrupt -ahboncettecommandeacceptenimportequoi
  * some commands have /Â e.g. print,x, disassemble,ptype, ... commands
    For print and x, what follows / is a FMT,
    while for disassemble, these are more like flag options.

Based on the above, it is not very clear what is the 
'less unusual' usual way to specify flags/options.

In parallel, I have started another patch to have e.g.
    info var [-t TYPE_REGEXP ]  REGEXP
  to only show the var having a type matching TYPE_REGEXP
This is showing yet another difficulty: how to put a space
in an argument ?

At that point, it looked like one of my next patch should
be an 'option/argument parser', basically the same as what
you describe above with the 'generic framework for command
options'. This framework should provide option parsing
and argument parsing, and allow optional quoting.
IMO, we better base this as much as possible to be
similar to the usual getopt, with at least the following
differences:
  * for backward compatibility, we should support sometimes
   alternative backward compatible behaviour, together with
   a new 'standard behaviour'  e.g.
      backtrace [--full | -f  | full ] ....
   or
      thread apply all [--ascending | -a | -ascending ]
  * have -- systematically allowing to terminate options
    (if we go for /, then should // terminate options then?)
  * have a way to (explicitely) quote an argument e.g.
      info var  -Qt 'long int'  regexpofsomevars
    where -Q indicates that the next "value argument" is
    explicitely quoted.
  * allow combining short one letter args such as -cs
    but also allow -c -s
  * support (at least for disassemble/ptype/..., maybe not
    for FMT that would still be only like it is now) the
    /x backward compatible form, but provide also new
   'standard' options e.g. 
      disassemble [-m | /m | --mixed] [-s | /s | --source] ...
  * common behaviour of such option/arg parsing should be centrally
    documented, in particular quoting, abbreviation of long options,
    terminating options with --, ...
  * ... any other idea/needed feature ?

What this patch gives is compatible with this (future generic
framework) option/arg parser to be developed. If we assume that
the 'long single -' option (such as -ascending) cannot be
abbreviated, then  I think we can make something backward compatible
but go to a more uniform option/arg parsing (and avoid
'local' re-implementation of option parsing logic such as
skip spaces etc).
Of course, if in this patchn, we mandate -v -v -c -s
instead of -vvcs, that would be equally compatible with the
future option/arg parser
('future' is the politically correct synonym of vapourware :).

So, in summary, from my point of view, in the future,
we better go for something getopt compatible, and have
backward compatibility for the existing flags/modifier,
and so have -FLAGS in this patch rather than /FLAGS.

> 
> > 
> > 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.
I could not make an alias that was specifying -s or any option,
e.g.
  (gdb) alias inta = interrupt -a
  Invalid command to alias to: interrupt -a
  (gdb)
and I found e.g.
   t a a -s f a a -s 
quite long to type, and so worth the aliases.
 
> 
> > 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.
Yes, the idea is to push several commits (like in the RFA).
So, I will split the ChangeLog (I am not yet used to
all the subtilities of the ChangeLog cargo-cult :).

Thanks for the feedback/discussion,
the most central point is for sure the - versus / discussion.

Philippe