[lldb-dap] Creating a 'capabilities' event helper. #142831
Open
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
|
@llvm/pr-subscribers-lldb Author: John Harrison (ashgti) ChangesThis adds a new 'CapabilitiesEventBody' type for having a well structured type for the event and updates the 'stepInTargets' and 'restart' request to dynamically set their capabilities. This also fixes the 'stepInTargets' test on non-x86 platforms. Full diff: https://github.com/llvm/llvm-project/pull/142831.diff 11 Files Affected:
diff --git a/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py b/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py
index f1e3cab06ccde..c3c8dbb12e9d0 100644
--- a/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py
+++ b/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py
@@ -135,6 +135,10 @@ def as_dict(self):
return source_dict
+class NotSupportedError(Exception):
+ """Raised if a feature is not supported due to its capabilities."""
+
+
class DebugCommunication(object):
def __init__(
self,
@@ -153,7 +157,7 @@ def __init__(
self.recv_thread = threading.Thread(target=self._read_packet_thread)
self.process_event_body = None
self.exit_status: Optional[int] = None
- self.initialize_body: dict[str, Any] = {}
+ self.capabilities: dict[str, Any] = {}
self.progress_events: list[Event] = []
self.reverse_requests = []
self.sequence = 1
@@ -301,8 +305,8 @@ def _handle_recv_packet(self, packet: Optional[ProtocolMessage]) -> bool:
# Breakpoint events are sent when a breakpoint is resolved
self._update_verified_breakpoints([body["breakpoint"]])
elif event == "capabilities":
- # update the capabilities with new ones from the event.
- self.initialize_body.update(body["capabilities"])
+ # Update the capabilities with new ones from the event.
+ self.capabilities.update(body["capabilities"])
elif packet_type == "response":
if packet["command"] == "disconnect":
@@ -491,13 +495,11 @@ def wait_for_terminated(self, timeout: Optional[float] = None):
raise ValueError("didn't get terminated event")
return event_dict
- def get_initialize_value(self, key):
+ def get_capability(self, key, default=None):
"""Get a value for the given key if it there is a key/value pair in
- the "initialize" request response body.
+ the capabilities reported by the adapter.
"""
- if self.initialize_body and key in self.initialize_body:
- return self.initialize_body[key]
- raise ValueError(f"no value for key: {key} in {self.initialize_body}")
+ return self.capabilities.get(key, default)
def get_threads(self):
if self.threads is None:
@@ -763,6 +765,10 @@ def request_continue(self, threadId=None, singleThread=False):
return response
def request_restart(self, restartArguments=None):
+ if self.exit_status is not None:
+ raise ValueError("request_restart called after process exited")
+ if not self.get_capability("supportsRestartRequest", False):
+ raise NotSupportedError("supportsRestartRequest is not set")
command_dict = {
"command": "restart",
"type": "request",
@@ -870,7 +876,7 @@ def request_initialize(self, sourceInitFile=False):
response = self.send_recv(command_dict)
if response:
if "body" in response:
- self.initialize_body = response["body"]
+ self.capabilities = response["body"]
return response
def request_launch(
@@ -975,6 +981,8 @@ def request_stepIn(self, threadId, targetId, granularity="statement"):
def request_stepInTargets(self, frameId):
if self.exit_status is not None:
raise ValueError("request_stepInTargets called after process exited")
+ if not self.get_capability("supportsStepInTargetsRequest", False):
+ raise NotSupportedError("supportsStepInTargetsRequest is not set")
args_dict = {"frameId": frameId}
command_dict = {
"command": "stepInTargets",
diff --git a/lldb/test/API/tools/lldb-dap/launch/TestDAP_launch.py b/lldb/test/API/tools/lldb-dap/launch/TestDAP_launch.py
index c24df9a9bfcf4..ae8142ae4f484 100644
--- a/lldb/test/API/tools/lldb-dap/launch/TestDAP_launch.py
+++ b/lldb/test/API/tools/lldb-dap/launch/TestDAP_launch.py
@@ -566,7 +566,7 @@ def test_version(self):
)
version_eval_output = version_eval_response["body"]["result"]
- version_string = self.dap_server.get_initialize_value("$__lldb_version")
+ version_string = self.dap_server.get_capability("$__lldb_version")
self.assertEqual(
version_eval_output.splitlines(),
version_string.splitlines(),
diff --git a/lldb/test/API/tools/lldb-dap/stepInTargets/TestDAP_stepInTargets.py b/lldb/test/API/tools/lldb-dap/stepInTargets/TestDAP_stepInTargets.py
index af698074f3479..ae827cde823c0 100644
--- a/lldb/test/API/tools/lldb-dap/stepInTargets/TestDAP_stepInTargets.py
+++ b/lldb/test/API/tools/lldb-dap/stepInTargets/TestDAP_stepInTargets.py
@@ -30,6 +30,8 @@ def test_basic(self):
self.assertEqual(
len(breakpoint_ids), len(lines), "expect correct number of breakpoints"
)
+ # Target based capability 'supportsStepInTargetsRequest' is sent in
+ # 'configurationDone' which is called prior to continue.
self.continue_to_breakpoints(breakpoint_ids)
threads = self.dap_server.get_threads()
@@ -89,17 +91,14 @@ def test_supported_capability_x86_arch(self):
self.assertEqual(
len(breakpoint_ids), len(bp_lines), "expect correct number of breakpoints"
)
- is_supported = self.dap_server.get_initialize_value(
- "supportsStepInTargetsRequest"
+ is_supported = self.dap_server.get_capability(
+ "supportsStepInTargetsRequest", False
)
- self.assertEqual(
+ self.assertTrue(
is_supported,
- True,
f"expect capability `stepInTarget` is supported with architecture {self.getArchitecture()}",
)
- # clear breakpoints.
- self.set_source_breakpoints(source, [])
self.continue_to_exit()
@skipIf(archs=["x86", "x86_64"])
@@ -112,15 +111,16 @@ def test_supported_capability_other_archs(self):
self.assertEqual(
len(breakpoint_ids), len(bp_lines), "expect correct number of breakpoints"
)
- is_supported = self.dap_server.get_initialize_value(
- "supportsStepInTargetsRequest"
+ # Target based capability 'supportsStepInTargetsRequest' is sent in
+ # 'configurationDone' which is called prior to continue.
+ self.continue_to_breakpoints(breakpoint_ids)
+
+ is_supported = self.dap_server.get_capability(
+ "supportsStepInTargetsRequest", False
)
- self.assertEqual(
+ self.assertFalse(
is_supported,
- False,
f"expect capability `stepInTarget` is not supported with architecture {self.getArchitecture()}",
)
- # clear breakpoints.
- self.set_source_breakpoints(source, [])
self.continue_to_exit()
diff --git a/lldb/tools/lldb-dap/CMakeLists.txt b/lldb/tools/lldb-dap/CMakeLists.txt
index f65c987d4c9ce..8bbb402fdf782 100644
--- a/lldb/tools/lldb-dap/CMakeLists.txt
+++ b/lldb/tools/lldb-dap/CMakeLists.txt
@@ -67,6 +67,7 @@ add_lldb_library(lldbDAP
Handler/VariablesRequestHandler.cpp
Protocol/ProtocolBase.cpp
+ Protocol/ProtocolEvents.cpp
Protocol/ProtocolTypes.cpp
Protocol/ProtocolRequests.cpp
diff --git a/lldb/tools/lldb-dap/EventHelper.cpp b/lldb/tools/lldb-dap/EventHelper.cpp
index 33bc7c2cbef11..ebf488287aeb6 100644
--- a/lldb/tools/lldb-dap/EventHelper.cpp
+++ b/lldb/tools/lldb-dap/EventHelper.cpp
@@ -11,6 +11,8 @@
#include "DAPLog.h"
#include "JSONUtils.h"
#include "LLDBUtils.h"
+#include "Protocol/ProtocolEvents.h"
+#include "Protocol/ProtocolTypes.h"
#include "lldb/API/SBFileSpec.h"
#if defined(_WIN32)
@@ -37,20 +39,26 @@ void SendTargetBasedCapabilities(DAP &dap) {
if (!dap.target.IsValid())
return;
+ protocol::CapabilitiesEventBody body;
+
// FIXME: stepInTargets request is only supported by the x86
// architecture remove when `lldb::InstructionControlFlowKind` is
// supported by other architectures
const llvm::StringRef target_triple = dap.target.GetTriple();
if (target_triple.starts_with("x86"))
- return;
+ body.capabilities.supportedFeatures.insert(
+ protocol::eAdapterFeatureStepInTargetsRequest);
- protocol::Event event;
- event.event = "capabilities";
- event.body = llvm::json::Object{
- {"capabilities",
- llvm::json::Object{{"supportsStepInTargetsRequest", false}}}};
- dap.Send(event);
+ // We only support restarting launch requests not attach requests.
+ if (dap.last_launch_request)
+ body.capabilities.supportedFeatures.insert(
+ protocol::eAdapterFeatureRestartRequest);
+
+ // Only notify the client if supportedFeatures changed.
+ if (!body.capabilities.supportedFeatures.empty())
+ dap.Send(protocol::Event{"capabilities", body});
}
+
// "ProcessEvent": {
// "allOf": [
// { "$ref": "#/definitions/Event" },
diff --git a/lldb/tools/lldb-dap/EventHelper.h b/lldb/tools/lldb-dap/EventHelper.h
index e648afbf67e59..bf177d0017075 100644
--- a/lldb/tools/lldb-dap/EventHelper.h
+++ b/lldb/tools/lldb-dap/EventHelper.h
@@ -16,6 +16,7 @@ struct DAP;
enum LaunchMethod { Launch, Attach, AttachForSuspendedLaunch };
+/// Update capabilities based on the configured target.
void SendTargetBasedCapabilities(DAP &dap);
void SendProcessEvent(DAP &dap, LaunchMethod launch_method);
diff --git a/lldb/tools/lldb-dap/Handler/RequestHandler.h b/lldb/tools/lldb-dap/Handler/RequestHandler.h
index 559929ffb21e8..0eebc0bb6c9e6 100644
--- a/lldb/tools/lldb-dap/Handler/RequestHandler.h
+++ b/lldb/tools/lldb-dap/Handler/RequestHandler.h
@@ -334,9 +334,6 @@ class RestartRequestHandler : public LegacyRequestHandler {
public:
using LegacyRequestHandler::LegacyRequestHandler;
static llvm::StringLiteral GetCommand() { return "restart"; }
- FeatureSet GetSupportedFeatures() const override {
- return {protocol::eAdapterFeatureRestartRequest};
- }
void operator()(const llvm::json::Object &request) const override;
};
@@ -363,23 +360,10 @@ class StepInTargetsRequestHandler
public:
using RequestHandler::RequestHandler;
static llvm::StringLiteral GetCommand() { return "stepInTargets"; }
- FeatureSet GetSupportedFeatures() const override {
- return {protocol::eAdapterFeatureStepInTargetsRequest};
- }
llvm::Expected<protocol::StepInTargetsResponseBody>
Run(const protocol::StepInTargetsArguments &args) const override;
};
-class StepInTargetsRequestHandler2 : public LegacyRequestHandler {
-public:
- using LegacyRequestHandler::LegacyRequestHandler;
- static llvm::StringLiteral GetCommand() { return "stepInTargets"; }
- FeatureSet GetSupportedFeatures() const override {
- return {protocol::eAdapterFeatureStepInTargetsRequest};
- }
- void operator()(const llvm::json::Object &request) const override;
-};
-
class StepOutRequestHandler : public RequestHandler<protocol::StepOutArguments,
protocol::StepOutResponse> {
public:
diff --git a/lldb/tools/lldb-dap/Handler/RestartRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/RestartRequestHandler.cpp
index 58091b622f7e5..c7ef63472ad21 100644
--- a/lldb/tools/lldb-dap/Handler/RestartRequestHandler.cpp
+++ b/lldb/tools/lldb-dap/Handler/RestartRequestHandler.cpp
@@ -69,23 +69,6 @@ void RestartRequestHandler::operator()(
dap.SendJSON(llvm::json::Value(std::move(response)));
return;
}
- // Check if we were in a "launch" session or an "attach" session.
- //
- // Restarting is not well defined when we started the session by attaching to
- // an existing process, because we don't know how the process was started, so
- // we don't support it.
- //
- // Note that when using runInTerminal we're technically attached, but it's an
- // implementation detail. The adapter *did* launch the process in response to
- // a "launch" command, so we can still stop it and re-run it. This is why we
- // don't just check `dap.is_attach`.
- if (!dap.last_launch_request) {
- response["success"] = llvm::json::Value(false);
- EmplaceSafeString(response, "message",
- "Restarting an \"attach\" session is not supported.");
- dap.SendJSON(llvm::json::Value(std::move(response)));
- return;
- }
const llvm::json::Object *arguments = request.getObject("arguments");
if (arguments) {
diff --git a/lldb/tools/lldb-dap/Protocol/ProtocolBase.h b/lldb/tools/lldb-dap/Protocol/ProtocolBase.h
index 724da59b50cd2..81496380d412f 100644
--- a/lldb/tools/lldb-dap/Protocol/ProtocolBase.h
+++ b/lldb/tools/lldb-dap/Protocol/ProtocolBase.h
@@ -17,8 +17,8 @@
//
//===----------------------------------------------------------------------===//
-#ifndef LLDB_TOOLS_LLDB_DAP_PROTOCOL_H
-#define LLDB_TOOLS_LLDB_DAP_PROTOCOL_H
+#ifndef LLDB_TOOLS_LLDB_DAP_PROTOCOL_PROTOCOL_BASE_H
+#define LLDB_TOOLS_LLDB_DAP_PROTOCOL_PROTOCOL_BASE_H
#include "llvm/Support/JSON.h"
#include <cstdint>
diff --git a/lldb/tools/lldb-dap/Protocol/ProtocolEvents.cpp b/lldb/tools/lldb-dap/Protocol/ProtocolEvents.cpp
new file mode 100644
index 0000000000000..ec4fef76f9d27
--- /dev/null
+++ b/lldb/tools/lldb-dap/Protocol/ProtocolEvents.cpp
@@ -0,0 +1,21 @@
+//===-- ProtocolEvents.cpp
+//--------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "Protocol/ProtocolEvents.h"
+#include "llvm/Support/JSON.h"
+
+using namespace llvm;
+
+namespace lldb_dap::protocol {
+
+json::Value toJSON(const CapabilitiesEventBody &CEB) {
+ return json::Object{{"capabilities", CEB.capabilities}};
+}
+
+} // namespace lldb_dap::protocol
diff --git a/lldb/tools/lldb-dap/Protocol/ProtocolEvents.h b/lldb/tools/lldb-dap/Protocol/ProtocolEvents.h
new file mode 100644
index 0000000000000..512106222362c
--- /dev/null
+++ b/lldb/tools/lldb-dap/Protocol/ProtocolEvents.h
@@ -0,0 +1,46 @@
+//===-- ProtocolEvents.h --------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// This file contains POD structs based on the DAP specification at
+// https://microsoft.github.io/debug-adapter-protocol/specification
+//
+// This is not meant to be a complete implementation, new interfaces are added
+// when they're needed.
+//
+// Each struct has a toJSON and fromJSON function, that converts between
+// the struct and a JSON representation. (See JSON.h)
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLDB_TOOLS_LLDB_DAP_PROTOCOL_PROTOCOL_EVENTS_H
+#define LLDB_TOOLS_LLDB_DAP_PROTOCOL_PROTOCOL_EVENTS_H
+
+#include "Protocol/ProtocolTypes.h"
+#include "llvm/Support/JSON.h"
+
+namespace lldb_dap::protocol {
+
+/// The event indicates that one or more capabilities have changed.
+///
+/// Since the capabilities are dependent on the client and its UI, it might not
+/// be possible to change that at random times (or too late).
+///
+/// Consequently this event has a hint characteristic: a client can only be
+/// expected to make a 'best effort' in honoring individual capabilities but
+/// there are no guarantees.
+///
+/// Only changed capabilities need to be included, all other capabilities keep
+/// their values.
+struct CapabilitiesEventBody {
+ Capabilities capabilities;
+};
+llvm::json::Value toJSON(const CapabilitiesEventBody &);
+
+} // end namespace lldb_dap::protocol
+
+#endif
|
da-viper
reviewed
Jun 4, 2025
lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py
Outdated
Show resolved
Hide resolved
da-viper
approved these changes
Jun 4, 2025
lldb/test/API/tools/lldb-dap/stepInTargets/TestDAP_stepInTargets.py
Outdated
Show resolved
Hide resolved
da-viper
reviewed
Jun 4, 2025
lldb/test/API/tools/lldb-dap/stepInTargets/TestDAP_stepInTargets.py
Outdated
Show resolved
Hide resolved
This was referenced Jun 5, 2025
|
I see some of this has been reverted. Should I undo the revert and add that to my other PR or should I rebase my changes on HEAD and we can take another stab at updating the stepInTargets request? |
|
I think the latter is better. That way we don't have to revert both of them |
|
Working on an update rebased on HEAD. |
This adds a new 'CapabilitiesEventBody' type for having a well structured type for the event and updates the 'stepInTargets' and 'restart' request to dynamically set their capabilities. This also fixes the 'stepInTargets' test on non-x86 platforms.
ashgti
force-pushed
the
dynamic-capabilities
branch
from
3c85cd3 to
c099f04
Compare
|
Done, rebased on main. |
|
I also added some unit tests for the new 'CapabilitiesEventBody' serialization logic. But I have a style question. The existing // style 1, no initializers:
struct Capabilities {
llvm::DenseSet<AdapterFeature> supportedFeatures;
std::optional<std::vector<ExceptionBreakpointsFilter>> exceptionBreakpointFilters;
std::optional<std::vector<std::string>> completionTriggerCharacters;
....
};
// all fields must be set or we get a warning on `-Wmissing-field-initializers`
Capabilities cap{
/*supportedFeatures=*/{...},
/*exceptionBreakpointFilters=*/std::nullopt,
/*completionTriggerCharacters=*/std::nullopt
...
};
// style 2, = initial value:
struct Capabilities {
llvm::DenseSet<AdapterFeature> supportedFeatures = {};
std::optional<std::vector<ExceptionBreakpointsFilter>> exceptionBreakpointFilters = std::nullopt;
std::optional<std::vector<std::string>> completionTriggerCharacters = std::nullopt;
....
};
Capabilities cap{/*supportedFeatures=*/{...}}; // no need to specify the nullopts.
// style 3, brace initializers:
struct Capabilities {
llvm::DenseSet<AdapterFeature> supportedFeatures{};
std::optional<std::vector<ExceptionBreakpointsFilter>> exceptionBreakpointFilters{};
std::optional<std::vector<std::string>> completionTriggerCharacters{};
....
};
Capabilities cap{/*supportedFeatures=*/{...}}; // no need to specify the nullopts.
// style 4, no direct initialization:
Capabilities cap;
cap.supportedFeatures = {...}; |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 adds a new 'CapabilitiesEventBody' type for having a well structured type for the event and updates the 'stepInTargets' and 'restart' request to dynamically set their capabilities.
This also fixes the 'stepInTargets' test on non-x86 platforms.