public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
* [PATCH] Fix Python completion when using the "complete" command
@ 2015-03-31 23:10 Sergio Durigan Junior
  2015-04-01  2:07 ` Sergio Durigan Junior
  0 siblings, 1 reply; 7+ messages in thread
From: Sergio Durigan Junior @ 2015-03-31 23:10 UTC (permalink / raw)
  To: GDB Patches; +Cc: Keith Seitz

Hi,

This patch is related to PR python/16699, and is an improvement over the
patch posted here:

  <https://sourceware.org/ml/gdb-patches/2014-03/msg00301.html>

Keith noticed that, when using the "complete" command on GDB to complete
a Python command, some strange things could happen.  In order to
understand what can go wrong, I need to explain how the Python
completion mechanism works.

When the user requests a completion of a Python command by using TAB,
GDB will first try to determine the right set of "brkchars" that will be
used when doing the completion.  This is done by actually calling the
"complete" method of the Python class.  Then, when we already know the
"brkchars" that will be used, we call the "complete" method again, for
the same values.

If you read the thread mentioned above, you will see that one of the
design decisions was to make the "cmdpy_completer_helper" (which is the
function the does the actual calling of the "complete" method) cache the
first result of the completion, since this result will be used in the
second call, to do the actual completion.

The problem is that the "complete" command does not process the
brkchars, and the current Python completion mechanism (improved by the
patch mentioned above) relies on GDB trying to determine the brkchars,
and then doing the completion itself.  Therefore, when we use the
"complete" command instead of doing a TAB-completion on GDB, there is a
scenario where we can use the invalid cache of a previous Python command
that was completed before.  For example:

  (gdb) A <TAB>
  (gdb) complete B 
  B value1
  B value10
  B value2
  B value3
  B value4
  B value5
  B value6
  B value7
  B value8
  B value9
  (gdb) B <TAB>
  comp1   comp2   comp4   comp6   comp8
  comp10  comp3   comp5   comp7   comp9

Here, we see that "complete B " gave a different result than "B <TAB>".
The reason for that is because "A <TAB>" was called before, and its
completion results were "value*", so when GDB tried to "complete B " it
wrongly answered with the results for A.  The problem here is using a
wrong cache (A's cache) for completing B.

We tried to come up with a solution that would preserve the caching
mechanism, but it wasn't really possible.  So I decided to completely
remove the cache, and doing the method calling twice for every
completion.  This is not optimal, but I do not think it will impact
users noticeably.

It is worth mentioning another small issue that I found.  The code was
doing:

  wordobj = PyUnicode_Decode (word, sizeof (word), host_charset (), NULL);

which is totally wrong, because using "sizeof" here will lead to always
the same result.  So I changed this to use "strlen".  The testcase also
catches this problem.

Keith kindly expanded the existing testcase to cover the problem
described above, and everything is passing.

OK to apply?

-- 
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..2af22f6 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'" {
+	"cl1-1"
+	"cl1-2"
+	"cl1-3"
+    }
+
+# 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'" {
+	    "cl2-1"
+	    "cl2-10"
+	    "cl2-2"
+	    "cl2-3"
+	    "cl2-4"
+	    "cl2-5"
+	    "cl2-6"
+	    "cl2-7"
+	    "cl2-8"
+	    "cl2-9"
+	}
+}
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()

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] Fix Python completion when using the "complete" command
  2015-03-31 23:10 [PATCH] Fix Python completion when using the "complete" command Sergio Durigan Junior
@ 2015-04-01  2:07 ` Sergio Durigan Junior
  2015-04-07 19:13   ` Sergio Durigan Junior
  0 siblings, 1 reply; 7+ messages in thread
From: Sergio Durigan Junior @ 2015-04-01  2:07 UTC (permalink / raw)
  To: GDB Patches; +Cc: Keith Seitz

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.

-- 
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()

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] Fix Python completion when using the "complete" command
  2015-04-01  2:07 ` Sergio Durigan Junior
@ 2015-04-07 19:13   ` Sergio Durigan Junior
  2015-04-08 18:50     ` Keith Seitz
  0 siblings, 1 reply; 7+ messages in thread
