[lldb-dap] Updating the 'next' request handler use well structured types (#136642)

This updates the 'next' request to use well structured types. While
working on this I also simplified the 'RequestHandler' implementation to
better handle void responses by allowing requests to return a
'llvm::Error' instead of an 'llvm::Expected<std::monostate>'. This makes
it easier to write and understand request handles that have simple ack
responses.

Author: ashgti

lldb/tools/lldb-dap/DAP.cpp

@@ -28,6 +28,7 @@
 #include "lldb/Utility/Status.h"
 #include "lldb/lldb-defines.h"
 #include "lldb/lldb-enumerations.h"
+#include "lldb/lldb-types.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/ScopeExit.h"
@@ -499,6 +500,10 @@ ExceptionBreakpoint *DAP::GetExceptionBPFromStopReason(lldb::SBThread &thread) {
   return exc_bp;
 }
 
+lldb::SBThread DAP::GetLLDBThread(lldb::tid_t tid) {
+  return target.GetProcess().GetThreadByID(tid);
+}
+
 lldb::SBThread DAP::GetLLDBThread(const llvm::json::Object &arguments) {
   auto tid = GetInteger<int64_t>(arguments, "threadId")
                  .value_or(LLDB_INVALID_THREAD_ID);

lldb/tools/lldb-dap/DAP.h

@@ -266,6 +266,7 @@ struct DAP {
 
   ExceptionBreakpoint *GetExceptionBPFromStopReason(lldb::SBThread &thread);
 
+  lldb::SBThread GetLLDBThread(lldb::tid_t id);
   lldb::SBThread GetLLDBThread(const llvm::json::Object &arguments);
 
   lldb::SBFrame GetLLDBFrame(const llvm::json::Object &arguments);

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

@@ -10,7 +10,7 @@
 #include "Protocol/ProtocolRequests.h"
 #include "llvm/Support/Error.h"
 
-using namespace lldb_dap;
+using namespace llvm;
 using namespace lldb_dap::protocol;
 
 namespace lldb_dap {
@@ -45,12 +45,11 @@ namespace lldb_dap {
 ///
 /// A client cannot assume that progress just got cancelled after sending
 /// the `cancel` request.
-llvm::Expected<CancelResponseBody>
-CancelRequestHandler::Run(const CancelArguments &arguments) const {
+Error CancelRequestHandler::Run(const CancelArguments &arguments) const {
   // Cancel support is built into the DAP::Loop handler for detecting
   // cancellations of pending or inflight requests.
   dap.ClearCancelRequest(arguments);
-  return CancelResponseBody();
+  return Error::success();
 }
 
 } // namespace lldb_dap

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

@@ -18,7 +18,7 @@ using namespace lldb_dap::protocol;
 namespace lldb_dap {
 
 /// Disconnect request; value of command field is 'disconnect'.
-Expected<DisconnectResponse> DisconnectRequestHandler::Run(
+Error DisconnectRequestHandler::Run(
     const std::optional<DisconnectArguments> &arguments) const {
   bool terminateDebuggee = dap.is_attach ? false : true;
 
@@ -28,6 +28,6 @@ Expected<DisconnectResponse> DisconnectRequestHandler::Run(
   if (Error error = dap.Disconnect(terminateDebuggee))
     return error;
 
-  return DisconnectResponse();
+  return Error::success();
 }
 } // namespace lldb_dap

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

@@ -8,72 +8,37 @@
 
 #include "DAP.h"
 #include "EventHelper.h"
-#include "JSONUtils.h"
+#include "Protocol/ProtocolTypes.h"
 #include "RequestHandler.h"
+#include "llvm/Support/Error.h"
+
+using namespace llvm;
+using namespace lldb_dap::protocol;
 
 namespace lldb_dap {
 
-// "NextRequest": {
-//   "allOf": [ { "$ref": "#/definitions/Request" }, {
-//     "type": "object",
-//     "description": "Next request; value of command field is 'next'. The
-//                     request starts the debuggee to run again for one step.
-//                     The debug adapter first sends the NextResponse and then
-//                     a StoppedEvent (event type 'step') after the step has
-//                     completed.",
-//     "properties": {
-//       "command": {
-//         "type": "string",
-//         "enum": [ "next" ]
-//       },
-//       "arguments": {
-//         "$ref": "#/definitions/NextArguments"
-//       }
-//     },
-//     "required": [ "command", "arguments"  ]
-//   }]
-// },
-// "NextArguments": {
-//   "type": "object",
-//   "description": "Arguments for 'next' request.",
-//   "properties": {
-//     "threadId": {
-//       "type": "integer",
-//       "description": "Execute 'next' for this thread."
-//     },
-//     "granularity": {
-//       "$ref": "#/definitions/SteppingGranularity",
-//       "description": "Stepping granularity. If no granularity is specified, a
-//                       granularity of `statement` is assumed."
-//     }
-//   },
-//   "required": [ "threadId" ]
-// },
-// "NextResponse": {
-//   "allOf": [ { "$ref": "#/definitions/Response" }, {
-//     "type": "object",
-//     "description": "Response to 'next' request. This is just an
-//                     acknowledgement, so no body field is required."
-//   }]
-// }
-void NextRequestHandler::operator()(const llvm::json::Object &request) const {
-  llvm::json::Object response;
-  FillResponse(request, response);
-  const auto *arguments = request.getObject("arguments");
-  lldb::SBThread thread = dap.GetLLDBThread(*arguments);
-  if (thread.IsValid()) {
-    // Remember the thread ID that caused the resume so we can set the
-    // "threadCausedFocus" boolean value in the "stopped" events.
-    dap.focus_tid = thread.GetThreadID();
-    if (HasInstructionGranularity(*arguments)) {
-      thread.StepInstruction(/*step_over=*/true);
-    } else {
-      thread.StepOver();
-    }
+/// The request executes one step (in the given granularity) for the specified
+/// thread and allows all other threads to run freely by resuming them. If the
+/// debug adapter supports single thread execution (see capability
+/// `supportsSingleThreadExecutionRequests`), setting the `singleThread`
+/// argument to true prevents other suspended threads from resuming. The debug
+/// adapter first sends the response and then a `stopped` event (with reason
+/// `step`) after the step has completed.
+Error NextRequestHandler::Run(const NextArguments &args) const {
+  lldb::SBThread thread = dap.GetLLDBThread(args.threadId);
+  if (!thread.IsValid())
+    return make_error<DAPError>("invalid thread");
+
+  // Remember the thread ID that caused the resume so we can set the
+  // "threadCausedFocus" boolean value in the "stopped" events.
+  dap.focus_tid = thread.GetThreadID();
+  if (args.granularity == eSteppingGranularityInstruction) {
+    thread.StepInstruction(/*step_over=*/true);
   } else {
-    response["success"] = llvm::json::Value(false);
+    thread.StepOver();
   }
-  dap.SendJSON(llvm::json::Value(std::move(response)));
+
+  return Error::success();
 }
 
 } // namespace lldb_dap

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

@@ -94,10 +94,10 @@ class LegacyRequestHandler : public BaseRequestHandler {
 /// Base class for handling DAP requests. Handlers should declare their
 /// arguments and response body types like:
 ///
-/// class MyRequestHandler : public RequestHandler<Arguments, ResponseBody> {
+/// class MyRequestHandler : public RequestHandler<Arguments, Response> {
 ///   ....
 /// };
-template <typename Args, typename Body>
+template <typename Args, typename Resp>
 class RequestHandler : public BaseRequestHandler {
   using BaseRequestHandler::BaseRequestHandler;
 
@@ -128,41 +128,29 @@ class RequestHandler : public BaseRequestHandler {
          << "': " << llvm::toString(root.getError()) << "\n";
       root.printErrorContext(*request.arguments, OS);
 
-      protocol::ErrorMessage error_message;
-      error_message.format = parse_failure;
-
-      protocol::ErrorResponseBody body;
-      body.error = error_message;
-
       response.success = false;
-      response.body = std::move(body);
+      response.body = ToResponse(llvm::make_error<DAPError>(parse_failure));
 
       dap.Send(response);
       return;
     }
 
-    llvm::Expected<Body> body = Run(arguments);
-    if (auto Err = body.takeError()) {
-      protocol::ErrorMessage error_message;
-      error_message.sendTelemetry = false;
-      if (llvm::Error unhandled = llvm::handleErrors(
-              std::move(Err), [&](const DAPError &E) -> llvm::Error {
-                error_message.format = E.getMessage();
-                error_message.showUser = E.getShowUser();
-                error_message.id = E.convertToErrorCode().value();
-                error_message.url = E.getURL();
-                error_message.urlLabel = E.getURLLabel();
-                return llvm::Error::success();
-              }))
-        error_message.format = llvm::toString(std::move(unhandled));
-      protocol::ErrorResponseBody body;
-      body.error = error_message;
-      response.success = false;
-      response.body = std::move(body);
+    if constexpr (std::is_same_v<Resp, llvm::Error>) {
+      if (llvm::Error err = Run(arguments)) {
+        response.success = false;
+        response.body = ToResponse(std::move(err));
+      } else {
+        response.success = true;
+      }
     } else {
-      response.success = true;
-      if constexpr (!std::is_same_v<Body, std::monostate>)
+      Resp body = Run(arguments);
+      if (llvm::Error err = body.takeError()) {
+        response.success = false;
+        response.body = ToResponse(std::move(err));
+      } else {
+        response.success = true;
         response.body = std::move(*body);
+      }
     }
 
     // Mark the request as 'cancelled' if the debugger was interrupted while
@@ -177,7 +165,25 @@ class RequestHandler : public BaseRequestHandler {
     dap.Send(response);
   };
 
-  virtual llvm::Expected<Body> Run(const Args &) const = 0;
+  virtual Resp Run(const Args &) const = 0;
+
+  protocol::ErrorResponseBody ToResponse(llvm::Error err) const {
+    protocol::ErrorMessage error_message;
+    error_message.sendTelemetry = false;
+    if (llvm::Error unhandled = llvm::handleErrors(
+            std::move(err), [&](const DAPError &E) -> llvm::Error {
+              error_message.format = E.getMessage();
+              error_message.showUser = E.getShowUser();
+              error_message.id = E.convertToErrorCode().value();
+              error_message.url = E.getURL();
+              error_message.urlLabel = E.getURLLabel();
+              return llvm::Error::success();
+            }))
+      error_message.format = llvm::toString(std::move(unhandled));
+    protocol::ErrorResponseBody body;
+    body.error = error_message;
+    return body;
+  }
 };
 
 class AttachRequestHandler : public LegacyRequestHandler {
@@ -233,7 +239,7 @@ class DisconnectRequestHandler
   FeatureSet GetSupportedFeatures() const override {
     return {protocol::eAdapterFeatureTerminateDebuggee};
   }
-  llvm::Expected<protocol::DisconnectResponse>
+  llvm::Error
   Run(const std::optional<protocol::DisconnectArguments> &args) const override;
 };
 
@@ -259,7 +265,7 @@ class ExceptionInfoRequestHandler : public LegacyRequestHandler {
 
 class InitializeRequestHandler
     : public RequestHandler<protocol::InitializeRequestArguments,
-                            protocol::InitializeResponseBody> {
+                            llvm::Expected<protocol::InitializeResponseBody>> {
 public:
   using RequestHandler::RequestHandler;
   static llvm::StringLiteral GetCommand() { return "initialize"; }
@@ -284,11 +290,12 @@ class RestartRequestHandler : public LegacyRequestHandler {
   void operator()(const llvm::json::Object &request) const override;
 };
 
-class NextRequestHandler : public LegacyRequestHandler {
+class NextRequestHandler
+    : public RequestHandler<protocol::NextArguments, protocol::NextResponse> {
 public:
-  using LegacyRequestHandler::LegacyRequestHandler;
+  using RequestHandler::RequestHandler;
   static llvm::StringLiteral GetCommand() { return "next"; }
-  void operator()(const llvm::json::Object &request) const override;
+  llvm::Error Run(const protocol::NextArguments &args) const override;
 };
 
 class StepInRequestHandler : public LegacyRequestHandler {
@@ -418,7 +425,7 @@ class SetVariableRequestHandler : public LegacyRequestHandler {
 
 class SourceRequestHandler
     : public RequestHandler<protocol::SourceArguments,
-                            protocol::SourceResponseBody> {
+                            llvm::Expected<protocol::SourceResponseBody>> {
 public:
   using RequestHandler::RequestHandler;
   static llvm::StringLiteral GetCommand() { return "source"; }
@@ -486,8 +493,7 @@ class CancelRequestHandler
   FeatureSet GetSupportedFeatures() const override {
     return {protocol::eAdapterFeatureCancelRequest};
   }
-  llvm::Expected<protocol::CancelResponseBody>
-  Run(const protocol::CancelArguments &args) const override;
+  llvm::Error Run(const protocol::CancelArguments &args) const override;
 };
 
 /// A request used in testing to get the details on all breakpoints that are

lldb/tools/lldb-dap/Protocol/ProtocolBase.h

@@ -149,7 +149,7 @@ struct ErrorResponseBody {
 llvm::json::Value toJSON(const ErrorResponseBody &);
 
 /// This is just an acknowledgement, so no body field is required.
-using VoidResponse = std::monostate;
+using VoidResponse = llvm::Error;
 
 } // namespace lldb_dap::protocol
 

lldb/tools/lldb-dap/Protocol/ProtocolRequests.cpp

@@ -7,7 +7,6 @@
 //===----------------------------------------------------------------------===//
 
 #include "Protocol/ProtocolRequests.h"
-#include "DAP.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/JSON.h"
@@ -114,4 +113,12 @@ json::Value toJSON(const SourceResponseBody &SA) {
   return std::move(Result);
 }
 
+bool fromJSON(const llvm::json::Value &Params, NextArguments &NA,
+              llvm::json::Path P) {
+  json::ObjectMapper OM(Params, P);
+  return OM && OM.map("threadId", NA.threadId) &&
+         OM.mapOptional("singleThread", NA.singleThread) &&
+         OM.mapOptional("granularity", NA.granularity);
+}
+
 } // namespace lldb_dap::protocol

lldb/tools/lldb-dap/Protocol/ProtocolRequests.h

@@ -22,6 +22,7 @@
 
 #include "Protocol/ProtocolBase.h"
 #include "Protocol/ProtocolTypes.h"
+#include "lldb/lldb-defines.h"
 #include "llvm/ADT/DenseSet.h"
 #include "llvm/Support/JSON.h"
 #include <cstdint>
@@ -236,6 +237,25 @@ struct SourceResponseBody {
 };
 llvm::json::Value toJSON(const SourceResponseBody &);
 
+/// Arguments for `next` request.
+struct NextArguments {
+  /// Specifies the thread for which to resume execution for one step (of the
+  /// given granularity).
+  uint64_t threadId = LLDB_INVALID_THREAD_ID;
+
+  /// If this flag is true, all other suspended threads are not resumed.
+  bool singleThread = false;
+
+  /// Stepping granularity. If no granularity is specified, a granularity of
+  /// `statement` is assumed.
+  SteppingGranularity granularity = eSteppingGranularityStatement;
+};
+bool fromJSON(const llvm::json::Value &, NextArguments &, llvm::json::Path);
+
+/// Response to `next` request. This is just an acknowledgement, so no
+/// body field is required.
+using NextResponse = VoidResponse;
+
 } // namespace lldb_dap::protocol
 
 #endif