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

[Sergio Durigan Junior <sergiodj at redhat dot com>]

On Wednesday, September 03 2014, Doug Evans wrote:

> Hi.

Hi.

> I've spent some more time looking at the patch.

Thanks.

> I have one style nit and one request.
>
> Style nit: I see lots of places that do "if (!ptr)".
> Convention is to write this as "if (ptr == NULL)".

I know.  However, this code already existed; I am only moving parts of
it.  I was in doubt if it would be good to rewrite those bad style
checks, but decided to keep them.  Anyway, I will rewrite before I push.

> Request:
> The "why" of things is explained sufficiently well in your
> original email:
> https://sourceware.org/ml/gdb-patches/2014-03/msg00301.html
> And while I can appreciate this text being added to the commit message,
> I don't want to have to read commit logs to have a basic understanding
> of the "why" of the code. [I'm all for more deeper understanding
> being deferred to commit logs as appropriate, but I should be able
> to get a basic understanding from the code itself.]
>
> Can you add something descriptive to the code?

Fair enough.

> For example, how about something like this?
>
> --- python/py-cmd.c=    2014-09-02 20:28:57.838267408 -0700
> +++ python/py-cmd.c     2014-09-02 20:59:40.026083464 -0700
> @@ -219,6 +219,14 @@ cmdpy_function (struct cmd_list_element
>     returns that variable, without actually calling the Python method
>     again.  This saves us one Python method call.
>
> +   The reason for this two step dance is that we need to know the set
> +   of "brkchars" to use early on, before we actually try to perform the
> +   completion.  But if a Python command supplies a "complete" method then
> +   we have to call that method first: it may return as its result the kind
> +   of completion to perform and that will in turn specify which brkchars
> +   to use.  IOW, we need the result of the "complete" method before we
> +   actually perform the completion.
> +
>     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

Yeah, looks good, I will add this.

> I have no more comments so LGTM with those changes.

Thanks, this is what I pushed.

  <https://sourceware.org/ml/gdb-cvs/2014-09/msg00007.html>

-- 
Sergio
GPG key ID: 0x65FC5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 80961d6..d416623 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,25 @@
+2014-09-03  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 (completer_ftype_void): New typedef.
+	(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_helper): New function.
+	(cmdpy_completer_handle_brkchars): New function.
+	(cmdpy_completer): Adjust to use cmdpy_completer_helper.
+	(cmdpy_init): Set completer_handle_brkchars to
+	cmdpy_completer_handle_brkchars.
+
 2014-09-03  Gary Benson  <gbenson@redhat.com>
 
 	* nat/x86-dregs.h (ALL_DEBUG_REGISTERS): Renamed as...
diff --git a/gdb/cli/cli-decode.c b/gdb/cli/cli-decode.c
index c409d9c..7bc51a1 100644
--- a/gdb/cli/cli-decode.c
+++ b/gdb/cli/cli-decode.c
@@ -161,6 +161,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
@@ -236,6 +245,7 @@ add_cmd (const char *name, enum command_class class, cmd_cfunc_ftype *fun,
   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 865d4a0..5920559 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).  */
+
+    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.  */
diff --git a/gdb/command.h b/gdb/command.h
index 4ac3bfb..3b96212 100644
--- a/gdb/command.h
+++ b/gdb/command.h
@@ -159,8 +159,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,
diff --git a/gdb/completer.c b/gdb/completer.c
index 44920dd..67d7f45 100644
--- a/gdb/completer.c
+++ b/gdb/completer.c
@@ -449,6 +449,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.
@@ -677,6 +692,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);
 		}
@@ -750,6 +768,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 7aa0f3b..bc7ed96 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,
 				      const 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 21f2a20..8bc4bf7 100644
--- a/gdb/python/py-cmd.c
+++ b/gdb/python/py-cmd.c
@@ -208,45 +208,163 @@ 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.
+
+   The reason for this two step dance is that we need to know the set
+   of "brkchars" to use early on, before we actually try to perform
+   the completion.  But if a Python command supplies a "complete"
+   method then we have to call that method first: it may return as its
+   result the kind of completion to perform and that will in turn
+   specify which brkchars to use.  IOW, we need the result of the
+   "complete" method before we actually perform the completion.
+
+   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.  */
+      if (resultobj != NULL)
+	Py_DECREF (resultobj);
+
+      resultobj = NULL;
+      if (obj == NULL)
+	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 == NULL)
+	error (_("Could not convert argument to Python string."));
+      wordobj = PyUnicode_Decode (word, sizeof (word), host_charset (), NULL);
+      if (wordobj == NULL)
+	{
+	  Py_DECREF (textobj);
+	  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 ();
+	}
+
+      Py_XINCREF (resultobj);
+    }
+
+  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))
@@ -302,7 +420,6 @@ cmdpy_completer (struct cmd_list_element *command,
 
  done:
 
-  Py_XDECREF (resultobj);
   do_cleanups (cleanup);
 
   return result;
@@ -548,6 +665,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/ChangeLog b/gdb/testsuite/ChangeLog
index d996b7c..d6db24b 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,9 @@
+2014-09-03  Sergio Durigan Junior  <sergiodj@redhat.com>
+
+	PR python/16699
+	* gdb.python/py-completion.exp: New file.
+	* gdb.python/py-completion.py: Likewise.
+
 2014-08-28  Doug Evans  <dje@google.com>
 
 	* gdb.arch/amd64-pseudo.c (main): Rewrite to better specify when
diff --git a/gdb/testsuite/gdb.python/py-completion.exp b/gdb/testsuite/gdb.python/py-completion.exp
new file mode 100644
index 0000000..a3bac8b
--- /dev/null
+++ b/gdb/testsuite/gdb.python/py-completion.exp
@@ -0,0 +1,69 @@
+# 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 "[standard_output_file "py-completion-testdir"]/"
+set testdir_regex [string_to_regexp $testdir]
+set testdir_complete [standard_output_file "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.
+gdb_test " " ".*" "discard #1"
+
+# 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
+gdb_test " " ".*" "discard #2"
+
+# Another problematic
+set completion_regex "[string_to_regexp [standard_output_file "py-completion-t"]]"
+send_gdb "completefilecommandcond [standard_output_file "py-completion-t\t"]"
+gdb_test_multiple "" "completefilecommandcond completion" {
+    -re "^completefilecommandcond ${testdir}$" {
+	fail "completefilecommandcond completion (completed filename instead of command)"
+    }
+    -re "^completefilecommandcond ${completion_regex}\007$" {
+	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()