Skip to content

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

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 1 commit into from
Apr 23, 2025

Conversation

ashgti
Copy link
Contributor

@ashgti ashgti commented Apr 22, 2025

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::Expectedstd::monostate'. This makes it easier to write and understand request handles that have simple ack responses.

…pes.

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.
@ashgti ashgti requested a review from JDevlieghere as a code owner April 22, 2025 02:19
@ashgti ashgti requested review from labath and vogelsgesang and removed request for JDevlieghere April 22, 2025 02:19
@ashgti ashgti requested a review from JDevlieghere April 22, 2025 02:20
@llvmbot
Copy link
Member

llvmbot commented Apr 22, 2025

@llvm/pr-subscribers-lldb

Author: John Harrison (ashgti)

Changes

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.


Full diff: https://github.com/llvm/llvm-project/pull/136642.diff

12 Files Affected:

  • (modified) lldb/tools/lldb-dap/DAP.cpp (+5)
  • (modified) lldb/tools/lldb-dap/DAP.h (+1)
  • (modified) lldb/tools/lldb-dap/Handler/CancelRequestHandler.cpp (+3-4)
  • (modified) lldb/tools/lldb-dap/Handler/DisconnectRequestHandler.cpp (+2-2)
  • (modified) lldb/tools/lldb-dap/Handler/NextRequestHandler.cpp (+25-60)
  • (modified) lldb/tools/lldb-dap/Handler/RequestHandler.h (+44-38)
  • (modified) lldb/tools/lldb-dap/Protocol/ProtocolBase.h (+1-1)
  • (modified) lldb/tools/lldb-dap/Protocol/ProtocolRequests.cpp (+8-1)
  • (modified) lldb/tools/lldb-dap/Protocol/ProtocolRequests.h (+20)
  • (modified) lldb/tools/lldb-dap/Protocol/ProtocolTypes.cpp (+21)
  • (modified) lldb/tools/lldb-dap/Protocol/ProtocolTypes.h (+19)
  • (modified) lldb/tools/lldb-dap/Transport.cpp (+2-1)
diff --git a/lldb/tools/lldb-dap/DAP.cpp b/lldb/tools/lldb-dap/DAP.cpp
index 597fe3a1e323b..134762711b89d 100644
--- a/lldb/tools/lldb-dap/DAP.cpp
+++ b/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);
diff --git a/lldb/tools/lldb-dap/DAP.h b/lldb/tools/lldb-dap/DAP.h
index b79a0d9d0f25c..727e5c00623e8 100644
--- a/lldb/tools/lldb-dap/DAP.h
+++ b/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);
diff --git a/lldb/tools/lldb-dap/Handler/CancelRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/CancelRequestHandler.cpp
index f09de13c3ff72..995fe38362f60 100644
--- a/lldb/tools/lldb-dap/Handler/CancelRequestHandler.cpp
+++ b/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
diff --git a/lldb/tools/lldb-dap/Handler/DisconnectRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/DisconnectRequestHandler.cpp
index f12fecfb2ff65..81e94c7551836 100644
--- a/lldb/tools/lldb-dap/Handler/DisconnectRequestHandler.cpp
+++ b/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
diff --git a/lldb/tools/lldb-dap/Handler/NextRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/NextRequestHandler.cpp
index 216e710035cb1..1603563841005 100644
--- a/lldb/tools/lldb-dap/Handler/NextRequestHandler.cpp
+++ b/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
diff --git a/lldb/tools/lldb-dap/Handler/RequestHandler.h b/lldb/tools/lldb-dap/Handler/RequestHandler.h
index 7e56c258ad78a..edb9de7d0dc20 100644
--- a/lldb/tools/lldb-dap/Handler/RequestHandler.h
+++ b/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
diff --git a/lldb/tools/lldb-dap/Protocol/ProtocolBase.h b/lldb/tools/lldb-dap/Protocol/ProtocolBase.h
index 2c647610de11c..bad0e886d94d2 100644
--- a/lldb/tools/lldb-dap/Protocol/ProtocolBase.h
+++ b/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
 
