On Tuesday, May 20 2014, Tom Tromey wrote:
>>>>>> "Sergio" == Sergio Durigan Junior <sergiodj@redhat.com> writes:
>
> Sergio> Unfortunately I couldn't come up with a good way to do that. readline
> Sergio> messes with the text/word being parsed while it is parsing them, so it
> Sergio> is hard to restart the parsing because we may not have the full context
> Sergio> in all situations. Or at least that's what I found.
>
> I looked through the readline docs here.
> Ok, I see now, the completion API is irredeemably wacky.
:-/
> Sergio> But I fixed my patch according to your comment above, and I think now it
> Sergio> is right. What I did is simple: instead of providing dummy arguments to
> Sergio> the completer, I am now passing the real arguments. As far as I have
> Sergio> tested, it works.
>
> Cute.
>
> Sergio> What do you think? Is it too hacky?
>
> Someday we will invent a hackiness metric to let us know for sure.
> "M(h) is greater than 0.98! Patch rejected by the robot."
Haha, I can only imagine how the source code of this bot would be...
Probably too hacky? ;-)
> Seriously, at first I thought this was probably a bad idea.
> And it is a little weird, since first it word-breaks some random way,
> then redoes the breaking later.
>
> Is there a way to call the Python function just once and store the
> results in the non-enum-return case? Since otherwise it seems that
> every completion request requires two calls to a
> possibly-expensive-though-we-hope-not completer.
Hm, OK, I came up with the following solution. Basically, the code that
calls the Python function is isolated and, through a static variable and
a flag to inform whether we are handling brkchars or doing a completion
itself, the function now "caches" the result object of the function
call.
This code is built on the assumption that every completion will handle
brkchars first, and then do the completion itself second, in that order.
I guess it's a safe assumption given the logic behind the code.
> Anyway I'm ok-enough with it I suppose.
>
> Sergio> +void
> Sergio> +set_cmd_completer_handle_brkchars (struct cmd_list_element *cmd,
> Sergio> + void (*completer_handle_brkchars) (struct cmd_list_element *self,
> Sergio> + const char *text,
> Sergio> + const char *word))
> Sergio> +{
> Sergio> + cmd->completer_handle_brkchars = completer_handle_brkchars; /* Ok. */
>
> I think the "Ok" comment usually is there as a note to the ARI.
> However, does ARI actually check this line?
> If not -> no comment needed.
Done.
> Sergio> +/* Set the completer_handle_brkchars callback. */
> Sergio> +
> Sergio> +extern void set_cmd_completer_handle_brkchars (struct cmd_list_element *,
> Sergio> + void (*f)
> Sergio> + (struct cmd_list_element *,
> Sergio> + const char *,
> Sergio> + const char *));
>
> I think the "f" argument should have type "completer_ftype *" rather
> than being spelled out.
I created a new "completer_ftype_void", because the "handle_brkchars"
function does not return anything.
> Sergio> +static void
> Sergio> +cmdpy_completer_handle_brkchars (struct cmd_list_element *command,
> Sergio> + const char *text, const char *word)
> Sergio> +{
> Sergio> + cmdpy_object *obj = (cmdpy_object *) get_cmd_context (command);
> Sergio> + PyObject *textobj, *wordobj, *resultobj = NULL;
> Sergio> + /* const char dummy_text[] = "dummytext";
> Sergio> + const char dummy_word[] = "dummyword"; */
>
> No need for the commented-out bits.
Ops, sorry. Removed.
> Sergio> +# This one should always pass.
> Sergio> +send_gdb "completefileinit ${testdir_complete}\t"
> Sergio> +gdb_test_multiple "" "completefileinit completion" {
> Sergio> + -re "^completefileinit ${testdir_regex}$" {
> Sergio> + pass "completefileinit completion"
> Sergio> + }
>
> FWIW I generally find it simpler to test the "complete" command rather
> than the send_gdb dance.
Me too, but given:
<https://sourceware.org/bugzilla/show_bug.cgi?id=16265>
And:
<https://sourceware.org/ml/gdb-patches/2013-05/msg00567.html>
I am now using TAB whenever I can in the testcases.
> Either way is ok though.
Thanks. WDYT of the following patch?
Sergio Durigan Junior <sergiodj@redhat.com> writes:
> [...]
> Thanks. WDYT of the following patch?
Hi.
fwiw it's too bad the ability to plug in different completers isn't more,
I dunno, parameterized (couldn't pick a better term, apologies -
I thought of "object oriented" but that carries its own baggage).
Performing completion obviously involves specifying more than a just
single function (witness the comparison of the completer with specific
functions).
Plus it's more than specifying brkchars.
Witness code like this:
/* Many commands which want to complete on
file names accept several file names, as
in "run foo bar >>baz". So we don't want
to complete the entire text after the
command, just the last word. To this
end, we need to find the beginning of the
file name by starting at `word' and going
backwards. */
for (p = word;
p > tmp_command
&& strchr (gdb_completer_file_name_break_characters, p[-1]) == NULL;
p--)
;
IWBN if a "completer" object described how to do all these three things.
Then the special case code for filename_completer (and location_completer)
in completer.c could disappear. But maybe that's a patch for another day.
Regarding the hack of using a static local to pass data from
handle_brkchars to handle_completion, I know it's a hacky pragmatic
choice. To get the reference counting right the code assumes that
if the handle_brkchars phase is done then the handle_completion
phase will be done too, right? I wonder if a SIGINT could sneak in
there between the two passes (either today or tomorrow).
Maybe the code in cmdpy_completer_helper for handle_brkchars_p could
first check whether resultobj is already non-NULL, and decrement its
reference count before setting it to NULL? And cmdpy_completer_helper
could be defined to return a borrowed reference to resultobj?
Dunno, just thinking out loud.
Something puzzles me though: If it's ok to cache the completion result from the
handle_brkchars pass to the handle_completion pass, why have two passes?
It feels like there must be a case where this caching of the result
in a static local from one pass to the next won't work.
Another question:
I noticed complete_command doesn't do this two-phase dance
of handle_brkchars followed by handle_completions. Should it?
It just goes straight to handle_completions.
[Maybe that explains the difference from using TAB. Dunno off hand.]
It seems like complete_command is trying to hand-code its own
handle_brkchars handling:
static void
complete_command (char *arg, int from_tty)
{
int argpoint;
char *point, *arg_prefix;
VEC (char_ptr) *completions;
dont_repeat ();
if (arg == NULL)
arg = "";
argpoint = strlen (arg);
/* complete_line assumes that its first argument is somewhere
within, and except for filenames at the beginning of, the word to
be completed. The following crude imitation of readline's
word-breaking tries to accomodate this. */
point = arg + argpoint;
while (point > arg)
{
if (strchr (rl_completer_word_break_characters, point[-1]) != 0)
break;
point--;
}
arg_prefix = alloca (point - arg + 1);
memcpy (arg_prefix, arg, point - arg);
arg_prefix[point - arg] = 0;
completions = complete_line (point, arg, argpoint);
...
}
TAB and the complete command should work identically of course,
but for your testcase, maybe you should test both just to verify
both work reasonably well (even if not identically).
Given that complete_command doesn't do the two phase dance,
does it work with your patch?
Maybe it does, but IWBN to confirm that.
@@ -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,
+ completer_ftype_void *completer_handle_brkchars)
+{
+ cmd->completer_handle_brkchars = completer_handle_brkchars;
+}
+
/* 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;
@@ -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). */
+
+ completer_ftype_void *completer_handle_brkchars;
+
/* 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. */
@@ -158,8 +158,16 @@ extern void set_cmd_sfunc (struct cmd_list_element *cmd,
typedef VEC (char_ptr) *completer_ftype (struct cmd_list_element *,
const char *, const char *);
+typedef void completer_ftype_void (struct cmd_list_element *,
+ const char *, const char *);
+
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 *,
+ completer_ftype_void *);
+
/* 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,
@@ -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.
@@ -678,6 +693,9 @@ complete_line_internal (const char *text,
p--)
;
}
+ if (reason == handle_brkchars
+ && c->completer_handle_brkchars != NULL)
+ (*c->completer_handle_brkchars) (c, p, word);
if (reason != handle_brkchars && c->completer != NULL)
list = (*c->completer) (c, p, word);
}
@@ -751,6 +769,9 @@ complete_line_internal (const char *text,
p--)
;
}
+ if (reason == handle_brkchars
+ && c->completer_handle_brkchars != NULL)
+ (*c->completer_handle_brkchars) (c, p, word);
if (reason != handle_brkchars && c->completer != NULL)
list = (*c->completer) (c, p, word);
}
@@ -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 *,
@@ -208,45 +208,149 @@ cmdpy_function (struct cmd_list_element *command, char *args, int from_tty)
do_cleanups (cleanup);
}
+/* Helper function for the Python command completers (both "pure"
+ completer and brkchar handler). This function takes COMMAND, TEXT
+ and WORD and tries to call the Python method for completion with
+ these arguments. It also takes HANDLE_BRKCHARS_P, an argument to
+ identify whether it is being called from the brkchar handler or
+ from the "pure" completer. In the first case, it effectively calls
+ the Python method for completion, and records the PyObject in a
+ static variable (used as a "cache"). In the second case, it just
+ returns that variable, without actually calling the Python method
+ again. This saves us one Python method call.
+
+ It is important to mention that this function is built on the
+ assumption that the calls to cmdpy_completer_handle_brkchars and
+ cmdpy_completer will be subsequent with nothing intervening. This
+ is true for our completer mechanism.
+
+ This function returns the PyObject representing the Python method
+ call. */
+
+static PyObject *
+cmdpy_completer_helper (struct cmd_list_element *command,
+ const char *text, const char *word,
+ int handle_brkchars_p)
+{
+ cmdpy_object *obj = (cmdpy_object *) get_cmd_context (command);
+ PyObject *textobj, *wordobj;
+ /* This static variable will server as a "cache" for us, in order to
+ store the PyObject that results from calling the Python
+ function. */
+ static PyObject *resultobj = NULL;
+
+ if (handle_brkchars_p)
+ {
+ /* If we were called to handle brkchars, it means this is the
+ first function call of two that will happen in a row.
+ Therefore, we need to call the completer ourselves, and cache
+ the return value in the static variable RESULTOBJ. Then, in
+ the second call, we can just use the value of RESULTOBJ to do
+ our job. */
+ resultobj = NULL;
+ 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. */
+ return NULL;
+ }
+
+ textobj = PyUnicode_Decode (text, strlen (text), host_charset (), NULL);
+ if (!textobj)
+ error (_("Could not convert argument to Python string."));
+ wordobj = PyUnicode_Decode (word, sizeof (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 ();
+ }
+ }
+
+ /* The caller is responsible for deciding whether to call Py_XDECREF
+ or not. */
+ return resultobj;
+}
+
+/* Python function called to determine the break characters of a
+ certain completer. We are 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,
+ const char *text, const char *word)
+{
+ PyObject *resultobj = NULL;
+ struct cleanup *cleanup;
+
+ cleanup = ensure_python_env (get_current_arch (), current_language);
+
+ /* Calling our helper to obtain the PyObject of the Python
+ function. */
+ resultobj = cmdpy_completer_helper (command, text, word, 1);
+
+ /* Check if there was an error. */
+ if (resultobj == NULL)
+ 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:
+
+ /* We do not call Py_XDECREF here because RESULTOBJ will be used in
+ the subsequent call to cmdpy_completer function. */
+ do_cleanups (cleanup);
+}
+
/* Called by gdb for command completion. */
static VEC (char_ptr) *
cmdpy_completer (struct cmd_list_element *command,
const char *text, const char *word)
{
- cmdpy_object *obj = (cmdpy_object *) get_cmd_context (command);
- PyObject *textobj, *wordobj, *resultobj = NULL;
+ PyObject *resultobj = NULL;
VEC (char_ptr) *result = NULL;
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 -- instead, just
- say that there are no completions. */
- goto done;
- }
+ /* Calling our helper to obtain the PyObject of the Python
+ function. */
+ resultobj = cmdpy_completer_helper (command, text, word, 0);
- textobj = PyUnicode_Decode (text, strlen (text), host_charset (), NULL);
- if (! textobj)
- error (_("Could not convert argument to Python string."));
- wordobj = PyUnicode_Decode (word, strlen (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 the result object of calling the Python function is NULL, it
+ means that there was an error. In this case, just give up and
+ return NULL. */
+ if (resultobj == NULL)
+ goto done;
result = NULL;
if (PyInt_Check (resultobj))
@@ -548,6 +652,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)
{
new file mode 100644
@@ -0,0 +1,70 @@
+# 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"
+
+# Create a temporary directory
+set testdir "${objdir}/${subdir}/py-completion-testdir/"
+set testdir_regex [string_to_regexp $testdir]
+set testdir_complete "${objdir}/${subdir}/py-completion-test"
+file mkdir $testdir
+
+# This one should always pass.
+send_gdb "completefileinit ${testdir_complete}\t"
+gdb_test_multiple "" "completefileinit completion" {
+ -re "^completefileinit ${testdir_regex}$" {
+ pass "completefileinit completion"
+ }
+}
+
+# Just discarding whatever we typed.
+send_gdb "\n"
+gdb_test "print" ".*"
+
+# This is the problematic one.
+send_gdb "completefilemethod ${testdir_complete}\t"
+gdb_test_multiple "" "completefilemethod completion" {
+ -re "^completefilemethod ${testdir_regex} $" {
+ fail "completefilemethod completion (completed filename as wrong command arg)"
+ }
+ -re "^completefilemethod ${testdir_regex}$" {
+ pass "completefilemethod completion"
+ }
+}
+
+# Discarding again
+send_gdb "\n"
+gdb_test "print" ".*"
+
+# Another problematic
+send_gdb "completefilecommandcond ${objdir}/${subdir}/py-completion-t\t"
+gdb_test_multiple "" "completefilecommandcond completion" {
+ -re "^completefilecommandcond ${testdir}$" {
+ fail "completefilecommandcond completion (completed filename instead of command)"
+ }
+ -re "^completefilecommandcond ${objdir}/${subdir}/py-completion-t$" {
+ pass "completefilecommandcond completion"
+ }
+}
new file mode 100644
@@ -0,0 +1,58 @@
+# 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 CompleteFileInit(gdb.Command):
+ def __init__(self):
+ gdb.Command.__init__(self,'completefileinit',gdb.COMMAND_USER,gdb.COMPLETE_FILENAME)
+
+ def invoke(self,argument,from_tty):
+ raise gdb.GdbError('not implemented')
+
+class CompleteFileMethod(gdb.Command):
+ def __init__(self):
+ gdb.Command.__init__(self,'completefilemethod',gdb.COMMAND_USER)
+
+ def invoke(self,argument,from_tty):
+ raise gdb.GdbError('not implemented')
+
+ def complete(self,text,word):
+ return gdb.COMPLETE_FILENAME
+
+class CompleteFileCommandCond(gdb.Command):
+ def __init__(self):
+ gdb.Command.__init__(self,'completefilecommandcond',gdb.COMMAND_USER)
+
+ def invoke(self,argument,from_tty):
+ raise gdb.GdbError('not implemented')
+
+ def complete(self,text,word):
+ # This is a test made to know if the command
+ # completion still works fine. When the user asks to
+ # complete something like "completefilecommandcond
+ # /path/to/py-completion-t", it should not complete to
+ # "/path/to/py-completion-test/", but instead just
+ # wait for input.
+ if "py-completion-t" in text:
+ return gdb.COMPLETE_COMMAND
+ else:
+ return gdb.COMPLETE_FILENAME
+
+CompleteFileInit()
+CompleteFileMethod()
+CompleteFileCommandCond()