From: Sergio Durigan Junior @ 2015-04-07 19:13 UTC (permalink / raw)
  To: GDB Patches; +Cc: Keith Seitz

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/

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] Fix Python completion when using the "complete" command
  2015-04-07 19:13   ` Sergio Durigan Junior
@ 2015-04-08 18:50     ` Keith Seitz
  2015-04-08 19:59       ` Sergio Durigan Junior
  0 siblings, 1 reply; 7+ messages in thread
From: Keith Seitz @ 2015-04-08 18:50 UTC (permalink / raw)
  To: Sergio Durigan Junior, GDB Patches

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] Fix Python completion when using the "complete" command
  2015-04-08 18:50     ` Keith Seitz
@ 2015-04-08 19:59       ` Sergio Durigan Junior
  2015-04-08 20:39         ` Doug Evans
  0 siblings, 1 reply; 7+ messages in thread
From: Sergio Durigan Junior @ 2015-04-08 19:59 UTC (permalink / raw)
  To: Keith Seitz; +Cc: GDB Patches

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()

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] Fix Python completion when using the "complete" command
  2015-04-08 19:59       ` Sergio Durigan Junior
@ 2015-04-08 20:39         ` Doug Evans
  2015-04-08 22:29           ` Sergio Durigan Junior
  0 siblings, 1 reply; 7+ messages in thread
From: Doug Evans @ 2015-04-08 20:39 UTC (permalink / raw)
  To: Sergio Durigan Junior; +Cc: Keith Seitz, GDB Patches

Sergio Durigan Junior writes:
 > 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.

LGTM.

One comment below.

 > +  resultobj = PyObject_CallMethodObjArgs ((PyObject *) obj, complete_cst,
 > +					  textobj, wordobj, NULL);
 > +  Py_DECREF (textobj);
 > +  Py_DECREF (wordobj);
 > +  if (!resultobj)
 > +    {
 > +      /* Just swallow errors here.  */
 > +      PyErr_Clear ();
 >      }

I realize this is just copying the previous code,
but does swallowing errors here make it hard to debug problems
in the completer function?
Is this something we want to address in a future patch?

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] Fix Python completion when using the "complete" command
  2015-04-08 20:39         ` Doug Evans
@ 2015-04-08 22:29           ` Sergio Durigan Junior
  0 siblings, 0 replies; 7+ messages in thread
From: Sergio Durigan Junior @ 2015-04-08 22:29 UTC (permalink / raw)
  To: Doug Evans; +Cc: Keith Seitz, GDB Patches

On Wednesday, April 08 2015, Doug Evans wrote:

>  > +  resultobj = PyObject_CallMethodObjArgs ((PyObject *) obj, complete_cst,
>  > +					  textobj, wordobj, NULL);
>  > +  Py_DECREF (textobj);
>  > +  Py_DECREF (wordobj);
>  > +  if (!resultobj)
>  > +    {
>  > +      /* Just swallow errors here.  */
>  > +      PyErr_Clear ();
>  >      }
>
> I realize this is just copying the previous code,
> but does swallowing errors here make it hard to debug problems
> in the completer function?
> Is this something we want to address in a future patch?

Thanks for the review, Doug.

You have a valid point: swallowing errors in this part of the code does
make it hard to debug problems in the user-provided completer
functions.  And a small hack in the code proved me that it should be
fairly easy to print the error instead, just like cmdpy_function already
does.  I will see if I can prepare a patch to do that.

Meanwhile, I pushed the current patch upstream.

  <https://sourceware.org/ml/gdb-cvs/2015-04/msg00062.html>
  6d62641c832525382336c1b04731d85cb6c398e7

Thanks,

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2015-04-08 22:29 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-03-31 23:10 [PATCH] Fix Python completion when using the "complete" command Sergio Durigan Junior
2015-04-01  2:07 ` Sergio Durigan Junior
2015-04-07 19:13   ` Sergio Durigan Junior
2015-04-08 18:50     ` Keith Seitz
2015-04-08 19:59       ` Sergio Durigan Junior
2015-04-08 20:39         ` Doug Evans
2015-04-08 22:29           ` Sergio Durigan Junior

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).