gdb/python: display errors from command completion

This commit makes the gdb.Command.complete methods more verbose when
it comes to error handling.

Previous to this commit if any commands implemented in Python
implemented the complete method, and if there were any errors
encountered when calling that complete method, then GDB would silently
hide the error and continue as if there were no completions.

This makes is difficult to debug any errors encountered when writing
completion methods, and encourages the idea that Python extensions can
be broken, and GDB will just silently work around them.

I don't think this is a good idea.  GDB should encourage extensions to
be written correctly, and robustly, and one way in which GDB can (I
think) support this, is by pointing out when an extension goes wrong.

In this commit I've gone through the Python command completion code,
and added calls to gdbpy_print_stack() or gdbpy_print_stack_or_quit()
in places where we were either clearing the Python error, or, in some
cases, just not handling the error at all.

One thing I have not changed is in cmdpy_completer (py-cmd.c) where we
process the list of completions returned from the Command.complete
method; this routine includes a call to gdbpy_is_string to check a
possible completion is a string, if not the completion is ignored.

I was tempted to remove this check, attempt to complete each result to
a string, and display an error if the conversion fails.  After all,
returning anything but a string is surely a mistake by the extension
author.

However, the docs clearly say that only strings within the returned
list will be considered as completions.  Anything else is ignored.  As
such, and to avoid (what I think is pretty unlikely) breakage of
existing code, I've retained the gdbpy_is_string check.

After the gdbpy_is_string check we call python_string_to_host_string,
if this call fails then I do now print the error, where before we
ignored the error.  I think this is OK; if GDB thinks something is a
string, but still can't convert it to a string, then I think it's OK
to display the error in that case.

Another case which I was a little unsure about was in
cmdpy_completer_helper, and the call to PyObject_CallMethodObjArgs,
which is when we actually call Command.complete.  Previously, if this
call resulted in an exception then we would ignore this and just
pretend there were no completions.

Of all the changes, this is possibly the one with the biggest
potential for breaking existing scripts, but also, is, I think, the
most useful change.  If the user code is wrong in some way, such that
an exception is raised, then previously the user would have no obvious
feedback about this breakage.  Now GDB will print the exception for
them, making it, I think, much easier to debug their extension.  But,
if there is user code in the wild that relies on raising an exception
as a means to indicate there are no completions .... well, that code
is going to break after this commit.  I think we can live with this
though, the exceptions means no completions thing was never documented
behaviour.

I also added a new error() call if the PyObject_CallMethodObjArgs call
raises an exception.  This causes the completion mechanism within GDB
to stop.  Within GDB the completion code is called twice, the first
time to compute the work break characters, and then a second time to
compute the actual completions.

If PyObject_CallMethodObjArgs raises an exception when computing the
word break character, and we print it by calling
gdbpy_print_stack_or_quit(), but then carry on as if
PyObject_CallMethodObjArgs had returns no completions, GDB will
call the Python completion code again, which results in another call
to PyObject_CallMethodObjArgs, which might raise the same exception
again.  This results in the Python exception being printed twice.

By throwing a C++ exception after the failed
PyObject_CallMethodObjArgs call, the completion mechanism is aborted,
and no completions are offered.  But importantly, the Python exception
is only printed once.  I think this gives a much better user
experience.

I've added some tests to cover this case, as I think this is the most
likely case that a user will run into.

Approved-By: Tom Tromey <tom@tromey.com>

3 files changed, 67 insertions, 24 deletions

