[PATCH 0/6] Implement DAP cancellation

[PATCH 0/6] Implement DAP cancellation

[Tom Tromey]

DAP specifies a way to cancel requests.  Previously, I didn't
implement this, because it didn't seem necessary.  However, I realized
later that a 'repl' evaluation can cause the inferior to continue (or
gdb to invoke a long-running CLI or Python command), and a request
like this will not finish -- cancellation is necessary.

This series implements this feature.  I've split it into bite-sized
pieces.

Tested on x86-64 Fedora 38.

---
Tom Tromey (6):
      Clean up handling of DAP not-stopped response
      Move DAP JSON reader to its own thread
      Introduce gdb.interrupt
      Rename a couple of DAP procs in the testsuite
      Catch BaseException in send_gdb_with_response
      Implement DAP cancellation

 gdb/NEWS                          |   2 +
 gdb/doc/gdb.texinfo               |  16 ++++++
 gdb/doc/python.texi               |  11 ++++
 gdb/python/lib/gdb/dap/server.py  | 115 ++++++++++++++++++++++++++++++++++++--
 gdb/python/lib/gdb/dap/startup.py |   4 +-
 gdb/python/python.c               |  19 +++++++
 gdb/testsuite/gdb.dap/pause.exp   |  71 +++++++++++++++++++++++
 gdb/testsuite/lib/dap-support.exp |  10 ++--
 8 files changed, 236 insertions(+), 12 deletions(-)
---
base-commit: 946df73fa09e782b15f75fc82729bff6a00d2554
change-id: 20231201-dap-cancel-2c292d80ba37

Best regards,
-- 
Tom Tromey <tromey@adacore.com>

[PATCH 1/6] Clean up handling of DAP not-stopped response

[Tom Tromey]

This patch introduces a new NotStoppedException type and changes the
DAP implementation of "not stopped" to use it.  I was already touching
some code in this area and I thought this looked a little cleaner.
This also has the advantage that we can now choose not to log the
exception -- previously I was sometimes a bit alarmed when seeing this
in the logs, even though it is harmless.
---
 gdb/python/lib/gdb/dap/server.py | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/gdb/python/lib/gdb/dap/server.py b/gdb/python/lib/gdb/dap/server.py
index ead911d11da..d865dee31a8 100644
--- a/gdb/python/lib/gdb/dap/server.py
+++ b/gdb/python/lib/gdb/dap/server.py
@@ -42,6 +42,12 @@ _commands = {}
 _server = None
 
 
+# A subclass of Exception that is used solely for reporting that a
+# request needs the inferior to be stopped, but it is not stopped.
+class NotStoppedException(Exception):
+    pass
+
+
 class Server:
     """The DAP server class."""
 
@@ -78,6 +84,9 @@ class Server:
             if body is not None:
                 result["body"] = body
             result["success"] = True
+        except NotStoppedException:
+            result["success"] = False
+            result["message"] = "notStopped"
         except BaseException as e:
             log_stack()
             result["success"] = False
@@ -169,7 +178,7 @@ def _check_not_running(func):
         from .events import inferior_running
 
         if inferior_running:
-            raise Exception("notStopped")
+            raise NotStoppedException()
         return func(*args, **kwargs)
 
     return check

-- 
2.41.0

[PATCH 2/6] Move DAP JSON reader to its own thread

[Tom Tromey]

This changes the DAP server to move the JSON reader to a new thread.
This is key to implementing request cancellation, as now requests can
be read while an earlier one is being serviced.
---
 gdb/python/lib/gdb/dap/server.py | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/gdb/python/lib/gdb/dap/server.py b/gdb/python/lib/gdb/dap/server.py
index d865dee31a8..53a0ca7f448 100644
--- a/gdb/python/lib/gdb/dap/server.py
+++ b/gdb/python/lib/gdb/dap/server.py
@@ -60,6 +60,9 @@ class Server:
         # DAP client.  Writing is done in a separate thread to avoid
         # blocking the read loop.
         self.write_queue = DAPQueue()
+        # Reading is also done in a separate thread, and a queue of
+        # requests is kept.
+        self.read_queue = DAPQueue()
         self.done = False
         global _server
         _server = self
@@ -111,6 +114,14 @@ class Server:
         log("WROTE: <<<" + json.dumps(obj) + ">>>")
         self.write_queue.put(obj)
 
+    # This is run in a separate thread and simply reads requests from
+    # the client and puts them into a queue.
+    def _reader_thread(self):
+        while True:
+            cmd = read_json(self.in_stream)
+            log("READ: <<<" + json.dumps(cmd) + ">>>")
+            self.read_queue.put(cmd)
+
     @in_dap_thread
     def main_loop(self):
         """The main loop of the DAP server."""
@@ -118,9 +129,9 @@ class Server:
         # client, and the thread that reads output from the inferior.
         start_thread("output reader", self._read_inferior_output)
         start_json_writer(self.out_stream, self.write_queue)
+        start_thread("JSON reader", self._reader_thread)
         while not self.done:
-            cmd = read_json(self.in_stream)
-            log("READ: <<<" + json.dumps(cmd) + ">>>")
+            cmd = self.read_queue.get()
             result = self._handle_command(cmd)
             self._send_json(result)
             events = self.delayed_events

-- 
2.41.0

[PATCH 3/6] Introduce gdb.interrupt

[Tom Tromey]

DAP cancellation needs a way to interrupt whatever is happening on
gdb's main thread -- whether that is the inferior, a gdb CLI command,
or Python code.

This patch adds a new gdb.interrupt() function for this purpose.  It
simply sets the quit flag and lets gdb do the rest.

No tests in this patch -- instead this is tested via the DAP
cancellation tests.
---
 gdb/NEWS            |  2 ++
 gdb/doc/python.texi | 11 +++++++++++
 gdb/python/python.c | 19 +++++++++++++++++++
 3 files changed, 32 insertions(+)

diff --git a/gdb/NEWS b/gdb/NEWS
index 1073e38dfc6..0c44f0253f6 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -57,6 +57,8 @@ show remote thread-options-packet
      sub-classed to create handlers for objfiles with missing debug
      information.
 
+  ** New function gdb.interrupt(), that interrupts GDB's main thread.
+
 * New remote packets
 
 New stop reason: clone
diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
index b65991bbad0..7d6b7e82edb 100644
--- a/gdb/doc/python.texi
+++ b/gdb/doc/python.texi
@@ -664,6 +664,17 @@ this an easy-to-use drop-in replacement for creating threads that will
 work well in @value{GDBN}.
 @end deftp
 
+@defun gdb.interrupt ()
+This causes the main thread of @value{GDBN} to react as if the user
+had typed a control-C character at the terminal.  That is, if the
+inferior is running, it is interrupted; if a @value{GDBN} command is
+executing, it is stopped; and if a Python command is running,
+@code{KeyboardInterrupt} will be raised.
+
+Unlike most Python APIs in @value{GDBN}, @code{interrupt} is
+thread-safe.
+@end defun
+
 @defun gdb.post_event (event)
 Put @var{event}, a callable object taking no arguments, into
 @value{GDBN}'s internal event queue.  This callable will be invoked at
diff --git a/gdb/python/python.c b/gdb/python/python.c
index 8a36673a3e4..832f374b39f 100644
--- a/gdb/python/python.c
+++ b/gdb/python/python.c
@@ -1118,6 +1118,23 @@ gdbpy_post_event (PyObject *self, PyObject *args)
   Py_RETURN_NONE;
 }
 
