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

[Keith Seitz <keiths@redhat.com>]

On 04/07/2015 12:13 PM, Sergio Durigan Junior wrote:
>> >diff --git a/gdb/python/py-cmd.c b/gdb/python/py-cmd.c
>> >index 1d89912..29d8d90 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: one when we are figuring out

nitpick (sorry): "once" instead of "one"

>> >+   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;
>> >  }
>> >

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.

Thank you for taking a look at this so quickly!

Keith