Re: [PATCH] Fix Python completion when using the "complete" command

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

On Wednesday, April 08 2015, Keith Seitz wrote:

>>> >+   This function is usually called twice: one when we are figuring out
>
> nitpick (sorry): "once" instead of "one"

Thanks, fixed.

> This looks good to me.
>
> I have applied this patch to my completer branch, and I can verify
> that it fixes the (other) completion problems I've seen. I recommend
> that a maintainer approve this.

Thanks for the review, Keith!

Here is the updated patch + ChangeLog entry.

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

gdb/ChangeLog:
2015-04-08  Sergio Durigan Junior  <sergiodj@redhat.com>

	PR python/16699
	* python/py-cmd.c (cmdpy_completer_helper): Adjust function to not
	use a caching mechanism.  Adjust comments and code to reflect
	that.  Replace 'sizeof' by 'strlen' when fetching 'wordobj'.
	(cmdpy_completer_handle_brkchars): Adjust call to
	cmdpy_completer_helper.  Call Py_XDECREF for 'resultobj'.
	(cmdpy_completer): Likewise.

gdb/testsuite/ChangeLog:
2015-04-08  Keith Seitz  <keiths@redhat.com>

	PR python/16699
	* gdb.python/py-completion.exp: New tests for completion.
	* gdb.python/py-completion.py (CompleteLimit1): New class.
	(CompleteLimit2): Likewise.
	(CompleteLimit3): Likewise.
	(CompleteLimit4): Likewise.
	(CompleteLimit5): Likewise.
	(CompleteLimit6): Likewise.
	(CompleteLimit7): Likewise.

diff --git a/gdb/python/py-cmd.c b/gdb/python/py-cmd.c
index 1d89912..017d0b6 100644
--- a/gdb/python/py-cmd.c
+++ b/gdb/python/py-cmd.c
@@ -210,85 +210,70 @@ cmdpy_function (struct cmd_list_element *command, char *args, int from_tty)
 /* 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.
+   these arguments.
+
+   This function is usually called twice: once when we are figuring out
+   the break characters to be used, and another to perform the real
+   completion itself.  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.  The only situation when this function is
+   not called twice is when the user uses the "complete" command: in
+   this scenario, there is no call to determine the "brkchars".
+
+   Ideally, it would be nice to cache the result of the first call (to
+   determine the "brkchars") and return this value directly in the
+   second call (to perform the actual completion).  However, due to
+   the peculiarity of the "complete" command mentioned above, it is
+   possible to put GDB in a bad state if you perform a TAB-completion
+   and then a "complete"-completion sequentially.  Therefore, we just
+   recalculate everything twice for TAB-completions.
 
    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)
+			const char *text, const char *word)
 {
   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;
+  PyObject *resultobj;
 
-  if (handle_brkchars_p)
+  if (obj == NULL)
+    error (_("Invalid invocation of Python command object."));
+  if (!PyObject_HasAttr ((PyObject *) obj, complete_cst))
     {
-      /* 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."));
-	}
+      /* If there is no complete method, don't error.  */
+      return NULL;
+    }
 
-      resultobj = PyObject_CallMethodObjArgs ((PyObject *) obj, complete_cst,
-					      textobj, wordobj, 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, strlen (word), host_charset (), NULL);
+  if (wordobj == NULL)
+    {
       Py_DECREF (textobj);
-      Py_DECREF (wordobj);
-      if (!resultobj)
-	{
-	  /* Just swallow errors here.  */
-	  PyErr_Clear ();
-	}
+      error (_("Could not convert argument to Python string."));
+    }
 
-      Py_XINCREF (resultobj);
+  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;
 }
 
