-
Notifications
You must be signed in to change notification settings - Fork 14.3k
[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
Conversation
…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.
@llvm/pr-subscribers-lldb Author: John Harrison (ashgti) ChangesThis 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:
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) {
|
There was a problem hiding this 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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
LLVM Buildbot has detected a new failure on builder 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
|
…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.
…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.
…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.
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.