[lldb-dap] Migrate restart request to structured types #172488
+175
−148
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Member
|
@llvm/pr-subscribers-lldb Author: Sergei Druzhkov (DrSergei) ChangesThis patch migrates Full diff: https://github.com/llvm/llvm-project/pull/172488.diff 5 Files Affected:
diff --git a/lldb/tools/lldb-dap/Handler/RequestHandler.h b/lldb/tools/lldb-dap/Handler/RequestHandler.h
index 0669be50597e9..a18a8049c804d 100644
--- a/lldb/tools/lldb-dap/Handler/RequestHandler.h
+++ b/lldb/tools/lldb-dap/Handler/RequestHandler.h
@@ -340,11 +340,14 @@ class LaunchRequestHandler
void PostRun() const override;
};
-class RestartRequestHandler : public LegacyRequestHandler {
+class RestartRequestHandler
+ : public RequestHandler<std::optional<protocol::RestartArguments>,
+ protocol::RestartResponse> {
public:
- using LegacyRequestHandler::LegacyRequestHandler;
+ using RequestHandler::RequestHandler;
static llvm::StringLiteral GetCommand() { return "restart"; }
- void operator()(const llvm::json::Object &request) const override;
+ llvm::Error
+ Run(const std::optional<protocol::RestartArguments> &args) const override;
};
class NextRequestHandler
diff --git a/lldb/tools/lldb-dap/Handler/RestartRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/RestartRequestHandler.cpp
index 100173bfc3082..3a2be3f30c7cf 100644
--- a/lldb/tools/lldb-dap/Handler/RestartRequestHandler.cpp
+++ b/lldb/tools/lldb-dap/Handler/RestartRequestHandler.cpp
@@ -7,91 +7,37 @@
//===----------------------------------------------------------------------===//
#include "DAP.h"
+#include "DAPError.h"
#include "EventHelper.h"
-#include "JSONUtils.h"
#include "LLDBUtils.h"
#include "Protocol/ProtocolRequests.h"
#include "RequestHandler.h"
-#include "llvm/Support/JSON.h"
-#include "llvm/Support/raw_ostream.h"
-namespace lldb_dap {
+using namespace lldb_dap;
+using namespace lldb_dap::protocol;
-// "RestartRequest": {
-// "allOf": [ { "$ref": "#/definitions/Request" }, {
-// "type": "object",
-// "description": "Restarts a debug session. Clients should only call this
-// request if the corresponding capability `supportsRestartRequest` is
-// true.\nIf the capability is missing or has the value false, a typical
-// client emulates `restart` by terminating the debug adapter first and then
-// launching it anew.",
-// "properties": {
-// "command": {
-// "type": "string",
-// "enum": [ "restart" ]
-// },
-// "arguments": {
-// "$ref": "#/definitions/RestartArguments"
-// }
-// },
-// "required": [ "command" ]
-// }]
-// },
-// "RestartArguments": {
-// "type": "object",
-// "description": "Arguments for `restart` request.",
-// "properties": {
-// "arguments": {
-// "oneOf": [
-// { "$ref": "#/definitions/LaunchRequestArguments" },
-// { "$ref": "#/definitions/AttachRequestArguments" }
-// ],
-// "description": "The latest version of the `launch` or `attach`
-// configuration."
-// }
-// }
-// },
-// "RestartResponse": {
-// "allOf": [ { "$ref": "#/definitions/Response" }, {
-// "type": "object",
-// "description": "Response to `restart` request. This is just an
-// acknowledgement, so no body field is required."
-// }]
-// },
-void RestartRequestHandler::operator()(
- const llvm::json::Object &request) const {
- llvm::json::Object response;
- FillResponse(request, response);
- if (!dap.target.GetProcess().IsValid()) {
- response["success"] = llvm::json::Value(false);
- EmplaceSafeString(response, "message",
- "Restart request received but no process was launched.");
- dap.SendJSON(llvm::json::Value(std::move(response)));
- return;
- }
+/// Restarts a debug session. Clients should only call this request if the
+/// corresponding capability `supportsRestartRequest` is true.
+/// If the capability is missing or has the value false, a typical client
+/// emulates `restart` by terminating the debug adapter first and then launching
+/// it anew.
+llvm::Error
+RestartRequestHandler::Run(const std::optional<RestartArguments> &args) const {
+ if (!dap.target.GetProcess().IsValid())
+ return llvm::make_error<DAPError>(
+ "Restart request received but no process was launched.");
- const llvm::json::Object *arguments = request.getObject("arguments");
- if (arguments) {
- // The optional `arguments` field in RestartRequest can contain an updated
- // version of the launch arguments. If there's one, use it.
- if (const llvm::json::Value *restart_arguments =
- arguments->get("arguments")) {
- protocol::LaunchRequestArguments updated_arguments;
- llvm::json::Path::Root root;
- if (!fromJSON(*restart_arguments, updated_arguments, root)) {
- response["success"] = llvm::json::Value(false);
- EmplaceSafeString(
- response, "message",
- llvm::formatv("Failed to parse updated launch arguments: {0}",
- llvm::toString(root.getError()))
- .str());
- dap.SendJSON(llvm::json::Value(std::move(response)));
- return;
- }
- dap.last_launch_request = updated_arguments;
+ if (args) {
+ if (std::holds_alternative<AttachRequestArguments>(args->arguments))
+ return llvm::make_error<DAPError>(
+ "Restarting an AttachRequest is not supported.");
+ if (const auto *arguments =
+ std::get_if<LaunchRequestArguments>(&args->arguments);
+ arguments) {
+ dap.last_launch_request = *arguments;
// Update DAP configuration based on the latest copy of the launch
// arguments.
- dap.SetConfiguration(updated_arguments.configuration, false);
+ dap.SetConfiguration(arguments->configuration, false);
dap.ConfigureSourceMaps();
}
}
@@ -117,12 +63,8 @@ void RestartRequestHandler::operator()(
// FIXME: Should we run 'preRunCommands'?
// FIXME: Should we add a 'preRestartCommands'?
- if (llvm::Error err = LaunchProcess(*dap.last_launch_request)) {
- response["success"] = llvm::json::Value(false);
- EmplaceSafeString(response, "message", llvm::toString(std::move(err)));
- dap.SendJSON(llvm::json::Value(std::move(response)));
- return;
- }
+ if (llvm::Error err = LaunchProcess(*dap.last_launch_request))
+ return llvm::make_error<DAPError>(llvm::toString(std::move(err)));
SendProcessEvent(dap, Launch);
@@ -130,16 +72,11 @@ void RestartRequestHandler::operator()(
// Because we're restarting, configuration has already happened so we can
// continue the process right away.
if (dap.stop_at_entry) {
- if (llvm::Error err = SendThreadStoppedEvent(dap, /*on_entry=*/true)) {
- EmplaceSafeString(response, "message", llvm::toString(std::move(err)));
- dap.SendJSON(llvm::json::Value(std::move(response)));
- return;
- }
+ if (llvm::Error err = SendThreadStoppedEvent(dap, /*on_entry=*/true))
+ return llvm::make_error<DAPError>(llvm::toString(std::move(err)));
} else {
dap.target.GetProcess().Continue();
}
- dap.SendJSON(llvm::json::Value(std::move(response)));
+ return llvm::Error::success();
}
-
-} // namespace lldb_dap
diff --git a/lldb/tools/lldb-dap/Protocol/ProtocolRequests.cpp b/lldb/tools/lldb-dap/Protocol/ProtocolRequests.cpp
index bf470b78077df..1c2cd158ffd29 100644
--- a/lldb/tools/lldb-dap/Protocol/ProtocolRequests.cpp
+++ b/lldb/tools/lldb-dap/Protocol/ProtocolRequests.cpp
@@ -296,31 +296,53 @@ bool fromJSON(const json::Value &Params, Console &C, json::Path P) {
bool fromJSON(const json::Value &Params, LaunchRequestArguments &LRA,
json::Path P) {
json::ObjectMapper O(Params, P);
- return O && fromJSON(Params, LRA.configuration, P) &&
- O.mapOptional("noDebug", LRA.noDebug) &&
- O.mapOptional("launchCommands", LRA.launchCommands) &&
- O.mapOptional("cwd", LRA.cwd) && O.mapOptional("args", LRA.args) &&
- O.mapOptional("detachOnError", LRA.detachOnError) &&
- O.mapOptional("disableASLR", LRA.disableASLR) &&
- O.mapOptional("disableSTDIO", LRA.disableSTDIO) &&
- O.mapOptional("shellExpandArguments", LRA.shellExpandArguments) &&
- O.mapOptional("runInTerminal", LRA.console) &&
- O.mapOptional("console", LRA.console) &&
- O.mapOptional("stdio", LRA.stdio) && parseEnv(Params, LRA.env, P);
+ bool success =
+ O && fromJSON(Params, LRA.configuration, P) &&
+ O.mapOptional("noDebug", LRA.noDebug) &&
+ O.mapOptional("launchCommands", LRA.launchCommands) &&
+ O.mapOptional("cwd", LRA.cwd) && O.mapOptional("args", LRA.args) &&
+ O.mapOptional("detachOnError", LRA.detachOnError) &&
+ O.mapOptional("disableASLR", LRA.disableASLR) &&
+ O.mapOptional("disableSTDIO", LRA.disableSTDIO) &&
+ O.mapOptional("shellExpandArguments", LRA.shellExpandArguments) &&
+ O.mapOptional("runInTerminal", LRA.console) &&
+ O.mapOptional("console", LRA.console) &&
+ O.mapOptional("stdio", LRA.stdio) && parseEnv(Params, LRA.env, P);
+ if (!success)
+ return false;
+ if (LRA.configuration.program.empty() && LRA.launchCommands.empty()) {
+ P.report("`program` or `launchCommands` should be provided");
+ return false;
+ }
+ return true;
}
bool fromJSON(const json::Value &Params, AttachRequestArguments &ARA,
json::Path P) {
json::ObjectMapper O(Params, P);
- return O && fromJSON(Params, ARA.configuration, P) &&
- O.mapOptional("attachCommands", ARA.attachCommands) &&
- O.mapOptional("pid", ARA.pid) &&
- O.mapOptional("waitFor", ARA.waitFor) &&
- O.mapOptional("gdb-remote-port", ARA.gdbRemotePort) &&
- O.mapOptional("gdb-remote-hostname", ARA.gdbRemoteHostname) &&
- O.mapOptional("coreFile", ARA.coreFile) &&
- O.mapOptional("targetId", ARA.targetId) &&
- O.mapOptional("debuggerId", ARA.debuggerId);
+ bool success = O && fromJSON(Params, ARA.configuration, P) &&
+ O.mapOptional("attachCommands", ARA.attachCommands) &&
+ O.mapOptional("pid", ARA.pid) &&
+ O.mapOptional("waitFor", ARA.waitFor) &&
+ O.mapOptional("gdb-remote-port", ARA.gdbRemotePort) &&
+ O.mapOptional("gdb-remote-hostname", ARA.gdbRemoteHostname) &&
+ O.mapOptional("coreFile", ARA.coreFile) &&
+ O.mapOptional("targetId", ARA.targetId) &&
+ O.mapOptional("debuggerId", ARA.debuggerId);
+ if (!success)
+ return false;
+ if (ARA.debuggerId.has_value())
+ return true;
+ if (ARA.targetId.has_value())
+ return true;
+ if ((ARA.pid == LLDB_INVALID_PROCESS_ID) &&
+ ARA.configuration.program.empty() && ARA.attachCommands.empty() &&
+ ARA.coreFile.empty() && (ARA.gdbRemotePort == LLDB_DAP_INVALID_PORT)) {
+ P.report("`pid` or `program` or `attachCommands` or `coreFile` or "
+ "`gdbRemotePort` should be provided");
+ return false;
+ }
+ return true;
}
bool fromJSON(const json::Value &Params, ContinueArguments &CA, json::Path P) {
@@ -737,4 +759,30 @@ llvm::json::Value toJSON(const TestGetTargetBreakpointsResponseBody &Body) {
return result;
}
+bool fromJSON(const llvm::json::Value &Params, RestartArguments &Args,
+ llvm::json::Path Path) {
+ const json::Object *O = Params.getAsObject();
+ if (!O) {
+ Path.report("expected object");
+ return false;
+ }
+ const json::Value *arguments = O->get("arguments");
+ if (arguments == nullptr)
+ return true;
+ LaunchRequestArguments launchArguments;
+ llvm::json::Path::Root root;
+ if (fromJSON(*arguments, launchArguments, root)) {
+ Args.arguments = std::move(launchArguments);
+ return true;
+ }
+ AttachRequestArguments attachArguments;
+ if (fromJSON(*arguments, attachArguments, root)) {
+ Args.arguments = std::move(attachArguments);
+ return true;
+ }
+ Path.report(
+ "failed to parse arguments, expected `launch` or `attach` arguments");
+ return false;
+}
+
} // namespace lldb_dap::protocol
diff --git a/lldb/tools/lldb-dap/Protocol/ProtocolRequests.h b/lldb/tools/lldb-dap/Protocol/ProtocolRequests.h
index cc123943ec0b6..33fcaae1710b5 100644
--- a/lldb/tools/lldb-dap/Protocol/ProtocolRequests.h
+++ b/lldb/tools/lldb-dap/Protocol/ProtocolRequests.h
@@ -31,6 +31,7 @@
#include <cstdint>
#include <optional>
#include <string>
+#include <variant>
#include <vector>
namespace lldb_dap::protocol {
@@ -1255,6 +1256,18 @@ struct TestGetTargetBreakpointsResponseBody {
};
llvm::json::Value toJSON(const TestGetTargetBreakpointsResponseBody &);
+/// Arguments for `restart` request.
+struct RestartArguments {
+ /// The latest version of the `launch` or `attach` configuration.
+ std::variant<std::monostate, LaunchRequestArguments, AttachRequestArguments>
+ arguments = std::monostate{};
+};
+bool fromJSON(const llvm::json::Value &, RestartArguments &, llvm::json::Path);
+
+/// Response to `restart` request. This is just an acknowledgement, so no body
+/// field is required.
+using RestartResponse = VoidResponse;
+
} // namespace lldb_dap::protocol
#endif
diff --git a/lldb/unittests/DAP/ProtocolRequestsTest.cpp b/lldb/unittests/DAP/ProtocolRequestsTest.cpp
index 710b78960ef09..c639e40453fb0 100644
--- a/lldb/unittests/DAP/ProtocolRequestsTest.cpp
+++ b/lldb/unittests/DAP/ProtocolRequestsTest.cpp
@@ -304,3 +304,43 @@ TEST(ProtocolRequestsTest, TestGetTargetBreakpointsResponseBody) {
ASSERT_THAT_EXPECTED(expected, llvm::Succeeded());
EXPECT_EQ(PrettyPrint(*expected), PrettyPrint(body));
}
+
+TEST(ProtocolRequestsTest, RestartArguments) {
+ llvm::Expected<RestartArguments> expected = parse<RestartArguments>(R"({})");
+ ASSERT_THAT_EXPECTED(expected, llvm::Succeeded());
+ EXPECT_TRUE(std::holds_alternative<std::monostate>(expected->arguments));
+
+ // Check missed keys.
+ expected = parse<RestartArguments>(R"({
+ "arguments": {}
+ })");
+ EXPECT_THAT_EXPECTED(expected,
+ FailedWithMessage("failed to parse arguments, expected "
+ "`launch` or `attach` arguments"));
+
+ // Check launch arguments.
+ expected = parse<RestartArguments>(R"({
+ "arguments": {
+ "program": "main.exe",
+ "cwd": "/home/root"
+ }
+ })");
+ ASSERT_THAT_EXPECTED(expected, llvm::Succeeded());
+ const LaunchRequestArguments *launch_args =
+ std::get_if<LaunchRequestArguments>(&expected->arguments);
+ EXPECT_NE(launch_args, nullptr);
+ EXPECT_EQ(launch_args->configuration.program, "main.exe");
+ EXPECT_EQ(launch_args->cwd, "/home/root");
+
+ // Check attach arguments.
+ expected = parse<RestartArguments>(R"({
+ "arguments": {
+ "pid": 123
+ }
+ })");
+ ASSERT_THAT_EXPECTED(expected, llvm::Succeeded());
+ const AttachRequestArguments *attach_args =
+ std::get_if<AttachRequestArguments>(&expected->arguments);
+ EXPECT_NE(attach_args, nullptr);
+ EXPECT_EQ(attach_args->pid, 123U);
+}
|
ashgti
reviewed
Dec 16, 2025
| return; | ||
| } | ||
| if (llvm::Error err = LaunchProcess(*dap.last_launch_request)) | ||
| return llvm::make_error<DAPError>(llvm::toString(std::move(err))); |
Contributor
There was a problem hiding this comment.
You should be able to just return the error without getting the message and converting that into a DAPError
| return; | ||
| } | ||
| if (llvm::Error err = SendThreadStoppedEvent(dap, /*on_entry=*/true)) | ||
| return llvm::make_error<DAPError>(llvm::toString(std::move(err))); |
Comment on lines
313
to
316
| if (LRA.configuration.program.empty() && LRA.launchCommands.empty()) { | ||
| P.report("`program` or `launchCommands` should be provided"); | ||
| return false; | ||
| } |
Contributor
There was a problem hiding this comment.
One other validation we could add is launchCommands being set and console != 'internal', those two are mutually exclusive. See
llvm-project/lldb/tools/lldb-dap/Handler/LaunchRequestHandler.cpp
Lines 29 to 33 in 1bd0ec4
Comment on lines
338
to
344
| if ((ARA.pid == LLDB_INVALID_PROCESS_ID) && | ||
| ARA.configuration.program.empty() && ARA.attachCommands.empty() && | ||
| ARA.coreFile.empty() && (ARA.gdbRemotePort == LLDB_DAP_INVALID_PORT)) { | ||
| P.report("`pid` or `program` or `attachCommands` or `coreFile` or " | ||
| "`gdbRemotePort` should be provided"); | ||
| return false; | ||
| } |
Contributor
There was a problem hiding this comment.
We could also check that pid and gdbRemotePort are not both provided, those are mutually exclusive.
See
llvm-project/lldb/tools/lldb-dap/Handler/AttachRequestHandler.cpp
Lines 59 to 63 in 1bd0ec4
| ARA.configuration.program.empty() && ARA.attachCommands.empty() && | ||
| ARA.coreFile.empty() && (ARA.gdbRemotePort == LLDB_DAP_INVALID_PORT)) { | ||
| P.report("`pid` or `program` or `attachCommands` or `coreFile` or " | ||
| "`gdbRemotePort` should be provided"); |
Contributor
There was a problem hiding this comment.
I think the field name is gdb-remote-port.
No need to change it, but its one of the only fields with dashes, I wonder if we could migrate that to gdbRemotePort and add a message that the old spelling is deprecated? I can add a bug for that.
Contributor
Author
There was a problem hiding this comment.
It looks like that the best solution here is adding conversion from new style arguments to old style arguments on extension side (resolveDebugConfiguration method). It doesn't break backward compatibility for clients. But for new users error message might be confused.
Contributor
There was a problem hiding this comment.
I filed #172555 to look into the naming consistency.
da-viper
reviewed
Dec 16, 2025
| if (std::holds_alternative<AttachRequestArguments>(args->arguments)) | ||
| return llvm::make_error<DAPError>( | ||
| "Restarting an AttachRequest is not supported."); | ||
| if (const auto *arguments = |
Contributor
There was a problem hiding this comment.
could we assert it is not monostate as it should be unreachable.
Contributor
Author
There was a problem hiding this comment.
According to DAP arguments property could be omitted. I use std::monostate to represent this case, so it is a valid state.
Contributor
There was a problem hiding this comment.
We only assert it only if we have args.
Contributor
Author
There was a problem hiding this comment.
I don't understand what you mean, I check args above (line 30).
| return true; | ||
| } | ||
| AttachRequestArguments attachArguments; | ||
| if (fromJSON(*arguments, attachArguments, root)) { |
Contributor
There was a problem hiding this comment.
Is there a need to parse AttachRequestArguments if it cannot be used when restarting ?
Contributor
Author
There was a problem hiding this comment.
To be honest, we can skip it now. Older version assumes that it is always launch arguments, but new version is more type safe from my point of view.
da-viper
approved these changes
Dec 17, 2025
| if (std::holds_alternative<AttachRequestArguments>(args->arguments)) | ||
| return llvm::make_error<DAPError>( | ||
| "Restarting an AttachRequest is not supported."); | ||
| if (const auto *arguments = |
Contributor
There was a problem hiding this comment.
We only assert it only if we have args.
ashgti
approved these changes
Dec 17, 2025
da-viper
approved these changes
Dec 18, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This patch migrates
restartrequest to structured types. Also, I added some checks that at least one of the required fields was provided forlaunchandattachrequests. Maybe I missed some possible configurations, so please double check.