@@ -308,7 +293,7 @@ cmdpy_completer_handle_brkchars (struct cmd_list_element *command,
 
   /* Calling our helper to obtain the PyObject of the Python
      function.  */
-  resultobj = cmdpy_completer_helper (command, text, word, 1);
+  resultobj = cmdpy_completer_helper (command, text, word);
 
   /* Check if there was an error.  */
   if (resultobj == NULL)
@@ -338,8 +323,7 @@ cmdpy_completer_handle_brkchars (struct cmd_list_element *command,
 
  done:
 
-  /* We do not call Py_XDECREF here because RESULTOBJ will be used in
-     the subsequent call to cmdpy_completer function.  */
+  Py_XDECREF (resultobj);
   do_cleanups (cleanup);
 }
 
@@ -357,7 +341,7 @@ cmdpy_completer (struct cmd_list_element *command,
 
   /* Calling our helper to obtain the PyObject of the Python
      function.  */
-  resultobj = cmdpy_completer_helper (command, text, word, 0);
+  resultobj = cmdpy_completer_helper (command, text, word);
 
   /* 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
@@ -419,6 +403,7 @@ cmdpy_completer (struct cmd_list_element *command,
 
  done:
 
+  Py_XDECREF (resultobj);
   do_cleanups (cleanup);
 
   return result;
diff --git a/gdb/testsuite/gdb.python/py-completion.exp b/gdb/testsuite/gdb.python/py-completion.exp
index 0e283e8..5e45087 100644
--- a/gdb/testsuite/gdb.python/py-completion.exp
+++ b/gdb/testsuite/gdb.python/py-completion.exp
@@ -40,7 +40,8 @@ gdb_test_multiple "" "completefileinit completion" {
 }
 
 # Just discarding whatever we typed.
-gdb_test " " ".*" "discard #1"
+set discard 0
+gdb_test " " ".*" "discard #[incr discard]"
 
 # This is the problematic one.
 send_gdb "completefilemethod ${testdir_complete}\t"
@@ -54,7 +55,7 @@ gdb_test_multiple "" "completefilemethod completion" {
 }
 
 # Discarding again
-gdb_test " " ".*" "discard #2"
+gdb_test " " ".*" "discard #[incr discard]"
 
 # Another problematic
 set completion_regex "[string_to_regexp [standard_output_file "py-completion-t"]]"
@@ -67,3 +68,63 @@ gdb_test_multiple "" "completefilecommandcond completion" {
 	pass "completefilecommandcond completion"
     }
 }
+
+# Start gdb over again to clear out current state.  This can interfere
+# with the expected output of the below tests in a buggy gdb.
+gdb_exit
+gdb_start
+gdb_test_no_output "source ${srcdir}/${subdir}/${testfile}.py"
+
+gdb_test_sequence "complete completel" \
+    "list all completions of 'complete completel'" {
+	"completelimit1"
+	"completelimit2"
+	"completelimit3"
+	"completelimit4"
+	"completelimit5"
+	"completelimit6"
+	"completelimit7"
+    }
+
+# Discarding again
+gdb_test " " ".*" "discard #[incr discard]"
+
+gdb_test_sequence "complete completelimit1 c" \
+    "list all completions of 'complete completelimit1 c'" {
+	"completelimit1 cl11"
+	"completelimit1 cl12"
+	"completelimit1 cl13"
+    }
+
+# Discarding again
+gdb_test " " ".*" "discard #[incr discard]"
+
+# If using readline, we can TAB-complete.  This used to trigger a bug
+# because the cached result from the completer was being reused for
+# a different python command.
+if {[readline_is_used]} {
+    set testname "tab-complete 'completelimit1 c'"
+    send_gdb "completelimit1 c\t"
+    gdb_test_multiple "" $testname {
+	-re "^completelimit1 c\\\x07l1$" {
+	    pass $testname
+
+	    # Discard the command line
+	    gdb_test " " ".*" "discard #[incr discard]"
+	}
+    }
+
+    gdb_test_sequence "complete completelimit2 c" \
+	"list all completions of 'complete completelimit2 c'" {
+	    "completelimit2 cl21"
+	    "completelimit2 cl210"
+	    "completelimit2 cl22"
+	    "completelimit2 cl23"
+	    "completelimit2 cl24"
+	    "completelimit2 cl25"
+	    "completelimit2 cl26"
+	    "completelimit2 cl27"
+	    "completelimit2 cl28"
+	    "completelimit2 cl29"
+	}
+}
diff --git a/gdb/testsuite/gdb.python/py-completion.py b/gdb/testsuite/gdb.python/py-completion.py
index e75b6fc..1413c08 100644
--- a/gdb/testsuite/gdb.python/py-completion.py
+++ b/gdb/testsuite/gdb.python/py-completion.py
@@ -53,6 +53,95 @@ class CompleteFileCommandCond(gdb.Command):
 		else:
 			return gdb.COMPLETE_FILENAME
 
+class CompleteLimit1(gdb.Command):
+	def __init__(self):
+		gdb.Command.__init__(self,'completelimit1',gdb.COMMAND_USER)
+
+	def invoke(self,argument,from_tty):
+		raise gdb.GdbError('not implemented')
+
+	def complete(self,text,word):
+                return ["cl11", "cl12", "cl13"]
+
+class CompleteLimit2(gdb.Command):
+	def __init__(self):
+		gdb.Command.__init__(self,'completelimit2',
+                                     gdb.COMMAND_USER)
+
+	def invoke(self,argument,from_tty):
+		raise gdb.GdbError('not implemented')
+
+	def complete(self,text,word):
+		return ["cl21", "cl23", "cl25", "cl27", "cl29",
+                        "cl22", "cl24", "cl26", "cl28", "cl210"]
+
+class CompleteLimit3(gdb.Command):
+	def __init__(self):
+		gdb.Command.__init__(self,'completelimit3',
+                                     gdb.COMMAND_USER)
+
+	def invoke(self,argument,from_tty):
+		raise gdb.GdbError('not implemented')
+
+	def complete(self,text,word):
+		return ["cl31", "cl33", "cl35", "cl37", "cl39",
+                        "cl32", "cl34", "cl36", "cl38", "cl310"]
+
+class CompleteLimit4(gdb.Command):
+	def __init__(self):
+		gdb.Command.__init__(self,'completelimit4',
+                                     gdb.COMMAND_USER)
+
+	def invoke(self,argument,from_tty):
+		raise gdb.GdbError('not implemented')
+
+	def complete(self,text,word):
+		return ["cl41", "cl43", "cl45", "cl47", "cl49",
+                        "cl42", "cl44", "cl46", "cl48", "cl410"]
+
+class CompleteLimit5(gdb.Command):
+	def __init__(self):
+		gdb.Command.__init__(self,'completelimit5',
+                                     gdb.COMMAND_USER)
+
+	def invoke(self,argument,from_tty):
+		raise gdb.GdbError('not implemented')
+
+	def complete(self,text,word):
+		return ["cl51", "cl53", "cl55", "cl57", "cl59",
+                        "cl52", "cl54", "cl56", "cl58", "cl510"]
+
+class CompleteLimit6(gdb.Command):
+	def __init__(self):
+		gdb.Command.__init__(self,'completelimit6',
+                                     gdb.COMMAND_USER)
+
+	def invoke(self,argument,from_tty):
+		raise gdb.GdbError('not implemented')
+
+	def complete(self,text,word):
+		return ["cl61", "cl63", "cl65", "cl67", "cl69",
+                        "cl62", "cl64", "cl66", "cl68", "cl610"]
+
+class CompleteLimit7(gdb.Command):
+	def __init__(self):
+		gdb.Command.__init__(self,'completelimit7',
+                                     gdb.COMMAND_USER)
+
+	def invoke(self,argument,from_tty):
+		raise gdb.GdbError('not implemented')
+
+	def complete(self,text,word):
+		return ["cl71", "cl73", "cl75", "cl77", "cl79",
+                        "cl72", "cl74", "cl76", "cl78", "cl710"]
+
 CompleteFileInit()
 CompleteFileMethod()
 CompleteFileCommandCond()
+CompleteLimit1()
+CompleteLimit2()
+CompleteLimit3()
+CompleteLimit4()
+CompleteLimit5()
+CompleteLimit6()
+CompleteLimit7()