Re: [patch v2] gdb: add completion handler for "handle"

[Pedro Alves <palves@redhat.com>]

On 08/02/2012 04:34 PM, Mike Frysinger wrote:
> On Thursday 02 August 2012 07:24:07 Pedro Alves wrote:
>> On 08/02/2012 04:58 AM, Mike Frysinger wrote:
>>> +VEC (char_ptr) *
>>> +string_array_completer (struct cmd_list_element *ignore,
>>> +			char *text, char *word, const char * const strings[],
>>> +			size_t num_strings)
>>> +{
>>> +  size_t i;
>>> +  VEC (char_ptr) *return_val = NULL;
>>> +  size_t len = strlen (word);
>>> +
>>> +  for (i = 0; i < num_strings; ++i)
>>> +    {
>>> +      if (strncasecmp (strings[i], word, len) == 0)
>>> +	VEC_safe_push (char_ptr, return_val, xstrdup (strings[i]));
>>> +    }
>>> +
>>> +  return return_val;
>>> +}
>>
>> See complete_on_enum.
> 
> hrm, i was only looking in completer.h.  i don't suppose there's a central 
> place for these APIs ?  or is that expecting too much ? ;)
> 
> also, i think this function is broken.  when i try to call it, i have:
> 	text = "SIGINT a"
> 	word = "a"
> 
> instead of completing "a" into "all", it tries to complete "SIGINT a" and so 
> returns no matches.  i'd have to lie and pass it (word, word) instead of 
> (text, word) ...

I think it's the documentation or parameter names of the function that might
not be as clear as possible.

/* Return a vector of char pointers which point to the different
   possible completions in CMD of TEXT.

   WORD points in the same buffer as TEXT, and completions should be
   returned relative to this position.  For example, suppose TEXT is "foo"
   and we want to complete to "foobar".  If WORD is "oo", return
   "oobar"; if WORD is "baz/foo", return "baz/foobar".  */

From the example, we see that is is TEXT that points at the word
that needs to be completed.  WORD might start before or after the beginning
of the word that is to be completed.

E.g., this comment of this related function says more or less the same:

/* Generate completions all at once.  Returns a vector of strings.
   Each element is allocated with xmalloc.  It can also return NULL if
   there are no completions.

   TEXT is the caller's idea of the "word" we are looking at.

   LINE_BUFFER is available to be looked at; it contains the entire
   text of the line.

   POINT is the offset in that line of the cursor.  You
   should pretend that the line ends at POINT.  */

VEC (char_ptr) *
complete_line (const char *text, char *line_buffer, int point)



"text" being the word, and having a parameter named "word" at
the same time isn't a happy choice of naming...


So if we just swap the args:

 	text = "a"
 	word = "SIGINT a"

completing that yields "SIGINT all".  But that's not what we want.
So to complete "a" into "all", we need to have WORD also pointing
at "a".  passing it (word, word) is therefore not really a lie, but the
right thing for this interface.

> 
>>> +/* Complete the "handle" command.  */
>>> +
>>> +static VEC (char_ptr) *
>>> +handle_completer (struct cmd_list_element *ignore,
>>> +		  char *text, char *word)
>>> +{
>>> +  /* First word is a signal, while the rest are keywords.  */
>>
>> Actually, "handle" accepts more than one signal.  E.g.,
> 
> ok, but that's not what the documentation says:
> http://sourceware.org/gdb/current/onlinedocs/gdb/Signals.html
> 
> handle signal [keywords...]
> 	Change the way gdb handles signal signal. signal can be the number of a 
> signal or its name (with or without the `SIG' at the beginning); a list of 
> signal numbers of the form `low-high'; or the word `all', meaning all the 
> known signals. Optional arguments keywords, described below, say what change 
> to make.
> 
> guess that needs updating too

Indeed.  The online help already says such a thing:

(gdb) help handle
Specify how to handle a signal.
Args are signals and actions to apply to those signals.

"signals".

And it's not an accident, since the code says:

  /* Walk through the args, looking for signal oursigs, signal names, and
     actions.  Signal numbers and signal names may be interspersed with
     actions, with the actions being performed for all signals cumulatively
     specified.  Signal ranges can be specified as <LOW>-<HIGH>.  */

> 
>> (gdb) handle SIGCANCEL SIGABRT stop
>> Signal        Stop      Print   Pass to program Description
>> SIGABRT       Yes       Yes     Yes             Aborted
>> SIGCANCEL     Yes       Yes     Yes             LWP internal signal
>>
>> Can we make that work?  Basically, for all but the first arg, you'd
>> accept the union of the signal names, and the possible actions.

("handle stop" with no signal specified is accepted without error, though
it's a nop.  If it's easier for the completer to not special case the
first arg, by all means.)

> 
> is there a VEC_safe_merge helper ?  i didn't see anything in common/vec.h.  if 
> there was, then it should be easy to make this work.

There isn't, but feel free to add one.  :-)  You can do VEC_safe_grow, and
then memcpy + VEC_address.  There's something similar in insert_catch_syscall.

> 
>>> -  add_com ("handle", class_run, handle_command, _("\
>>> -Specify how to handle a signal.\n\
>>> +  c = add_com ("handle", class_run, handle_command, _("\
>>> +Handle signals: handle SIGNAL [KEYWORDS]\n\
>>>
>>>  Args are signals and actions to apply to those signals.\n\
>>
>> The first line of the doc is special.  It's what "apropos" shows.
> 
> ok ...
> 
>> before:
>> (gdb) apropos signals
>> handle -- Specify how to handle a signal
>> info handle -- What debugger does when program gets various signals
>> info signals -- What debugger does when program gets various signals
>>
>> after:
>> (gdb) apropos signals
>> handle -- Handle signals: handle SIGNAL [KEYWORDS]
>> info handle -- What debugger does when program gets various signals
>> info signals -- What debugger does when program gets various signals
>>
>> The old strings looked better here to me.
> 
> it does.  i was just trying to improve the `help handle` output since it (plus 
> `help signal`) suck atm.  it's really information dense and hard to pick out 
> anything quickly.

And that's much appreciated.

> 
>> This and the "signal" docu change are a bit of a tangent wrt completion,
>> so split them out, please.
> 
> np
> -mike
> 


-- 
Pedro Alves