[lldb] Refactoring JSONTransport into an abstract RPC Message Handler and transport layer. #153121
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
ashgti
force-pushed
the
lldb-message-handler
branch
2 times, most recently
from
2cd196e to
6a846c2
Compare
|
@llvm/pr-subscribers-lldb Author: John Harrison (ashgti) ChangesThis abstracts the base Transport handler to have a MessageHandler component and allows us to generalize both JSON-RPC 2.0 for MCP (or an LSP) and DAP format. This should allow us to create clearly defined clients and servers for protocols, both for testing and for RPC between the lldb instances and an lldb-mcp multiplexer. This basic model is inspiried by the clangd/Transport.h file and the mlir/lsp-server-support/Transport.h that are both used for LSP servers within the llvm project. Additionally, this helps with testing by subclassing Patch is 105.01 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/153121.diff 21 Files Affected:
diff --git a/lldb/include/lldb/Host/JSONTransport.h b/lldb/include/lldb/Host/JSONTransport.h
index 72f4404c92887..dd00e5f739dd7 100644
--- a/lldb/include/lldb/Host/JSONTransport.h
+++ b/lldb/include/lldb/Host/JSONTransport.h
@@ -13,29 +13,25 @@
#ifndef LLDB_HOST_JSONTRANSPORT_H
#define LLDB_HOST_JSONTRANSPORT_H
+#include "lldb/Host/MainLoop.h"
#include "lldb/Host/MainLoopBase.h"
#include "lldb/Utility/IOObject.h"
#include "lldb/Utility/Status.h"
#include "lldb/lldb-forward.h"
+#include "llvm/ADT/StringExtras.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/Support/Error.h"
+#include "llvm/Support/ErrorHandling.h"
#include "llvm/Support/FormatVariadic.h"
#include "llvm/Support/JSON.h"
+#include "llvm/Support/raw_ostream.h"
#include <string>
#include <system_error>
+#include <variant>
#include <vector>
namespace lldb_private {
-class TransportEOFError : public llvm::ErrorInfo<TransportEOFError> {
-public:
- static char ID;
-
- TransportEOFError() = default;
- void log(llvm::raw_ostream &OS) const override;
- std::error_code convertToErrorCode() const override;
-};
-
class TransportUnhandledContentsError
: public llvm::ErrorInfo<TransportUnhandledContentsError> {
public:
@@ -54,112 +50,219 @@ class TransportUnhandledContentsError
std::string m_unhandled_contents;
};
-class TransportInvalidError : public llvm::ErrorInfo<TransportInvalidError> {
+/// A transport is responsible for maintaining the connection to a client
+/// application, and reading/writing structured messages to it.
+///
+/// Transports have limited thread safety requirements:
+/// - messages will not be sent concurrently
+/// - messages MAY be sent while Run() is reading, or its callback is active
+template <typename Req, typename Resp, typename Evt> class Transport {
public:
- static char ID;
-
- TransportInvalidError() = default;
+ using Message = std::variant<Req, Resp, Evt>;
+
+ virtual ~Transport() = default;
+
+ // Called by transport to send outgoing messages.
+ virtual void Event(const Evt &) = 0;
+ virtual void Request(const Req &) = 0;
+ virtual void Response(const Resp &) = 0;
+
+ /// Implemented to handle incoming messages. (See Run() below).
+ class MessageHandler {
+ public:
+ virtual ~MessageHandler() = default;
+ virtual void OnEvent(const Evt &) = 0;
+ virtual void OnRequest(const Req &) = 0;
+ virtual void OnResponse(const Resp &) = 0;
+ };
+
+ /// Called by server or client to receive messages from the connection.
+ /// The transport should in turn invoke the handler to process messages.
+ /// The MainLoop is used to handle reading from the incoming connection and
+ /// will run until the loop is terminated.
+ virtual llvm::Error Run(MainLoop &, MessageHandler &) = 0;
- void log(llvm::raw_ostream &OS) const override;
- std::error_code convertToErrorCode() const override;
+ template <typename... Ts> inline auto Logv(const char *Fmt, Ts &&...Vals) {
+ Log(llvm::formatv(Fmt, std::forward<Ts>(Vals)...).str());
+ }
+ virtual void Log(llvm::StringRef message) = 0;
};
-/// A transport class that uses JSON for communication.
-class JSONTransport {
+/// A JSONTransport will encode and decode messages using JSON.
+template <typename Req, typename Resp, typename Evt>
+class JSONTransport : public Transport<Req, Resp, Evt> {
public:
- using ReadHandleUP = MainLoopBase::ReadHandleUP;
- template <typename T>
- using Callback = std::function<void(MainLoopBase &, const llvm::Expected<T>)>;
-
- JSONTransport(lldb::IOObjectSP input, lldb::IOObjectSP output);
- virtual ~JSONTransport() = default;
-
- /// Transport is not copyable.
- /// @{
- JSONTransport(const JSONTransport &rhs) = delete;
- void operator=(const JSONTransport &rhs) = delete;
- /// @}
-
- /// Writes a message to the output stream.
- template <typename T> llvm::Error Write(const T &t) {
- const std::string message = llvm::formatv("{0}", toJSON(t)).str();
- return WriteImpl(message);
+ using Transport<Req, Resp, Evt>::Transport;
+
+ JSONTransport(lldb::IOObjectSP in, lldb::IOObjectSP out)
+ : m_in(in), m_out(out) {}
+
+ void Event(const Evt &evt) override { Write(evt); }
+ void Request(const Req &req) override { Write(req); }
+ void Response(const Resp &resp) override { Write(resp); }
+
+ /// Run registers the transport with the given MainLoop and handles any
+ /// incoming messages using the given MessageHandler.
+ llvm::Error
+ Run(MainLoop &loop,
+ typename Transport<Req, Resp, Evt>::MessageHandler &handler) override {
+ llvm::Error error = llvm::Error::success();
+ Status status;
+ auto read_handle = loop.RegisterReadObject(
+ m_in,
+ std::bind(&JSONTransport::OnRead, this, &error, std::placeholders::_1,
+ std::ref(handler)),
+ status);
+ if (status.Fail()) {
+ // This error is only set if the read object handler is invoked, mark it
+ // as consumed if registration of the handler failed.
+ llvm::consumeError(std::move(error));
+ return status.takeError();
+ }
+
+ status = loop.Run();
+ if (status.Fail())
+ return status.takeError();
+ return error;
}
- /// Registers the transport with the MainLoop.
- template <typename T>
- llvm::Expected<ReadHandleUP> RegisterReadObject(MainLoopBase &loop,
- Callback<T> read_cb) {
- Status error;
- ReadHandleUP handle = loop.RegisterReadObject(
- m_input,
- [read_cb, this](MainLoopBase &loop) {
- char buf[kReadBufferSize];
- size_t num_bytes = sizeof(buf);
- if (llvm::Error error = m_input->Read(buf, num_bytes).takeError()) {
- read_cb(loop, std::move(error));
- return;
- }
- if (num_bytes)
- m_buffer.append(std::string(buf, num_bytes));
-
- // If the buffer has contents, try parsing any pending messages.
- if (!m_buffer.empty()) {
- llvm::Expected<std::vector<std::string>> messages = Parse();
- if (llvm::Error error = messages.takeError()) {
- read_cb(loop, std::move(error));
- return;
- }
-
- for (const auto &message : *messages)
- if constexpr (std::is_same<T, std::string>::value)
- read_cb(loop, message);
- else
- read_cb(loop, llvm::json::parse<T>(message));
- }
-
- // On EOF, notify the callback after the remaining messages were
- // handled.
- if (num_bytes == 0) {
- if (m_buffer.empty())
- read_cb(loop, llvm::make_error<TransportEOFError>());
- else
- read_cb(loop, llvm::make_error<TransportUnhandledContentsError>(
- std::string(m_buffer)));
- }
- },
- error);
- if (error.Fail())
- return error.takeError();
- return handle;
- }
+ /// Public for testing purposes, otherwise this should be an implementation
+ /// detail.
+ static constexpr size_t kReadBufferSize = 1024;
protected:
- template <typename... Ts> inline auto Logv(const char *Fmt, Ts &&...Vals) {
- Log(llvm::formatv(Fmt, std::forward<Ts>(Vals)...).str());
+ virtual llvm::Expected<std::vector<std::string>> Parse() = 0;
+ virtual std::string Encode(const llvm::json::Value &message) = 0;
+ void Write(const llvm::json::Value &message) {
+ this->Logv("<-- {0}", message);
+ std::string output = Encode(message);
+ size_t bytes_written = output.size();
+ Status status = m_out->Write(output.data(), bytes_written);
+ if (status.Fail()) {
+ this->Logv("writing failed: {0}", status.AsCString());
+ }
}
- virtual void Log(llvm::StringRef message);
- virtual llvm::Error WriteImpl(const std::string &message) = 0;
- virtual llvm::Expected<std::vector<std::string>> Parse() = 0;
+ llvm::SmallString<kReadBufferSize> m_buffer;
- static constexpr size_t kReadBufferSize = 1024;
+private:
+ void OnRead(llvm::Error *err, MainLoopBase &loop,
+ typename Transport<Req, Resp, Evt>::MessageHandler &handler) {
+ llvm::ErrorAsOutParameter ErrAsOutParam(err);
+ char buf[kReadBufferSize];
+ size_t num_bytes = sizeof(buf);
+ if (Status status = m_in->Read(buf, num_bytes); status.Fail()) {
+ *err = status.takeError();
+ loop.RequestTermination();
+ return;
+ }
+
+ if (num_bytes)
+ m_buffer.append(llvm::StringRef(buf, num_bytes));
+
+ // If the buffer has contents, try parsing any pending messages.
+ if (!m_buffer.empty()) {
+ llvm::Expected<std::vector<std::string>> raw_messages = Parse();
+ if (llvm::Error error = raw_messages.takeError()) {
+ *err = std::move(error);
+ loop.RequestTermination();
+ return;
+ }
+
+ for (const auto &raw_message : *raw_messages) {
+ auto message =
+ llvm::json::parse<typename Transport<Req, Resp, Evt>::Message>(
+ raw_message);
+ if (!message) {
+ *err = message.takeError();
+ loop.RequestTermination();
+ return;
+ }
+
+ if (Evt *evt = std::get_if<Evt>(&*message)) {
+ handler.OnEvent(*evt);
+ } else if (Req *req = std::get_if<Req>(&*message)) {
+ handler.OnRequest(*req);
+ } else if (Resp *resp = std::get_if<Resp>(&*message)) {
+ handler.OnResponse(*resp);
+ } else {
+ llvm_unreachable("unknown message type");
+ }
+ }
+ }
+
+ if (num_bytes == 0) {
+ // If we're at EOF and we have unhandled contents in the buffer, return an
+ // error for the partial message.
+ if (m_buffer.empty())
+ *err = llvm::Error::success();
+ else
+ *err = llvm::make_error<TransportUnhandledContentsError>(
+ std::string(m_buffer));
+ loop.RequestTermination();
+ }
+ }
- lldb::IOObjectSP m_input;
- lldb::IOObjectSP m_output;
- llvm::SmallString<kReadBufferSize> m_buffer;
+ lldb::IOObjectSP m_in;
+ lldb::IOObjectSP m_out;
};
/// A transport class for JSON with a HTTP header.
-class HTTPDelimitedJSONTransport : public JSONTransport {
+template <typename Req, typename Resp, typename Evt>
+class HTTPDelimitedJSONTransport : public JSONTransport<Req, Resp, Evt> {
public:
- HTTPDelimitedJSONTransport(lldb::IOObjectSP input, lldb::IOObjectSP output)
- : JSONTransport(input, output) {}
- virtual ~HTTPDelimitedJSONTransport() = default;
+ using JSONTransport<Req, Resp, Evt>::JSONTransport;
protected:
- llvm::Error WriteImpl(const std::string &message) override;
- llvm::Expected<std::vector<std::string>> Parse() override;
+ /// Encodes messages based on
+ /// https://microsoft.github.io/debug-adapter-protocol/overview#base-protocol
+ std::string Encode(const llvm::json::Value &message) override {
+ std::string output;
+ std::string raw_message = llvm::formatv("{0}", message).str();
+ llvm::raw_string_ostream OS(output);
+ OS << kHeaderContentLength << kHeaderFieldSeparator << ' '
+ << std::to_string(raw_message.size()) << kEndOfHeader << raw_message;
+ return output;
+ }
+
+ /// Parses messages based on
+ /// https://microsoft.github.io/debug-adapter-protocol/overview#base-protocol
+ llvm::Expected<std::vector<std::string>> Parse() override {
+ std::vector<std::string> messages;
+ llvm::StringRef buffer = this->m_buffer;
+ while (buffer.contains(kEndOfHeader)) {
+ auto [headers, rest] = buffer.split(kEndOfHeader);
+ size_t content_length = 0;
+ // HTTP Headers are formatted like `<field-name> ':' [<field-value>]`.
+ for (const auto &header : llvm::split(headers, kHeaderSeparator)) {
+ auto [key, value] = header.split(kHeaderFieldSeparator);
+ // 'Content-Length' is the only meaningful key at the moment. Others are
+ // ignored.
+ if (!key.equals_insensitive(kHeaderContentLength))
+ continue;
+
+ value = value.trim();
+ if (!llvm::to_integer(value, content_length, 10))
+ return llvm::createStringError(std::errc::invalid_argument,
+ "invalid content length: %s",
+ value.str().c_str());
+ }
+
+ // Check if we have enough data.
+ if (content_length > rest.size())
+ break;
+
+ llvm::StringRef body = rest.take_front(content_length);
+ buffer = rest.drop_front(content_length);
+ messages.emplace_back(body.str());
+ this->Logv("--> {0}", body);
+ }
+
+ // Store the remainder of the buffer for the next read callback.
+ this->m_buffer = buffer.str();
+
+ return std::move(messages);
+ }
static constexpr llvm::StringLiteral kHeaderContentLength = "Content-Length";
static constexpr llvm::StringLiteral kHeaderFieldSeparator = ":";
@@ -168,15 +271,31 @@ class HTTPDelimitedJSONTransport : public JSONTransport {
};
/// A transport class for JSON RPC.
-class JSONRPCTransport : public JSONTransport {
+template <typename Req, typename Resp, typename Evt>
+class JSONRPCTransport : public JSONTransport<Req, Resp, Evt> {
public:
- JSONRPCTransport(lldb::IOObjectSP input, lldb::IOObjectSP output)
- : JSONTransport(input, output) {}
- virtual ~JSONRPCTransport() = default;
+ using JSONTransport<Req, Resp, Evt>::JSONTransport;
protected:
- llvm::Error WriteImpl(const std::string &message) override;
- llvm::Expected<std::vector<std::string>> Parse() override;
+ std::string Encode(const llvm::json::Value &message) override {
+ return llvm::formatv("{0}{1}", message, kMessageSeparator).str();
+ }
+
+ llvm::Expected<std::vector<std::string>> Parse() override {
+ std::vector<std::string> messages;
+ llvm::StringRef buf = this->m_buffer;
+ while (buf.contains(kMessageSeparator)) {
+ auto [raw_json, rest] = buf.split(kMessageSeparator);
+ buf = rest;
+ messages.emplace_back(raw_json.str());
+ this->Logv("--> {0}", raw_json);
+ }
+
+ // Store the remainder of the buffer for the next read callback.
+ this->m_buffer = buf.str();
+
+ return messages;
+ }
static constexpr llvm::StringLiteral kMessageSeparator = "\n";
};
diff --git a/lldb/include/lldb/Protocol/MCP/MCPError.h b/lldb/include/lldb/Protocol/MCP/MCPError.h
index 2bdbb9b7a6874..55dd40f124a15 100644
--- a/lldb/include/lldb/Protocol/MCP/MCPError.h
+++ b/lldb/include/lldb/Protocol/MCP/MCPError.h
@@ -26,7 +26,7 @@ class MCPError : public llvm::ErrorInfo<MCPError> {
const std::string &getMessage() const { return m_message; }
- lldb_protocol::mcp::Error toProtcolError() const;
+ lldb_protocol::mcp::Error toProtocolError() const;
static constexpr int64_t kResourceNotFound = -32002;
static constexpr int64_t kInternalError = -32603;
diff --git a/lldb/include/lldb/Protocol/MCP/Protocol.h b/lldb/include/lldb/Protocol/MCP/Protocol.h
index 6448416eee08f..d682abe3490f0 100644
--- a/lldb/include/lldb/Protocol/MCP/Protocol.h
+++ b/lldb/include/lldb/Protocol/MCP/Protocol.h
@@ -15,6 +15,7 @@
#define LLDB_PROTOCOL_MCP_PROTOCOL_H
#include "llvm/Support/JSON.h"
+#include "llvm/Support/raw_ostream.h"
#include <optional>
#include <string>
#include <variant>
@@ -23,41 +24,59 @@ namespace lldb_protocol::mcp {
static llvm::StringLiteral kProtocolVersion = "2024-11-05";
+/// A Request or Response 'id'.
+using Id = std::variant<std::string, int64_t>;
+
/// A request that expects a response.
struct Request {
- uint64_t id = 0;
+ Id id; // Per the spec, this must be a string or number.
std::string method;
std::optional<llvm::json::Value> params;
};
llvm::json::Value toJSON(const Request &);
bool fromJSON(const llvm::json::Value &, Request &, llvm::json::Path);
+inline bool operator==(const Request &a, const Request &b) {
+ return a.id == b.id && a.method == b.method && a.params == b.params;
+}
-struct ErrorInfo {
+struct Error {
int64_t code = 0;
std::string message;
- std::string data;
-};
-
-llvm::json::Value toJSON(const ErrorInfo &);
-bool fromJSON(const llvm::json::Value &, ErrorInfo &, llvm::json::Path);
-
-struct Error {
- uint64_t id = 0;
- ErrorInfo error;
+ std::optional<llvm::json::Value> data;
};
llvm::json::Value toJSON(const Error &);
bool fromJSON(const llvm::json::Value &, Error &, llvm::json::Path);
+inline bool operator==(const Error &a, const Error &b) {
+ return a.code == b.code && a.message == b.message && a.data == b.data;
+}
+inline llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const Request &R) {
+ OS << toJSON(R);
+ return OS;
+}
struct Response {
- uint64_t id = 0;
+ Id id; // Per the spec, this must be a string or number.
std::optional<llvm::json::Value> result;
- std::optional<ErrorInfo> error;
+ std::optional<Error> error;
};
llvm::json::Value toJSON(const Response &);
bool fromJSON(const llvm::json::Value &, Response &, llvm::json::Path);
+inline bool operator==(const Response &a, const Response &b) {
+ return a.id == b.id && a.result == b.result && a.error == b.error;
+}
+inline llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const Response &R) {
+ OS << toJSON(R);
+ return OS;
+}
+inline void PrintTo(const Response &R, std::ostream *OS) {
+ std::string O;
+ llvm::raw_string_ostream ROS(O);
+ ROS << R;
+ *OS << O;
+}
/// A notification which does not expect a response.
struct Notification {
@@ -67,6 +86,14 @@ struct Notification {
llvm::json::Value toJSON(const Notification &);
bool fromJSON(const llvm::json::Value &, Notification &, llvm::json::Path);
+inline bool operator==(const Notification &a, const Notification &b) {
+ return a.method == b.method && a.params == b.params;
+}
+inline llvm::raw_ostream &operator<<(llvm::raw_ostream &OS,
+ const Notification &N) {
+ OS << toJSON(N);
+ return OS;
+}
struct ToolCapability {
/// Whether this server supports notifications for changes to the tool list.
@@ -176,10 +203,20 @@ struct ToolDefinition {
llvm::json::Value toJSON(const ToolDefinition &);
bool fromJSON(const llvm::json::Value &, ToolDefinition &, llvm::json::Path);
-using Message = std::variant<Request, Response, Notification, Error>;
+using Message = std::variant<Request, Response, Notification>;
bool fromJSON(const llvm::json::Value &, Message &, llvm::json::Path);
llvm::json::Value toJSON(const Message &);
+inline llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const Message &M) {
+ OS << toJSON(M);
+ return OS;
+}
+inline void PrintTo(const Message &M, std::ostream *OS) {
+ std::string O;
+ llvm::raw_string_ostream ROS(O);
+ ROS << M;
+ *OS << O;
+}
using ToolArguments = std::variant<std::monostate, llvm::json::Value>;
diff --git a/lldb/source/Host/common/JSONTransport.cpp b/lldb/source/Host/common/JSONTransport.cpp
index 5f0fb3ce562c3..c4b42eafc85d3 100644
--- a/lldb/source/Host/common/JSONTransport.cpp
+++ b/lldb/source/Host/common/JSONTransport.cpp
@@ -7,136 +7,26 @@
//===----------------------------------------------------------------------===//
#include "lldb/Host/JSONTransport.h"
-#include "lldb/Utility/LLDBLog.h"
#include "lldb/Utility/Log.h"
#include "lldb/Utility/Status.h"
-#include "lldb/lldb-forward.h"
#include "llvm/ADT/StringExtras.h"
-#include "llvm/ADT/StringRef.h"
-#include "llvm/Support/Error.h"
#include "llvm/Support/raw_ostream.h"
#include <string>
-#include <utility>
using namespace llvm;
using namespace lldb;
using namespace lldb_private;
-void TransportEOFError::log(llvm::raw_ostream &OS) const {
- OS << "transport EOF";
-}
-
-std::error_code TransportEOFError::convertToErrorCode() const {
- return std::make_error_code(std::errc::io_error);
-}
+char TransportUnhandledContentsError::ID;
TransportUnhandledContentsError::TransportUnhandledContentsError(
std::string unhandled_contents)
: m_unhandled_contents(unhandled_contents) {}
void TransportUnhandledContentsError::log(llvm::raw_ostream &OS) const {
- OS << "transport EOF with unhandled contents " << m_unhandled_contents;
+ OS << "transport EOF with unhandled contents: '" << m_unhandled_contents
+ << "'";
}
std::error_code TransportUnhandledContentsError::convertToErrorCode() const {
return std::make_error_code(std::errc::bad_message);
}
-
-void TransportInvalidError::log(llvm::raw_ostream &OS) const {
- OS << "transport IO obj...
[truncated]
|
There was a problem hiding this comment.
Overall this LGTM. I like the design of the Transport class, albeit still very much tied to the protocols it's meant to support. Both DAP and MCP have requests, responses and events, but I don't know how generic that is. Anyway, it should be easily extensible and/or use std::monostate if they don't support one of the types.
The PR includes some unrelated reflowing of comments, might be nice to unstage that. There's some other changes (like the Id for MCP) that look like they could go into their own PR, but there might be hidden dependencies.
Comment on lines
141
to
144
| if (status.Fail()) { | ||
| this->Logv("writing failed: {0}", status.AsCString()); | ||
| } |
There was a problem hiding this comment.
Leftover braces from LLDB_LOG I presume.
Suggested change
| if (status.Fail()) { | |
| this->Logv("writing failed: {0}", status.AsCString()); | |
| } | |
| if (status.Fail()) | |
| this->Logv("writing failed: {0}", status.AsCString()); |
There was a problem hiding this comment.
Logv is expanding using llvm::formatv, so it still uses the {0}.
| /// A request that expects a response. | ||
| struct Request { | ||
| uint64_t id = 0; | ||
| Id id; // Per the spec, this must be a string or number. |
There was a problem hiding this comment.
Maybe move this comment to line 27 so we don't need to repeat it everywhere that used Id?
/// A Request or Response 'id'. Per the spec, this must be a string or number.
using Id = std::variant<std::string, int64_t>;
There was a problem hiding this comment.
Done, also moved these changes into #153297
I'll pull this apart and start with the |
|
Split out the MCP changes into #153297 Once thats in I'll rebase on that and see if I can revert/simplify anything else. |
ashgti
force-pushed
the
lldb-message-handler
branch
from
6a846c2 to
572e2ca
Compare
… and transport layer. This abstracts the base Transport handler to have a MessageHandler component and allows us to generalize both JSON-RPC 2.0 for MCP (or an LSP) and DAP format. This should allow us to create clearly defined clients and servers for protocols, both for testing and for RPC between the lldb instances and an lldb-mcp multiplexer. This basic model is inspiried by the clangd/Transport.h file and the mlir/lsp-server-support/Transport.h that are both used for LSP servers within the llvm project.
ashgti
force-pushed
the
lldb-message-handler
branch
from
572e2ca to
d5f998c
Compare
|
Rebased on main with #153297 included and reverted the comment changes. |
JDevlieghere
approved these changes
Aug 13, 2025
Comment on lines
183
to
191
| if (Evt *evt = std::get_if<Evt>(&*message)) { | ||
| handler.OnEvent(*evt); | ||
| } else if (Req *req = std::get_if<Req>(&*message)) { | ||
| handler.OnRequest(*req); | ||
| } else if (Resp *resp = std::get_if<Resp>(&*message)) { | ||
| handler.OnResponse(*resp); | ||
| } else { | ||
| llvm_unreachable("unknown message type"); | ||
| } |
There was a problem hiding this comment.
Either no braces or, alternatively you could do something like
if (Evt *evt = std::get_if<Evt>(&*message)) {
handler.OnEvent(*evt);
continue;
}
if (Req *req = std::get_if<Req>(&*message)) {
handler.OnRequest(*req);
continue;
}
There was a problem hiding this comment.
I went with the ifs + continue, I think that looks a bit nicer.
lldb/tools/lldb-dap/DAP.cpp
Outdated
Comment on lines
276
to
285
| if (debugger.InterruptRequested()) | ||
| // If the debugger was interrupted, convert this response into a | ||
| // 'cancelled' response because we might have a partial result. | ||
| transport.Response(Response{/*request_seq=*/resp->request_seq, | ||
| /*command=*/resp->command, | ||
| /*success=*/false, | ||
| /*message=*/eResponseMessageCancelled, | ||
| /*body=*/std::nullopt}); | ||
| else | ||
| transport.Response(*resp); |
There was a problem hiding this comment.
According to the style guide, this does require braces:
Suggested change
| if (debugger.InterruptRequested()) | |
| // If the debugger was interrupted, convert this response into a | |
| // 'cancelled' response because we might have a partial result. | |
| transport.Response(Response{/*request_seq=*/resp->request_seq, | |
| /*command=*/resp->command, | |
| /*success=*/false, | |
| /*message=*/eResponseMessageCancelled, | |
| /*body=*/std::nullopt}); | |
| else | |
| transport.Response(*resp); | |
| if (debugger.InterruptRequested()) { | |
| // If the debugger was interrupted, convert this response into a | |
| // 'cancelled' response because we might have a partial result. | |
| transport.Response(Response{/*request_seq=*/resp->request_seq, | |
| /*command=*/resp->command, | |
| /*success=*/false, | |
| /*message=*/eResponseMessageCancelled, | |
| /*body=*/std::nullopt}); | |
| } else { | |
| transport.Response(*resp); | |
| } |
lldb/tools/lldb-dap/DAP.cpp
Outdated
| llvm::ErrorAsOutParameter ErrAsOutParam(*error); | ||
| auto cleanup = llvm::make_scope_exit([&]() { | ||
| // Ensure we're marked as disconnecting when the reader exits. | ||
| disconnecting = true; |
There was a problem hiding this comment.
I know this was already there, but technically we're racing on disconnecting here.
There was a problem hiding this comment.
Added a lock guard around disconnecting and made it private.
labath
reviewed
Aug 14, 2025
…e errors to the MessageHandler instead of internally logging and ignoring them. This allows the MessageHandler to determine what to do when an error occurs.
|
I refactored this a bit to not have the This changed the flow of how errors are handled. Now instead of simply logging any errors I surface them to the LMKWYT |
labath
reviewed
Aug 18, 2025
| continue; | ||
| } | ||
|
|
||
| if (Evt *evt = std::get_if<Evt>(&*message)) { |
There was a problem hiding this comment.
I'm tempted to use std::visit here as well, although that would mean using the same name for all of these functions (Handle? operator() ?), so 🤷 ...
There was a problem hiding this comment.
I adjusted the Transport and MessageHandler classes to have more uniform names, which allows us to use std::visit for dispatching to the correct method.
Also refactored the Transport and MessageHandler classes to have more uniform names, which allows us to use `std::visit` for dispatching to the correct method.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This abstracts the base Transport handler to have a MessageHandler component and allows us to generalize both JSON-RPC 2.0 for MCP (or an LSP) and DAP format.
This should allow us to create clearly defined clients and servers for protocols, both for testing and for RPC between the lldb instances and an lldb-mcp multiplexer.
This basic model is inspiried by the clangd/Transport.h file and the mlir/lsp-server-support/Transport.h that are both used for LSP servers within the llvm project.
Additionally, this helps with testing by subclassing
Transportto allow us to simplify sending/receiving messages without needing to use a toJSON/fromJSON and a pair of pipes, seeTestTransportin DAP/TestBase.h.