This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


On 06/01/2018 08:38 PM, Philippe Waroquiers wrote:
> 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 :).
> 

:-)

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

Yes, these will tend to be very old commands / options.

>   * some commands have options such as -x, and use -- as option terminator
>       e.g.       alias -a -- -myaliasfordownstartswithaminus = down
>                  demangle [-l language] [--] name

Right, newer commands started doing that.  Since we don't have a generic
framework, it's been done on an as-needed, or in-case-we-need-it-in-future
basis.  I know I pushed to add it more than once in review.

>   * some commands have long option that starts with a -, and cannot be
>     abbreviated e.g.   thread apply all -ascending p 1

Right.  I'd call that a bug.

>   * some commands have a short -a option, but accepts whatever after
>     e.g.   (gdb) interrupt -ahboncettecommandeacceptenimportequoi

LOL

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

It should at least be clear that single-'-' is used throughout,
while double-'-' like gnuopt does (by default) for long options
is never used, except in the '--' terminator.

The explicit location options for example, is a clear
example of more modern GDB options.  There, we accept
"break -function", "break -line", etc. and all abbreviations,
like "b -fun", "b -f", etc.  And very importantly, we
support TAB completion of command options.  I think that's
the killer feature.  It helps with the typing _and_ the
discoverability of the user interface.  I.e., you can
type "b -TAB" to see all the options you can pass to the
command.  I'd like to be able to have that for _all_ commands.

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

Unless it's the last/rightmost argument, then I think the
best choice is to have the user quote or escape if
necessary?

Note that "b -function foo (int) -line 10" works, even
though there's a space in "foo (int)", because the explicit
location's parser was taught to skip past function names,
and in that case, we know that ()s are balanced.  But it's
tricky code, and the same probably can't be done for
REGEXPs, I think.

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

That's basically unlike any command option in GDB currently.
We've settled on single-'-' a long time ago, so I'm not
seeing how breaking that buys us much other than another
long partial transition, lots of confusion and breaking
scripts and user habits.

Because we currently accept single '-' as long option
specifier, I don't see how we can both keep backward
compatibility with current "b -function" etc, _and_
allow single '-' as specifier for
short-options-that-can-be-combined, because that causes ambiguity
like, what does "(gdb) cmd -fu" mean:

 #1 - (gdb) cmd -function
 #2 - (gdb) cmd -function -unique

?

I don't think that saying 'it's "-function" if "-unique" doesn't
exist yet', because then we need worry about the fact that adding
any option can break any abbreviated use of existing options.

>   * have -- systematically allowing to terminate options

*nod*

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

Not sure we need that -Q.  We can support optional quotes, and
tell users to add quotes if necessary?  That's what we've done
since forever in linespecs and expressions, for example.

>   * allow combining short one letter args such as -cs
>     but also allow -c -s

I think that way lies madness.

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

As mentioned above, I think the combining the short options under
a single '-' is problematic.  As such, I'd rather see that removed
from the patch, and the flags implemented as regular
individually-specified options.  I.e., have the user type "-s -c"
instead of "-sc". It's really not much of a difference for the user.

> If we assume that
> the 'long single -' option (such as -ascending) cannot be
> abbreviated, 

I don't think that's desirable.  I think making GDB support
"-ascending" abbreviations would be an obviously desirable patch.

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

I resent the "vapourware" remark. :-P  It's real! :-P

See here:

  https://github.com/palves/gdb/commits/palves/cmd-options

That started out with some discussions about changing the
defaults of some of the "set print" options, like
"set print static-members" and "set print object".

It occurred to me then that part of the problem of picking
defaults is that it's not possible to override the
"set print" options in individual single-shot "print"
command invocations.  If we could do that, then the
defaults are a little bit less important (though still
important).

So that branch wires in all the "set print FOO" toggles
backed by value_print_options in the code as "print" command options,
and along the way introduces a framework to make it happen:

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

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

