PR python/16699: GDB Python command completion with overriden complete vs. completer class

Message ID m361ln20e8.fsf@redhat.com
State New, archived
Headers

Commit Message

Sergio Durigan Junior May 3, 2014, 12:04 a.m. UTC
  On Thursday, April 10 2014, Tom Tromey wrote:

> Sergio> +/* Python function called to determine the break characters of a
> Sergio> +   certain completer.  We use dummy variables for the "text" and
> Sergio> +   "word" arguments for the completer, and call it.  We're actually
> Sergio> +   only interested in knowing if the completer registered by the user
> Sergio> +   will return one of the integer codes (see COMPLETER_* symbols).  */
> Sergio> +
> Sergio> +static void
> Sergio> +cmdpy_completer_handle_brkchars (struct cmd_list_element *command)
> Sergio> +{

Thanks for the review, Tom.

> IIUC this is calling the command's complete method with some dummy strings
> to see what it will do.
>
> I don't think this approach will work since in general the reason to
> write a complete method is to be able to adapt to different arguments.
> That is, there's no reason to expect that passing dummy arguments will
> yield the same result.

Hm, you are right, and I even managed to extend the testcase in order to
trigger a failure because of this bug in my patch.

> I didn't look deeply into the problem but is there a way to "reparse"
> the text when the complete method returns one of the enum values?

Unfortunately I couldn't come up with a good way to do that.  readline
messes with the text/word being parsed while it is parsing them, so it
is hard to restart the parsing because we may not have the full context
in all situations.  Or at least that's what I found.

But I fixed my patch according to your comment above, and I think now it
is right.  What I did is simple: instead of providing dummy arguments to
the completer, I am now passing the real arguments.  As far as I have
tested, it works.

One extra comment worth making is that now the gdb.COMPLETE_COMMAND case
also works as expected, i.e., it doesn't complete filenames if it is not
told to do so.  The reported behavior on the bugzilla, which said that
"c /hom" was being completed as "c /home " is also wrong.

What do you think?  Is it too hacky?

Thank you,
  

Comments

Tom Tromey May 20, 2014, 7:12 p.m. UTC | #1
>>>>> "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."


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.

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.

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.

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.

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.

Either way is ok though.

Tom
  

Patch

diff --git a/gdb/cli/cli-decode.c b/gdb/cli/cli-decode.c
index d36ab4e..b6365ba 100644
--- a/gdb/cli/cli-decode.c
+++ b/gdb/cli/cli-decode.c
@@ -164,6 +164,17 @@  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,
+					     const char *text,
+					     const char *word))
+{
+  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 +250,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..d7ec9b0 100644
--- a/gdb/cli/cli-decode.h
+++ b/gdb/cli/cli-decode.h
@@ -176,6 +176,16 @@  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,
+				       const char *text, const char *word);
+
     /* 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..c3b267c 100644
--- a/gdb/command.h
+++ b/gdb/command.h
@@ -160,6 +160,14 @@  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 *,
+						const char *,
+						const char *));
+
 /* 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..db074af 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.
@@ -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);
 	    }
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..a022b4e 100644
--- a/gdb/python/py-cmd.c
+++ b/gdb/python/py-cmd.c
@@ -206,6 +206,78 @@  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,
+				 const char *text, const char *word)
+{
+  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 (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 ();
+      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 +618,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..b8b821e
--- /dev/null
+++ b/gdb/testsuite/gdb.python/py-completion.exp
@@ -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"
+    }
+}
diff --git a/gdb/testsuite/gdb.python/py-completion.py b/gdb/testsuite/gdb.python/py-completion.py
new file mode 100644
index 0000000..23592d0
--- /dev/null
+++ b/gdb/testsuite/gdb.python/py-completion.py
@@ -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()