Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 13 commits into from
Mar 17, 2025

Conversation

ashgti
Copy link
Contributor

@ashgti ashgti commented Mar 6, 2025

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 #130026

Copy link
Member

@vogelsgesang vogelsgesang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 I like the overall direction this is going

// },
// "required": [ "sourceReference" ]
// },
struct SourceArguments {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should change how we structure our files.

IMO, we should have SourceArguments, SourceResponse and SourceRequestHandler all in Handlers/SourceRequestHandler.{h,cpp}. That way, we have all the SourceRequest-related things in one place.

Not sure where Source should go. If it's only used by the SourceRequestHandler, I would also put it into Handlers/SourceRequestHandler.{h,cpp}, otherwise we might move it to ProtocolTypes.{h,cpp} or even ProtocolTypes/Source.{h,cpp}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

otherwise we might move it to ProtocolTypes.{h,cpp} or even ProtocolTypes/Source.{h,cpp}

and maybe move the existing Protocol.{h,cpp} to BaseProtocol.{h,cpp}? To keep the base protocol separated from the other request / response / event types?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally like the protocol being isolated into its own namespace and its own file.

There are actually a few examples of this already in llvm, e.g. clang-tools-extra/clangd/Protocol.h or mlir/include/mlir/Tools/lsp-server-support/Protocol.h.

This is nice because we can easily look in the file for other types defined in the protocol and add new types as we need them. For example, you mentioned the Source type, which is used in a few events and requests.

The DAP spec already groups at a high level into Base Protocol, Events, Requests, Reverse Requests and Types, which I was roughly following in this file using // MARK: <category> headings between sections.

I only left the description of the SourceRequest next to the handler for reference for the implementation.

I'm fine with changing how we structure things, but I liked having the distinction between lldb_dap::protocol for the underlying protocol, lldb_dap for our intermediate components and lldb/lldb_private for the SB API layer of the debugger itself.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with changing how we structure things, but I liked having the distinction between lldb_dap::protocol for the underlying protocol, lldb_dap for our intermediate components and lldb/lldb_private for the SB API layer of the debugger itself.

Indeed, you brought up a couple of good arguments for keeping the protocol separate. I am undecided. Let's see if anyone else jumps in and also has additional arguments in either direction. Otherwise, I would be fine with either way.

The DAP spec already groups at a high level into Base Protocol, Events, Requests, Reverse Requests and Types, which I was roughly following in this file using // MARK: headings between sections.

Even if we don't move *Args, *Request and *Response types to the corresponding *RequestHandler, I think we should still separate the base protocol from the remaining protocol types. I would prefer separate files over // MARK: <category> comments for that purpose, i.e. a Protocol.{h,cpp} (containing the events, request and response types) and a separate BaseProtocol.{h,cpp} (containing the base protocol)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JDevlieghere Any thoughts on the organization of these types?

I was keeping things simple by putting the types in a single place, but we don't strictly need to do that.

We could have ProtocolBase.h, ProtocolTypes.h, ProtocolRequests.h, ProtocolEvents.h, etc.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see an argument for either. If it were up to me, I'd go with the different Protocol* files in a Protocol subdirectory. My (entirely subjective) reasoning is that the protocol should match the spec, rather than how it's used in our implementation and therefore it warrants standing on its own. Things were different when we didn't have the abstraction and type safety offered by the Protocol class and we had to consult the spec to decode a message, but now that's not a concern anymore if you're using an IDE or editor with code completion.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Split up the Protocol.{h,cpp} > Protocol/Protocol{Base,Requests,Types}.{h,cpp}.

ashgti added 2 commits March 12, 2025 12:36
…ponse.

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' request is updated to show how the new flow works.
…stead extracting the comments to their c++ repr.
@ashgti ashgti force-pushed the request-handler-types branch from 36d0fec to 9908931 Compare March 12, 2025 20:29
@ashgti ashgti requested a review from labath March 12, 2025 20:29
@ashgti ashgti marked this pull request as ready for review March 12, 2025 20:33
@llvmbot llvmbot added the lldb label Mar 12, 2025
@llvmbot
Copy link
Member

llvmbot commented Mar 12, 2025

@llvm/pr-subscribers-lldb

Author: John Harrison (ashgti)

Changes

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' request is updated to show how the new flow works.

This is built on top of #130026


Patch is 43.12 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/130090.diff

8 Files Affected:

