[lldb-dap] Updating RequestHandler to encode/decode arguments and response. (llvm#130090)

This is a work in progress refactor to add explicit types instead of
generic 'llvm::json::Value' types to the DAP protocol.

This updates RequestHandler to have take the type of the arguments and
response body for serialization for requests.

The 'source' and 'disconnect' request is updated to show how the new
flow
works and includes serialization handling for optional arguments and
'void'
responses.

This is built on top of llvm#130026

---------

Co-authored-by: Adrian Vogelsgesang <[email protected]>

Author: ashgti

lldb/tools/lldb-dap/CMakeLists.txt

@@ -32,7 +32,6 @@ add_lldb_tool(lldb-dap
   LLDBUtils.cpp
   OutputRedirector.cpp
   ProgressEvent.cpp
-  Protocol.cpp
   RunInTerminal.cpp
   SourceBreakpoint.cpp
   Transport.cpp
@@ -74,6 +73,10 @@ add_lldb_tool(lldb-dap
   Handler/TestGetTargetBreakpointsRequestHandler.cpp
   Handler/ThreadsRequestHandler.cpp
   Handler/VariablesRequestHandler.cpp
+  
+  Protocol/ProtocolBase.cpp
+  Protocol/ProtocolTypes.cpp
+  Protocol/ProtocolRequests.cpp
 
   LINK_LIBS
     liblldb

lldb/tools/lldb-dap/DAP.cpp

@@ -8,10 +8,12 @@
 
 #include "DAP.h"
 #include "DAPLog.h"
+#include "Handler/RequestHandler.h"
 #include "Handler/ResponseHandler.h"
 #include "JSONUtils.h"
 #include "LLDBUtils.h"
 #include "OutputRedirector.h"
+#include "Protocol/ProtocolBase.h"
 #include "Transport.h"
 #include "lldb/API/SBBreakpoint.h"
 #include "lldb/API/SBCommandInterpreter.h"
@@ -25,6 +27,7 @@
 #include "lldb/lldb-defines.h"
 #include "lldb/lldb-enumerations.h"
 #include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringRef.h"
@@ -41,6 +44,7 @@
 #include <fstream>
 #include <memory>
 #include <mutex>
+#include <string>
 #include <utility>
 
 #if defined(_WIN32)
@@ -233,6 +237,10 @@ void DAP::SendJSON(const llvm::json::Value &json) {
                   transport.GetClientName());
     return;
   }
+  Send(message);
+}
+
+void DAP::Send(const protocol::Message &message) {
   if (llvm::Error err = transport.Write(message))
     DAP_LOG_ERROR(log, std::move(err), "({1}) write failed: {0}",
                   transport.GetClientName());
@@ -665,58 +673,65 @@ void DAP::SetTarget(const lldb::SBTarget target) {
 }
 
 bool DAP::HandleObject(const protocol::Message &M) {
-  // FIXME: Directly handle `Message` instead of serializing to JSON.
-  llvm::json::Value v = toJSON(M);
-  llvm::json::Object object = *v.getAsObject();
-  const auto packet_type = GetString(object, "type");
-  if (packet_type == "request") {
-    const auto command = GetString(object, "command").value_or("");
-
-    auto new_handler_pos = request_handlers.find(command);
-    if (new_handler_pos != request_handlers.end()) {
-      (*new_handler_pos->second)(object);
+  if (const auto *req = std::get_if<protocol::Request>(&M)) {
+    auto handler_pos = request_handlers.find(req->command);
+    if (handler_pos != request_handlers.end()) {
+      (*handler_pos->second)(*req);
       return true; // Success
     }
 
     DAP_LOG(log, "({0}) error: unhandled command '{1}'",
-            transport.GetClientName(), command);
+            transport.GetClientName(), req->command);
     return false; // Fail
   }
 
-  if (packet_type == "response") {
-    auto id = GetInteger<int64_t>(object, "request_seq").value_or(0);
-
+  if (const auto *resp = std::get_if<protocol::Response>(&M)) {
     std::unique_ptr<ResponseHandler> response_handler;
     {
       std::lock_guard<std::mutex> locker(call_mutex);
-      auto inflight = inflight_reverse_requests.find(id);
+      auto inflight = inflight_reverse_requests.find(resp->request_seq);
       if (inflight != inflight_reverse_requests.end()) {
         response_handler = std::move(inflight->second);
         inflight_reverse_requests.erase(inflight);
       }
     }
 
     if (!response_handler)
-      response_handler = std::make_unique<UnknownResponseHandler>("", id);
+      response_handler =
+          std::make_unique<UnknownResponseHandler>("", resp->request_seq);
 
     // Result should be given, use null if not.
-    if (GetBoolean(object, "success").value_or(false)) {
-      llvm::json::Value Result = nullptr;
-      if (auto *B = object.get("body"))
-        Result = std::move(*B);
-      (*response_handler)(Result);
+    if (resp->success) {
+      (*response_handler)(resp->body);
     } else {
-      llvm::StringRef message = GetString(object, "message").value_or("");
-      if (message.empty()) {
-        message = "Unknown error, response failed";
+      llvm::StringRef message = "Unknown error, response failed";
+      if (resp->message) {
+        message =
+            std::visit(llvm::makeVisitor(
+                           [](const std::string &message) -> llvm::StringRef {
+                             return message;
+                           },
+                           [](const protocol::Response::Message &message)
+                               -> llvm::StringRef {
+                             switch (message) {
+                             case protocol::Response::Message::cancelled:
+                               return "cancelled";
+                             case protocol::Response::Message::notStopped:
+                               return "notStopped";
+                             }
+                           }),
+                       *resp->message);
       }
+
       (*response_handler)(llvm::createStringError(
           std::error_code(-1, std::generic_category()), message));
     }
 
     return true;
   }
 
+  DAP_LOG(log, "Unsupported protocol message");
+
   return false;
 }
 
@@ -730,9 +745,9 @@ void DAP::SendTerminatedEvent() {
   });
 }
 
-lldb::SBError DAP::Disconnect() { return Disconnect(is_attach); }
+llvm::Error DAP::Disconnect() { return Disconnect(is_attach); }
 
-lldb::SBError DAP::Disconnect(bool terminateDebuggee) {
+llvm::Error DAP::Disconnect(bool terminateDebuggee) {
   lldb::SBError error;
   lldb::SBProcess process = target.GetProcess();
   auto state = process.GetState();
@@ -760,7 +775,7 @@ lldb::SBError DAP::Disconnect(bool terminateDebuggee) {
 
   disconnecting = true;
 
-  return error;
+  return ToError(error);
 }
 
 llvm::Error DAP::Loop() {

lldb/tools/lldb-dap/DAP.h

@@ -12,12 +12,10 @@
 #include "DAPForward.h"
 #include "ExceptionBreakpoint.h"
 #include "FunctionBreakpoint.h"
-#include "Handler/RequestHandler.h"
-#include "Handler/ResponseHandler.h"
 #include "InstructionBreakpoint.h"
 #include "OutputRedirector.h"
 #include "ProgressEvent.h"
-#include "Protocol.h"
+#include "Protocol/ProtocolBase.h"
 #include "SourceBreakpoint.h"
 #include "Transport.h"
 #include "lldb/API/SBBroadcaster.h"
@@ -187,7 +185,7 @@ struct DAP {
   // the old process here so we can detect this case and keep running.
   lldb::pid_t restarting_process_id;
   bool configuration_done_sent;
-  llvm::StringMap<std::unique_ptr<RequestHandler>> request_handlers;
+  llvm::StringMap<std::unique_ptr<BaseRequestHandler>> request_handlers;
   bool waiting_for_run_in_terminal;
   ProgressEventReporter progress_event_reporter;
   // Keep track of the last stop thread index IDs as threads won't go away
@@ -245,9 +243,11 @@ struct DAP {
   /// Stop event handler threads.
   void StopEventHandlers();
 
-  // Serialize the JSON value into a string and send the JSON packet to
-  // the "out" stream.
+  /// Serialize the JSON value into a string and send the JSON packet to the
+  /// "out" stream.
   void SendJSON(const llvm::json::Value &json);
+  /// Send the given message to the client
+  void Send(const protocol::Message &message);
 
   void SendOutput(OutputType o, const llvm::StringRef output);
 
@@ -324,10 +324,10 @@ struct DAP {
   bool HandleObject(const protocol::Message &M);
 
   /// Disconnect the DAP session.
-  lldb::SBError Disconnect();
+  llvm::Error Disconnect();
 
   /// Disconnect the DAP session and optionally terminate the debuggee.
-  lldb::SBError Disconnect(bool terminateDebuggee);
+  llvm::Error Disconnect(bool terminateDebuggee);
 
   /// Send a "terminated" event to indicate the process is done being debugged.
   void SendTerminatedEvent();

lldb/tools/lldb-dap/DAPForward.h

@@ -19,6 +19,8 @@ struct SourceBreakpoint;
 struct Watchpoint;
 struct InstructionBreakpoint;
 struct DAP;
+class BaseRequestHandler;
+class ResponseHandler;
 } // namespace lldb_dap
 
 namespace lldb {

lldb/tools/lldb-dap/Handler/DisconnectRequestHandler.cpp

@@ -7,70 +7,27 @@
 //===----------------------------------------------------------------------===//
 
 #include "DAP.h"
-#include "EventHelper.h"
-#include "JSONUtils.h"
+#include "Protocol/ProtocolRequests.h"
 #include "RequestHandler.h"
+#include "llvm/Support/Error.h"
+#include <optional>
+
+using namespace llvm;
+using namespace lldb_dap::protocol;
 
 namespace lldb_dap {
 
-// "DisconnectRequest": {
-//   "allOf": [ { "$ref": "#/definitions/Request" }, {
-//     "type": "object",
-//     "description": "Disconnect request; value of command field is
-//                     'disconnect'.",
-//     "properties": {
-//       "command": {
-//         "type": "string",
-//         "enum": [ "disconnect" ]
-//       },
-//       "arguments": {
-//         "$ref": "#/definitions/DisconnectArguments"
-//       }
-//     },
-//     "required": [ "command" ]
-//   }]
-// },
-// "DisconnectArguments": {
-//   "type": "object",
-//   "description": "Arguments for 'disconnect' request.",
-//   "properties": {
-//     "terminateDebuggee": {
-//       "type": "boolean",
-//       "description": "Indicates whether the debuggee should be terminated
-//                       when the debugger is disconnected. If unspecified,
-//                       the debug adapter is free to do whatever it thinks
-//                       is best. A client can only rely on this attribute
-//                       being properly honored if a debug adapter returns
-//                       true for the 'supportTerminateDebuggee' capability."
-//     },
-//     "restart": {
-//       "type": "boolean",
-//       "description": "Indicates whether the debuggee should be restart
-//                       the process."
-//     }
-//   }
-// },
-// "DisconnectResponse": {
-//   "allOf": [ { "$ref": "#/definitions/Response" }, {
-//     "type": "object",
-//     "description": "Response to 'disconnect' request. This is just an
-//                     acknowledgement, so no body field is required."
-//   }]
-// }
-void DisconnectRequestHandler::operator()(
-    const llvm::json::Object &request) const {
-  llvm::json::Object response;
-  FillResponse(request, response);
-  const auto *arguments = request.getObject("arguments");
+/// Disconnect request; value of command field is 'disconnect'.
+Expected<DisconnectResponse> DisconnectRequestHandler::Run(
+    const std::optional<DisconnectArguments> &arguments) const {
+  bool terminateDebuggee = dap.is_attach ? false : true;
 
-  bool defaultTerminateDebuggee = dap.is_attach ? false : true;
-  bool terminateDebuggee = GetBoolean(arguments, "terminateDebuggee")
-                               .value_or(defaultTerminateDebuggee);
+  if (arguments && arguments->terminateDebuggee)
+    terminateDebuggee = *arguments->terminateDebuggee;
 
-  lldb::SBError error = dap.Disconnect(terminateDebuggee);
-  if (error.Fail())
-    EmplaceSafeString(response, "error", error.GetCString());
+  if (Error error = dap.Disconnect(terminateDebuggee))
+    return error;
 
-  dap.SendJSON(llvm::json::Value(std::move(response)));
+  return DisconnectResponse();
 }
 } // namespace lldb_dap

lldb/tools/lldb-dap/Handler/RequestHandler.cpp

@@ -6,8 +6,9 @@
 //
 //===----------------------------------------------------------------------===//
 
-#include "RequestHandler.h"
+#include "Handler/RequestHandler.h"
 #include "DAP.h"
+#include "Handler/ResponseHandler.h"
 #include "JSONUtils.h"
 #include "LLDBUtils.h"
 #include "RunInTerminal.h"
@@ -45,7 +46,7 @@ static uint32_t SetLaunchFlag(uint32_t flags, const llvm::json::Object *obj,
 
 // Both attach and launch take either a sourcePath or a sourceMap
 // argument (or neither), from which we need to set the target.source-map.
-void RequestHandler::SetSourceMapFromArguments(
+void BaseRequestHandler::SetSourceMapFromArguments(
     const llvm::json::Object &arguments) const {
   const char *sourceMapHelp =
       "source must be be an array of two-element arrays, "
@@ -159,7 +160,7 @@ static llvm::Error RunInTerminal(DAP &dap,
 }
 
 lldb::SBError
-RequestHandler::LaunchProcess(const llvm::json::Object &request) const {
+BaseRequestHandler::LaunchProcess(const llvm::json::Object &request) const {
   lldb::SBError error;
   const auto *arguments = request.getObject("arguments");
   auto launchCommands = GetStrings(arguments, "launchCommands");
@@ -228,13 +229,13 @@ RequestHandler::LaunchProcess(const llvm::json::Object &request) const {
   return error;
 }
 
-void RequestHandler::PrintWelcomeMessage() const {
+void BaseRequestHandler::PrintWelcomeMessage() const {
 #ifdef LLDB_DAP_WELCOME_MESSAGE
   dap.SendOutput(OutputType::Console, LLDB_DAP_WELCOME_MESSAGE);
 #endif
 }
 
-bool RequestHandler::HasInstructionGranularity(
+bool BaseRequestHandler::HasInstructionGranularity(
     const llvm::json::Object &arguments) const {
   if (std::optional<llvm::StringRef> value = arguments.getString("granularity"))
     return value == "instruction";