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 1/2] Add completion for COMMAND in 'thread apply all|ID... COMMAND'


On Wed, 2019-05-01 at 11:18 +0100, Pedro Alves wrote:
> > I will take a look at how to have completions for command arguments,
> > but IMO that is better done as a separate patch to provide a more general
> > solution (i.e. not only for commands that are launching other commands,
> > which is what this patch is providing).
> 
> I'm going to post the "print -OPT" patch soon, which includes a generic
> framework for command options, and integrates with completion.  It's the
> patch that I mentioned to you a while ago when we discussed the "qvcs" flags
> patches.  I've pushed it to the users/palves/cli-options branch on
> sourceware last night, if you guys want to take a look meanwhile.
I quickly tried it, it is really nice to have command arguments completion.

The "/" command aims at solving a problem (temporary change some option
for one command) that is at least partially covered by this.
=> possibly having both this patch and the "/" command is an overkill.

I first give some comments about the patch (by pasting/commenting the
commit log).  Then at the end, I will list a few points that "/" covers
and that this does not.  Then to be seen if the additional points provided
by "/" are worth keeping the "/" on top of this patch.

>commit log:   Introduce generic CLI options framework, support "print -option val --"
>commit log:    
>commit log:    And "bt -OPTS" too.  And probably others I no longer remember.
>commit log:    
>commit log:    TAB completion fully supported.  That's pretty neat I think.  Give it
>commit log:    a go!  Try:
>commit log:    
>commit log:      (gdb) p -[TAB]
>commit log:      -address         -array-indexes   -null-stop       -pretty          -
static-members  -union
>commit log:      -array           -elements        -object          -repeats         -
symbol          -vtbl
>commit log:    
>commit log:      (gdb) bt -[TAB]
>commit log:      -entry-values         -frame-arguments      -full                 -
hide                 -no-filters           -raw-frame-arguments
>commit log:    
>commit log:    Note that:
>commit log:    
>commit log:     - you can shorten option names, as long as unambiguous.
I effectively see the bt command detecting the ambiguity:
  (gdb) bt -f
  Ambiguous option at: -f
  (gdb) 
but the print command does not:
  (gdb) p -a<TAB>
  -address        -array          -array-indexes  
  (gdb) p -a 1
  $2 = 1
  (gdb)
So, unclear which of the 3 options was activated by -a.


>commit log:     - For boolean options, 0/1 stand for off/on.
>commit log:     - For boolean options, "true" is implied.
Good idea to not oblige to input 0/1.
Why not the same for the 'integer options' ?
When no integer value is provided, that could equally mean:
  'unlimited' (or UINT/INT_MAX).
(that is what "/" considers).

>commit log:    
>commit log:    So these are all equivalent:
>commit log:    
>commit log:     (gdb) print -object on -static-members off -pretty on foo
>commit log:     (gdb) print -object -static-members off -pretty foo
>commit log:     (gdb) print -object -static-members 0 -pretty foo
>commit log:     (gdb) print -o -st 0 -p foo
>commit log:    
>commit log:    And so are these:
>commit log:    
>commit log:     (gdb) bt -entry-values both
>commit log:     (gdb) bt -e b
>commit log:    
>commit log:    "--" explicitly terminates options.  This is necessary because some
>commit log:    expressions may start with "-".  E.g., if you have a variable called
>commit log:    "object" and you want to print its negative: "-object":
>commit log:    
>commit log:       (gdb) print -- -object
>commit log:    
>commit log:    That's a minor potential script breakage, but I think it's well worth
>commit log:    it.
Yes, that does not look to be a big problem.  If ever, it might be possible
to reduce the nr of breakage, e.g. : when an unrecognised option is at the end
of the args or is only followed by non options and/or non recognised options,
then the whole set of args starting at the unrecognised option might be considered
to be the expression.

I have however seen a regression in the print command
HEAD:
(gdb) p -1
$1 = -1
(gdb) 

With the patch:
(gdb) p -1
$3 = 1
(gdb) 

So, either print should tell that -1 is an unknown option, 
or (maybe better?) it should guess that -1 is the expression to print,
as suggested above ?
(there are a bunch of tests failing with the patch, I guess most
are due to the above breakage reason).

>commit log:    
>commit log:    Note that the code is organized such that some of the options and the
>commit log:    "set/show" commands code is shared.  In particular, the "print"
>commit log:    options and the corresponding "set print" commands are defined with
>commit log:    the same structures.  Same thing for some of the "bt" options.
>commit log:    
>commit log:    I think this is a better approach than trying to cram a bunch of
>commit log:    unrelated settings under a single global one-character namespace.
Note that the "/" command uses the upper case range as "prefix" characters,
so the namespace is one-character per level, with as many levels as desired.

>commit log:    
>commit log:    Still missing:
>commit log:    
>commit log:     - run it by upstream, as an RFC.
>commit log:     - comments.  mention why polymorphism isn't done with virtual methods.
>commit log:     - convert more commands.
>commit log:     - not integrated with the new "thread apply" "vqs" flags yet, which
>commit log:       had gone in after this was originally written.
>commit log:     - write tests.
>commit log:     - write docs.

>   I'd
> like to post it for discussion before we decide how to go forward on the
> "/" patches.  I'm on holiday today, and likely OOO for most of it, so not
> sure I'll be able to post it today.
> 
> Funny enough, "thread apply all -ascen[TAB]" is one of the options
> that I converted to the new scheme in the branch.
> 
> Thanks,
> Pedro Alves

There are some differences between what this patch provides
and the "/" command.  I am listing them below, and discuss
if these differences are worth keeping "/" or not.

"/" provides a quicker/faster to type way to set options,
but it is less 'discoverable' than the completion feature,
which probably all GDB users know already (and would like
to have for the options).
For very often options that share an initial word, we might
define an alternate option e.g.
   print [-A | -array] [-i | -array-indexes]
if we believe these options are so frequently used that
they must be fast to type.

Also, the idea is that t"/" command will allow to redo
the previous command with additional options, eg.
(gdb) p larger
$2 = <error reading variable: object size is larger than varsize-limit
(gdb) /v
$3 = "1234567890|1234567890|1234567890"
(gdb) 

This might also be implemented with this patch e.g.
(gdb) p larger
$2 = <error reading variable: object size is larger than varsize-limit
(gdb) -v
$3 = "1234567890|1234567890|1234567890"
(gdb)
where -v would be a shortcut for relaunching the previous command as:
(gdb) p -varsize-list unlimited Larger

This patch implies to add new options to existing commands
(such as the options added to
print) to have a quick way
to change them for one command.
It looks however easy to do (in
particular as some of
the code is shared with the 'set option' code).

"/" changes the global options, then launches the given command,
and then restore the global options.
The given command can e.g. be a user/python defined command
that itself launches a bunch of other commands, which should
be influenced by the global settings.
The user must then (like with GDB 8.3) type a bunch of
  'set this'
  'set that'
  launch the user defined command
  'reset this'
  'reset that'
Or define a new user command that automates the above.

Then if the user types C-c while the middle command runs,
the options will not be restored.
Possibly we could allow a command to be optionally given
after the existing 'set command' (which means:
 change the option, runs the given command, restore the option).
That is in fact the equivalent of the "/" command, but
for just one option.
(compare with the shell:
   export DISPLAY=xxxx
   some_command
  versus
   DISPLAY=xxxx some_command).
Then most of the "/" is still available (e.g. in user
defined commands), but without the alternate "/" letters
to activate.
Alternatively, the 'try/catch' in the CLI or the 'block
of command' patch I started and never finished some
years ago.

In summary: IMO, there is not a huge set of reasons to
have both the "/" and this patch, or at least there are
reasonable ways to do what "/" provides, maybe with
little additional features such as:
  * the optional COMMAND after 
      'set some_option [COMMAND]'
  * add a systematic way to relaunch the previous command
    by starting a line with a '-' option.

Now, of course, if hundreds of GDB users are suddenly wearing
a yellow jacket, and go in the street destroying everything
while shouting 'we want the "/" command', then we might have
to rediscuss :).

Philippe





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