From 532ed363291f2a21fb8ee0ed936a6876f7433318 Mon Sep 17 00:00:00 2001 From: Jonas Devlieghere Date: Mon, 24 Feb 2025 16:47:08 -0600 Subject: [PATCH] [lldb-dap] Refactor reverse request response handlers (NFC) This refactors the response handlers for reverse request to follow the same architecture as the request handlers. With only two implementation that might be overkill, but it reduces code duplication and improves error reporting by storing the sequence ID. This PR also fixes an unchecked Expected in the old callback for unknown sequence IDs. --- lldb/tools/lldb-dap/CMakeLists.txt | 1 + lldb/tools/lldb-dap/DAP.cpp | 42 ++++---------- lldb/tools/lldb-dap/DAP.h | 29 +++++++--- .../tools/lldb-dap/Handler/RequestHandler.cpp | 11 +--- .../lldb-dap/Handler/ResponseHandler.cpp | 35 ++++++++++++ lldb/tools/lldb-dap/Handler/ResponseHandler.h | 56 +++++++++++++++++++ 6 files changed, 124 insertions(+), 50 deletions(-) create mode 100644 lldb/tools/lldb-dap/Handler/ResponseHandler.cpp create mode 100644 lldb/tools/lldb-dap/Handler/ResponseHandler.h diff --git a/lldb/tools/lldb-dap/CMakeLists.txt b/lldb/tools/lldb-dap/CMakeLists.txt index 804dd8e4cc2a0..8b3c520ec4360 100644 --- a/lldb/tools/lldb-dap/CMakeLists.txt +++ b/lldb/tools/lldb-dap/CMakeLists.txt @@ -37,6 +37,7 @@ add_lldb_tool(lldb-dap SourceBreakpoint.cpp Watchpoint.cpp + Handler/ResponseHandler.cpp Handler/AttachRequestHandler.cpp Handler/BreakpointLocationsHandler.cpp Handler/CompileUnitsRequestHandler.cpp diff --git a/lldb/tools/lldb-dap/DAP.cpp b/lldb/tools/lldb-dap/DAP.cpp index 01f294e14de6a..37bc1f68e4b08 100644 --- a/lldb/tools/lldb-dap/DAP.cpp +++ b/lldb/tools/lldb-dap/DAP.cpp @@ -7,6 +7,7 @@ //===----------------------------------------------------------------------===// #include "DAP.h" +#include "Handler/ResponseHandler.h" #include "JSONUtils.h" #include "LLDBUtils.h" #include "OutputRedirector.h" @@ -34,6 +35,7 @@ #include #include #include +#include #include #include @@ -769,10 +771,8 @@ bool DAP::HandleObject(const llvm::json::Object &object) { if (packet_type == "response") { auto id = GetSigned(object, "request_seq", 0); - ResponseCallback response_handler = [](llvm::Expected) { - llvm::errs() << "Unhandled response\n"; - }; + std::unique_ptr response_handler; { std::lock_guard locker(call_mutex); auto inflight = inflight_reverse_requests.find(id); @@ -782,19 +782,22 @@ bool DAP::HandleObject(const llvm::json::Object &object) { } } + if (!response_handler) + response_handler = std::make_unique("", id); + // Result should be given, use null if not. if (GetBoolean(object, "success", false)) { llvm::json::Value Result = nullptr; if (auto *B = object.get("body")) { Result = std::move(*B); } - response_handler(Result); + (*response_handler)(Result); } else { llvm::StringRef message = GetString(object, "message"); if (message.empty()) { message = "Unknown error, response failed"; } - response_handler(llvm::createStringError( + (*response_handler)(llvm::createStringError( std::error_code(-1, std::generic_category()), message)); } @@ -875,24 +878,6 @@ llvm::Error DAP::Loop() { return llvm::Error::success(); } -void DAP::SendReverseRequest(llvm::StringRef command, - llvm::json::Value arguments, - ResponseCallback callback) { - int64_t id; - { - std::lock_guard locker(call_mutex); - id = ++reverse_request_seq; - inflight_reverse_requests.emplace(id, std::move(callback)); - } - - SendJSON(llvm::json::Object{ - {"type", "request"}, - {"seq", id}, - {"command", command}, - {"arguments", std::move(arguments)}, - }); -} - lldb::SBError DAP::WaitForProcessToStop(uint32_t seconds) { lldb::SBError error; lldb::SBProcess process = target.GetProcess(); @@ -1007,17 +992,10 @@ bool StartDebuggingRequestHandler::DoExecute( return false; } - dap.SendReverseRequest( + dap.SendReverseRequest( "startDebugging", llvm::json::Object{{"request", request}, - {"configuration", std::move(*configuration)}}, - [](llvm::Expected value) { - if (!value) { - llvm::Error err = value.takeError(); - llvm::errs() << "reverse start debugging request failed: " - << llvm::toString(std::move(err)) << "\n"; - } - }); + {"configuration", std::move(*configuration)}}); result.SetStatus(lldb::eReturnStatusSuccessFinishNoResult); diff --git a/lldb/tools/lldb-dap/DAP.h b/lldb/tools/lldb-dap/DAP.h index f87841a56f4d3..7ceb1d114a57d 100644 --- a/lldb/tools/lldb-dap/DAP.h +++ b/lldb/tools/lldb-dap/DAP.h @@ -13,6 +13,7 @@ #include "ExceptionBreakpoint.h" #include "FunctionBreakpoint.h" #include "Handler/RequestHandler.h" +#include "Handler/ResponseHandler.h" #include "IOStream.h" #include "InstructionBreakpoint.h" #include "OutputRedirector.h" @@ -68,8 +69,6 @@ enum DAPBroadcasterBits { eBroadcastBitStopProgressThread = 1u << 1 }; -typedef void (*ResponseCallback)(llvm::Expected value); - enum class PacketStatus { Success = 0, EndOfFile, @@ -197,7 +196,7 @@ struct DAP { llvm::DenseSet thread_ids; uint32_t reverse_request_seq; std::mutex call_mutex; - std::map + llvm::SmallDenseMap> inflight_reverse_requests; ReplMode repl_mode; std::string command_escape_prefix = "`"; @@ -327,12 +326,24 @@ struct DAP { /// The reverse request command. /// /// \param[in] arguments - /// The reverse request arguements. - /// - /// \param[in] callback - /// A callback to execute when the response arrives. - void SendReverseRequest(llvm::StringRef command, llvm::json::Value arguments, - ResponseCallback callback); + /// The reverse request arguments. + template + void SendReverseRequest(llvm::StringRef command, + llvm::json::Value arguments) { + int64_t id; + { + std::lock_guard locker(call_mutex); + id = ++reverse_request_seq; + inflight_reverse_requests[id] = std::make_unique(command, id); + } + + SendJSON(llvm::json::Object{ + {"type", "request"}, + {"seq", id}, + {"command", command}, + {"arguments", std::move(arguments)}, + }); + } /// Registers a request handler. template void RegisterRequest() { diff --git a/lldb/tools/lldb-dap/Handler/RequestHandler.cpp b/lldb/tools/lldb-dap/Handler/RequestHandler.cpp index ad00e43947212..0a32e39ea3aff 100644 --- a/lldb/tools/lldb-dap/Handler/RequestHandler.cpp +++ b/lldb/tools/lldb-dap/Handler/RequestHandler.cpp @@ -101,15 +101,8 @@ static llvm::Error RunInTerminal(DAP &dap, #endif llvm::json::Object reverse_request = CreateRunInTerminalReverseRequest( launch_request, dap.debug_adaptor_path, comm_file.m_path, debugger_pid); - dap.SendReverseRequest("runInTerminal", std::move(reverse_request), - [](llvm::Expected value) { - if (!value) { - llvm::Error err = value.takeError(); - llvm::errs() - << "runInTerminal request failed: " - << llvm::toString(std::move(err)) << "\n"; - } - }); + dap.SendReverseRequest("runInTerminal", + std::move(reverse_request)); if (llvm::Expected pid = comm_channel.GetLauncherPid()) attach_info.SetProcessID(*pid); diff --git a/lldb/tools/lldb-dap/Handler/ResponseHandler.cpp b/lldb/tools/lldb-dap/Handler/ResponseHandler.cpp new file mode 100644 index 0000000000000..27a1437de5502 --- /dev/null +++ b/lldb/tools/lldb-dap/Handler/ResponseHandler.cpp @@ -0,0 +1,35 @@ +//===-- ResponseHandler.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 "ResponseHandler.h" +#include "llvm/ADT/StringRef.h" +#include "llvm/Support/Error.h" +#include "llvm/Support/raw_ostream.h" + +namespace lldb_dap { + +void UnknownResponseHandler::operator()( + llvm::Expected value) const { + llvm::errs() << "unexpected response: "; + if (value) { + if (std::optional str = value->getAsString()) + llvm::errs() << *str; + } else { + llvm::errs() << "error: " << llvm::toString(value.takeError()); + } + llvm::errs() << '\n'; +} + +void LogFailureResponseHandler::operator()( + llvm::Expected value) const { + if (!value) + llvm::errs() << "reverse request \"" << m_command << "\" (" << m_id + << ") failed: " << llvm::toString(value.takeError()) << '\n'; +} + +} // namespace lldb_dap diff --git a/lldb/tools/lldb-dap/Handler/ResponseHandler.h b/lldb/tools/lldb-dap/Handler/ResponseHandler.h new file mode 100644 index 0000000000000..b09153ac2edad --- /dev/null +++ b/lldb/tools/lldb-dap/Handler/ResponseHandler.h @@ -0,0 +1,56 @@ +//===-- ResponseHandler.h -------------------------------------------------===// +// +// 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 +// +//===----------------------------------------------------------------------===// + +#ifndef LLDB_TOOLS_LLDB_DAP_HANDLER_RESPONSEHANDLER_H +#define LLDB_TOOLS_LLDB_DAP_HANDLER_RESPONSEHANDLER_H + +#include "llvm/ADT/StringRef.h" +#include "llvm/Support/JSON.h" +#include + +namespace lldb_dap { +struct DAP; + +/// Handler for responses to reverse requests. +class ResponseHandler { +public: + ResponseHandler(llvm::StringRef command, int64_t id) + : m_command(command), m_id(id) {} + + /// ResponseHandlers are not copyable. + /// @{ + ResponseHandler(const ResponseHandler &) = delete; + ResponseHandler &operator=(const ResponseHandler &) = delete; + /// @} + + virtual ~ResponseHandler() = default; + + virtual void operator()(llvm::Expected value) const = 0; + +protected: + llvm::StringRef m_command; + int64_t m_id; +}; + +/// Response handler used for unknown responses. +class UnknownResponseHandler : public ResponseHandler { +public: + using ResponseHandler::ResponseHandler; + void operator()(llvm::Expected value) const override; +}; + +/// Response handler which logs to stderr in case of a failure. +class LogFailureResponseHandler : public ResponseHandler { +public: + using ResponseHandler::ResponseHandler; + void operator()(llvm::Expected value) const override; +}; + +} // namespace lldb_dap + +#endif