[lldb] Add Model Context Protocol (MCP) support to LLDB #143628
Conversation
JDevlieghere
force-pushed
the
MCP
branch
9 times, most recently
from
3c1cb49 to
4156ce2
Compare
JDevlieghere
changed the title
|
@llvm/pr-subscribers-lldb Author: Jonas Devlieghere (JDevlieghere) ChangesThis PR adds an MCP (Model Context Protocol ) server to LLDB. For motivation and background, please refer to the corresponding RFC: https://discourse.llvm.org/t/rfc-adding-mcp-support-to-lldb/86798 I implemented this as a new kind of plugin. The idea is that we could support multiple protocol servers (e.g. if we want to support DAP from within LLDB). This also introduces a corresponding top-level command ( The MCP sever supports one tool ( Patch is 71.14 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/143628.diff 32 Files Affected:
diff --git a/lldb/include/lldb/Core/Debugger.h b/lldb/include/lldb/Core/Debugger.h
index d73aba1e3ce58..0f6659d1a0bf7 100644
--- a/lldb/include/lldb/Core/Debugger.h
+++ b/lldb/include/lldb/Core/Debugger.h
@@ -598,6 +598,10 @@ class Debugger : public std::enable_shared_from_this<Debugger>,
void FlushProcessOutput(Process &process, bool flush_stdout,
bool flush_stderr);
+ void AddProtocolServer(lldb::ProtocolServerSP protocol_server_sp);
+ void RemoveProtocolServer(lldb::ProtocolServerSP protocol_server_sp);
+ lldb::ProtocolServerSP GetProtocolServer(llvm::StringRef protocol) const;
+
SourceManager::SourceFileCache &GetSourceFileCache() {
return m_source_file_cache;
}
@@ -768,6 +772,8 @@ class Debugger : public std::enable_shared_from_this<Debugger>,
mutable std::mutex m_progress_reports_mutex;
/// @}
+ llvm::SmallVector<lldb::ProtocolServerSP> m_protocol_servers;
+
std::mutex m_destroy_callback_mutex;
lldb::callback_token_t m_destroy_callback_next_token = 0;
struct DestroyCallbackInfo {
diff --git a/lldb/include/lldb/Core/PluginManager.h b/lldb/include/lldb/Core/PluginManager.h
index e7b1691031111..e50bf97189cfc 100644
--- a/lldb/include/lldb/Core/PluginManager.h
+++ b/lldb/include/lldb/Core/PluginManager.h
@@ -327,6 +327,17 @@ class PluginManager {
static void AutoCompleteProcessName(llvm::StringRef partial_name,
CompletionRequest &request);
+ // Protocol
+ static bool RegisterPlugin(llvm::StringRef name, llvm::StringRef description,
+ ProtocolServerCreateInstance create_callback);
+
+ static bool UnregisterPlugin(ProtocolServerCreateInstance create_callback);
+
+ static llvm::StringRef GetProtocolServerPluginNameAtIndex(uint32_t idx);
+
+ static ProtocolServerCreateInstance
+ GetProtocolCreateCallbackForPluginName(llvm::StringRef name);
+
// Register Type Provider
static bool RegisterPlugin(llvm::StringRef name, llvm::StringRef description,
RegisterTypeBuilderCreateInstance create_callback);
diff --git a/lldb/include/lldb/Core/ProtocolServer.h b/lldb/include/lldb/Core/ProtocolServer.h
new file mode 100644
index 0000000000000..fafe460904323
--- /dev/null
+++ b/lldb/include/lldb/Core/ProtocolServer.h
@@ -0,0 +1,39 @@
+//===-- ProtocolServer.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_CORE_PROTOCOLSERVER_H
+#define LLDB_CORE_PROTOCOLSERVER_H
+
+#include "lldb/Core/PluginInterface.h"
+#include "lldb/Host/Socket.h"
+#include "lldb/lldb-private-interfaces.h"
+
+namespace lldb_private {
+
+class ProtocolServer : public PluginInterface {
+public:
+ ProtocolServer() = default;
+ virtual ~ProtocolServer() = default;
+
+ static lldb::ProtocolServerSP Create(llvm::StringRef name,
+ Debugger &debugger);
+
+ struct Connection {
+ Socket::SocketProtocol protocol;
+ std::string name;
+ };
+
+ virtual llvm::Error Start(Connection connection) = 0;
+ virtual llvm::Error Stop() = 0;
+
+ virtual Socket *GetSocket() const = 0;
+};
+
+} // namespace lldb_private
+
+#endif
diff --git a/lldb/include/lldb/Interpreter/CommandOptionArgumentTable.h b/lldb/include/lldb/Interpreter/CommandOptionArgumentTable.h
index 8535dfcf46da5..4face717531b1 100644
--- a/lldb/include/lldb/Interpreter/CommandOptionArgumentTable.h
+++ b/lldb/include/lldb/Interpreter/CommandOptionArgumentTable.h
@@ -315,6 +315,7 @@ static constexpr CommandObject::ArgumentTableEntry g_argument_table[] = {
{ lldb::eArgTypeCPUName, "cpu-name", lldb::CompletionType::eNoCompletion, {}, { nullptr, false }, "The name of a CPU." },
{ lldb::eArgTypeCPUFeatures, "cpu-features", lldb::CompletionType::eNoCompletion, {}, { nullptr, false }, "The CPU feature string." },
{ lldb::eArgTypeManagedPlugin, "managed-plugin", lldb::CompletionType::eNoCompletion, {}, { nullptr, false }, "Plugins managed by the PluginManager" },
+ { lldb::eArgTypeProtocol, "protocol", lldb::CompletionType::eNoCompletion, {}, { nullptr, false }, "The name of the protocol." },
// clang-format on
};
diff --git a/lldb/include/lldb/lldb-enumerations.h b/lldb/include/lldb/lldb-enumerations.h
index eeb7299a354e1..69e8671b6e21b 100644
--- a/lldb/include/lldb/lldb-enumerations.h
+++ b/lldb/include/lldb/lldb-enumerations.h
@@ -664,6 +664,7 @@ enum CommandArgumentType {
eArgTypeCPUName,
eArgTypeCPUFeatures,
eArgTypeManagedPlugin,
+ eArgTypeProtocol,
eArgTypeLastArg // Always keep this entry as the last entry in this
// enumeration!!
};
diff --git a/lldb/include/lldb/lldb-forward.h b/lldb/include/lldb/lldb-forward.h
index c664d1398f74d..558818e8e2309 100644
--- a/lldb/include/lldb/lldb-forward.h
+++ b/lldb/include/lldb/lldb-forward.h
@@ -164,13 +164,13 @@ class PersistentExpressionState;
class Platform;
class Process;
class ProcessAttachInfo;
-class ProcessLaunchInfo;
class ProcessInfo;
class ProcessInstanceInfo;
class ProcessInstanceInfoMatch;
class ProcessLaunchInfo;
class ProcessModID;
class Property;
+class ProtocolServer;
class Queue;
class QueueImpl;
class QueueItem;
@@ -391,6 +391,7 @@ typedef std::shared_ptr<lldb_private::Platform> PlatformSP;
typedef std::shared_ptr<lldb_private::Process> ProcessSP;
typedef std::shared_ptr<lldb_private::ProcessAttachInfo> ProcessAttachInfoSP;
typedef std::shared_ptr<lldb_private::ProcessLaunchInfo> ProcessLaunchInfoSP;
+typedef std::shared_ptr<lldb_private::ProtocolServer> ProtocolServerSP;
typedef std::weak_ptr<lldb_private::Process> ProcessWP;
typedef std::shared_ptr<lldb_private::RegisterCheckpoint> RegisterCheckpointSP;
typedef std::shared_ptr<lldb_private::RegisterContext> RegisterContextSP;
diff --git a/lldb/include/lldb/lldb-private-interfaces.h b/lldb/include/lldb/lldb-private-interfaces.h
index d366dbd1d7832..34eaaa8e581e9 100644
--- a/lldb/include/lldb/lldb-private-interfaces.h
+++ b/lldb/include/lldb/lldb-private-interfaces.h
@@ -81,6 +81,8 @@ typedef lldb::PlatformSP (*PlatformCreateInstance)(bool force,
typedef lldb::ProcessSP (*ProcessCreateInstance)(
lldb::TargetSP target_sp, lldb::ListenerSP listener_sp,
const FileSpec *crash_file_path, bool can_connect);
+typedef lldb::ProtocolServerSP (*ProtocolServerCreateInstance)(
+ Debugger &debugger);
typedef lldb::RegisterTypeBuilderSP (*RegisterTypeBuilderCreateInstance)(
Target &target);
typedef lldb::ScriptInterpreterSP (*ScriptInterpreterCreateInstance)(
diff --git a/lldb/source/Commands/CMakeLists.txt b/lldb/source/Commands/CMakeLists.txt
index 1ea51acec5f15..69e4c45f0b8e5 100644
--- a/lldb/source/Commands/CMakeLists.txt
+++ b/lldb/source/Commands/CMakeLists.txt
@@ -23,6 +23,7 @@ add_lldb_library(lldbCommands NO_PLUGIN_DEPENDENCIES
CommandObjectPlatform.cpp
CommandObjectPlugin.cpp
CommandObjectProcess.cpp
+ CommandObjectProtocolServer.cpp
CommandObjectQuit.cpp
CommandObjectRegexCommand.cpp
CommandObjectRegister.cpp
diff --git a/lldb/source/Commands/CommandObjectProtocolServer.cpp b/lldb/source/Commands/CommandObjectProtocolServer.cpp
new file mode 100644
index 0000000000000..3ee1a0ccc4ffa
--- /dev/null
+++ b/lldb/source/Commands/CommandObjectProtocolServer.cpp
@@ -0,0 +1,184 @@
+//===-- CommandObjectProtocolServer.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 "CommandObjectProtocolServer.h"
+#include "lldb/Core/PluginManager.h"
+#include "lldb/Core/ProtocolServer.h"
+#include "lldb/Host/Socket.h"
+#include "lldb/Interpreter/CommandInterpreter.h"
+#include "lldb/Interpreter/CommandReturnObject.h"
+#include "lldb/Utility/UriParser.h"
+#include "llvm/ADT/STLExtras.h"
+#include "llvm/Support/FormatAdapters.h"
+
+using namespace llvm;
+using namespace lldb;
+using namespace lldb_private;
+
+#define LLDB_OPTIONS_mcp
+#include "CommandOptions.inc"
+
+static std::vector<llvm::StringRef> GetSupportedProtocols() {
+ std::vector<llvm::StringRef> supported_protocols;
+ size_t i = 0;
+
+ for (llvm::StringRef protocol_name =
+ PluginManager::GetProtocolServerPluginNameAtIndex(i++);
+ !protocol_name.empty();
+ protocol_name = PluginManager::GetProtocolServerPluginNameAtIndex(i++)) {
+ supported_protocols.push_back(protocol_name);
+ }
+
+ return supported_protocols;
+}
+
+static llvm::Expected<std::pair<Socket::SocketProtocol, std::string>>
+validateConnection(llvm::StringRef conn) {
+ auto uri = lldb_private::URI::Parse(conn);
+
+ if (uri && (uri->scheme == "tcp" || uri->scheme == "connect" ||
+ !uri->hostname.empty() || uri->port)) {
+ return std::make_pair(
+ Socket::ProtocolTcp,
+ formatv("[{0}]:{1}", uri->hostname.empty() ? "0.0.0.0" : uri->hostname,
+ uri->port.value_or(0)));
+ }
+
+ if (uri && (uri->scheme == "unix" || uri->scheme == "unix-connect" ||
+ uri->path != "/")) {
+ return std::make_pair(Socket::ProtocolUnixDomain, uri->path.str());
+ }
+
+ return llvm::createStringError(
+ "Unsupported connection specifier, expected 'unix-connect:///path' or "
+ "'connect://[host]:port', got '%s'.",
+ conn.str().c_str());
+}
+
+class CommandObjectProtocolServerStart : public CommandObjectParsed {
+public:
+ CommandObjectProtocolServerStart(CommandInterpreter &interpreter)
+ : CommandObjectParsed(interpreter, "mcp start", "start MCP server",
+ "mcp start <connection>") {
+ AddSimpleArgumentList(lldb::eArgTypeProtocol, eArgRepeatPlain);
+ AddSimpleArgumentList(lldb::eArgTypeConnectURL, eArgRepeatPlain);
+ }
+
+ ~CommandObjectProtocolServerStart() override = default;
+
+protected:
+ void DoExecute(Args &args, CommandReturnObject &result) override {
+ if (args.GetArgumentCount() < 1) {
+ result.AppendError("no protocol specified");
+ return;
+ }
+
+ llvm::StringRef protocol = args.GetArgumentAtIndex(0);
+ std::vector<llvm::StringRef> supported_protocols = GetSupportedProtocols();
+ if (llvm::find(supported_protocols, protocol) ==
+ supported_protocols.end()) {
+ result.AppendErrorWithFormatv(
+ "unsupported protocol: {0}. Supported protocols are: {1}", protocol,
+ llvm::join(GetSupportedProtocols(), ", "));
+ return;
+ }
+
+ if (args.GetArgumentCount() < 2) {
+ result.AppendError("no connection specified");
+ return;
+ }
+ llvm::StringRef connection_uri = args.GetArgumentAtIndex(1);
+
+ ProtocolServerSP server_sp = GetDebugger().GetProtocolServer(protocol);
+ if (!server_sp)
+ server_sp = ProtocolServer::Create(protocol, GetDebugger());
+
+ auto maybeProtoclAndName = validateConnection(connection_uri);
+ if (auto error = maybeProtoclAndName.takeError()) {
+ result.AppendErrorWithFormatv("{0}", llvm::fmt_consume(std::move(error)));
+ return;
+ }
+
+ ProtocolServer::Connection connection;
+ std::tie(connection.protocol, connection.name) = *maybeProtoclAndName;
+
+ if (llvm::Error error = server_sp->Start(connection)) {
+ result.AppendErrorWithFormatv("{0}", llvm::fmt_consume(std::move(error)));
+ return;
+ }
+
+ GetDebugger().AddProtocolServer(server_sp);
+
+ if (Socket *socket = server_sp->GetSocket()) {
+ std::string address =
+ llvm::join(socket->GetListeningConnectionURI(), ", ");
+ result.AppendMessageWithFormatv(
+ "{0} server started with connection listeners: {1}", protocol,
+ address);
+ }
+ }
+};
+
+class CommandObjectProtocolServerStop : public CommandObjectParsed {
+public:
+ CommandObjectProtocolServerStop(CommandInterpreter &interpreter)
+ : CommandObjectParsed(interpreter, "protocol-server stop",
+ "stop protocol server", "protocol-server stop") {
+ AddSimpleArgumentList(lldb::eArgTypeProtocol, eArgRepeatPlain);
+ }
+
+ ~CommandObjectProtocolServerStop() override = default;
+
+protected:
+ void DoExecute(Args &args, CommandReturnObject &result) override {
+ if (args.GetArgumentCount() < 1) {
+ result.AppendError("no protocol specified");
+ return;
+ }
+
+ llvm::StringRef protocol = args.GetArgumentAtIndex(0);
+ std::vector<llvm::StringRef> supported_protocols = GetSupportedProtocols();
+ if (llvm::find(supported_protocols, protocol) ==
+ supported_protocols.end()) {
+ result.AppendErrorWithFormatv(
+ "unsupported protocol: {0}. Supported protocols are: {1}", protocol,
+ llvm::join(GetSupportedProtocols(), ", "));
+ return;
+ }
+
+ Debugger &debugger = GetDebugger();
+
+ ProtocolServerSP server_sp = debugger.GetProtocolServer(protocol);
+ if (!server_sp) {
+ result.AppendError(
+ llvm::formatv("no {0} protocol server running", protocol).str());
+ return;
+ }
+
+ if (llvm::Error error = server_sp->Stop()) {
+ result.AppendErrorWithFormatv("{0}", llvm::fmt_consume(std::move(error)));
+ return;
+ }
+
+ debugger.RemoveProtocolServer(server_sp);
+ }
+};
+
+CommandObjectProtocolServer::CommandObjectProtocolServer(
+ CommandInterpreter &interpreter)
+ : CommandObjectMultiword(interpreter, "protocol-server",
+ "Start and stop a protocol server.",
+ "protocol-server") {
+ LoadSubCommand("start", CommandObjectSP(new CommandObjectProtocolServerStart(
+ interpreter)));
+ LoadSubCommand("stop", CommandObjectSP(
+ new CommandObjectProtocolServerStop(interpreter)));
+}
+
+CommandObjectProtocolServer::~CommandObjectProtocolServer() = default;
diff --git a/lldb/source/Commands/CommandObjectProtocolServer.h b/lldb/source/Commands/CommandObjectProtocolServer.h
new file mode 100644
index 0000000000000..3591216b014cb
--- /dev/null
+++ b/lldb/source/Commands/CommandObjectProtocolServer.h
@@ -0,0 +1,25 @@
+//===-- CommandObjectProtocolServer.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_SOURCE_COMMANDS_COMMANDOBJECTPROTOCOLSERVER_H
+#define LLDB_SOURCE_COMMANDS_COMMANDOBJECTPROTOCOLSERVER_H
+
+#include "lldb/Interpreter/CommandObjectMultiword.h"
+
+namespace lldb_private {
+
+class CommandObjectProtocolServer : public CommandObjectMultiword {
+public:
+ CommandObjectProtocolServer(CommandInterpreter &interpreter);
+ ~CommandObjectProtocolServer() override;
+};
+
+} // namespace lldb_private
+
+#endif // LLDB_SOURCE_COMMANDS_COMMANDOBJECTMCP_H
diff --git a/lldb/source/Core/CMakeLists.txt b/lldb/source/Core/CMakeLists.txt
index d6b75bca7f2d6..df35bd5c025f3 100644
--- a/lldb/source/Core/CMakeLists.txt
+++ b/lldb/source/Core/CMakeLists.txt
@@ -46,6 +46,7 @@ add_lldb_library(lldbCore NO_PLUGIN_DEPENDENCIES
Opcode.cpp
PluginManager.cpp
Progress.cpp
+ ProtocolServer.cpp
Statusline.cpp
RichManglingContext.cpp
SearchFilter.cpp
diff --git a/lldb/source/Core/Debugger.cpp b/lldb/source/Core/Debugger.cpp
index 81037d3def811..2bc9c7ead79d3 100644
--- a/lldb/source/Core/Debugger.cpp
+++ b/lldb/source/Core/Debugger.cpp
@@ -16,6 +16,7 @@
#include "lldb/Core/ModuleSpec.h"
#include "lldb/Core/PluginManager.h"
#include "lldb/Core/Progress.h"
+#include "lldb/Core/ProtocolServer.h"
#include "lldb/Core/StreamAsynchronousIO.h"
#include "lldb/Core/Telemetry.h"
#include "lldb/DataFormatters/DataVisualization.h"
@@ -2363,3 +2364,26 @@ llvm::ThreadPoolInterface &Debugger::GetThreadPool() {
"Debugger::GetThreadPool called before Debugger::Initialize");
return *g_thread_pool;
}
+
+void Debugger::AddProtocolServer(lldb::ProtocolServerSP protocol_server_sp) {
+ assert(protocol_server_sp &&
+ GetProtocolServer(protocol_server_sp->GetPluginName()) == nullptr);
+ m_protocol_servers.push_back(protocol_server_sp);
+}
+
+void Debugger::RemoveProtocolServer(lldb::ProtocolServerSP protocol_server_sp) {
+ auto it = llvm::find(m_protocol_servers, protocol_server_sp);
+ if (it != m_protocol_servers.end())
+ m_protocol_servers.erase(it);
+}
+
+lldb::ProtocolServerSP
+Debugger::GetProtocolServer(llvm::StringRef protocol) const {
+ for (ProtocolServerSP protocol_server_sp : m_protocol_servers) {
+ if (!protocol_server_sp)
+ continue;
+ if (protocol_server_sp->GetPluginName() == protocol)
+ return protocol_server_sp;
+ }
+ return nullptr;
+}
diff --git a/lldb/source/Core/PluginManager.cpp b/lldb/source/Core/PluginManager.cpp
index 5d44434033c55..a59a390e40bb6 100644
--- a/lldb/source/Core/PluginManager.cpp
+++ b/lldb/source/Core/PluginManager.cpp
@@ -1006,6 +1006,38 @@ void PluginManager::AutoCompleteProcessName(llvm::StringRef name,
}
}
+#pragma mark ProtocolServer
+
+typedef PluginInstance<ProtocolServerCreateInstance> ProtocolServerInstance;
+typedef PluginInstances<ProtocolServerInstance> ProtocolServerInstances;
+
+static ProtocolServerInstances &GetProtocolServerInstances() {
+ static ProtocolServerInstances g_instances;
+ return g_instances;
+}
+
+bool PluginManager::RegisterPlugin(
+ llvm::StringRef name, llvm::StringRef description,
+ ProtocolServerCreateInstance create_callback) {
+ return GetProtocolServerInstances().RegisterPlugin(name, description,
+ create_callback);
+}
+
+bool PluginManager::UnregisterPlugin(
+ ProtocolServerCreateInstance create_callback) {
+ return GetProtocolServerInstances().UnregisterPlugin(create_callback);
+}
+
+llvm::StringRef
+PluginManager::GetProtocolServerPluginNameAtIndex(uint32_t idx) {
+ return GetProtocolServerInstances().GetNameAtIndex(idx);
+}
+
+ProtocolServerCreateInstance
+PluginManager::GetProtocolCreateCallbackForPluginName(llvm::StringRef name) {
+ return GetProtocolServerInstances().GetCallbackForName(name);
+}
+
#pragma mark RegisterTypeBuilder
struct RegisterTypeBuilderInstance
diff --git a/lldb/source/Core/ProtocolServer.cpp b/lldb/source/Core/ProtocolServer.cpp
new file mode 100644
index 0000000000000..d57a047afa7b2
--- /dev/null
+++ b/lldb/source/Core/ProtocolServer.cpp
@@ -0,0 +1,21 @@
+//===-- ProtocolServer.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 "lldb/Core/ProtocolServer.h"
+#include "lldb/Core/PluginManager.h"
+
+using namespace lldb_private;
+using namespace lldb;
+
+ProtocolServerSP ProtocolServer::Create(llvm::StringRef name,
+ Debugger &debugger) {
+ if (ProtocolServerCreateInstance create_callback =
+ PluginManager::GetProtocolCreateCallbackForPluginName(name))
+ return create_callback(debugger);
+ return nullptr;
+}
diff --git a/lldb/source/Interpreter/CommandInterpreter.cpp b/lldb/source/Interpreter/CommandInterpreter.cpp
index 4f9ae104dedea..00c3472444d2e 100644
--- a/lldb/source/Interpreter/CommandInterpreter.cpp
+++ b/lldb/source/Interpreter/CommandInterpreter.cpp
@@ -30,6 +30,7 @@
#include "Commands/CommandObjectPlatform.h"
#include "Commands/CommandObjectPlugin.h"
#include "Commands/CommandObjectProcess.h"
+#include "Commands/CommandObjectProtocolServer.h"
#include "Commands/CommandObjectQuit.h"
#include "Commands/CommandObjectRegexCommand.h"
#include "Commands/CommandObjectRegister.h"
@@ -574,6 +575,7 @@ void CommandInterpreter::LoadCommandDictionary() {
REGISTER_COMMAND_OBJECT("...
[truncated]
|
JDevlieghere
requested review from
ashgti,
kastiglione,
walter-erquinigo and
jimingham
|
I'm adding the folks that chimed in on the RFC as reviewers. |
There was a problem hiding this comment.
Would it be worth it to include a file in lldb/docs/resources about running this? For folks wanting to get started trying this out?
Maybe in a follow up PR.
Also, thinking about the overall flow of the MCP server and the debugger instance.
I am curious how this would work with lldb-dap. In the DAP, we make a debugger instance for each debug session and it goes away at the end of the debug session. So, I may not be able to run both lldb-dap and an MCP server in the same debug session, or at least the DAP session would stop the MCP server once the debugger session is over.
Should the MCP server mode have some way of creating a debugger instance itself? Like, should each client have its own debugger instance? If so, that sort of inverts how this should be arranged such that the MCP server should own the debugger not the other way around.
| } | ||
|
|
||
| CommandReturnObject result(/*colors=*/false); | ||
| m_debugger.GetCommandInterpreter().HandleCommand(arguments.c_str(), |
There was a problem hiding this comment.
Should we prevent some commands from being executed here?
For example, quit?
There was a problem hiding this comment.
I know a team which tries to do that, but I'm not sure they realize how easily workaroundable that is (command alias print_help quit; print_help). Given what I've read about AI, I'm fairly certain it could come up with this sequence if it sets its mind to it.
To make this reliable, I think we'd at least need to resolve all aliases down to core commands.
There was a problem hiding this comment.
I've added a FIXME for now.
| struct ToolCapability { | ||
| bool listChanged = false; | ||
| }; |
There was a problem hiding this comment.
No changes needed, but reading up on the schema for this, I wonder if we could also support completion (https://modelcontextprotocol.io/specification/2025-03-26/server/utilities/completion#user-interaction-model). Maybe in the future that would be reasonable to implement.
There was a problem hiding this comment.
Yup, I think that's something we could support. Completion was added in the latest revision of the spec and to support older tools I've settled on the previous revision for now, but this might be a good motivator to move to the next version.
|
Reading more about this in the VSCode docs and on the MCP website I think I understand the flow of logic a bit more. I think this is definitely helpful for allowing an agent to help control a debug session. However, I'm still not sure how we should handle the MCP client and debugger instance association. The main limitation I see is being able to launch a debug session (either by starting a binary or attaching). I suppose that if you were to just use |
|
I've been thinking about this too. I think you're right to distinguish two use cases:
The current implementation focuses on (1). I think (2) is a lot a harder, because depending on where I'm using this, I might want it to do something different. For example, from Claude desktop, I probably just want to have the model interact with the debugger behind the scenes. But in VS Code, I want to see the debug session in the IDE, and it should use DAP and from another IDE that doesn't support DAP, it might be something different. I think it's a useful use case, but currently not one I'm motivated to solve. That said, it does impact the design, so I think it should be considered so that we don't come up with something that's fundamentally incompatible. The second problem is that Claude assumes you have a single MCP server instance. VS Code works in a very similar way, though you can specify different servers per project. But you can have multiple concurrent active debug sessions per project, so we need a way to support that. Right now you need to use netcat with a fixed address and port and that's obviously a pretty awful user experience. What I had in mind for that (and hinted at in the RFC) is to create a binary, If we go with the multiplexing approach, then I think it's fine for every debugger to have its own server instance. If we do something different, it might make more sense to have one server per lldb instance and do the "multiplexing" there by just using the debugger ID. That would work for |
|
|
||
| // Wait for all our clients to exit. | ||
| for (auto &client : m_clients) { | ||
| client.first->Close(); |
There was a problem hiding this comment.
This is a very wrong way to shut down a connection. The problem is that closed fd can get recycled and then the reading thread will end up reading from a random file. And since there's no way to guarantee this does not happen, linux doesn't even bother to support the case it theoretically could support: closing an fd while a thread is already blocked reading from it (as opposed to "a thread that is about to read from it) -- the thread will just end up blocked there ~forever.
You really need a way to have the thread wait for data on the connection and the termination signal. Currently, I think the only portable way we have is with MainLoop::AddPendingCallback, but since you already have a main loop for accepting the connections, maybe you could structure the code such that the loop also handles reading from those connections (and submits them to the worker thread via a (mutex-protected) std::queue or whatever)? Then it would be easy to make that wait for termination as well.
This is going to make reading serialized, but I doubt that's going to be the bottleneck here. And maybe then we don't need one thread for every connection, and we could have some sort of a thread pool of threads waiting to process requests?
There was a problem hiding this comment.
Thanks Pavel, I knew about all those things in isolation but I didn't put the pieces together. There's no reason we can't do this fully in the MainLoop.
| } | ||
|
|
||
| CommandReturnObject result(/*colors=*/false); | ||
| m_debugger.GetCommandInterpreter().HandleCommand(arguments.c_str(), |
There was a problem hiding this comment.
I know a team which tries to do that, but I'm not sure they realize how easily workaroundable that is (command alias print_help quit; print_help). Given what I've read about AI, I'm fairly certain it could come up with this sequence if it sets its mind to it.
To make this reliable, I think we'd at least need to resolve all aliases down to core commands.
|
| if (const json::Object *args_obj = args.getAsObject()) { | ||
| if (const json::Value *s = args_obj->get("arguments")) { | ||
| arguments = s->getAsString().value_or(""); | ||
| } | ||
| } |
There was a problem hiding this comment.
Should we add a helper for decoding the arguments the LLDB command expects? We have the schema specified, so I think we could define this type at least internally to this file.
|
Rebased and addressed @ashgti's comments:
|
This PR adds an MCP (Model Context Protocol ) server to LLDB. For motivation and background, please refer to the corresponding RFC: https://discourse.llvm.org/t/rfc-adding-mcp-support-to-lldb/86798
I implemented this as a new kind of plugin. The idea is that we could support multiple protocol servers (e.g. if we want to support DAP from within LLDB). This also introduces a corresponding top-level command (
protocol-server) with two subcommands tostartandstopthe server.The MCP sever supports one tool (
lldb_command) which executes a command, but can easily be extended with more commands.