Cherrypick lldb-mcp improvements

lldb/include/lldb/Host/JSONTransport.h

@@ -68,6 +68,8 @@ class InvalidParams : public llvm::ErrorInfo<InvalidParams> {
 public:
   static char ID;
 
+  static constexpr int kErrorCode = -32602;
+
   explicit InvalidParams(std::string method, std::string context)
       : m_method(std::move(method)), m_context(std::move(context)) {}
 
@@ -83,6 +85,25 @@ class InvalidParams : public llvm::ErrorInfo<InvalidParams> {
   std::string m_context;
 };
 
+/// An error to indicate that an incoming message could not be parsed as a
+/// valid protocol message.
+class InvalidMessage : public llvm::ErrorInfo<InvalidMessage> {
+public:
+  static char ID;
+
+  static constexpr int kErrorCode = -32700;
+
+  explicit InvalidMessage(std::string raw_message, std::string reason)
+      : m_raw_message(std::move(raw_message)), m_reason(std::move(reason)) {}
+
+  void log(llvm::raw_ostream &OS) const override;
+  std::error_code convertToErrorCode() const override;
+
+private:
+  std::string m_raw_message;
+  std::string m_reason;
+};
+
 /// An error to indicate that no handler was registered for a given method.
 class MethodNotFound : public llvm::ErrorInfo<MethodNotFound> {
 public:
@@ -139,6 +160,14 @@ class JSONTransport {
   /// Sends a response to a specific request.
   virtual llvm::Error Send(const Resp &) = 0;
 
+  /// Sends an error response for a message that failed to parse, described by
+  /// \p reason. Sends nothing if no request id can be recovered from it, since
+  /// there is then no request to respond to.
+  virtual llvm::Error ReplyWithParseError(llvm::StringRef raw_message,
+                                          llvm::StringRef reason) {
+    return llvm::Error::success();
+  }
+
   /// Implemented to handle incoming messages. (See `RegisterMessageHandler()`
   /// below).
   class MessageHandler {
@@ -245,8 +274,15 @@ template <typename Proto> class IOTransport : public JSONTransport<Proto> {
         llvm::Expected<Message> message =
             llvm::json::parse<Message>(raw_message);
         if (!message) {
-          handler.OnError(message.takeError());
-          return;
+          // Messages are independent, so one that fails to parse must not
+          // discard those already buffered behind it.
+          std::string reason = llvm::toString(message.takeError());
+          if (llvm::Error error =
+                  this->ReplyWithParseError(raw_message, reason))
+            handler.OnError(std::move(error));
+          handler.OnError(
+              llvm::make_error<InvalidMessage>(raw_message, std::move(reason)));
+          continue;
         }
 
         std::visit([&handler](auto &&msg) { handler.Received(msg); }, *message);
@@ -546,7 +582,7 @@ class Binder : public JSONTransport<Proto>::MessageHandler {
     auto it = m_event_handlers.find(Proto::KeyFor(evt));
     if (it == m_event_handlers.end()) {
       OnError(llvm::createStringError(
-          llvm::formatv("no handled for event {0}", toJSON(evt))));
+          llvm::formatv("no handler for event {0}", toJSON(evt))));
       return;
     }
     it->second(evt);
@@ -558,7 +594,8 @@ class Binder : public JSONTransport<Proto>::MessageHandler {
     std::scoped_lock<std::recursive_mutex> guard(m_mutex);
     auto it = m_request_handlers.find(Proto::KeyFor(req));
     if (it == m_request_handlers.end()) {
-      reply(Proto::Make(req, llvm::createStringError("method not found")));
+      reply(Proto::Make(req,
+                        llvm::make_error<MethodNotFound>(Proto::KeyFor(req))));
       return;
     }
 

lldb/include/lldb/Protocol/MCP/Transport.h

@@ -95,6 +95,9 @@ class Transport final
 
   void Log(llvm::StringRef message) override;
 
+  llvm::Error ReplyWithParseError(llvm::StringRef raw_message,
+                                  llvm::StringRef reason) override;
+
 private:
   LogCallback m_log_callback;
 };

lldb/source/Host/common/JSONTransport.cpp

@@ -37,7 +37,19 @@ void InvalidParams::log(raw_ostream &OS) const {
      << "'";
 }
 std::error_code InvalidParams::convertToErrorCode() const {
-  return std::make_error_code(std::errc::invalid_argument);
+  // JSON-RPC Invalid params
+  return std::error_code(InvalidParams::kErrorCode, std::generic_category());
+}
+
+char InvalidMessage::ID;
+
+void InvalidMessage::log(raw_ostream &OS) const {
+  OS << "invalid message '" << m_raw_message << "': '" << m_reason << "'";
+}
+
+std::error_code InvalidMessage::convertToErrorCode() const {
+  // JSON-RPC Parse error
+  return std::error_code(InvalidMessage::kErrorCode, std::generic_category());
 }
 
 char MethodNotFound::ID;

lldb/source/Plugins/Protocol/MCP/Tool.cpp

@@ -183,6 +183,10 @@ DebuggerCreateTool::Call(const lldb_protocol::mcp::ToolArguments &) {
   debugger_sp->SetOutputFile(out);
   debugger_sp->SetErrorFile(out);
 
+  // A debugger driven over MCP has no event loop to service asynchronous
+  // stops, so a resume must not return before the process has stopped.
+  debugger_sp->SetAsyncExecution(false);
+
   return createTextResult(to_uri(debugger_sp));
 }
 

lldb/source/Protocol/MCP/Transport.cpp

@@ -22,3 +22,32 @@ void Transport::Log(StringRef message) {
   if (m_log_callback)
     m_log_callback(message);
 }
+
+llvm::Error Transport::ReplyWithParseError(StringRef raw_message,
+                                           StringRef reason) {
+  llvm::Expected<json::Value> value = json::parse(raw_message);
+  if (!value) {
+    // JSON-RPC forbids guessing an id, and malformed JSON carries none.
+    consumeError(value.takeError());
+    return llvm::Error::success();
+  }
+
+  const json::Object *object = value->getAsObject();
+  if (!object)
+    return llvm::Error::success();
+
+  // A message without an id is a notification, which takes no response.
+  const json::Value *raw_id = object->get("id");
+  if (!raw_id)
+    return llvm::Error::success();
+
+  Id id;
+  if (std::optional<StringRef> str = raw_id->getAsString())
+    id = str->str();
+  else if (std::optional<int64_t> num = raw_id->getAsInteger())
+    id = *num;
+  else
+    return llvm::Error::success();
+
+  return Send(Response{id, mcp::Error{eErrorCodeInvalidRequest, reason.str()}});
+}

lldb/test/CMakeLists.txt

@@ -126,6 +126,10 @@ if(TARGET lldb-dap)
   add_lldb_test_dependency(lldb-dap)
 endif()
 
+if(TARGET lldb-mcp)
+  add_lldb_test_dependency(lldb-mcp)
+endif()
+
 if(TARGET liblldb)
   add_lldb_test_dependency(liblldb)
 endif()

lldb/test/Shell/MCP/TestHelp.test

@@ -0,0 +1,7 @@
+# RUN: lldb-mcp --help | FileCheck %s
+# RUN: lldb-mcp -h | FileCheck %s
+# CHECK: OVERVIEW: LLDB MCP
+# CHECK: USAGE: lldb-mcp
+# CHECK: OPTIONS:
+# CHECK: --help
+# CHECK: --version

lldb/test/Shell/MCP/TestUnknownArgument.test

@@ -0,0 +1,3 @@
+# RUN: not lldb-mcp --bogus 2>&1 | FileCheck %s
+# CHECK: error: unknown argument '--bogus'
+# CHECK: USAGE: lldb-mcp

lldb/test/Shell/MCP/TestVersion.test

@@ -0,0 +1,4 @@
+# RUN: lldb-mcp --version | FileCheck %s
+# RUN: lldb-mcp -v | FileCheck %s
+# CHECK: lldb-mcp{{.*}}:
+# CHECK: liblldb:

lldb/test/Shell/helper/toolchain.py

@@ -168,6 +168,7 @@ def use_lldb_substitutions(config):
         ),
         "lldb-test",
         "lldb-dap",
+        "lldb-mcp",
         ToolSubst(
             "%build", command="'" + sys.executable + "'", extra_args=build_script_args
         ),

lldb/tools/lldb-mcp/lldb-mcp.cpp

@@ -25,9 +25,11 @@
 #include "lldb/lldb-forward.h"
 #include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/InitLLVM.h"
+#include "llvm/Support/Path.h"
 #include "llvm/Support/Process.h"
 #include "llvm/Support/Signals.h"
 #include "llvm/Support/WithColor.h"
@@ -117,6 +119,24 @@ llvm::Error connectBackend(lldb_mcp::Multiplexer &multiplexer, MainLoop &loop,
   return llvm::Error::success();
 }
 
+void printHelp(StringRef tool_name) {
+  outs() << "OVERVIEW: LLDB MCP\n\nUSAGE: " << tool_name << " [options]\n";
+  outs() << R"___(
+Multiplexes the Model Context Protocol over stdio across the running LLDB
+instances, and hosts its own debug sessions.
+
+OPTIONS:
+  -h, --help     Display this help message
+  -v, --version  Display the version
+)___";
+}
+
+void printVersion(StringRef tool_name) {
+  outs() << tool_name << ": ";
+  cl::PrintVersionMessage();
+  outs() << "liblldb: " << SBDebugger::GetVersionString() << '\n';
+}
+
 } // namespace
 
 int main(int argc, char *argv[]) {
@@ -141,6 +161,22 @@ int main(int argc, char *argv[]) {
   assert(result);
 #endif
 
+  StringRef tool_name = sys::path::filename(argv[0]);
+  for (int i = 1; i < argc; ++i) {
+    StringRef arg(argv[i]);
+    if (arg == "-h" || arg == "--help") {
+      printHelp(tool_name);
+      return EXIT_SUCCESS;
+    }
+    if (arg == "-v" || arg == "--version") {
+      printVersion(tool_name);
+      return EXIT_SUCCESS;
+    }
+    WithColor::error(errs()) << "unknown argument '" << arg << "'\n";
+    printHelp(tool_name);
+    return EXIT_FAILURE;
+  }
+
   // Bring up the debug engine (through the public SB API) so lldb-mcp can host
   // debug sessions in its own process.
   SBDebugger::Initialize();

lldb/unittests/Host/JSONTransportTest.cpp

@@ -535,6 +535,19 @@ TEST_F(JSONRPCTransportTest, MalformedRequests) {
   ASSERT_THAT_ERROR(Run(), Succeeded());
 }
 
+TEST_F(JSONRPCTransportTest, MalformedRequestDoesNotDropLaterMessages) {
+  InSequence seq;
+  std::string messages =
+      "notjson\n" + Encode(Message{Req{1, "foo", std::nullopt}});
+  ASSERT_THAT_EXPECTED(input.Write(messages.data(), messages.size()),
+                       Succeeded());
+  EXPECT_CALL(message_handler, OnError(_)).WillOnce([](llvm::Error err) {
+    consumeError(std::move(err));
+  });
+  EXPECT_CALL(message_handler, Received(Req{1, "foo", std::nullopt}));
+  ASSERT_THAT_ERROR(Run(), Succeeded());
+}
+
 TEST_F(JSONRPCTransportTest, Read) {
   Write(Message{Req{1, "foo", std::nullopt}});
   EXPECT_CALL(message_handler, Received(Req{1, "foo", std::nullopt}));
@@ -812,6 +825,14 @@ TEST_F(TransportBinderTest, InBoundAsyncRequestsError) {
   Run();
 }
 
+TEST_F(TransportBinderTest, InBoundRequestUnknownMethod) {
+  EXPECT_THAT_ERROR(from_remote->Send(Req{4, "nosuch", MyFnParams{1, 2}}),
+                    Succeeded());
+  EXPECT_CALL(remote, Received(Resp{4, MethodNotFound::kErrorCode,
+                                    "method not found: 'nosuch'"}));
+  Run();
+}
+
 TEST_F(TransportBinderTest, FailPendingRequests) {
   OutgoingRequest<MyFnResult, MyFnParams> addFn =
       binder->Bind<MyFnResult, MyFnParams>("add");