Skip to content

Conversation

ashgti
Copy link
Contributor

@ashgti ashgti commented Sep 3, 2025

Moving lldb_protocol::mcp::MCPTransport into its own file and renaming to lldb_protocol::mcp::Transport.

Moving `lldb_protocol::mcp::MCPTransport` into its own file and renaming to `lldb_protocol::mcp::Transport`.
@ashgti ashgti marked this pull request as ready for review September 3, 2025 16:36
@ashgti ashgti requested a review from JDevlieghere as a code owner September 3, 2025 16:36
@llvmbot llvmbot added the lldb label Sep 3, 2025
@llvmbot
Copy link
Member

llvmbot commented Sep 3, 2025

@llvm/pr-subscribers-lldb

Author: John Harrison (ashgti)

Changes

Moving lldb_protocol::mcp::MCPTransport into its own file and renaming to lldb_protocol::mcp::Transport.


Full diff: https://github.com/llvm/llvm-project/pull/156712.diff

7 Files Affected:

  • (modified) lldb/include/lldb/Host/JSONTransport.h (+7-4)
  • (modified) lldb/include/lldb/Protocol/MCP/Server.h (+6-22)
  • (added) lldb/include/lldb/Protocol/MCP/Transport.h (+48)
  • (modified) lldb/source/Plugins/Protocol/MCP/ProtocolServerMCP.cpp (+3-3)
  • (modified) lldb/source/Protocol/MCP/CMakeLists.txt (+1)
  • (added) lldb/source/Protocol/MCP/Transport.cpp (+23)
  • (modified) lldb/unittests/Protocol/ProtocolMCPServerTest.cpp (+4-16)