(gdb) p array
$1 = {{elem = 1, static s = 0}, {elem = 1, static s = 0}, {elem = 1, static s = 0}, {elem = 1, static s = 0}, {elem = 1, static s = 0}, {elem = 1, static s = 0}, {
    elem = 1, static s = 0}, {elem = 1, static s = 0}, {elem = 1, static s = 0}, {elem = 1, static s = 0}}

(gdb) p -static-members off array
$2 = {{elem = 1}, {elem = 1}, {elem = 1}, {elem = 1}, {elem = 1}, {elem = 1}, {elem = 1}, {elem = 1}, {elem = 1}, {elem = 1}}

(gdb) p -pretty -static-members off -- array
$3 = {{
    elem = 1
  }, {
    elem = 1
  }, {
    elem = 1
  }, {
    elem = 1
  }, {
    elem = 1
  }, {
    elem = 1
  }, {
    elem = 1
  }, {
    elem = 1
  }, {
    elem = 1
  }, {
    elem = 1
  }}

etc.

I've also hooked it to a few other commands for experimentation:

 (gdb) bt -[TAB]
 -entry-values         -frame-arguments      -full                 -hide                 -no-filters           -raw-frame-arguments  

 (gdb) info threads [TAB]
 -gid            THREAD_ID_LIST  

 (gdb) thread apply all -[TAB]
 (gdb) thread apply all -ascending  # there are no other options currently

 (gdb) help print
 Print value of expression EXP.
 Usage: print [[OPTION]... --] [EXP]

 Variables accessible are those of the lexical environment of the selected
 stack frame, plus all those whose scope is global or an entire file.

 Options:
   -address [on|off]
     Set printing of addresses.

   -array [on|off]
     Set pretty formatting of arrays.

   -array-indexes [on|off]
     Set printing of array indexes.

   -elements NUMBER | unlimited
     Set limit on string chars or array elements to print.
     "unlimited" causes there to be no limit.

   -null-stop [on|off]
     Set printing of char arrays to stop at first null char.

   -object [on|off]
     Set printing of C++ virtual function tables.

   -pretty [on|off]
     Set pretty formatting of structures.

   -repeats NUMBER | unlimited
     Set threshold for repeated print elements.
     "unlimited" causes all elements to be individually printed.

   -static-members [on|off]
     Set printing of C++ static members.

   -symbol [on|off]
     Set printing of symbol names when printing pointers.

   -union [on|off]
     Set printing of unions interior to structures.

   -vtbl [on|off]
     Set printing of C++ virtual function tables.

 [...]
 (gdb)

I rebased that branch this weekend and played with it a bit more,
but there's still a lot to do.  In particular, the internal API
grew organically as I threw new commands and use cases at it,
and I wouldn't call it final.  To reiterate, a couple design goals
played into its current form:

 #1 - desire to have normal parsing and completion take
      most of the same code paths, so that we avoid
      normal parsing and completion parsing going out
      of sync.  This is a continuation of the same idea that
      I used for the linespec parser rework from last year.

 #2 - share options / data structures with "set/show"
      command options parsing.

I wrote most of this about 6 months ago, and haven't
context switched it all back in, but I think I wanted to
look to see if we could share more for point #2 above.

I encourage you (and others) to give it a try, particularly
play with TAB completion, see how that makes options more
discoverable and the CLI a better joy.  IMO.

(and I think we could do a lot of neat things with
completion, like maybe display the completion matches
in one column, and option usage in another column, all
in the same display.  And color!)

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

That sounds like something we should fix, regardless.

> and I found e.g.
>    t a a -s f a a -s 
> quite long to type, and so worth the aliases.

I wonder whether that's a real use case in practice though.
What sort of thing does one want to print in all frames
of all threads?  Genuinely curious.

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

I could bite that, even though it looks a bit contrived to me.

Thanks,
Pedro Alves


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]