+/* Interrupt the current operation on the main thread.  */
+static PyObject *
+gdbpy_interrupt (PyObject *self, PyObject *args)
+{
+  {
+    /* Make sure the interrupt isn't delivered immediately somehow.
+       This probably is not truly needed, but at the same time it
+       seems more clear to be explicit about the intent.  */
+    gdbpy_allow_threads temporarily_exit_python;
+    scoped_disable_cooperative_sigint_handling no_python_sigint;
+
+    set_quit_flag ();
+  }
+
+  Py_RETURN_NONE;
+}
+
 \f
 
 /* This is the extension_language_ops.before_prompt "method".  */
@@ -2678,6 +2695,8 @@ Parse String as an expression, evaluate it, and return the result as a Value."
 
   { "post_event", gdbpy_post_event, METH_VARARGS,
     "Post an event into gdb's event loop." },
+  { "interrupt", gdbpy_interrupt, METH_NOARGS,
+    "Interrupt gdb's current operation." },
 
   { "target_charset", gdbpy_target_charset, METH_NOARGS,
     "target_charset () -> string.\n\

-- 
2.41.0

[PATCH 4/6] Rename a couple of DAP procs in the testsuite

[Tom Tromey]

This renames a couple of DAP procs in the testsuite, to clarify that
they are now exported.  The cancellation test will need these.
---
 gdb/testsuite/lib/dap-support.exp | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/gdb/testsuite/lib/dap-support.exp b/gdb/testsuite/lib/dap-support.exp
index 82d83d95b9f..b9ac314fee5 100644
--- a/gdb/testsuite/lib/dap-support.exp
+++ b/gdb/testsuite/lib/dap-support.exp
@@ -111,7 +111,7 @@ proc _dap_send_ton {obj} {
 # omitted.  The sequence number of the request is automatically added,
 # and this is also the return value.  OBJ is assumed to already be in
 # TON form.
-proc _dap_send_request {command {obj {}}} {
+proc dap_send_request {command {obj {}}} {
     # We can construct this directly as a TON object.
     set result $::dap_seq
     incr ::dap_seq
@@ -186,7 +186,7 @@ proc _dap_read_json {} {
 # response is seen, this leaves the global "last_ton" set to the TON
 # for the response.
 
-proc _dap_read_response {cmd num} {
+proc dap_read_response {cmd num} {
     set result {}
     while 1 {
 	set d [_dap_read_json]
@@ -204,11 +204,11 @@ proc _dap_read_response {cmd num} {
     }
 }
 
-# A wrapper for _dap_send_request and _dap_read_response.  This sends a
+# A wrapper for dap_send_request and dap_read_response.  This sends a
 # request to gdb and returns the response as a dict.
 proc dap_request_and_response {command {obj {}}} {
-    set seq [_dap_send_request $command $obj]
-    return [_dap_read_response $command $seq]
+    set seq [dap_send_request $command $obj]
+    return [dap_read_response $command $seq]
 }
 
 # Like dap_request_and_response, but also checks that the response

-- 
2.41.0

[PATCH 5/6] Catch BaseException in send_gdb_with_response

[Tom Tromey]

Cancellation will generally be seen by the DAP code as a
KeyboardInterrupt.  However, this derives from BaseException and not
Exception, so a small change is needed to send_gdb_with_response, to
forward the exception to the DAP server thread.
---
 gdb/python/lib/gdb/dap/startup.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/gdb/python/lib/gdb/dap/startup.py b/gdb/python/lib/gdb/dap/startup.py
index a16b51f7cf5..1a7ec17e5e3 100644
--- a/gdb/python/lib/gdb/dap/startup.py
+++ b/gdb/python/lib/gdb/dap/startup.py
@@ -172,11 +172,11 @@ def send_gdb_with_response(fn):
         try:
             val = fn()
             result_q.put(val)
-        except Exception as e:
+        except BaseException as e:
             result_q.put(e)
 
     send_gdb(message)
     val = result_q.get()
-    if isinstance(val, Exception):
+    if isinstance(val, BaseException):
         raise val
     return val

-- 
2.41.0

[PATCH 6/6] Implement DAP cancellation

[Tom Tromey]

This implements DAP cancellation.  A new object is introduced that
handles the details of cancellation.  While cancellation is inherently
racy, this code attempts to make it so that gdb doesn't inadvertently
cancel the wrong request.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30472
---
 gdb/doc/gdb.texinfo              | 16 +++++++
 gdb/python/lib/gdb/dap/server.py | 91 ++++++++++++++++++++++++++++++++++++++--
 gdb/testsuite/gdb.dap/pause.exp  | 71 +++++++++++++++++++++++++++++++
 3 files changed, 175 insertions(+), 3 deletions(-)

diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index e4c00143fd1..c7c56f2e967 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -39541,6 +39541,22 @@ to return the bytes of each instruction in an implementation-defined
 format.  @value{GDBN} implements this by sending a string with the
 bytes encoded in hex, like @code{"55a2b900"}.
 
+When the @code{repl} context is used for the @code{evaluate} request,
+@value{GDBN} evaluates the provided expression as a CLI command.
+
+Evaluation in general can cause the inferior to continue execution.
+For example, evaluating the @code{continue} command could do this, as
+could evaluating an expression that involves an inferior function
+call.
+
+@code{repl} evaluation can also cause @value{GDBN} to appear to stop
+responding to requests, for example if a CLI script does a lengthy
+computation.
+
+Evaluations like this can be interrupted using the DAP @code{cancel}
+request.  (In fact, @code{cancel} should work for any request, but it
+is unlikely to be useful for most of them.)
+
 @node JIT Interface
 @chapter JIT Compilation Interface
 @cindex just-in-time compilation
diff --git a/gdb/python/lib/gdb/dap/server.py b/gdb/python/lib/gdb/dap/server.py
index 53a0ca7f448..44dffb1b809 100644
--- a/gdb/python/lib/gdb/dap/server.py
+++ b/gdb/python/lib/gdb/dap/server.py
@@ -14,8 +14,11 @@
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
 import functools
+import gdb
+import heapq
 import inspect
 import json
+import threading
 
 from .io import start_json_writer, read_json
 from .startup import (
@@ -48,6 +51,52 @@ class NotStoppedException(Exception):
     pass
 
 
+# This is used to handle cancellation requests.  It tracks all the
+# needed state, so that we can cancel both requests that are in flight
+# as well as queued requests.
+class CancellationHandler:
+    def __init__(self):
+        # Methods on this class acquire this lock before proceeding.
+        self.lock = threading.Lock()
+        # The request currently being handled, or None.
+        self.in_flight = None
+        self.reqs = []
+
+    def starting(self, req):
+        """Call at the start of the given request.
+
+        Throws the appropriate exception if the request should be
+        immediately cancelled."""
+        with self.lock:
+            self.in_flight = req
+            while len(self.reqs) > 0 and self.reqs[0] <= req:
+                if heapq.heappop(self.reqs) == req:
+                    raise KeyboardInterrupt()
+
+    def done(self, req):
+        """Indicate that the request is done."""
+        with self.lock:
+            self.in_flight = None
+
+    def cancel(self, req):
+        """Call to cancel a request.
+
+        If the request has already finished, this is ignored.
+        If the request is in flight, it is interrupted.
+        If the request has not yet been seen, the cancellation is queued."""
+        with self.lock:
+            if req == self.in_flight:
+                gdb.interrupt()
+            else:
+                # We don't actually ignore the request here, but in
+                # the 'starting' method.  This way we don't have to
+                # track as much state.  Also, this implementation has
+                # the weird property that a request can be cancelled
+                # before it is even sent.  It didn't seem worthwhile
+                # to try to check for this.
+                heapq.heappush(self.reqs, req)
+
+
 class Server:
     """The DAP server class."""
 
@@ -64,6 +113,7 @@ class Server:
         # requests is kept.
         self.read_queue = DAPQueue()
         self.done = False
+        self.canceller = CancellationHandler()
         global _server
         _server = self
 
@@ -71,13 +121,14 @@ class Server:
     # PARAMS is just a dictionary from the JSON.
     @in_dap_thread
     def _handle_command(self, params):
-        # We don't handle 'cancel' for now.
+        req = params["seq"]
         result = {
-            "request_seq": params["seq"],
+            "request_seq": req,
             "type": "response",
             "command": params["command"],
         }
         try:
+            self.canceller.starting(req)
             if "arguments" in params:
                 args = params["arguments"]
             else:
@@ -90,10 +141,15 @@ class Server:
         except NotStoppedException:
             result["success"] = False
             result["message"] = "notStopped"
+        except KeyboardInterrupt:
+            # This can only happen when a request has been canceled.
+            result["success"] = False
+            result["message"] = "cancelled"
         except BaseException as e:
             log_stack()
             result["success"] = False
             result["message"] = str(e)
+        self.canceller.done(req)
         return result
 
     # Read inferior output and sends OutputEvents to the client.  It
@@ -115,11 +171,25 @@ class Server:
         self.write_queue.put(obj)
 
     # This is run in a separate thread and simply reads requests from
-    # the client and puts them into a queue.
+    # the client and puts them into a queue.  A separate thread is
+    # used so that 'cancel' requests can be handled -- the DAP thread
+    # will normally block, waiting for each request to complete.
     def _reader_thread(self):
         while True:
             cmd = read_json(self.in_stream)
             log("READ: <<<" + json.dumps(cmd) + ">>>")
+            # Be extra paranoid about the form here.  If anything is
+            # missing, it will be put in the queue and then an error
+            # issued by ordinary request processing.
+            if (
+                "command" in cmd
+                and cmd["command"] == "cancel"
+                and "arguments" in cmd
+                # gdb does not implement progress, so there's no need
+                # to check for progressId.
+                and "requestId" in cmd["arguments"]
+            ):
+                self.canceller.cancel(cmd["arguments"]["requestId"])
             self.read_queue.put(cmd)
 
     @in_dap_thread
@@ -316,3 +386,18 @@ def disconnect(*, terminateDebuggee: bool = False, **args):
     if terminateDebuggee:
         send_gdb_with_response("kill")
     _server.shutdown()
+
+
+@request("cancel", on_dap_thread=True, expect_stopped=False)
+@capability("supportsCancelRequest")
+def cancel(**args):
+    # If a 'cancel' request can actually be satisfied, it will be
+    # handled specially in the reader thread.  However, in order to
+    # construct a proper response, the request is also added to the
+    # command queue and so ends up here.  Additionally, the spec says:
+    #    The cancel request may return an error if it could not cancel
+    #    an operation but a client should refrain from presenting this
+    #    error to end users.
+    # ... which gdb takes to mean that it is fine for all cancel
+    # requests to report success.
+    return None
diff --git a/gdb/testsuite/gdb.dap/pause.exp b/gdb/testsuite/gdb.dap/pause.exp
index 1b65dcac836..0f2fcbc6b2d 100644
--- a/gdb/testsuite/gdb.dap/pause.exp
+++ b/gdb/testsuite/gdb.dap/pause.exp
@@ -75,4 +75,75 @@ foreach event [lindex $result 1] {
 }
 gdb_assert {$seen == "pass"} "continue event from inferior call"
 
+#
+# Test that a repl evaluation that causes a continue can be canceled.
+#
+
+set cont_id [dap_send_request evaluate \
+		 {o expression [s continue] context [s repl]}]
+dap_wait_for_event_and_check "continued" continued
+
+set cancel_id [dap_send_request cancel \
+		   [format {o requestId [i %d]} $cont_id]]
+
+# The stop event will come before any responses to the requests.
+dap_wait_for_event_and_check "stopped by cancel" stopped
+
+# Now we can wait for the 'continue' request to complete, and then the
+# 'cancel' request.
+dap_read_response evaluate $cont_id
+dap_read_response cancel $cancel_id
+
+#
+# Test that a repl evaluation of a long-running gdb command (that does
+# not continue the inferior) can be canceled.
+#
+
+proc write_file {suffix contents} {
+    global testfile
+
+    set gdbfile [standard_output_file ${testfile}.$suffix]
+    set ofd [open $gdbfile w]
+    puts $ofd $contents
+    close $ofd
+    return $gdbfile
+}
+
+set gdbfile [write_file gdb "set \$x = 0\nwhile 1\nset \$x = \$x + 1\nend"]
+set cont_id [dap_send_request evaluate \
+		 [format {o expression [s "source %s"] context [s repl]} \
+		      $gdbfile]]
+
+# Wait a little to try to ensure the command is running.
+sleep 0.2
+set cancel_id [dap_send_request cancel \
+		   [format {o requestId [i %d]} $cont_id]]
+
+set info [lindex [dap_read_response evaluate $cont_id] 0]
+gdb_assert {[dict get $info success] == "false"} "gdb command failed"
+gdb_assert {[dict get $info message] == "cancelled"} "gdb command canceled"
+
+dap_read_response cancel $cancel_id
+
+#
+# Test that a repl evaluation of a long-running Python command (that
+# does not continue the inferior) can be canceled.
+#
+
+write_file py "while True:\n  pass"
+set cont_id [dap_send_request evaluate \
+		 [format {o expression [s "source %s"] context [s repl]} \
+		      $gdbfile]]
+
+# Wait a little to try to ensure the command is running.
+sleep 0.2
+set cancel_id [dap_send_request cancel \
+		   [format {o requestId [i %d]} $cont_id]]
+
+set info [lindex [dap_read_response evaluate $cont_id] 0]
+gdb_assert {[dict get $info success] == "false"} "python command failed"
+gdb_assert {[dict get $info message] == "cancelled"} "python command canceled"
+
+dap_read_response cancel $cancel_id
+
 dap_shutdown

-- 
2.41.0

Re: [PATCH 3/6] Introduce gdb.interrupt

[Eli Zaretskii]

> From: Tom Tromey <tromey@adacore.com>
> Date: Fri, 01 Dec 2023 08:40:59 -0700
> 
> DAP cancellation needs a way to interrupt whatever is happening on
> gdb's main thread -- whether that is the inferior, a gdb CLI command,
> or Python code.
> 
> This patch adds a new gdb.interrupt() function for this purpose.  It
> simply sets the quit flag and lets gdb do the rest.

Thanks.

> diff --git a/gdb/NEWS b/gdb/NEWS
> index 1073e38dfc6..0c44f0253f6 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -57,6 +57,8 @@ show remote thread-options-packet
>       sub-classed to create handlers for objfiles with missing debug
>       information.
>  
> +  ** New function gdb.interrupt(), that interrupts GDB's main thread.
> +
>  * New remote packets

Do we have to mention the main thread?  Why not say just that it
interrupts GDB as Ctrl-C keypress would?

> +@defun gdb.interrupt ()
> +This causes the main thread of @value{GDBN} to react as if the user
> +had typed a control-C character at the terminal.

Same here: I would avoid talking about the GDB's main thread, as I
think it just muddies the waters, and could really confuse someone who
doesn't have a good mental picture of what threads in general and the
main thread in particular do in GDB.

Reviewed-By: Eli Zaretskii <eliz@gnu.org>

Re: [PATCH 6/6] Implement DAP cancellation

[Eli Zaretskii]

> From: Tom Tromey <tromey@adacore.com>
> Date: Fri, 01 Dec 2023 08:41:02 -0700
> 
> This implements DAP cancellation.  A new object is introduced that
> handles the details of cancellation.  While cancellation is inherently
> racy, this code attempts to make it so that gdb doesn't inadvertently
> cancel the wrong request.
> 
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30472
> ---
>  gdb/doc/gdb.texinfo              | 16 +++++++
>  gdb/python/lib/gdb/dap/server.py | 91 ++++++++++++++++++++++++++++++++++++++--
>  gdb/testsuite/gdb.dap/pause.exp  | 71 +++++++++++++++++++++++++++++++
>  3 files changed, 175 insertions(+), 3 deletions(-)

Thanks, the documentation part is OK.

Reviewed-By: Eli Zaretskii <eliz@gnu.org>

Re: [PATCH 5/6] Catch BaseException in send_gdb_with_response

[Kévin Le Gouguec]

Tom Tromey <tromey@adacore.com> writes:

> Cancellation will generally be seen by the DAP code as a
> KeyboardInterrupt.  However, this derives from BaseException and not
> Exception, so a small change is needed to send_gdb_with_response, to
> forward the exception to the DAP server thread.
> ---
>  gdb/python/lib/gdb/dap/startup.py | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/gdb/python/lib/gdb/dap/startup.py b/gdb/python/lib/gdb/dap/startup.py
> index a16b51f7cf5..1a7ec17e5e3 100644
> --- a/gdb/python/lib/gdb/dap/startup.py
> +++ b/gdb/python/lib/gdb/dap/startup.py
> @@ -172,11 +172,11 @@ def send_gdb_with_response(fn):
>          try:
>              val = fn()
>              result_q.put(val)
> -        except Exception as e:
> +        except BaseException as e:
>              result_q.put(e)
>  
>      send_gdb(message)
>      val = result_q.get()
> -    if isinstance(val, Exception):
> +    if isinstance(val, BaseException):
>          raise val
>      return val

Looking at [the hierarchy], it looks like BaseException will catch more
than we really want?  Should we go for

    except (Exception, KeyboardInterrupt) as e:

    if isinstance(val, (Exception, KeyboardInterrupt)):

?


[the hierarchy]: https://docs.python.org/3/library/exceptions.html#exception-hierarchy

Re: [PATCH 5/6] Catch BaseException in send_gdb_with_response

[Tom Tromey]

Kévin> Looking at [the hierarchy], it looks like BaseException will catch more
Kévin> than we really want?  Should we go for

Kévin>     except (Exception, KeyboardInterrupt) as e:

Kévin>     if isinstance(val, (Exception, KeyboardInterrupt)):

Kévin> ?

Yeah, makes sense.  I've made this change locally.
I also updated the commit message a little to match.

Tom

Re: [PATCH 0/6] Implement DAP cancellation

[Kévin Le Gouguec]

Tom Tromey <tromey@adacore.com> writes:

> DAP specifies a way to cancel requests.  Previously, I didn't
> implement this, because it didn't seem necessary.  However, I realized
> later that a 'repl' evaluation can cause the inferior to continue (or
> gdb to invoke a long-running CLI or Python command), and a request
> like this will not finish -- cancellation is necessary.
>
> This series implements this feature.  I've split it into bite-sized
> pieces.
>
> Tested on x86-64 Fedora 38.
>
> ---
> Tom Tromey (6):
>       Clean up handling of DAP not-stopped response
>       Move DAP JSON reader to its own thread
>       Introduce gdb.interrupt
>       Rename a couple of DAP procs in the testsuite
>       Catch BaseException in send_gdb_with_response
>       Implement DAP cancellation
>
>  gdb/NEWS                          |   2 +
>  gdb/doc/gdb.texinfo               |  16 ++++++
>  gdb/doc/python.texi               |  11 ++++
>  gdb/python/lib/gdb/dap/server.py  | 115 ++++++++++++++++++++++++++++++++++++--
>  gdb/python/lib/gdb/dap/startup.py |   4 +-
>  gdb/python/python.c               |  19 +++++++
>  gdb/testsuite/gdb.dap/pause.exp   |  71 +++++++++++++++++++++++
>  gdb/testsuite/lib/dap-support.exp |  10 ++--
>  8 files changed, 236 insertions(+), 12 deletions(-)
> ---
> base-commit: 946df73fa09e782b15f75fc82729bff6a00d2554
> change-id: 20231201-dap-cancel-2c292d80ba37
>
> Best regards,

Beside my remark re. BaseException, nothing more to add, this looks good
to me.  For the series:

Reviewed-By: Kévin Le Gouguec <legouguec@adacore.com>

Re: [PATCH 6/6] Implement DAP cancellation

[Tom Tromey]

>>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes:

>> From: Tom Tromey <tromey@adacore.com>
>> Date: Fri, 01 Dec 2023 08:41:02 -0700
>> 
>> This implements DAP cancellation.  A new object is introduced that
>> handles the details of cancellation.  While cancellation is inherently
>> racy, this code attempts to make it so that gdb doesn't inadvertently
>> cancel the wrong request.
>> 
>> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30472
>> ---
>> gdb/doc/gdb.texinfo              | 16 +++++++
>> gdb/python/lib/gdb/dap/server.py | 91 ++++++++++++++++++++++++++++++++++++++--
>> gdb/testsuite/gdb.dap/pause.exp  | 71 +++++++++++++++++++++++++++++++
>> 3 files changed, 175 insertions(+), 3 deletions(-)

Eli> Thanks, the documentation part is OK.

Eli> Reviewed-By: Eli Zaretskii <eliz@gnu.org>

Thanks.  I realized I should start adding NEWS entries for DAP changes.

Assuming the earlier one is approved:

+* Debugger Adapter Protocol changes
+
+  ** GDB now emits the "process" event.
+

... then for this cancel patch I'd propose adding:

  ** GDB now supports the "cancel" request.

Tom

Re: [PATCH 6/6] Implement DAP cancellation

[Eli Zaretskii]

> From: Tom Tromey <tromey@adacore.com>
> Cc: Tom Tromey <tromey@adacore.com>,  gdb-patches@sourceware.org
> Date: Mon, 11 Dec 2023 08:05:23 -0700
> 
> >>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> From: Tom Tromey <tromey@adacore.com>
> >> Date: Fri, 01 Dec 2023 08:41:02 -0700
> >> 
> >> This implements DAP cancellation.  A new object is introduced that
> >> handles the details of cancellation.  While cancellation is inherently
> >> racy, this code attempts to make it so that gdb doesn't inadvertently
> >> cancel the wrong request.
> >> 
> >> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30472
> >> ---
> >> gdb/doc/gdb.texinfo              | 16 +++++++
> >> gdb/python/lib/gdb/dap/server.py | 91 ++++++++++++++++++++++++++++++++++++++--
> >> gdb/testsuite/gdb.dap/pause.exp  | 71 +++++++++++++++++++++++++++++++
> >> 3 files changed, 175 insertions(+), 3 deletions(-)
> 
> Eli> Thanks, the documentation part is OK.
> 
> Eli> Reviewed-By: Eli Zaretskii <eliz@gnu.org>
> 
> Thanks.  I realized I should start adding NEWS entries for DAP changes.
> 
> Assuming the earlier one is approved:
> 
> +* Debugger Adapter Protocol changes
> +
> +  ** GDB now emits the "process" event.
> +
> 
> ... then for this cancel patch I'd propose adding:
> 
>   ** GDB now supports the "cancel" request.

This is OK.

Approved-By: Eli Zaretskii <eliz@gnu.org>

Re: [PATCH 3/6] Introduce gdb.interrupt

[Tom Tromey]

>>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes:

>> +  ** New function gdb.interrupt(), that interrupts GDB's main thread.
>> +
>> * New remote packets

Eli> Do we have to mention the main thread?  Why not say just that it
Eli> interrupts GDB as Ctrl-C keypress would?

>> +@defun gdb.interrupt ()
>> +This causes the main thread of @value{GDBN} to react as if the user
>> +had typed a control-C character at the terminal.

Eli> Same here: I would avoid talking about the GDB's main thread, as I
Eli> think it just muddies the waters, and could really confuse someone who
Eli> doesn't have a good mental picture of what threads in general and the
Eli> main thread in particular do in GDB.

I'll rewrite these in v2.

Tom