diff --git a/lldb/include/lldb/Host/JSONTransport.h b/lldb/include/lldb/Host/JSONTransport.h
index 0be60a8f3f96a..210f33edace6e 100644
--- a/lldb/include/lldb/Host/JSONTransport.h
+++ b/lldb/include/lldb/Host/JSONTransport.h
@@ -100,7 +100,8 @@ template <typename Req, typename Resp, typename Evt> class Transport {
   virtual llvm::Expected<MainLoop::ReadHandleUP>
   RegisterMessageHandler(MainLoop &loop, MessageHandler &handler) = 0;
 
-protected:
+  // FIXME: Refactor mcp::Server to not directly access log on the transport.
+  // protected:
   template <typename... Ts> inline auto Logv(const char *Fmt, Ts &&...Vals) {
     Log(llvm::formatv(Fmt, std::forward<Ts>(Vals)...).str());
   }
@@ -139,9 +140,7 @@ class JSONTransport : public Transport<Req, Resp, Evt> {
   /// detail.
   static constexpr size_t kReadBufferSize = 1024;
 
-protected:
-  virtual llvm::Expected<std::vector<std::string>> Parse() = 0;
-  virtual std::string Encode(const llvm::json::Value &message) = 0;
+  // FIXME: Write should be protected.
   llvm::Error Write(const llvm::json::Value &message) {
     this->Logv("<-- {0}", message);
     std::string output = Encode(message);
@@ -149,6 +148,10 @@ class JSONTransport : public Transport<Req, Resp, Evt> {
     return m_out->Write(output.data(), bytes_written).takeError();
   }
 
+protected:
+  virtual llvm::Expected<std::vector<std::string>> Parse() = 0;
+  virtual std::string Encode(const llvm::json::Value &message) = 0;
+
   llvm::SmallString<kReadBufferSize> m_buffer;
 
 private:
diff --git a/lldb/include/lldb/Protocol/MCP/Server.h b/lldb/include/lldb/Protocol/MCP/Server.h
index c6e78a9ea0cff..254b7d9680cd8 100644
--- a/lldb/include/lldb/Protocol/MCP/Server.h
+++ b/lldb/include/lldb/Protocol/MCP/Server.h
@@ -14,33 +14,17 @@
 #include "lldb/Protocol/MCP/Protocol.h"
 #include "lldb/Protocol/MCP/Resource.h"
 #include "lldb/Protocol/MCP/Tool.h"
+#include "lldb/Protocol/MCP/Transport.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/Support/Error.h"
-#include <mutex>
+#include "llvm/Support/JSON.h"
+#include <functional>
+#include <memory>
+#include <string>
+#include <vector>
 
 namespace lldb_protocol::mcp {
 
-class MCPTransport
-    : public lldb_private::JSONRPCTransport<Request, Response, Notification> {
-public:
-  using LogCallback = std::function<void(llvm::StringRef message)>;
-
-  MCPTransport(lldb::IOObjectSP in, lldb::IOObjectSP out,
-               std::string client_name, LogCallback log_callback = {})
-      : JSONRPCTransport(in, out), m_client_name(std::move(client_name)),
-        m_log_callback(log_callback) {}
-  virtual ~MCPTransport() = default;
-
-  void Log(llvm::StringRef message) override {
-    if (m_log_callback)
-      m_log_callback(llvm::formatv("{0}: {1}", m_client_name, message).str());
-  }
-
-private:
-  std::string m_client_name;
-  LogCallback m_log_callback;
-};
-
 /// Information about this instance of lldb's MCP server for lldb-mcp to use to
 /// coordinate connecting an lldb-mcp client.
 struct ServerInfo {
diff --git a/lldb/include/lldb/Protocol/MCP/Transport.h b/lldb/include/lldb/Protocol/MCP/Transport.h
new file mode 100644
index 0000000000000..47c2ccfc44dfe
--- /dev/null
+++ b/lldb/include/lldb/Protocol/MCP/Transport.h
@@ -0,0 +1,48 @@
+//===----------------------------------------------------------------------===//
+//
+// 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_PROTOCOL_MCP_TRANSPORT_H
+#define LLDB_PROTOCOL_MCP_TRANSPORT_H
+
+#include "lldb/Host/JSONTransport.h"
+#include "lldb/Protocol/MCP/Protocol.h"
+#include "lldb/lldb-forward.h"
+#include "llvm/ADT/FunctionExtras.h"
+#include "llvm/ADT/StringRef.h"
+
+namespace lldb_protocol::mcp {
+
+/// Generic transport that uses the MCP protocol.
+using MCPTransport = lldb_private::Transport<Request, Response, Notification>;
+
+/// Generic logging callback, to allow the MCP server / client / transport layer
+/// to be independent of the lldb log implementation.
+using LogCallback = llvm::unique_function<void(llvm::StringRef message)>;
+
+class Transport final
+    : public lldb_private::JSONRPCTransport<Request, Response, Notification> {
+public:
+  Transport(lldb::IOObjectSP in, lldb::IOObjectSP out,
+            LogCallback log_callback = {});
+  virtual ~Transport() = default;
+
+  /// Transport is not copyable.
+  /// @{
+  Transport(const Transport &) = delete;
+  void operator=(const Transport &) = delete;
+  /// @}
+
+  void Log(llvm::StringRef message) override;
+
+private:
+  LogCallback m_log_callback;
+};
+
+} // namespace lldb_protocol::mcp
+
+#endif
diff --git a/lldb/source/Plugins/Protocol/MCP/ProtocolServerMCP.cpp b/lldb/source/Plugins/Protocol/MCP/ProtocolServerMCP.cpp
index 12cb25788534e..2b004c19e88a6 100644
--- a/lldb/source/Plugins/Protocol/MCP/ProtocolServerMCP.cpp
+++ b/lldb/source/Plugins/Protocol/MCP/ProtocolServerMCP.cpp
@@ -70,9 +70,9 @@ void ProtocolServerMCP::AcceptCallback(std::unique_ptr<Socket> socket) {
   LLDB_LOG(log, "New MCP client connected: {0}", client_name);
 
   lldb::IOObjectSP io_sp = std::move(socket);
-  auto transport_up = std::make_unique<lldb_protocol::mcp::MCPTransport>(
-      io_sp, io_sp, std::move(client_name), [&](llvm::StringRef message) {
-        LLDB_LOG(GetLog(LLDBLog::Host), "{0}", message);
+  auto transport_up = std::make_unique<lldb_protocol::mcp::Transport>(
+      io_sp, io_sp, [client_name](llvm::StringRef message) {
+        LLDB_LOG(GetLog(LLDBLog::Host), "{0}: {1}", client_name, message);
       });
   auto instance_up = std::make_unique<lldb_protocol::mcp::Server>(
       std::string(kName), std::string(kVersion), std::move(transport_up),
diff --git a/lldb/source/Protocol/MCP/CMakeLists.txt b/lldb/source/Protocol/MCP/CMakeLists.txt
index a4f270a83c43b..5258cb61a7d10 100644
--- a/lldb/source/Protocol/MCP/CMakeLists.txt
+++ b/lldb/source/Protocol/MCP/CMakeLists.txt
@@ -3,6 +3,7 @@ add_lldb_library(lldbProtocolMCP NO_PLUGIN_DEPENDENCIES
   Protocol.cpp
   Server.cpp
   Tool.cpp
+  Transport.cpp
 
   LINK_COMPONENTS
     Support
diff --git a/lldb/source/Protocol/MCP/Transport.cpp b/lldb/source/Protocol/MCP/Transport.cpp
new file mode 100644
index 0000000000000..cccdc3b5bd65c
--- /dev/null
+++ b/lldb/source/Protocol/MCP/Transport.cpp
@@ -0,0 +1,23 @@
+//===----------------------------------------------------------------------===//
+//
+// 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 "lldb/Protocol/MCP/Transport.h"
+#include "llvm/ADT/StringRef.h"
+#include <utility>
+
+using namespace lldb_protocol::mcp;
+using namespace llvm;
+
+Transport::Transport(lldb::IOObjectSP in, lldb::IOObjectSP out,
+                     LogCallback log_callback)
+    : JSONRPCTransport(in, out), m_log_callback(std::move(log_callback)) {}
+
+void Transport::Log(StringRef message) {
+  if (m_log_callback)
+    m_log_callback(message);
+}
diff --git a/lldb/unittests/Protocol/ProtocolMCPServerTest.cpp b/lldb/unittests/Protocol/ProtocolMCPServerTest.cpp
index 9fa446133d46f..f686255c6d41d 100644
--- a/lldb/unittests/Protocol/ProtocolMCPServerTest.cpp
+++ b/lldb/unittests/Protocol/ProtocolMCPServerTest.cpp
@@ -21,6 +21,7 @@
 #include "lldb/Protocol/MCP/Resource.h"
 #include "lldb/Protocol/MCP/Server.h"
 #include "lldb/Protocol/MCP/Tool.h"
+#include "lldb/Protocol/MCP/Transport.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/JSON.h"
@@ -36,19 +37,6 @@ using namespace lldb_private;
 using namespace lldb_protocol::mcp;
 
 namespace {
-class TestMCPTransport final : public MCPTransport {
-public:
-  TestMCPTransport(lldb::IOObjectSP in, lldb::IOObjectSP out)
-      : lldb_protocol::mcp::MCPTransport(in, out, "unittest") {}
-
-  using MCPTransport::Write;
-
-  void Log(llvm::StringRef message) override {
-    log_messages.emplace_back(message);
-  }
-
-  std::vector<std::string> log_messages;
-};
 
 class TestServer : public Server {
 public:
@@ -134,7 +122,7 @@ class ProtocolServerMCPTest : public PipePairTest {
 public:
   SubsystemRAII<FileSystem, HostInfo, Socket> subsystems;
 
-  std::unique_ptr<TestMCPTransport> transport_up;
+  std::unique_ptr<lldb_protocol::mcp::Transport> transport_up;
   std::unique_ptr<TestServer> server_up;
   MainLoop loop;
   MockMessageHandler<Request, Response, Notification> message_handler;
@@ -163,7 +151,7 @@ class ProtocolServerMCPTest : public PipePairTest {
   void SetUp() override {
     PipePairTest::SetUp();
 
-    transport_up = std::make_unique<TestMCPTransport>(
+    transport_up = std::make_unique<lldb_protocol::mcp::Transport>(
         std::make_shared<NativeFile>(input.GetReadFileDescriptor(),
                                      File::eOpenOptionReadOnly,
                                      NativeFile::Unowned),
@@ -173,7 +161,7 @@ class ProtocolServerMCPTest : public PipePairTest {
 
     server_up = std::make_unique<TestServer>(
         "lldb-mcp", "0.1.0",
-        std::make_unique<TestMCPTransport>(
+        std::make_unique<lldb_protocol::mcp::Transport>(
             std::make_shared<NativeFile>(output.GetReadFileDescriptor(),
                                          File::eOpenOptionReadOnly,
                                          NativeFile::Unowned),

@ashgti ashgti merged commit 65f60fd into llvm:main Sep 4, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants