[PATCHv2] gdb/python: handle completion returning a non-sequence
Checks
Context |
Check |
Description |
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 |
success
|
Testing passed
|
linaro-tcwg-bot/tcwg_gdb_build--master-arm |
success
|
Testing passed
|
linaro-tcwg-bot/tcwg_gdb_check--master-arm |
success
|
Testing passed
|
linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 |
success
|
Testing passed
|
Commit Message
Thanks for the feedback.
In V2:
- Instead of calling 'PyErr_Clear ();' to ignore the error, I'm now
calling 'PySequence_Check (resultobj.get ())' to specifically
ignore non-sequence like things. I think this is a better choice,
and should have done this in V1,
- I'm no longer ignoring the error. The Python completion API
already ignores many errors/exceptions, so addressing Tom's
feedback would involve a bigger change, which can come later,
- As I no longer want to take a position on ignoring vs announcing
errors, I am no longer adding a test for what happens if the
completion code raises an exception, as a consequence, I'm not
going to change the docs as Paul suggested. If I change the docs
to explicitly state that exceptions are ignored and mean no
completions, then this is counter to Tom's feedback that
exceptions should be displayed. As both of these issues are not
the main focus of this patch, I'm just going to ignore them for
now,
- Commit message is updated to reflect narrower focus of the patch.
Thanks,
Andrew
---
GDB's Python API documentation for gdb.Command.complete() says:
The 'complete' method can return several values:
* If the return value is a sequence, the contents of the
sequence are used as the completions. It is up to 'complete'
to ensure that the contents actually do complete the word. A
zero-length sequence is allowed, it means that there were no
completions available. Only string elements of the sequence
are used; other elements in the sequence are ignored.
* If the return value is one of the 'COMPLETE_' constants
defined below, then the corresponding GDB-internal completion
function is invoked, and its result is used.
* All other results are treated as though there were no
available completions.
So, returning a non-sequence, and non-integer from a complete method
should be fine; it should just be treated as though there are no
completions.
However, if I write a complete method that returns None, I see this
behaviour:
(gdb) complete completefilenone x
Python Exception <class 'TypeError'>: 'NoneType' object is not iterable
warning: internal error: Unhandled Python exception
(gdb)
Which is caused because we currently assume that anything that is not
an integer must be iterable, and we call PyObject_GetIter on it. When
this call fails a Python exception is set, but instead of
clearing (and therefore ignoring) this exception as we do everywhere
else in the Python completion code, we instead just return with the
exception set.
In this commit I add a PySequence_Check call. If this call returns
false (and we've already checked the integer case) then we can assume
there are no completion results.
I've added a test which checks returning a non-sequence.
---
gdb/python/py-cmd.c | 2 +-
gdb/testsuite/gdb.python/py-completion.exp | 5 +++++
gdb/testsuite/gdb.python/py-completion.py | 12 ++++++++++++
3 files changed, 18 insertions(+), 1 deletion(-)
base-commit: 70fd94b244573225cb04ae41101d495def78b9e6
Comments
Tom Tromey <tom@tromey.com> writes:
>>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:
>
> Thanks for doing this.
>
> Andrew> - I'm no longer ignoring the error. The Python completion API
> Andrew> already ignores many errors/exceptions, so addressing Tom's
> Andrew> feedback would involve a bigger change, which can come later,
>
> It even does so incorrectly :(
> cmdpy_completer_helper does:
>
> gdbpy_ref<> textobj (PyUnicode_Decode (text, strlen (text), host_charset (),
> NULL));
> if (textobj == NULL)
> error (_("Could not convert argument to Python string."));
>
> ...this leaves the Python error set.
>
> There's at least one other instance of this in the function.
>
> Similarly in cmdpy_completer:
>
> gdbpy_ref<> iter (PyObject_GetIter (resultobj.get ()));
>
> if (iter == NULL)
> return;
>
> If this fails, the error remains set here as well.
>
> Anyway your patch itself seems fine.
> Approved-By: Tom Tromey <tom@tromey.com>
I pushed this patch. I'll take a look at improving the error handling
in this area.
Thanks,
Andrew
@@ -290,7 +290,7 @@ cmdpy_completer (struct cmd_list_element *command,
else if (value >= 0 && value < (long) N_COMPLETERS)
completers[value].completer (command, tracker, text, word);
}
- else
+ else if (PySequence_Check (resultobj.get ()))
{
gdbpy_ref<> iter (PyObject_GetIter (resultobj.get ()));
@@ -46,6 +46,11 @@ if { [readline_is_used] && ![is_remote host] } {
# Just discarding whatever we typed.
gdb_test " " ".*" "discard #[incr discard]"
+ # This should offer no suggestions - the complete() methods
+ # returns something that is neither an integer, or a sequence.
+ gdb_test_no_output "complete completefilenone ${testdir_complete}" \
+ "no suggestions given"
+
# This is the problematic one.
send_gdb "completefilemethod ${testdir_complete}\t"
gdb_test_multiple "" "completefilemethod completion" {
@@ -28,6 +28,17 @@ class CompleteFileInit(gdb.Command):
raise gdb.GdbError("not implemented")
+class CompleteFileNone(gdb.Command):
+ def __init__(self):
+ gdb.Command.__init__(self, "completefilenone", gdb.COMMAND_USER)
+
+ def invoke(self, argument, from_tty):
+ raise gdb.GdbError("not implemented")
+
+ def complete(self, text, word):
+ return None
+
+
class CompleteFileMethod(gdb.Command):
def __init__(self):
gdb.Command.__init__(self, "completefilemethod", gdb.COMMAND_USER)
@@ -203,6 +214,7 @@ class CompleteLimit7(gdb.Command):
CompleteFileInit()
+CompleteFileNone()
CompleteFileMethod()
CompleteFileCommandCond()
CompleteLimit1()