llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-lldb Author: John Harrison (ashgti) <details> <summary>Changes</summary> Adding support for cancelling requests. There are two forms of request cancellation. * Preemptively cancelling a request that is in the queue. * Actively cancelling the in progress request as a best effort attempt using `SBDebugger.RequestInterrupt()`. --- Patch is 24.20 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/130169.diff 15 Files Affected: - (added) lldb/test/API/tools/lldb-dap/cancel/Makefile (+3) - (added) lldb/test/API/tools/lldb-dap/cancel/TestDAP_cancel.py (+101) - (added) lldb/test/API/tools/lldb-dap/cancel/main.c (+6) - (modified) lldb/test/API/tools/lldb-dap/launch/TestDAP_launch.py (+1) - (modified) lldb/tools/lldb-dap/CMakeLists.txt (+1) - (modified) lldb/tools/lldb-dap/DAP.cpp (+136-9) - (modified) lldb/tools/lldb-dap/DAP.h (+3) - (added) lldb/tools/lldb-dap/Handler/CancelRequestHandler.cpp (+55) - (modified) lldb/tools/lldb-dap/Handler/InitializeRequestHandler.cpp (+2) - (modified) lldb/tools/lldb-dap/Handler/RequestHandler.h (+10) - (modified) lldb/tools/lldb-dap/Protocol/ProtocolRequests.cpp (+7) - (modified) lldb/tools/lldb-dap/Protocol/ProtocolRequests.h (+20) - (modified) lldb/tools/lldb-dap/Transport.cpp (+28-9) - (modified) lldb/tools/lldb-dap/Transport.h (+2-1) - (modified) lldb/tools/lldb-dap/lldb-dap.cpp (+4-1) ``````````diff diff --git a/lldb/test/API/tools/lldb-dap/cancel/Makefile b/lldb/test/API/tools/lldb-dap/cancel/Makefile new file mode 100644 index 0000000000000..10495940055b6 --- /dev/null +++ b/lldb/test/API/tools/lldb-dap/cancel/Makefile @@ -0,0 +1,3 @@ +C_SOURCES := main.c + +include Makefile.rules diff --git a/lldb/test/API/tools/lldb-dap/cancel/TestDAP_cancel.py b/lldb/test/API/tools/lldb-dap/cancel/TestDAP_cancel.py new file mode 100644 index 0000000000000..f3b2f9fcb7a92 --- /dev/null +++ b/lldb/test/API/tools/lldb-dap/cancel/TestDAP_cancel.py @@ -0,0 +1,101 @@ +""" +Test lldb-dap cancel request +""" + +import time + +from lldbsuite.test.decorators import * +from lldbsuite.test.lldbtest import * +import lldbdap_testcase + + +class TestDAP_launch(lldbdap_testcase.DAPTestCaseBase): + def send_async_req(self, command: str, arguments={}) -> int: + seq = self.dap_server.sequence + self.dap_server.send_packet( + { + "type": "request", + "command": command, + "arguments": arguments, + } + ) + return seq + + def async_blocking_request(self, duration: float) -> int: + """ + Sends an evaluate request that will sleep for the specified duration to + block the request handling thread. + """ + return self.send_async_req( + command="evaluate", + arguments={ + "expression": '`script import time; print("starting sleep", file=lldb.debugger.GetOutputFileHandle()); time.sleep({})'.format( + duration + ), + "context": "repl", + }, + ) + + def async_cancel(self, requestId: int) -> int: + return self.send_async_req(command="cancel", arguments={"requestId": requestId}) + + def test_pending_request(self): + """ + Tests cancelling a pending request. + """ + program = self.getBuildArtifact("a.out") + self.build_and_launch(program, stopOnEntry=True) + self.continue_to_next_stop() + + # Use a relatively short timeout since this is only to ensure the + # following request is queued. + blocking_seq = self.async_blocking_request(duration=1.0) + # Use a longer timeout to ensure we catch if the request was interrupted + # properly. + pending_seq = self.async_blocking_request(duration=self.timeoutval) + cancel_seq = self.async_cancel(requestId=pending_seq) + + blocking_resp = self.dap_server.recv_packet(filter_type=["response"]) + self.assertEqual(blocking_resp["request_seq"], blocking_seq) + self.assertEqual(blocking_resp["command"], "evaluate") + self.assertEqual(blocking_resp["success"], True) + + pending_resp = self.dap_server.recv_packet(filter_type=["response"]) + self.assertEqual(pending_resp["request_seq"], pending_seq) + self.assertEqual(pending_resp["command"], "evaluate") + self.assertEqual(pending_resp["success"], False) + self.assertEqual(pending_resp["message"], "cancelled") + + cancel_resp = self.dap_server.recv_packet(filter_type=["response"]) + self.assertEqual(cancel_resp["request_seq"], cancel_seq) + self.assertEqual(cancel_resp["command"], "cancel") + self.assertEqual(cancel_resp["success"], True) + self.continue_to_exit() + + def test_inflight_request(self): + """ + Tests cancelling an inflight request. + """ + program = self.getBuildArtifact("a.out") + self.build_and_launch(program, stopOnEntry=True) + self.continue_to_next_stop() + + blocking_seq = self.async_blocking_request(duration=self.timeoutval / 2) + # Wait for the sleep to start to cancel the inflight request. + self.collect_stdout( + timeout_secs=self.timeoutval, + pattern="starting sleep", + ) + cancel_seq = self.async_cancel(requestId=blocking_seq) + + blocking_resp = self.dap_server.recv_packet(filter_type=["response"]) + self.assertEqual(blocking_resp["request_seq"], blocking_seq) + self.assertEqual(blocking_resp["command"], "evaluate") + self.assertEqual(blocking_resp["success"], False) + self.assertEqual(blocking_resp["message"], "cancelled") + + cancel_resp = self.dap_server.recv_packet(filter_type=["response"]) + self.assertEqual(cancel_resp["request_seq"], cancel_seq) + self.assertEqual(cancel_resp["command"], "cancel") + self.assertEqual(cancel_resp["success"], True) + self.continue_to_exit() diff --git a/lldb/test/API/tools/lldb-dap/cancel/main.c b/lldb/test/API/tools/lldb-dap/cancel/main.c new file mode 100644 index 0000000000000..ecc0d99ec8db7 --- /dev/null +++ b/lldb/test/API/tools/lldb-dap/cancel/main.c @@ -0,0 +1,6 @@ +#include <stdio.h> + +int main(int argc, char const *argv[]) { + printf("Hello world!\n"); + return 0; +} diff --git a/lldb/test/API/tools/lldb-dap/launch/TestDAP_launch.py b/lldb/test/API/tools/lldb-dap/launch/TestDAP_launch.py index 0c92e5bff07c6..5fb088dc51cbb 100644 --- a/lldb/test/API/tools/lldb-dap/launch/TestDAP_launch.py +++ b/lldb/test/API/tools/lldb-dap/launch/TestDAP_launch.py @@ -27,6 +27,7 @@ def test_default(self): lines = output.splitlines() self.assertIn(program, lines[0], "make sure program path is in first argument") + @skipIfWindows def test_termination(self): """ Tests the correct termination of lldb-dap upon a 'disconnect' diff --git a/lldb/tools/lldb-dap/CMakeLists.txt b/lldb/tools/lldb-dap/CMakeLists.txt index adad75a79fa7a..1e87b28c6d1bd 100644 --- a/lldb/tools/lldb-dap/CMakeLists.txt +++ b/lldb/tools/lldb-dap/CMakeLists.txt @@ -40,6 +40,7 @@ add_lldb_tool(lldb-dap Handler/ResponseHandler.cpp Handler/AttachRequestHandler.cpp Handler/BreakpointLocationsHandler.cpp + Handler/CancelRequestHandler.cpp Handler/CompileUnitsRequestHandler.cpp Handler/CompletionsHandler.cpp Handler/ConfigurationDoneRequestHandler.cpp diff --git a/lldb/tools/lldb-dap/DAP.cpp b/lldb/tools/lldb-dap/DAP.cpp index a1e2187288768..ebdaa4949eb77 100644 --- a/lldb/tools/lldb-dap/DAP.cpp +++ b/lldb/tools/lldb-dap/DAP.cpp @@ -39,9 +39,11 @@ #include <algorithm> #include <cassert> #include <chrono> +#include <condition_variable> #include <cstdarg> #include <cstdio> #include <fstream> +#include <future> #include <memory> #include <mutex> #include <string> @@ -241,6 +243,21 @@ void DAP::SendJSON(const llvm::json::Value &json) { } void DAP::Send(const protocol::Message &message) { + if (auto *resp = std::get_if<protocol::Response>(&message); + resp && debugger.InterruptRequested()) { + // If the debugger was interrupted, convert this response into a 'cancelled' + // response. + protocol::Response cancelled; + cancelled.command = resp->command; + cancelled.request_seq = resp->request_seq; + cancelled.success = false; + cancelled.message = protocol::Response::Message::cancelled; + if (llvm::Error err = transport.Write(cancelled)) + DAP_LOG_ERROR(log, std::move(err), "({1}) write failed: {0}", + transport.GetClientName()); + return; + } + if (llvm::Error err = transport.Write(message)) DAP_LOG_ERROR(log, std::move(err), "({1}) write failed: {0}", transport.GetClientName()); @@ -674,6 +691,10 @@ void DAP::SetTarget(const lldb::SBTarget target) { bool DAP::HandleObject(const protocol::Message &M) { if (const auto *req = std::get_if<protocol::Request>(&M)) { + // Clear interrupt marker prior to handling the next request. + if (debugger.InterruptRequested()) + debugger.CancelInterruptRequest(); + auto handler_pos = request_handlers.find(req->command); if (handler_pos != request_handlers.end()) { (*handler_pos->second)(*req); @@ -778,28 +799,134 @@ llvm::Error DAP::Disconnect(bool terminateDebuggee) { return ToError(error); } +template <typename T> +static std::optional<T> getArgumentsIfRequest(const protocol::Message &pm, + llvm::StringLiteral command) { + auto *const req = std::get_if<protocol::Request>(&pm); + if (!req || req->command != command) + return std::nullopt; + + T args; + llvm::json::Path::Root root; + if (!fromJSON(req->arguments, args, root)) { + return std::nullopt; + } + + return std::move(args); +} + llvm::Error DAP::Loop() { - auto cleanup = llvm::make_scope_exit([this]() { + std::deque<protocol::Message> queue; + std::condition_variable queue_cv; + std::mutex queue_mutex; + std::future<llvm::Error> queue_reader = std::async([&]() -> llvm::Error { + llvm::set_thread_name(transport.GetClientName() + ".transport_handler"); + auto cleanup = llvm::make_scope_exit([&]() { + // Ensure we're marked as disconnecting when the reader exits. + disconnecting = true; + queue_cv.notify_all(); + }); + + while (!disconnecting) { + llvm::Expected<std::optional<protocol::Message>> next = + transport.Read(std::chrono::seconds(1)); + bool timeout = false; + if (llvm::Error Err = llvm::handleErrors( + next.takeError(), + [&](std::unique_ptr<llvm::StringError> Err) -> llvm::Error { + if (Err->convertToErrorCode() == std::errc::timed_out) { + timeout = true; + return llvm::Error::success(); + } + return llvm::Error(std::move(Err)); + })) + return Err; + + // If the read timed out, continue to check if we should disconnect. + if (timeout) + continue; + + // nullopt is returned on EOF. + if (!*next) + break; + + { + std::lock_guard<std::mutex> lock(queue_mutex); + + // If a cancel is requested for the active request, make a best + // effort attempt to interrupt. + if (const auto cancel_args = + getArgumentsIfRequest<protocol::CancelArguments>(**next, + "cancel"); + cancel_args && active_seq == cancel_args->requestId) { + DAP_LOG(log, "({0}) interrupting inflight request {1}", + transport.GetClientName(), active_seq); + debugger.RequestInterrupt(); + debugger.GetCommandInterpreter().InterruptCommand(); + } + + queue.push_back(std::move(**next)); + } + queue_cv.notify_one(); + } + + return llvm::Error::success(); + }); + + auto cleanup = llvm::make_scope_exit([&]() { out.Stop(); err.Stop(); StopEventHandlers(); }); + while (!disconnecting) { - llvm::Expected<std::optional<protocol::Message>> next = transport.Read(); - if (!next) - return next.takeError(); + protocol::Message next; + { + std::unique_lock<std::mutex> lock(queue_mutex); + queue_cv.wait(lock, [&] { return disconnecting || !queue.empty(); }); - // nullopt on EOF - if (!*next) - break; + if (queue.empty()) + break; + + next = queue.front(); + queue.pop_front(); + + if (protocol::Request *req = std::get_if<protocol::Request>(&next)) { + active_seq = req->seq; + + // Check if we should preempt this request from a queued cancel. + bool cancelled = false; + for (const auto &message : queue) { + if (const auto args = + getArgumentsIfRequest<protocol::CancelArguments>(message, + "cancel"); + args && args->requestId == req->seq) { + cancelled = true; + break; + } + } - if (!HandleObject(**next)) { + // Preempt the request and immeidately respond with cancelled. + if (cancelled) { + protocol::Response response; + response.request_seq = req->seq; + response.command = req->command; + response.success = false; + response.message = protocol::Response::Message::cancelled; + Send(response); + continue; + } + } else + active_seq = 0; + } + + if (!HandleObject(next)) { return llvm::createStringError(llvm::inconvertibleErrorCode(), "unhandled packet"); } } - return llvm::Error::success(); + return queue_reader.get(); } lldb::SBError DAP::WaitForProcessToStop(uint32_t seconds) { diff --git a/lldb/tools/lldb-dap/DAP.h b/lldb/tools/lldb-dap/DAP.h index 4c57f9fef3d89..17c5ec2bfee9f 100644 --- a/lldb/tools/lldb-dap/DAP.h +++ b/lldb/tools/lldb-dap/DAP.h @@ -395,6 +395,9 @@ struct DAP { InstructionBreakpoint *GetInstructionBreakpoint(const lldb::break_id_t bp_id); InstructionBreakpoint *GetInstructionBPFromStopReason(lldb::SBThread &thread); + +private: + std::atomic<int64_t> active_seq; }; } // namespace lldb_dap diff --git a/lldb/tools/lldb-dap/Handler/CancelRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/CancelRequestHandler.cpp new file mode 100644 index 0000000000000..e6746316e6e74 --- /dev/null +++ b/lldb/tools/lldb-dap/Handler/CancelRequestHandler.cpp @@ -0,0 +1,55 @@ +//===-- CancelRequestHandler.cpp ------------------------------------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#include "Handler/RequestHandler.h" +#include "Protocol/ProtocolRequests.h" +#include "llvm/Support/Error.h" + +using namespace lldb_dap; +using namespace lldb_dap::protocol; + +namespace lldb_dap { + +/// The `cancel` request is used by the client in two situations: +/// +/// - to indicate that it is no longer interested in the result produced by a +/// specific request issued earlier +/// - to cancel a progress sequence. +/// +/// Clients should only call this request if the corresponding capability +/// `supportsCancelRequest` is true. +/// +/// This request has a hint characteristic: a debug adapter can only be +/// expected to make a 'best effort' in honoring this request but there are no +/// guarantees. +/// +/// 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. +/// +/// The request that got cancelled still needs to send a response back. +/// This can either be a normal result (`success` attribute true) or an error +/// response (`success` attribute false and the `message` set to `cancelled`). +/// +/// Returning partial results from a cancelled request is possible but please +/// note that a client has no generic way for detecting that a response is +/// partial or not. +/// +/// The progress that got cancelled still needs to send a `progressEnd` event +/// back. +/// +/// A client should not assume that progress just got cancelled after sending +/// the `cancel` request. +llvm::Expected<CancelResponseBody> +CancelRequestHandler::Run(const CancelArguments &arguments) const { + // Cancel support is built into the DAP::Loop handler for detecting + // cancellations of pending or inflight requests. + return CancelResponseBody(); +} + +} // namespace lldb_dap diff --git a/lldb/tools/lldb-dap/Handler/InitializeRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/InitializeRequestHandler.cpp index 3262b70042a0e..4373c59798b75 100644 --- a/lldb/tools/lldb-dap/Handler/InitializeRequestHandler.cpp +++ b/lldb/tools/lldb-dap/Handler/InitializeRequestHandler.cpp @@ -463,6 +463,8 @@ void InitializeRequestHandler::operator()( body.try_emplace("supportsDataBreakpoints", true); // The debug adapter supports the `readMemory` request. body.try_emplace("supportsReadMemoryRequest", true); + // The debug adapter supports the `cancel` request. + body.try_emplace("supportsCancelRequest", true); // Put in non-DAP specification lldb specific information. llvm::json::Object lldb_json; diff --git a/lldb/tools/lldb-dap/Handler/RequestHandler.h b/lldb/tools/lldb-dap/Handler/RequestHandler.h index c9bcf15933c33..32c3199f4904b 100644 --- a/lldb/tools/lldb-dap/Handler/RequestHandler.h +++ b/lldb/tools/lldb-dap/Handler/RequestHandler.h @@ -381,6 +381,16 @@ class ReadMemoryRequestHandler : public LegacyRequestHandler { void operator()(const llvm::json::Object &request) const override; }; +class CancelRequestHandler + : public RequestHandler<protocol::CancelArguments, + protocol::CancelResponseBody> { +public: + using RequestHandler::RequestHandler; + static llvm::StringLiteral getCommand() { return "cancel"; } + llvm::Expected<protocol::CancelResponseBody> + Run(const protocol::CancelArguments &args) const override; +}; + /// A request used in testing to get the details on all breakpoints that are /// currently set in the target. This helps us to test "setBreakpoints" and /// "setFunctionBreakpoints" requests to verify we have the correct set of diff --git a/lldb/tools/lldb-dap/Protocol/ProtocolRequests.cpp b/lldb/tools/lldb-dap/Protocol/ProtocolRequests.cpp index 5cc5429227439..9b36c8329b0fc 100644 --- a/lldb/tools/lldb-dap/Protocol/ProtocolRequests.cpp +++ b/lldb/tools/lldb-dap/Protocol/ProtocolRequests.cpp @@ -15,6 +15,13 @@ using namespace llvm; namespace lldb_dap::protocol { +bool fromJSON(const llvm::json::Value &Params, CancelArguments &CA, + llvm::json::Path P) { + llvm::json::ObjectMapper O(Params, P); + return O && O.mapOptional("requestId", CA.requestId) && + O.mapOptional("progressId", CA.progressId); +} + bool fromJSON(const json::Value &Params, DisconnectArguments &DA, json::Path P) { json::ObjectMapper O(Params, P); diff --git a/lldb/tools/lldb-dap/Protocol/ProtocolRequests.h b/lldb/tools/lldb-dap/Protocol/ProtocolRequests.h index 5dc4a589178d2..8acdcd322f526 100644 --- a/lldb/tools/lldb-dap/Protocol/ProtocolRequests.h +++ b/lldb/tools/lldb-dap/Protocol/ProtocolRequests.h @@ -29,6 +29,26 @@ namespace lldb_dap::protocol { +/// Arguments for `cancel` request. +struct CancelArguments { + /// The ID (attribute `seq`) of the request to cancel. If missing no request + /// is cancelled. + /// + /// Both a `requestId` and a `progressId` can be specified in one request. + std::optional<int64_t> requestId; + + /// The ID (attribute `progressId`) of the progress to cancel. If missing no + /// progress is cancelled. + /// + /// Both a `requestId` and a `progressId` can be specified in one request. + std::optional<int64_t> progressId; +}; +bool fromJSON(const llvm::json::Value &, CancelArguments &, llvm::json::Path); + +/// Response to `cancel` request. This is just an acknowledgement, so no body +/// field is required. +using CancelResponseBody = VoidResponse; + /// Arguments for `disconnect` request. struct DisconnectArguments { /// A value of true indicates that this `disconnect` request is part of a diff --git a/lldb/tools/lldb-dap/Transport.cpp b/lldb/tools/lldb-dap/Transport.cpp index 4500e7cf909ba..2241ea586c19a 100644 --- a/lldb/tools/lldb-dap/Transport.cpp +++ b/lldb/tools/lldb-dap/Transport.cpp @@ -10,6 +10,7 @@ #include "DAPLog.h" #include "Protocol/ProtocolBase.h" #include "lldb/Utility/IOObject.h" +#include "lldb/Utility/SelectHelper.h" #include "lldb/Utility/Status.h" #include "lldb/lldb-forward.h" #include "llvm/ADT/StringExtras.h" @@ -27,23 +28,39 @@ using namespace lldb_dap::protocol; /// ReadFull attempts to read the specified number of bytes. If EOF is /// encountered, an empty string is returned. -static Expected<std::string> ReadFull(IOObject &descriptor, size_t length) { +static Expected<std::string> +ReadFull(IOObject &descriptor, size_t length, + const std::chrono::microseconds &timeout) { + if (!descriptor.IsValid()) + return createStringError("transport output is closed"); + +#ifndef _WIN32 + // FIXME: SelectHelper does not work with NativeFile on Win32. + SelectHelper sh; + sh.SetTimeout(timeout); + sh.FDSetRead(descriptor.GetWaitableHandle()); + Status status = sh.Select(); + if (status.Fail()) + return status.takeError(); +#endif + std::string data; data.resize(length); - auto status = descript... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/130169 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