diff --git a/gdb/python/py-cmd.c b/gdb/python/py-cmd.c
index d3845fc..7143c1c 100644
--- a/gdb/python/py-cmd.c
+++ b/gdb/python/py-cmd.c
@@ -183,7 +183,10 @@ cmdpy_completer_helper (struct cmd_list_element *command,
   gdbpy_ref<> textobj (PyUnicode_Decode (text, strlen (text), host_charset (),
 					 NULL));
   if (textobj == NULL)
-    error (_("Could not convert argument to Python string."));
+    {
+      gdbpy_print_stack ();
+      error (_("Could not convert argument to Python string."));
+    }
 
   gdbpy_ref<> wordobj;
   if (word == NULL)
@@ -196,17 +199,22 @@ cmdpy_completer_helper (struct cmd_list_element *command,
       wordobj.reset (PyUnicode_Decode (word, strlen (word), host_charset (),
 				       NULL));
       if (wordobj == NULL)
-	error (_("Could not convert argument to Python string."));
+	{
+	  gdbpy_print_stack ();
+	  error (_("Could not convert argument to Python string."));
+	}
     }
 
   gdbpy_ref<> resultobj (PyObject_CallMethodObjArgs ((PyObject *) obj,
 						     complete_cst,
 						     textobj.get (),
 						     wordobj.get (), NULL));
-  if (resultobj == NULL)
+
+  /* Check if an exception was raised by the Command.complete method.  */
+  if (resultobj == nullptr)
     {
-      /* Just swallow errors here.  */
-      PyErr_Clear ();
+      gdbpy_print_stack_or_quit ();
+      error (_("exception raised during Command.complete method"));
     }
 
   return resultobj;
@@ -240,10 +248,7 @@ cmdpy_completer_handle_brkchars (struct cmd_list_element *command,
       long value;
 
       if (!gdb_py_int_as_long (resultobj.get (), &value))
-	{
-	  /* Ignore.  */
-	  PyErr_Clear ();
-	}
+	gdbpy_print_stack ();
       else if (value >= 0 && value < (long) N_COMPLETERS)
 	{
 	  completer_handle_brkchars_ftype *brkchars_fn;
@@ -283,10 +288,7 @@ cmdpy_completer (struct cmd_list_element *command,
       long value;
 
       if (! gdb_py_int_as_long (resultobj.get (), &value))
-	{
-	  /* Ignore.  */
-	  PyErr_Clear ();
-	}
+	gdbpy_print_stack ();
       else if (value >= 0 && value < (long) N_COMPLETERS)
 	completers[value].completer (command, tracker, text, word);
     }
@@ -295,36 +297,36 @@ cmdpy_completer (struct cmd_list_element *command,
       gdbpy_ref<> iter (PyObject_GetIter (resultobj.get ()));
 
       if (iter == NULL)
-	return;
+	{
+	  gdbpy_print_stack ();
+	  return;
+	}
 
-      bool got_matches = false;
       while (true)
 	{
 	  gdbpy_ref<> elt (PyIter_Next (iter.get ()));
 	  if (elt == NULL)
-	    break;
+	    {
+	      if (PyErr_Occurred() != nullptr)
+		gdbpy_print_stack ();
+	      break;
+	    }
 
 	  if (! gdbpy_is_string (elt.get ()))
 	    {
 	      /* Skip problem elements.  */
 	      continue;
 	    }
+
 	  gdb::unique_xmalloc_ptr<char>
 	    item (python_string_to_host_string (elt.get ()));
 	  if (item == NULL)
 	    {
-	      /* Skip problem elements.  */
-	      PyErr_Clear ();
+	      gdbpy_print_stack ();
 	      continue;
 	    }
 	  tracker.add_completion (std::move (item));
-	  got_matches = true;
 	}
-
-      /* If we got some results, ignore problems.  Otherwise, report
-	 the problem.  */
-      if (got_matches && PyErr_Occurred ())
-	PyErr_Clear ();
     }
 }
 
diff --git a/gdb/testsuite/gdb.python/py-completion.exp b/gdb/testsuite/gdb.python/py-completion.exp
index 89843c9..13ebed0 100644
--- a/gdb/testsuite/gdb.python/py-completion.exp
+++ b/gdb/testsuite/gdb.python/py-completion.exp
@@ -87,6 +87,17 @@ gdb_exit
 gdb_start
 gdb_test_no_output "source ${pyfile}" "load python file again"
 
+# Check that GDB prints exceptions raised by Command.complete calls.
+# This first command raises an exception during the brkchars phase of
+# completion.
+gdb_test "complete complete_brkchar_exception " \
+    "Python Exception <class 'gdb\\.GdbError'>: brkchars exception"
+
+# In this test the brkchars phase of completion is fine, but an
+# exception is raised during the actual completion phase.
+gdb_test "complete complete_raise_exception " \
+    "Python Exception <class 'gdb\\.GdbError'>: completion exception"
+
 gdb_test_sequence "complete completel" \
     "list all completions of 'complete completel'" {
 	"completelimit1"
diff --git a/gdb/testsuite/gdb.python/py-completion.py b/gdb/testsuite/gdb.python/py-completion.py
index 61b6bef..9c3d517 100644
--- a/gdb/testsuite/gdb.python/py-completion.py
+++ b/gdb/testsuite/gdb.python/py-completion.py
@@ -213,6 +213,34 @@ class CompleteLimit7(gdb.Command):
         ]
 
 
+class CompleteBrkCharException(gdb.Command):
+    def __init__(self):
+        gdb.Command.__init__(self, "complete_brkchar_exception", gdb.COMMAND_USER)
+
+    def invoke(self, argument, from_tty):
+        raise gdb.GdbError("not implemented")
+
+    def complete(self, text, word):
+        if word is None:
+            raise gdb.GdbError("brkchars exception")
+        else:
+            raise gdb.GdbError("completion exception")
+
+
+class CompleteRaiseException(gdb.Command):
+    def __init__(self):
+        gdb.Command.__init__(self, "complete_raise_exception", gdb.COMMAND_USER)
+
+    def invoke(self, argument, from_tty):
+        raise gdb.GdbError("not implemented")
+
+    def complete(self, text, word):
+        if word is None:
+            return []
+        else:
+            raise gdb.GdbError("completion exception")
+
+
 CompleteFileInit()
 CompleteFileNone()
 CompleteFileMethod()
@@ -224,3 +252,5 @@ CompleteLimit4()
 CompleteLimit5()
 CompleteLimit6()
 CompleteLimit7()
+CompleteBrkCharException()
+CompleteRaiseException()