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

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

On Tuesday, March 31 2015, I wrote:

> On Tuesday, March 31 2015, I wrote:
>
>> Hi,
>
> Sorry, the previous patch contains a version of the testcase that does
> not pass (I was using this testcase to test another version of the
> patch).  Attached is the updated patch with the correct testcase.  All
> the rest is the same.

Ping.

> -- 
> Sergio
> GPG key ID: 0x65FC5E36
> Please send encrypted e-mail if possible
> http://sergiodj.net/
>
> gdb/ChangeLog:
> 2015-03-31  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-03-31  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..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
> +   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()

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