diff --git a/lldb/tools/lldb-dap/Protocol/ProtocolRequests.cpp b/lldb/tools/lldb-dap/Protocol/ProtocolRequests.cpp
index 3523f8ac87ec9..b113299affb0f 100644
--- a/lldb/tools/lldb-dap/Protocol/ProtocolRequests.cpp
+++ b/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
diff --git a/lldb/tools/lldb-dap/Protocol/ProtocolRequests.h b/lldb/tools/lldb-dap/Protocol/ProtocolRequests.h
index 6623dfa0db05c..6e3e2c6a9e2c8 100644
--- a/lldb/tools/lldb-dap/Protocol/ProtocolRequests.h
+++ b/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
diff --git a/lldb/tools/lldb-dap/Protocol/ProtocolTypes.cpp b/lldb/tools/lldb-dap/Protocol/ProtocolTypes.cpp
index 4d1e90215bbb4..e64998c4ca488 100644
--- a/lldb/tools/lldb-dap/Protocol/ProtocolTypes.cpp
+++ b/lldb/tools/lldb-dap/Protocol/ProtocolTypes.cpp
@@ -233,4 +233,25 @@ json::Value toJSON(const Capabilities &C) {
   return result;
 }
 
+bool fromJSON(const llvm::json::Value &Params, SteppingGranularity &SG,
+              llvm::json::Path P) {
+  auto raw_granularity = Params.getAsString();
+  if (!raw_granularity) {
+    P.report("expected a string");
+    return false;
+  }
+  std::optional<SteppingGranularity> granularity =
+      StringSwitch<std::optional<SteppingGranularity>>(*raw_granularity)
+          .Case("statement", eSteppingGranularityStatement)
+          .Case("line", eSteppingGranularityLine)
+          .Case("instruction", eSteppingGranularityInstruction)
+          .Default(std::nullopt);
+  if (!granularity) {
+    P.report("unexpected value");
+    return false;
+  }
+  SG = *granularity;
+  return true;
+}
+
 } // namespace lldb_dap::protocol
diff --git a/lldb/tools/lldb-dap/Protocol/ProtocolTypes.h b/lldb/tools/lldb-dap/Protocol/ProtocolTypes.h
index 8f38c524ea649..54941f24efbd9 100644
--- a/lldb/tools/lldb-dap/Protocol/ProtocolTypes.h
+++ b/lldb/tools/lldb-dap/Protocol/ProtocolTypes.h
@@ -303,6 +303,25 @@ struct Source {
 };
 bool fromJSON(const llvm::json::Value &, Source &, llvm::json::Path);
 
+/// The granularity of one `step` in the stepping requests `next`, `stepIn`,
+/// `stepOut` and `stepBack`.
+enum SteppingGranularity : unsigned {
+  /// The step should allow the program to run until the current statement has
+  /// finished executing. The meaning of a statement is determined by the
+  /// adapter and it may be considered equivalent to a line. For example
+  /// `for(int i = 0; i < 10; i++)` could be considered to have 3 statements
+  /// `int i = 0`, `i < 10`, and `i++`.
+  eSteppingGranularityStatement,
+  /// The step should allow the program to run until the current source line has
+  /// executed.
+  eSteppingGranularityLine,
+  /// The step should allow one instruction to execute (e.g. one x86
+  /// instruction).
+  eSteppingGranularityInstruction,
+};
+bool fromJSON(const llvm::json::Value &, SteppingGranularity &,
+              llvm::json::Path);
+
 } // namespace lldb_dap::protocol
 
 #endif
diff --git a/lldb/tools/lldb-dap/Transport.cpp b/lldb/tools/lldb-dap/Transport.cpp
index ffd0c49f1770b..4e322e9ff1358 100644
--- a/lldb/tools/lldb-dap/Transport.cpp
+++ b/lldb/tools/lldb-dap/Transport.cpp
@@ -137,7 +137,8 @@ Expected<Message> Transport::Read(const std::chrono::microseconds &timeout) {
 
   DAP_LOG(m_log, "--> ({0}) {1}", m_client_name, *raw_json);
 
-  return json::parse<Message>(*raw_json);
+  return json::parse<Message>(/*JSON=*/*raw_json,
+                              /*RootName=*/"protocol_message");
 }
 
 Error Transport::Write(const Message &message) {

@ashgti ashgti requested a review from walter-erquinigo April 22, 2025 02:20
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

/// 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);
Copy link
Member

Choose a reason for hiding this comment

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

Should this lock the API lock so that nobody can mess with the state of this thread between obtaining it and calling StepOver below? FWIW, I think we'll need to start doing this pretty much everywhere where we call more than one SB API consecutively.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you think we should take the lock while handling a request? We could do that in the BaseRequestHandler maybe.

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 think I'll send a separate patch for locking while handling DAP requests. This patch is mostly about translating the untyped JSON to a well defined structure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sent this as PR #137026

@ashgti ashgti merged commit 1041d54 into llvm:main Apr 23, 2025
13 checks passed
@ashgti ashgti deleted the next-req branch April 23, 2025 17:18
@llvm-ci
Copy link
Collaborator

llvm-ci commented Apr 23, 2025

LLVM Buildbot has detected a new failure on builder lldb-aarch64-ubuntu running on linaro-lldb-aarch64-ubuntu while building lldb at step 6 "test".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/59/builds/16513

Here is the relevant piece of the build log for the reference
Step 6 (test) failure: build (failure)
...
UNSUPPORTED: lldb-api :: tools/lldb-server/TestGdbRemoteForkNonStop.py (1204 of 2124)
UNSUPPORTED: lldb-api :: tools/lldb-server/TestGdbRemoteForkResume.py (1205 of 2124)
PASS: lldb-api :: tools/lldb-server/TestGdbRemoteExitCode.py (1206 of 2124)
PASS: lldb-api :: tools/lldb-server/TestGdbRemoteHostInfo.py (1207 of 2124)
PASS: lldb-api :: tools/lldb-server/TestGdbRemoteModuleInfo.py (1208 of 2124)
PASS: lldb-api :: tools/lldb-server/TestGdbRemoteAuxvSupport.py (1209 of 2124)
PASS: lldb-api :: terminal/TestEditlineCompletions.py (1210 of 2124)
UNSUPPORTED: lldb-api :: tools/lldb-server/TestGdbRemoteSaveCore.py (1211 of 2124)
PASS: lldb-api :: tools/lldb-server/TestGdbRemoteKill.py (1212 of 2124)
UNRESOLVED: lldb-api :: tools/lldb-dap/variables/TestDAP_variables.py (1213 of 2124)
******************** TEST 'lldb-api :: tools/lldb-dap/variables/TestDAP_variables.py' FAILED ********************
Script:
--
/usr/bin/python3.10 /home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/llvm-project/lldb/test/API/dotest.py -u CXXFLAGS -u CFLAGS --env LLVM_LIBS_DIR=/home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/./lib --env LLVM_INCLUDE_DIR=/home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/include --env LLVM_TOOLS_DIR=/home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/./bin --arch aarch64 --build-dir /home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/lldb-test-build.noindex --lldb-module-cache-dir /home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/lldb-test-build.noindex/module-cache-lldb/lldb-api --clang-module-cache-dir /home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/lldb-test-build.noindex/module-cache-clang/lldb-api --executable /home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/./bin/lldb --compiler /home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/./bin/clang --dsymutil /home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/./bin/dsymutil --make /usr/bin/gmake --llvm-tools-dir /home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/./bin --lldb-obj-root /home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/tools/lldb --lldb-libs-dir /home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/./lib /home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/llvm-project/lldb/test/API/tools/lldb-dap/variables -p TestDAP_variables.py
--
Exit Code: 1

Command Output (stdout):
--
lldb version 21.0.0git (https://github.com/llvm/llvm-project.git revision 1041d54bd4f693c1ac03077680ece67e03c99e22)
  clang revision 1041d54bd4f693c1ac03077680ece67e03c99e22
  llvm revision 1041d54bd4f693c1ac03077680ece67e03c99e22
Skipping the following test categories: ['libc++', 'dsym', 'gmodules', 'debugserver', 'objc']

--
Command Output (stderr):
--
UNSUPPORTED: LLDB (/home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/bin/clang-aarch64) :: test_darwin_dwarf_missing_obj (TestDAP_variables.TestDAP_variables) (requires one of macosx, darwin, ios, tvos, watchos, bridgeos, iphonesimulator, watchsimulator, appletvsimulator) 
UNSUPPORTED: LLDB (/home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/bin/clang-aarch64) :: test_darwin_dwarf_missing_obj_with_symbol_ondemand_enabled (TestDAP_variables.TestDAP_variables) (requires one of macosx, darwin, ios, tvos, watchos, bridgeos, iphonesimulator, watchsimulator, appletvsimulator) 
========= DEBUG ADAPTER PROTOCOL LOGS =========
1745429438.550805092 --> (stdin/stdout) {"command":"initialize","type":"request","arguments":{"adapterID":"lldb-native","clientID":"vscode","columnsStartAt1":true,"linesStartAt1":true,"locale":"en-us","pathFormat":"path","supportsRunInTerminalRequest":true,"supportsVariablePaging":true,"supportsVariableType":true,"supportsStartDebuggingRequest":true,"supportsProgressReporting":true,"$__lldb_sourceInitFile":false},"seq":1}
1745429438.552837133 <-- (stdin/stdout) {"body":{"$__lldb_version":"lldb version 21.0.0git (https://github.com/llvm/llvm-project.git revision 1041d54bd4f693c1ac03077680ece67e03c99e22)\n  clang revision 1041d54bd4f693c1ac03077680ece67e03c99e22\n  llvm revision 1041d54bd4f693c1ac03077680ece67e03c99e22","completionTriggerCharacters":["."," ","\t"],"exceptionBreakpointFilters":[{"default":false,"filter":"cpp_catch","label":"C++ Catch"},{"default":false,"filter":"cpp_throw","label":"C++ Throw"},{"default":false,"filter":"objc_catch","label":"Objective-C Catch"},{"default":false,"filter":"objc_throw","label":"Objective-C Throw"}],"supportTerminateDebuggee":true,"supportsBreakpointLocationsRequest":true,"supportsCancelRequest":true,"supportsCompletionsRequest":true,"supportsConditionalBreakpoints":true,"supportsConfigurationDoneRequest":true,"supportsDataBreakpoints":true,"supportsDelayedStackTraceLoading":true,"supportsDisassembleRequest":true,"supportsEvaluateForHovers":true,"supportsExceptionInfoRequest":true,"supportsExceptionOptions":true,"supportsFunctionBreakpoints":true,"supportsHitConditionalBreakpoints":true,"supportsInstructionBreakpoints":true,"supportsLogPoints":true,"supportsModulesRequest":true,"supportsReadMemoryRequest":true,"supportsRestartRequest":true,"supportsSetVariable":true,"supportsStepInTargetsRequest":true,"supportsSteppingGranularity":true,"supportsValueFormattingOptions":true},"command":"initialize","request_seq":1,"seq":0,"success":true,"type":"response"}
1745429438.553044558 --> (stdin/stdout) {"command":"launch","type":"request","arguments":{"program":"/home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/lldb-test-build.noindex/tools/lldb-dap/variables/TestDAP_variables.test_indexedVariables/a.out","initCommands":["settings clear -all","settings set symbols.enable-external-lookup false","settings set target.inherit-tcc true","settings set target.disable-aslr false","settings set target.detach-on-error false","settings set target.auto-apply-fixits false","settings set plugin.process.gdb-remote.packet-timeout 60","settings set symbols.clang-modules-cache-path \"/home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/lldb-test-build.noindex/module-cache-lldb/lldb-api\"","settings set use-color false","settings set show-statusline false"],"disableASLR":false,"enableAutoVariableSummaries":false,"enableSyntheticChildDebugging":false,"displayExtendedBacktrace":false,"commandEscapePrefix":null},"seq":2}
1745429438.553236723 <-- (stdin/stdout) {"body":{"category":"console","output":"Running initCommands:\n"},"event":"output","seq":0,"type":"event"}
1745429438.553258181 <-- (stdin/stdout) {"body":{"category":"console","output":"(lldb) settings clear -all\n"},"event":"output","seq":0,"type":"event"}
1745429438.553269863 <-- (stdin/stdout) {"body":{"category":"console","output":"(lldb) settings set symbols.enable-external-lookup false\n"},"event":"output","seq":0,"type":"event"}
1745429438.553279161 <-- (stdin/stdout) {"body":{"category":"console","output":"(lldb) settings set target.inherit-tcc true\n"},"event":"output","seq":0,"type":"event"}
1745429438.553287983 <-- (stdin/stdout) {"body":{"category":"console","output":"(lldb) settings set target.disable-aslr false\n"},"event":"output","seq":0,"type":"event"}
1745429438.553296328 <-- (stdin/stdout) {"body":{"category":"console","output":"(lldb) settings set target.detach-on-error false\n"},"event":"output","seq":0,"type":"event"}
1745429438.553304434 <-- (stdin/stdout) {"body":{"category":"console","output":"(lldb) settings set target.auto-apply-fixits false\n"},"event":"output","seq":0,"type":"event"}
1745429438.553312302 <-- (stdin/stdout) {"body":{"category":"console","output":"(lldb) settings set plugin.process.gdb-remote.packet-timeout 60\n"},"event":"output","seq":0,"type":"event"}
1745429438.553332329 <-- (stdin/stdout) {"body":{"category":"console","output":"(lldb) settings set symbols.clang-modules-cache-path \"/home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/lldb-test-build.noindex/module-cache-lldb/lldb-api\"\n"},"event":"output","seq":0,"type":"event"}
1745429438.553340435 <-- (stdin/stdout) {"body":{"category":"console","output":"(lldb) settings set use-color false\n"},"event":"output","seq":0,"type":"event"}
1745429438.553348303 <-- (stdin/stdout) {"body":{"category":"console","output":"(lldb) settings set show-statusline false\n"},"event":"output","seq":0,"type":"event"}
1745429438.630547285 <-- (stdin/stdout) {"command":"launch","request_seq":2,"seq":0,"success":true,"type":"response"}
1745429438.630607128 <-- (stdin/stdout) {"body":{"isLocalProcess":true,"name":"/home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/lldb-test-build.noindex/tools/lldb-dap/variables/TestDAP_variables.test_indexedVariables/a.out","startMethod":"launch","systemProcessId":1926822},"event":"process","seq":0,"type":"event"}
1745429438.630617142 <-- (stdin/stdout) {"event":"initialized","seq":0,"type":"event"}
1745429438.630911589 --> (stdin/stdout) {"command":"setBreakpoints","type":"request","arguments":{"source":{"name":"main.cpp","path":"main.cpp"},"sourceModified":false,"lines":[40],"breakpoints":[{"line":40}]},"seq":3}
1745429438.632362127 <-- (stdin/stdout) {"body":{"breakpoints":[{"column":1,"id":1,"instructionReference":"0xAAAAE25C0C54","line":41,"source":{"name":"main.cpp","path":"main.cpp"},"verified":true}]},"command":"setBreakpoints","request_seq":3,"seq":0,"success":true,"type":"response"}

IanWood1 pushed a commit to IanWood1/llvm-project that referenced this pull request May 6, 2025
…pes (llvm#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.
IanWood1 pushed a commit to IanWood1/llvm-project that referenced this pull request May 6, 2025
…pes (llvm#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.
IanWood1 pushed a commit to IanWood1/llvm-project that referenced this pull request May 6, 2025
…pes (llvm#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.
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.

4 participants