From patchwork Mon Dec 11 16:02:23 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Tom Tromey X-Patchwork-Id: 81917 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 59E95385701B for ; Mon, 11 Dec 2023 16:03:08 +0000 (GMT) X-Original-To: gdb-patches@sourceware.org Delivered-To: gdb-patches@sourceware.org Received: from mail-il1-x131.google.com (mail-il1-x131.google.com [IPv6:2607:f8b0:4864:20::131]) by sourceware.org (Postfix) with ESMTPS id 583BC38582AD for ; Mon, 11 Dec 2023 16:02:25 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 583BC38582AD Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=adacore.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=adacore.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 583BC38582AD Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2607:f8b0:4864:20::131 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1702310547; cv=none; b=lcRDRe3zSWqbDoj4EbL3LEL8FSOVBm55WmWQn1MI95PRhSihV7VyaHWbxBUgnI8qTXMtPzm3fb0nMAqwRe1dqW05Tp24poO305wzzOghFRi42X5G32vMGbtupWEJ1y2mJDmIQAHto7PkrIcyInZhHVBN6j+Gafwjazwk/H+6ozk= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1702310547; c=relaxed/simple; bh=FtTNIDZpSKPv+FfpAd8BmnqUPzOdP8ZEanBqI9ld65U=; h=DKIM-Signature:From:Date:Subject:MIME-Version:Message-Id:To; b=peteqJ8Ntf0Sh2hfoScyepThj3+HyJPF5usH626PJFk4ETMl1vbjjccRCv6UKNLagHuT+BBaN3jMqpE7FZPTsYXyHZ9UZeDjrDDzKSvYPF67v61gv3bwoBqlyBpOakFRG9lDqsGURYjCFq0lkpj+GJaDNMTfAsFAAzFRBE45500= ARC-Authentication-Results: i=1; server2.sourceware.org Received: by mail-il1-x131.google.com with SMTP id e9e14a558f8ab-35d3b220b64so18604445ab.2 for ; Mon, 11 Dec 2023 08:02:25 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=adacore.com; s=google; t=1702310544; x=1702915344; darn=sourceware.org; h=cc:to:in-reply-to:references:message-id:content-transfer-encoding :mime-version:subject:date:from:from:to:cc:subject:date:message-id :reply-to; bh=ksxN0tKg72qo/xSongPZd5qAw/nyO3t6aPk3JO/T3wY=; b=MDY7k+UFNmlBqiy7f1AHB1bCPiNpMHttFwtpKzGU2LqRu84ESo9exRA2R80UqSMITC yzmbjM9DByx1DH6oNEPsYhp+Zvvq7e+CjrJcy96kixxKwjtonTHexvEFVj1AtrV/RfPc vNg7MQSEkiIwIQggO5nvlWHzC08oUZmfL127DovKHTQmSwcngpoiDFKWBSnx9YYDKvds Oorq0LMX/72DTbueeKwrbckgFcGl1xWpg+IUw6maAwUCTOH87KfjHltT5+DbG3iCb9mj vQpXBGx1ULpNPDqkil76At55ZkwaSoHl4zrSDbLo3DDeV6ZmqKbAa5B/LKCPt8UKzBqG mhQQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1702310544; x=1702915344; h=cc:to:in-reply-to:references:message-id:content-transfer-encoding :mime-version:subject:date:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=ksxN0tKg72qo/xSongPZd5qAw/nyO3t6aPk3JO/T3wY=; b=c9dqGthFaoiChzZ0MWHFBVmmgrpoXLnQNAjcuPuVX8mJ/WXAVF4GYBppi8Os5nLNKD o7o/LN84t3J0bn+iRwiUlEhZKQfmh5KaqLby4cqmk90HZmuy3yXPXDCHhN7/L8m05P2V tCqayGrdfau5yjBU+iGAGlTa1ovB/BKQFlbBcM1WKNeykEMyTQyojqrnv1axaeFDGP8m 5Kd69AgLtViWy5zYU4a+v4n+KwKdqZbEmUPuS5J9yBYyVcCIZxsEkDZVd+3XGWTm3hHl LjhcoiTDzwVx/u1GgAdOeV5ozeK1JmWR95VQA7DwEuQMrAv4Uzni+w164qTh627jQRkI NrBQ== X-Gm-Message-State: AOJu0YxREYzUVmexJBce0gkEATx/8uARMkf4AlI/fJxXzwqoh9PZTs60 DtslbCZMBVXfUXpB+U7iebiBpJLsEMIxUPzjVzQ= X-Google-Smtp-Source: AGHT+IE2VdyO2JqcZlBEVZ/HnD31qr6NFTsuQOYGL6PgPKtmMaEgpO30gI4OPTLOdT8p0/ZCluhQGw== X-Received: by 2002:a05:6e02:12e6:b0:35d:4e2f:aacc with SMTP id l6-20020a056e0212e600b0035d4e2faaccmr6239223iln.22.1702310544472; Mon, 11 Dec 2023 08:02:24 -0800 (PST) Received: from localhost.localdomain (71-211-161-25.hlrn.qwest.net. [71.211.161.25]) by smtp.gmail.com with ESMTPSA id h25-20020a02cd39000000b0042acf934cbasm1879113jaq.72.2023.12.11.08.02.23 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 11 Dec 2023 08:02:24 -0800 (PST) From: Tom Tromey Date: Mon, 11 Dec 2023 09:02:23 -0700 Subject: [PATCH v2 6/6] Implement DAP cancellation MIME-Version: 1.0 Message-Id: <20231211-dap-cancel-v2-6-db7b52cf0329@adacore.com> References: <20231211-dap-cancel-v2-0-db7b52cf0329@adacore.com> In-Reply-To: <20231211-dap-cancel-v2-0-db7b52cf0329@adacore.com> To: gdb-patches@sourceware.org Cc: Eli Zaretskii , =?utf-8?q?K=C3=A9vin_Le_Gouguec?= X-Mailer: b4 0.12.4 X-Spam-Status: No, score=-9.7 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, RCVD_IN_ABUSEAT, RCVD_IN_DNSWL_NONE, RCVD_IN_XBL, SPF_HELO_NONE, SPF_PASS, 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 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 Approved-By: Eli Zaretskii Reviewed-By: Kévin Le Gouguec --- gdb/NEWS | 2 + gdb/doc/gdb.texinfo | 16 +++++++ gdb/python/lib/gdb/dap/server.py | 91 ++++++++++++++++++++++++++++++++++++++-- gdb/testsuite/gdb.dap/pause.exp | 71 +++++++++++++++++++++++++++++++ 4 files changed, 177 insertions(+), 3 deletions(-) diff --git a/gdb/NEWS b/gdb/NEWS index 900ac47ada9..3e804fb1e53 100644 --- a/gdb/NEWS +++ b/gdb/NEWS @@ -82,6 +82,8 @@ show remote thread-options-packet ** GDB now emits the "process" event. + ** GDB now supports the "cancel" request. + * New remote packets New stop reason: clone diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo index 58685a77fd4..6e4adf512ee 100644 --- a/gdb/doc/gdb.texinfo +++ b/gdb/doc/gdb.texinfo @@ -39595,6 +39595,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 . 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