This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] PR python/16699: GDB Python command completion with overriden complete vs. completer class
- From: Sergio Durigan Junior <sergiodj at redhat dot com>
- To: GDB Patches <gdb-patches at sourceware dot org>
- Cc: Phil Muldoon <pmuldoon at redhat dot com>
- Date: Fri, 04 Apr 2014 17:41:44 -0300
- Subject: Re: [PATCH] PR python/16699: GDB Python command completion with overriden complete vs. completer class
- Authentication-results: sourceware.org; auth=none
- References: <m3iorj3ud8 dot fsf at redhat dot com> <m3mwgjj638 dot fsf at redhat dot com>
On Friday, March 21 2014, I wrote:
> On Wednesday, March 12 2014, I wrote:
>
>> Hi,
>
> Ping.
Ping^2.
>> This PR came from a Red Hat bug that was filed recently. I checked and
>> it still exists on HEAD, so here's a proposed fix. Although this is
>> marked as a Python backend bug, this is really about the completion
>> mechanism used by GDB. Since this code reminds me of my first attempt
>> to make a good noodle, it took me quite some time to fix it in a
>> non-intrusive way.
>>
>> The problem is triggered when one registers a completion method inside a
>> class in a Python script, rather than registering the command using a
>> completer class directly. For example, consider the following script:
>>
>> class MyFirstCommand(gdb.Command):
>> def __init__(self):
>> gdb.Command.__init__(self,'myfirstcommand',gdb.COMMAND_USER,gdb.COMPLETE_FILENAME)
>>
>> def invoke(self,argument,from_tty):
>> raise gdb.GdbError('not implemented')
>>
>> class MySecondCommand(gdb.Command):
>> def __init__(self):
>> gdb.Command.__init__(self,'mysecondcommand',gdb.COMMAND_USER)
>>
>> def invoke(self,argument,from_tty):
>> raise gdb.GdbError('not implemented')
>>
>> def complete(self,text,word):
>> return gdb.COMPLETE_FILENAME
>>
>> MyFirstCommand ()
>> MySecondCommand ()
>>
>> When one loads this into GDB and tries to complete filenames for both
>> myfirstcommand and mysecondcommand, she gets:
>>
>> (gdb) myfirstcommand /hom<TAB>
>> (gdb) myfirstcommand /home/
>> ^
>> ...
>> (gdb) mysecondcommand /hom<TAB>
>> (gdb) mysecondcommand /home
>> ^
>>
>> (The "^" marks the final position of the cursor after the TAB).
>>
>> So we see that myfirstcommand honors the COMPLETE_FILENAME class (as
>> specified in the command creation), but mysecondcommand does not. After
>> some investigation, I found that the problem lies with the set of word
>> break characters that is used for each case. The set should be the same
>> for both commands, but it is not.
>>
>> During the process of deciding which type of completion should be used,
>> the code in gdb/completer.c:complete_line_internal analyses the command
>> that requested the completion and tries to determine the type of
>> completion wanted by checking which completion function will be called
>> (e.g., filename_completer for filenames, location_completer for
>> locations, etc.).
>>
>> This all works fine for myfirstcommand, because immediately after the
>> command registration the Python backend already sets its completion
>> function to filename_completer (which then causes the
>> complete_line_internal function to choose the right set of word break
>> chars). However, for mysecondcommand, this decision is postponed to
>> when the completer function is evaluated, and the Python backend uses an
>> internal completer (called cmdpy_completer). complete_line_internal
>> doesn't know about this internal completer, and can't choose the right
>> set of word break chars in time, which then leads to a bad decision when
>> completing the "/hom" word.
>>
>> So, after a few attempts, I decided to create another callback in
>> "struct cmd_list_element" that will be responsible for handling the case
>> when there is an unknown completer function for complete_line_internal
>> to work with. So far, only the Python backend uses this callback, and
>> only when the user provides a completer method instead of registering
>> the command directly with a completer class. I think this is the best
>> option because it not very intrusive (all the other commands will still
>> work normally), but especially because the whole completion code is so
>> messy that it would be hard to fix this without having to redesign
>> things.
>>
>> I have regtested this on Fedora 18 x86_64, without regressions. I also
>> included a testcase.
>>
>> OK to apply?
>>
>> Thanks,
>>
>> --
>> Sergio
>>
>> gdb/
>> 2014-03-12 Sergio Durigan Junior <sergiodj@redhat.com>
>>
>> PR python/16699
>> * cli/cli-decode.c (set_cmd_completer_handle_brkchars): New
>> function.
>> (add_cmd): Set "completer_handle_brkchars" to NULL.
>> * cli/cli-decode.h (struct cmd_list_element)
>> <completer_handle_brkchars>: New field.
>> * command.h (set_cmd_completer_handle_brkchars): New prototype.
>> * completer.c (set_gdb_completion_word_break_characters): New
>> function.
>> (complete_line_internal): Call "completer_handle_brkchars"
>> callback from command.
>> * completer.h: Include "command.h".
>> (set_gdb_completion_word_break_characters): New prototype.
>> * python/py-cmd.c (cmdpy_completer_handle_brkchars): New function.
>> (cmdpy_init): Set completer_handle_brkchars to
>> cmdpy_completer_handle_brkchars.
>>
>> gdb/testsuite/
>> 2014-03-12 Sergio Durigan Junior <sergiodj@redhat.com>
>>
>> PR python/16699
>> * gdb.python/py-completion.exp: New file.
>> * gdb.python/py-completion.py: Likewise.
>>
>> diff --git a/gdb/cli/cli-decode.c b/gdb/cli/cli-decode.c
>> index d36ab4e..0c4d996 100644
>> --- a/gdb/cli/cli-decode.c
>> +++ b/gdb/cli/cli-decode.c
>> @@ -164,6 +164,15 @@ set_cmd_completer (struct cmd_list_element *cmd, completer_ftype *completer)
>> cmd->completer = completer; /* Ok. */
>> }
>>
>> +/* See definition in commands.h. */
>> +
>> +void
>> +set_cmd_completer_handle_brkchars (struct cmd_list_element *cmd,
>> + void (*completer_handle_brkchars) (struct cmd_list_element *self))
>> +{
>> + cmd->completer_handle_brkchars = completer_handle_brkchars; /* Ok. */
>> +}
>> +
>> /* Add element named NAME.
>> Space for NAME and DOC must be allocated by the caller.
>> CLASS is the top level category into which commands are broken down
>> @@ -239,6 +248,7 @@ add_cmd (const char *name, enum command_class class, void (*fun) (char *, int),
>> c->prefix = NULL;
>> c->abbrev_flag = 0;
>> set_cmd_completer (c, make_symbol_completion_list_fn);
>> + c->completer_handle_brkchars = NULL;
>> c->destroyer = NULL;
>> c->type = not_set_cmd;
>> c->var = NULL;
>> diff --git a/gdb/cli/cli-decode.h b/gdb/cli/cli-decode.h
>> index c6edc87..a043734 100644
>> --- a/gdb/cli/cli-decode.h
>> +++ b/gdb/cli/cli-decode.h
>> @@ -176,6 +176,15 @@ struct cmd_list_element
>> "baz/foo", return "baz/foobar". */
>> completer_ftype *completer;
>>
>> + /* Handle the word break characters for this completer. Usually
>> + this function need not be defined, but for some types of
>> + completers (e.g., Python completers declared as methods inside
>> + a class) the word break chars may need to be redefined
>> + depending on the completer type (e.g., for filename
>> + completers). */
>> +
>> + void (*completer_handle_brkchars) (struct cmd_list_element *self);
>> +
>> /* Destruction routine for this command. If non-NULL, this is
>> called when this command instance is destroyed. This may be
>> used to finalize the CONTEXT field, if needed. */
>> diff --git a/gdb/command.h b/gdb/command.h
>> index a5040a4..058d133 100644
>> --- a/gdb/command.h
>> +++ b/gdb/command.h
>> @@ -160,6 +160,12 @@ typedef VEC (char_ptr) *completer_ftype (struct cmd_list_element *,
>>
>> extern void set_cmd_completer (struct cmd_list_element *, completer_ftype *);
>>
>> +/* Set the completer_handle_brkchars callback. */
>> +
>> +extern void set_cmd_completer_handle_brkchars (struct cmd_list_element *,
>> + void (*f)
>> + (struct cmd_list_element *));
>> +
>> /* HACK: cagney/2002-02-23: Code, mostly in tracepoints.c, grubs
>> around in cmd objects to test the value of the commands sfunc(). */
>> extern int cmd_cfunc_eq (struct cmd_list_element *cmd,
>> diff --git a/gdb/completer.c b/gdb/completer.c
>> index 94f70a9..a9809a2 100644
>> --- a/gdb/completer.c
>> +++ b/gdb/completer.c
>> @@ -450,6 +450,21 @@ expression_completer (struct cmd_list_element *ignore,
>> return location_completer (ignore, p, word);
>> }
>>
>> +/* See definition in completer.h. */
>> +
>> +void
>> +set_gdb_completion_word_break_characters (completer_ftype *fn)
>> +{
>> + /* So far we are only interested in differentiating filename
>> + completers from everything else. */
>> + if (fn == filename_completer)
>> + rl_completer_word_break_characters
>> + = gdb_completer_file_name_break_characters;
>> + else
>> + rl_completer_word_break_characters
>> + = gdb_completer_command_word_break_characters;
>> +}
>> +
>> /* Here are some useful test cases for completion. FIXME: These
>> should be put in the test suite. They should be tested with both
>> M-? and TAB.
>> @@ -481,7 +496,6 @@ typedef enum
>> }
>> complete_line_internal_reason;
>>
>> -
>> /* Internal function used to handle completions.
>>
>>
>> @@ -678,6 +692,9 @@ complete_line_internal (const char *text,
>> p--)
>> ;
>> }
>> + if (reason == handle_brkchars
>> + && c->completer_handle_brkchars != NULL)
>> + (*c->completer_handle_brkchars) (c);
>> if (reason != handle_brkchars && c->completer != NULL)
>> list = (*c->completer) (c, p, word);
>> }
>> @@ -751,6 +768,9 @@ complete_line_internal (const char *text,
>> p--)
>> ;
>> }
>> + if (reason == handle_brkchars
>> + && c->completer_handle_brkchars != NULL)
>> + (*c->completer_handle_brkchars) (c);
>> if (reason != handle_brkchars && c->completer != NULL)
>> list = (*c->completer) (c, p, word);
>> }
>> diff --git a/gdb/completer.h b/gdb/completer.h
>> index 5b90773..cb9c389 100644
>> --- a/gdb/completer.h
>> +++ b/gdb/completer.h
>> @@ -18,6 +18,7 @@
>> #define COMPLETER_H 1
>>
>> #include "gdb_vecs.h"
>> +#include "command.h"
>>
>> extern VEC (char_ptr) *complete_line (const char *text,
>> char *line_buffer,
>> @@ -48,6 +49,13 @@ extern char *get_gdb_completer_quote_characters (void);
>>
>> extern char *gdb_completion_word_break_characters (void);
>>
>> +/* Set the word break characters array to the corresponding set of
>> + chars, based on FN. This function is useful for cases when the
>> + completer doesn't know the type of the completion until some
>> + calculation is done (e.g., for Python functions). */
>> +
>> +extern void set_gdb_completion_word_break_characters (completer_ftype *fn);
>> +
>> /* Exported to linespec.c */
>>
>> extern const char *skip_quoted_chars (const char *, const char *,
>> diff --git a/gdb/python/py-cmd.c b/gdb/python/py-cmd.c
>> index c24bca7..df0aeed 100644
>> --- a/gdb/python/py-cmd.c
>> +++ b/gdb/python/py-cmd.c
>> @@ -206,6 +206,79 @@ cmdpy_function (struct cmd_list_element *command, char *args, int from_tty)
>> do_cleanups (cleanup);
>> }
>>
>> +/* Python function called to determine the break characters of a
>> + certain completer. We use dummy variables for the "text" and
>> + "word" arguments for the completer, and call it. We're actually
>> + only interested in knowing if the completer registered by the user
>> + will return one of the integer codes (see COMPLETER_* symbols). */
>> +
>> +static void
>> +cmdpy_completer_handle_brkchars (struct cmd_list_element *command)
>> +{
>> + cmdpy_object *obj = (cmdpy_object *) get_cmd_context (command);
>> + PyObject *textobj, *wordobj, *resultobj = NULL;
>> + const char dummy_text[] = "dummytext";
>> + const char dummy_word[] = "dummyword";
>> + struct cleanup *cleanup;
>> +
>> + cleanup = ensure_python_env (get_current_arch (), current_language);
>> +
>> + if (!obj)
>> + error (_("Invalid invocation of Python command object."));
>> + if (!PyObject_HasAttr ((PyObject *) obj, complete_cst))
>> + {
>> + /* If there is no complete method, don't error. */
>> + goto done;
>> + }
>> +
>> + textobj = PyUnicode_Decode (dummy_text, sizeof (dummy_text),
>> + host_charset (), NULL);
>> + if (!textobj)
>> + error (_("Could not convert argument to Python string."));
>> + wordobj = PyUnicode_Decode (dummy_word, sizeof (dummy_word),
>> + host_charset (), NULL);
>> + if (!wordobj)
>> + error (_("Could not convert argument to Python string."));
>> +
>> + resultobj = PyObject_CallMethodObjArgs ((PyObject *) obj, complete_cst,
>> + textobj, wordobj, NULL);
>> + Py_DECREF (textobj);
>> + Py_DECREF (wordobj);
>> + if (!resultobj)
>> + {
>> + /* Just swallow errors here. */
>> + PyErr_Clear ();
>> + goto done;
>> + }
>> +
>> + if (PyInt_Check (resultobj))
>> + {
>> + /* User code may also return one of the completion constants,
>> + thus requesting that sort of completion. We are only
>> + interested in this kind of return. */
>> + long value;
>> +
>> + if (!gdb_py_int_as_long (resultobj, &value))
>> + {
>> + /* Ignore. */
>> + PyErr_Clear ();
>> + }
>> + else if (value >= 0 && value < (long) N_COMPLETERS)
>> + {
>> + /* This is the core of this function. Depending on which
>> + completer type the Python function returns, we have to
>> + adjust the break characters accordingly. */
>> + set_gdb_completion_word_break_characters
>> + (completers[value].completer);
>> + }
>> + }
>> +
>> + done:
>> +
>> + Py_XDECREF (resultobj);
>> + do_cleanups (cleanup);
>> +}
>> +
>> /* Called by gdb for command completion. */
>>
>> static VEC (char_ptr) *
>> @@ -546,6 +619,9 @@ cmdpy_init (PyObject *self, PyObject *args, PyObject *kw)
>> set_cmd_context (cmd, self);
>> set_cmd_completer (cmd, ((completetype == -1) ? cmdpy_completer
>> : completers[completetype].completer));
>> + if (completetype == -1)
>> + set_cmd_completer_handle_brkchars (cmd,
>> + cmdpy_completer_handle_brkchars);
>> }
>> if (except.reason < 0)
>> {
>> diff --git a/gdb/testsuite/gdb.python/py-completion.exp b/gdb/testsuite/gdb.python/py-completion.exp
>> new file mode 100644
>> index 0000000..9f8a5b2
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.python/py-completion.exp
>> @@ -0,0 +1,49 @@
>> +# Copyright (C) 2014 Free Software Foundation, Inc.
>> +
>> +# This program is free software; you can redistribute it and/or modify
>> +# it under the terms of the GNU General Public License as published by
>> +# the Free Software Foundation; either version 3 of the License, or
>> +# (at your option) any later version.
>> +#
>> +# This program is distributed in the hope that it will be useful,
>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> +# GNU General Public License for more details.
>> +#
>> +# You should have received a copy of the GNU General Public License
>> +# along with this program. If not, see <http://www.gnu.org/licenses/>.
>> +
>> +set testfile "py-completion"
>> +
>> +load_lib gdb-python.exp
>> +
>> +gdb_exit
>> +gdb_start
>> +
>> +# Skip all tests if Python scripting is not enabled.
>> +if { [skip_python_tests] } { continue }
>> +
>> +gdb_test_no_output "source ${srcdir}/${subdir}/${testfile}.py"
>> +
>> +# This one should always pass.
>> +send_gdb "myfirstcommand /hom\t"
>> +gdb_test_multiple "" "myfirstcommand completion" {
>> + -re "^myfirstcommand /home/$" {
>> + pass "myfirstcommand completion"
>> + }
>> +}
>> +
>> +# Just discarding whatever we typed.
>> +send_gdb "\n"
>> +gdb_test ".*"
>> +
>> +# This is the problematic one.
>> +send_gdb "mysecondcommand /hom\t"
>> +gdb_test_multiple "" "mysecondcommand completion" {
>> + -re "^mysecondcommand /home $" {
>> + fail "mysecondcommand completion (did not correctly complete filename)"
>> + }
>> + -re "^mysecondcommand /home/$" {
>> + pass "mysecondcommand completion"
>> + }
>> +}
>> diff --git a/gdb/testsuite/gdb.python/py-completion.py b/gdb/testsuite/gdb.python/py-completion.py
>> new file mode 100644
>> index 0000000..bf7f77b
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.python/py-completion.py
>> @@ -0,0 +1,38 @@
>> +# Copyright (C) 2014 Free Software Foundation, Inc.
>> +
>> +# This program is free software; you can redistribute it and/or modify
>> +# it under the terms of the GNU General Public License as published by
>> +# the Free Software Foundation; either version 3 of the License, or
>> +# (at your option) any later version.
>> +#
>> +# This program is distributed in the hope that it will be useful,
>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> +# GNU General Public License for more details.
>> +#
>> +# You should have received a copy of the GNU General Public License
>> +# along with this program. If not, see <http://www.gnu.org/licenses/>.
>> +
>> +# This testcase tests PR python/16699
>> +
>> +import gdb
>> +
>> +class MyFirstCommand(gdb.Command):
>> + def __init__(self):
>> + gdb.Command.__init__(self,'myfirstcommand',gdb.COMMAND_USER,gdb.COMPLETE_FILENAME)
>> +
>> + def invoke(self,argument,from_tty):
>> + raise gdb.GdbError('not implemented')
>> +
>> +class MySecondCommand(gdb.Command):
>> + def __init__(self):
>> + gdb.Command.__init__(self,'mysecondcommand',gdb.COMMAND_USER)
>> +
>> + def invoke(self,argument,from_tty):
>> + raise gdb.GdbError('not implemented')
>> +
>> + def complete(self,text,word):
>> + return gdb.COMPLETE_FILENAME
>> +
>> +MyFirstCommand()
>> +MySecondCommand()
>
> --
> Sergio
--
Sergio