From patchwork Tue Nov 28 11:04:53 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andrew Burgess X-Patchwork-Id: 80906 Return-Path: X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id B907D384CBAC for ; Tue, 28 Nov 2023 11:05:15 +0000 (GMT) X-Original-To: gdb-patches@sourceware.org Delivered-To: gdb-patches@sourceware.org Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id 7CEB63858033 for ; Tue, 28 Nov 2023 11:04:59 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 7CEB63858033 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 7CEB63858033 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=170.10.129.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1701169501; cv=none; b=Cd4NRW1maz+aPTQS7wAfxZvLZqlqoPfkpEyJsdKsHtIdpPhUedXYTC0lDp2pgXmHU4HCbbJ8yYq3aUTQIW7ndMZ+xpDdpwAO6l06HFIS5YteSGIehbqYuZjV37BIZp8WwPDNZNeENr1vkE+Je1XkXq1CLncwIQQ3VvOa0WYF1DI= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1701169501; c=relaxed/simple; bh=WCl/3m5K83qKwlZ5SJjNOsxD3UWnFmyCGJutmp7f2XE=; h=DKIM-Signature:From:To:Subject:Date:Message-Id:MIME-Version; b=b0cLAWFfOBzmYRRzQ4VWGhD7RL+r5mwpikcwK50cn2xDO2XRt3Td7zL717rNVDEiOHZ4OsqsrAQIa8F1xDEXvW51+fupP2Ah8+FYxgb1q0ifmIBD6xHzdv8HfH32073kxthv4iJ+VGDpITNMwRmw0X8sTtNCgQxwE6ofwn/2Q9s= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1701169499; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=j/OdEPqQLV9aElOaZC0UPKVW9nmkcyk7gzatu2JHIw0=; b=JHc1lgCBvAuLrsFPqgfRkA5OvUt6ia8Bb2JHg4tWTh/oYQ8AZE2OxhlGcayzX/BgtBcUqW a4yUu05OqGWnUA9uDJFFc5wkARQC2c71LhTh6GQDIJKQth5sGqlQ1hs5pHyk0FgSIgdAvy 52pExFE0s14H+NJGEcyH2A2fN7ddpFc= Received: from mail-wr1-f72.google.com (mail-wr1-f72.google.com [209.85.221.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-691-eKSfyCshOyOVczjS4tIjHg-1; Tue, 28 Nov 2023 06:04:58 -0500 X-MC-Unique: eKSfyCshOyOVczjS4tIjHg-1 Received: by mail-wr1-f72.google.com with SMTP id ffacd0b85a97d-332f91f43d0so2166597f8f.1 for ; Tue, 28 Nov 2023 03:04:57 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1701169496; x=1701774296; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=j/OdEPqQLV9aElOaZC0UPKVW9nmkcyk7gzatu2JHIw0=; b=iflKm/xg5ESUVrUQW54oQh/HmQiIGvu+V5d6wLGBUZKPWBP8YdNROtxmYP0VeUXScD PXNgj3aYQ31GmyPSh84drl2uozsBfjhB+4PAoAdgHtDOn1W6kanDZQfAnwqo2Q4QlV+S CsRS3Ha6G2uYKfJ2z7nbUFDlDXyXXmI7+IPvCdUNq7dXqBnAiPbtnr8WQMSUokMnQiuu UXW2gUk7NhFiSajy1QFAKHTq9b125GkRJcBbZG7pKLjVbHs4UYOxANNlL7Be5fkP7EWB Zg/Ii8bFLIRjZb7IEFLDgg5x5oTb+QyiAZBimG23ZcPxIO+t2bYs5gHmolp+CIXxSkqc 057g== X-Gm-Message-State: AOJu0YxKOWCtsGLsoNg9umIt6l3tZftPAm99NRkw8rGbJksQ81fFCWp1 FBuDfngWWTIifcIHgFa68egUCMu8R7nuoxi4nEtzPG/ihHFdGGEtoq6sojPJxwf4nwQVOx7KPyn YMp8uBohshRawsmD9vjLYGPow0TbDIz6pHVkB678N7ZncIvwA1A0VCDvXz/Vk+q/BEn+xVacNTQ hyCUAE1A== X-Received: by 2002:a05:6000:50f:b0:333:83f:599b with SMTP id a15-20020a056000050f00b00333083f599bmr3062929wrf.22.1701169496565; Tue, 28 Nov 2023 03:04:56 -0800 (PST) X-Google-Smtp-Source: AGHT+IF0NvKkzyEl3XP4neleoOGpSnxQ9K/G3lcpdk4+CbSgPoutL2DJmP6IhC61oBEHE105Q0f6cA== X-Received: by 2002:a05:6000:50f:b0:333:83f:599b with SMTP id a15-20020a056000050f00b00333083f599bmr3062906wrf.22.1701169496083; Tue, 28 Nov 2023 03:04:56 -0800 (PST) Received: from localhost (105.226.159.143.dyn.plus.net. [143.159.226.105]) by smtp.gmail.com with ESMTPSA id l13-20020a5d480d000000b00332cb0937f4sm14673523wrq.33.2023.11.28.03.04.55 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 28 Nov 2023 03:04:55 -0800 (PST) From: Andrew Burgess To: gdb-patches@sourceware.org Cc: Andrew Burgess Subject: [PATCH] gdb/python: display errors from command completion Date: Tue, 28 Nov 2023 11:04:53 +0000 Message-Id: <66d012a42add48d890d0504b95c9f3a6daa37042.1701169440.git.aburgess@redhat.com> X-Mailer: git-send-email 2.25.4 MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com X-Spam-Status: No, score=-11.7 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NONE, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.30 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: gdb-patches-bounces+patchwork=sourceware.org@sourceware.org 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. --- gdb/python/py-cmd.c | 50 +++++++++++----------- gdb/testsuite/gdb.python/py-completion.exp | 11 +++++ gdb/testsuite/gdb.python/py-completion.py | 30 +++++++++++++ 3 files changed, 67 insertions(+), 24 deletions(-) base-commit: e5f1ee1832ff9e970833fa5773f46c3e0b93bc04 diff --git a/gdb/python/py-cmd.c b/gdb/python/py-cmd.c index d3845fc7509..7143c1c5f7f 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 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 89843c96f1f..13ebed0c1bf 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 : 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 : 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 61b6beffa25..9c3d5170fbc 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()