  • (modified) lldb/tools/lldb-dap/DAP.cpp (+34-23)
  • (modified) lldb/tools/lldb-dap/DAP.h (+1-3)
  • (modified) lldb/tools/lldb-dap/DAPForward.h (+2)
  • (modified) lldb/tools/lldb-dap/Handler/RequestHandler.cpp (+6-5)
  • (modified) lldb/tools/lldb-dap/Handler/RequestHandler.h (+139-75)
  • (modified) lldb/tools/lldb-dap/Handler/SourceRequestHandler.cpp (+26-97)
  • (modified) lldb/tools/lldb-dap/Protocol.cpp (+44)
  • (modified) lldb/tools/lldb-dap/Protocol.h (+117-163)
diff --git a/lldb/tools/lldb-dap/DAP.cpp b/lldb/tools/lldb-dap/DAP.cpp
index 4080e2c211035..9d42af2d7091f 100644
--- a/lldb/tools/lldb-dap/DAP.cpp
+++ b/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.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)
@@ -663,31 +667,23 @@ 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");
-
-    auto new_handler_pos = request_handlers.find(command);
+  if (const auto *req = std::get_if<protocol::Request>(&M)) {
+    auto new_handler_pos = request_handlers.find(req->command);
     if (new_handler_pos != request_handlers.end()) {
-      (*new_handler_pos->second)(object);
+      (*new_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);
@@ -695,19 +691,31 @@ bool DAP::HandleObject(const protocol::Message &M) {
     }
 
     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->rawBody);
     } else {
-      llvm::StringRef message = GetString(object, "message");
-      if (message.empty()) {
-        message = "Unknown error, response failed";
+      std::string message = "Unknown error, response failed";
+      if (resp->message) {
+        message = std::visit(
+            llvm::makeVisitor(
+                [](const std::string &message) -> std::string {
+                  return message;
+                },
+                [](const protocol::Response::Message &message) -> std::string {
+                  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));
     }
@@ -715,6 +723,9 @@ bool DAP::HandleObject(const protocol::Message &M) {
     return true;
   }
 
+  if (log)
+    *log << "Unsupported protocol message" << std::endl;
+
   return false;
 }
 
diff --git a/lldb/tools/lldb-dap/DAP.h b/lldb/tools/lldb-dap/DAP.h
index db3473b7c7027..f6b63e4a3e0bc 100644
--- a/lldb/tools/lldb-dap/DAP.h
+++ b/lldb/tools/lldb-dap/DAP.h
@@ -12,8 +12,6 @@
 #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"
@@ -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
diff --git a/lldb/tools/lldb-dap/DAPForward.h b/lldb/tools/lldb-dap/DAPForward.h
index 0196d83dcd6a9..667aef23abd0f 100644
--- a/lldb/tools/lldb-dap/DAPForward.h
+++ b/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 {
diff --git a/lldb/tools/lldb-dap/Handler/RequestHandler.cpp b/lldb/tools/lldb-dap/Handler/RequestHandler.cpp
index e43fa36d25e3f..13cf166e200e8 100644
--- a/lldb/tools/lldb-dap/Handler/RequestHandler.cpp
+++ b/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";
diff --git a/lldb/tools/lldb-dap/Handler/RequestHandler.h b/lldb/tools/lldb-dap/Handler/RequestHandler.h
index b44367518bcb9..44a1aa4cefcde 100644
--- a/lldb/tools/lldb-dap/Handler/RequestHandler.h
+++ b/lldb/tools/lldb-dap/Handler/RequestHandler.h
@@ -9,25 +9,36 @@
 #ifndef LLDB_TOOLS_LLDB_DAP_HANDLER_HANDLER_H
 #define LLDB_TOOLS_LLDB_DAP_HANDLER_HANDLER_H
 
+#include "DAP.h"
+#include "Protocol.h"
 #include "lldb/API/SBError.h"
+#include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/JSON.h"
 
 namespace lldb_dap {
 struct DAP;
 
-class RequestHandler {
+/// Base class for request handlers. Do not extend this directly: Extend
+/// the RequestHandler template subclass instead.
+class BaseRequestHandler {
 public:
-  RequestHandler(DAP &dap) : dap(dap) {}
+  BaseRequestHandler(DAP &dap) : dap(dap) {}
 
-  /// RequestHandler are not copyable.
+  /// BaseRequestHandler are not copyable.
   /// @{
-  RequestHandler(const RequestHandler &) = delete;
-  RequestHandler &operator=(const RequestHandler &) = delete;
+  BaseRequestHandler(const BaseRequestHandler &) = delete;
+  BaseRequestHandler &operator=(const BaseRequestHandler &) = delete;
   /// @}
 
-  virtual ~RequestHandler() = default;
+  virtual ~BaseRequestHandler() = default;
 
+  virtual void operator()(const protocol::Request &request) const {
+    auto req = toJSON(request);
+    (*this)(*req.getAsObject());
+  }
+
+  /// FIXME: Migrate callers to typed RequestHandler for improved type handling.
   virtual void operator()(const llvm::json::Object &request) const = 0;
 
 protected:
@@ -57,235 +68,288 @@ class RequestHandler {
   DAP &dap;
 };
 
-class AttachRequestHandler : public RequestHandler {
-public:
-  using RequestHandler::RequestHandler;
+/// Base class for handling DAP requests. Handlers should declare their
+/// arguments and response body types like:
+///
+/// class MyRequestHandler : public RequestHandler<Arguments, ResponseBody> {
+///   ....
+/// };
+template <typename Args, typename Body>
+class RequestHandler : public BaseRequestHandler {
+  using BaseRequestHandler::BaseRequestHandler;
+
+  void operator()(const llvm::json::Object &request) const override {
+    /* no-op, the other overload handles json coding. */
+  }
+
+  void operator()(const protocol::Request &request) const override {
+    protocol::Response response;
+    response.request_seq = request.seq;
+    response.command = request.command;
+    Args arguments;
+    llvm::json::Path::Root root;
+    if (request.rawArguments &&
+        !fromJSON(request.rawArguments, arguments, root)) {
+      std::string parseFailure;
+      llvm::raw_string_ostream OS(parseFailure);
+      root.printErrorContext(request.rawArguments, OS);
+      response.success = false;
+      response.message = parseFailure;
+      dap.SendJSON(std::move(response));
+      return;
+    }
+
+    auto ResponseBody = Run(arguments);
+    // FIXME: Add a dedicated DAPError for enhanced errors that are
+    // user-visibile.
+    if (auto Err = ResponseBody.takeError()) {
+      response.success = false;
+      // FIXME: Build ErrorMessage based on error details instead of using the
+      // 'message' field.
+      response.message = llvm::toString(std::move(Err));
+    } else {
+      response.success = true;
+      response.rawBody = std::move(*ResponseBody);
+    }
+
+    dap.SendJSON(std::move(response));
+  };
+
+  virtual llvm::Expected<Body> Run(const Args &) const = 0;
+};
+
+class AttachRequestHandler : public BaseRequestHandler {
+public:
+  using BaseRequestHandler::BaseRequestHandler;
   static llvm::StringLiteral getCommand() { return "attach"; }
   void operator()(const llvm::json::Object &request) const override;
 };
 
-class BreakpointLocationsRequestHandler : public RequestHandler {
+class BreakpointLocationsRequestHandler : public BaseRequestHandler {
 public:
-  using RequestHandler::RequestHandler;
+  using BaseRequestHandler::BaseRequestHandler;
   static llvm::StringLiteral getCommand() { return "breakpointLocations"; }
   void operator()(const llvm::json::Object &request) const override;
 };
 
-class CompletionsRequestHandler : public RequestHandler {
+class CompletionsRequestHandler : public BaseRequestHandler {
 public:
-  using RequestHandler::RequestHandler;
+  using BaseRequestHandler::BaseRequestHandler;
   static llvm::StringLiteral getCommand() { return "completions"; }
   void operator()(const llvm::json::Object &request) const override;
 };
 
-class ContinueRequestHandler : public RequestHandler {
+class ContinueRequestHandler : public BaseRequestHandler {
 public:
-  using RequestHandler::RequestHandler;
+  using BaseRequestHandler::BaseRequestHandler;
   static llvm::StringLiteral getCommand() { return "continue"; }
   void operator()(const llvm::json::Object &request) const override;
 };
 
-class ConfigurationDoneRequestHandler : public RequestHandler {
+class ConfigurationDoneRequestHandler : public BaseRequestHandler {
 public:
-  using RequestHandler::RequestHandler;
+  using BaseRequestHandler::BaseRequestHandler;
   static llvm::StringLiteral getCommand() { return "configurationDone"; }
   void operator()(const llvm::json::Object &request) const override;
 };
 
-class DisconnectRequestHandler : public RequestHandler {
+class DisconnectRequestHandler : public BaseRequestHandler {
 public:
-  using RequestHandler::RequestHandler;
+  using BaseRequestHandler::BaseRequestHandler;
   static llvm::StringLiteral getCommand() { return "disconnect"; }
   void operator()(const llvm::json::Object &request) const override;
 };
 
-class EvaluateRequestHandler : public RequestHandler {
+class EvaluateRequestHandler : public BaseRequestHandler {
 public:
-  using RequestHandler::RequestHandler;
+  using BaseRequestHandler::BaseRequestHandler;
   static llvm::StringLiteral getCommand() { return "evaluate"; }
   void operator()(const llvm::json::Object &request) const override;
 };
 
-class ExceptionInfoRequestHandler : public RequestHandler {
+class ExceptionInfoRequestHandler : public BaseRequestHandler {
 public:
-  using RequestHandler::RequestHandler;
+  using BaseRequestHandler::BaseRequestHandler;
   static llvm::StringLiteral getCommand() { return "exceptionInfo"; }
   void operator()(const llvm::json::Object &request) const override;
 };
 
-class InitializeRequestHandler : public RequestHandler {
+class InitializeRequestHandler : public BaseRequestHandler {
 public:
-  using RequestHandler::RequestHandler;
+  using BaseRequestHandler::BaseRequestHandler;
   static llvm::StringLiteral getCommand() { return "initialize"; }
   void operator()(const llvm::json::Object &request) const override;
 };
 
-class LaunchRequestHandler : public RequestHandler {
+class LaunchRequestHandler : public BaseRequestHandler {
 public:
-  using RequestHandler::RequestHandler;
+  using BaseRequestHandler::BaseRequestHandler;
   static llvm::StringLiteral getCommand() { return "launch"; }
   void operator()(const llvm::json::Object &request) const override;
 };
 
-class RestartRequestHandler : public RequestHandler {
+class RestartRequestHandler : public BaseRequestHandler {
 public:
-  using RequestHandler::RequestHandler;
+  using BaseRequestHandler::BaseRequestHandler;
   static llvm::StringLiteral getCommand() { return "restart"; }
   void operator()(const llvm::json::Object &request) const override;
 };
 
-class NextRequestHandler : public RequestHandler {
+class NextRequestHandler : public BaseRequestHandler {
 public:
-  using RequestHandler::RequestHandler;
+  using BaseRequestHandler::BaseRequestHandler;
   static llvm::StringLiteral getCommand() { return "next"; }
   void operator()(const llvm::json::Object &request) const override;
 };
 
-class StepInRequestHandler : public RequestHandler {
+class StepInRequestHandler : public BaseRequestHandler {
 public:
-  using RequestHandler::RequestHandler;
+  using BaseRequestHandler::BaseRequestHandler;
   static llvm::StringLiteral getCommand() { return "stepIn"; }
   void operator()(const llvm::json::Object &request) const override;
 };
 
-class StepInTargetsRequestHandler : public RequestHandler {
+class StepInTargetsRequestHandler : public BaseRequestHandler {
 public:
-  using RequestHandler::RequestHandler;
+  using BaseRequestHandler::BaseRequestHandler;
   static llvm::StringLiteral getCommand() { return "stepInTargets"; }
   void operator()(const llvm::json::Object &request) const override;
 };
 
-class StepOutRequestHandler : public RequestHandler {
+class StepOutRequestHandler : public BaseRequestHandler {
 public:
-  using RequestHandler::RequestHandler;
+  using BaseRequestHandler::BaseRequestHandler;
   static llvm::StringLiteral getCommand() { return "stepOut"; }
   void operator()(const llvm::json::Object &request) const override;
 };
 
-class SetBreakpointsRequestHandler : public RequestHandler {
+class SetBreakpointsRequestHandler : public BaseRequestHandler {
 public:
-  using RequestHandler::RequestHandler;
+  using BaseRequestHandler::BaseRequestHandler;
   static llvm::StringLiteral getCommand() { return "setBreakpoints"; }
   void operator()(const llvm::json::Object &request) const override;
 };
 
-class SetExceptionBreakpointsRequestHandler : public RequestHandler {
+class SetExceptionBreakpointsRequestHandler : public BaseRequestHandler {
 public:
-  using RequestHandler::RequestHandler;
+  using BaseRequestHandler::BaseRequestHandler;
   static llvm::StringLiteral getCommand() { return "setExceptionBreakpoints"; }
   void operator()(const llvm::json::Object &request) const override;
 };
 
-class SetFunctionBreakpointsRequestHandler : public RequestHandler {
+class SetFunctionBreakpointsRequestHandler : public BaseRequestHandler {
 public:
-  using RequestHandler::RequestHandler;
+  using BaseRequestHandler::BaseRequestHandler;
   static llvm::StringLiteral getCommand() { return "setFunctionBreakpoints"; }
   void operator()(const llvm::json::Object &request) const override;
 };
 
-class DataBreakpointInfoRequestHandler : public RequestHandler {
+class DataBreakpointInfoRequestHandler : public BaseRequestHandler {
 public:
-  using RequestHandler::RequestHandler;
+  using BaseRequestHandler::BaseRequestHandler;
   static llvm::StringLiteral getCommand() { return "dataBreakpointInfo"; }
   void operator()(const llvm::json::Object &request) const override;
 };
 
-class SetDataBreakpointsRequestHandler : public RequestHandler {
+class SetDataBreakpointsRequestHandler : public BaseRequestHandler {
 public:
-  using RequestHandler::RequestHandler;
+  using BaseRequestHandler::BaseRequestHandler;
   static llvm::StringLiteral getCommand() { return "setDataBreakpoints"; }
   void operator()(const llvm::json::Object &request) const override;
 };
 
-class SetInstructionBreakpointsRequestHandler : public RequestHandler {
+class SetInstructionBreakpointsRequestHandler : public BaseRequestHandler {
 public:
-  using RequestHandler::RequestHandler;
+  using BaseRequestHandler::BaseRequestHandler;
   static llvm::StringLiteral getCommand() {
     return "setInstructionBreakpoints";
   }
   void operator()(const llvm::json::Object &request) const override;
 };
 
-class CompileUnitsRequestHandler : public RequestHandler {
+class CompileUnitsRequestHandler : public BaseRequestHandler {
 public:
-  using RequestHandler::RequestHandler;
+  using BaseRequestHandler::BaseRequestHandler;
   static llvm::StringLiteral getCommand() { return "compileUnits"; }
   void operator()(const llvm::json::Object &request) const override;
 };
 
-class ModulesRequestHandler : public RequestHandler {
+class ModulesRequestHandler : public BaseRequestHandler {
 public:
-  using RequestHandler::RequestHandler;
+  using BaseRequestHandler::BaseRequestHandler;
   static llvm::StringLiteral getCommand() { return "modules"; }
   void operator()(const llvm::json::Object &request) const override;
 };
 
-class PauseRequestHandler : public RequestHandler {
+class PauseRequestHandler : public BaseRequestHandler {
 public:
-  using RequestHandler::RequestHandler;
+  using BaseRequestHandler::BaseRequestHandler;
   static llvm::StringLiteral getCommand() { return "pause"; }
   void operator()(const llvm::json::Object &request) const override;
 };
 
-class ScopesRequestHandler : public RequestHandler {
+class ScopesRequestHandler : public BaseRequestHandler {
 public:
-  using RequestHandler::RequestHandler;
+  using BaseRequestHandler::BaseRequestHandler;
   static llvm::StringLiteral getCommand() { return "scopes"; }
   void operator()(const llvm::json::Object &request) const override;
 };
 
-class SetVariableRequestHandler : public RequestHandler {
+class SetVariableRequestHandler : public BaseRequestHandler {
 public:
-  using RequestHandler::RequestHandler;
+  using BaseRequestHandler::BaseReque...
[truncated]

@vogelsgesang
Copy link
Member

What is your overall plan for this PR?

I assume the first step is to get buy-in from all reviewers into the new structure for our JSON types. But what comes after? Are you planning to update this PR to cover all JSON types / requests / responses? Are you planning to land this PR for the Source* types only (as currently in review) and do the remaining types in follow-up PRs?

(Both approaches would be fine by me. Just trying to understand the approach, so I know when it's time to hit the "Approve" button 😉)

ashgti and others added 2 commits March 13, 2025 08:24
Co-authored-by: Adrian Vogelsgesang <[email protected]>
…ssages without a response body.

To validate both of these changes I also migrated the 'disconnect' request to have new types, since it has an `optional<DisconnectArguments>` and a `VoidResponse` body.
@ashgti
Copy link
Contributor Author

ashgti commented Mar 13, 2025

What is your overall plan for this PR?

I assume the first step is to get buy-in from all reviewers into the new structure for our JSON types. But what comes after? Are you planning to update this PR to cover all JSON types / requests / responses? Are you planning to land this PR for the Source* types only (as currently in review) and do the remaining types in follow-up PRs?

(Both approaches would be fine by me. Just trying to understand the approach, so I know when it's time to hit the "Approve" button 😉)

I was thinking of doing this more incrementally. I was planning to build on this for handling the 'cancel' request first but then I'd like to follow up with requests to use the new explicit types for handling arguments and responses.

…`DAP::Disconnect` method a little more ergnomic.
Copy link

github-actions bot commented Mar 13, 2025

✅ With the latest revision this PR passed the C/C++ code formatter.

Copy link
Collaborator

@labath labath left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a small drive-by. This generally looks good to me, but I'll leave the approval to someone else.

Copy link
Member

@vogelsgesang vogelsgesang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

Please take another look at the two remaining open comments (one, two) and then feel free to merge this

ashgti added 3 commits March 14, 2025 17:15
…refix. The types should be enough to tell that theyre not specific values and this helps with the naming consistency.
Copy link
Member

@JDevlieghere JDevlieghere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM modulo comments

@ashgti ashgti merged commit fbb8929 into llvm:main Mar 17, 2025
7 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants