[lldb][lldb-dap] Migrate ScopesRequest to structured types #138116
Merged
+319
−129
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: Ebuka Ezike (da-viper) ChangesPatch is 27.92 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/138116.diff 14 Files Affected:
diff --git a/lldb/tools/lldb-dap/DAP.cpp b/lldb/tools/lldb-dap/DAP.cpp
index 4cb0d8e49004c..cad120ddd0621 100644
--- a/lldb/tools/lldb-dap/DAP.cpp
+++ b/lldb/tools/lldb-dap/DAP.cpp
@@ -514,9 +514,7 @@ lldb::SBThread DAP::GetLLDBThread(const llvm::json::Object &arguments) {
return target.GetProcess().GetThreadByID(tid);
}
-lldb::SBFrame DAP::GetLLDBFrame(const llvm::json::Object &arguments) {
- const uint64_t frame_id =
- GetInteger<uint64_t>(arguments, "frameId").value_or(UINT64_MAX);
+lldb::SBFrame DAP::GetLLDBFrame(uint64_t frame_id) {
lldb::SBProcess process = target.GetProcess();
// Upper 32 bits is the thread index ID
lldb::SBThread thread =
@@ -525,17 +523,6 @@ lldb::SBFrame DAP::GetLLDBFrame(const llvm::json::Object &arguments) {
return thread.GetFrameAtIndex(GetLLDBFrameID(frame_id));
}
-llvm::json::Value DAP::CreateTopLevelScopes() {
- llvm::json::Array scopes;
- scopes.emplace_back(
- CreateScope("Locals", VARREF_LOCALS, variables.locals.GetSize(), false));
- scopes.emplace_back(CreateScope("Globals", VARREF_GLOBALS,
- variables.globals.GetSize(), false));
- scopes.emplace_back(CreateScope("Registers", VARREF_REGS,
- variables.registers.GetSize(), false));
- return llvm::json::Value(std::move(scopes));
-}
-
ReplMode DAP::DetectReplMode(lldb::SBFrame frame, std::string &expression,
bool partial_expression) {
// Check for the escape hatch prefix.
diff --git a/lldb/tools/lldb-dap/DAP.h b/lldb/tools/lldb-dap/DAP.h
index 88eedb0860cf1..d0b4e3987b88c 100644
--- a/lldb/tools/lldb-dap/DAP.h
+++ b/lldb/tools/lldb-dap/DAP.h
@@ -275,9 +275,7 @@ struct DAP {
lldb::SBThread GetLLDBThread(lldb::tid_t id);
lldb::SBThread GetLLDBThread(const llvm::json::Object &arguments);
- lldb::SBFrame GetLLDBFrame(const llvm::json::Object &arguments);
-
- llvm::json::Value CreateTopLevelScopes();
+ lldb::SBFrame GetLLDBFrame(uint64_t frame_id);
void PopulateExceptionBreakpoints();
diff --git a/lldb/tools/lldb-dap/Handler/CompletionsHandler.cpp b/lldb/tools/lldb-dap/Handler/CompletionsHandler.cpp
index c72fc5686cd5b..65686cd23b243 100644
--- a/lldb/tools/lldb-dap/Handler/CompletionsHandler.cpp
+++ b/lldb/tools/lldb-dap/Handler/CompletionsHandler.cpp
@@ -136,7 +136,9 @@ void CompletionsRequestHandler::operator()(
const auto *arguments = request.getObject("arguments");
// If we have a frame, try to set the context for variable completions.
- lldb::SBFrame frame = dap.GetLLDBFrame(*arguments);
+ const uint64_t frame_id =
+ GetInteger<uint64_t>(*arguments, "frameId").value_or(UINT64_MAX);
+ lldb::SBFrame frame = dap.GetLLDBFrame(frame_id);
if (frame.IsValid()) {
frame.GetThread().GetProcess().SetSelectedThread(frame.GetThread());
frame.GetThread().SetSelectedFrame(frame.GetFrameID());
diff --git a/lldb/tools/lldb-dap/Handler/DataBreakpointInfoRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/DataBreakpointInfoRequestHandler.cpp
index 4d920f8556254..76407d230438d 100644
--- a/lldb/tools/lldb-dap/Handler/DataBreakpointInfoRequestHandler.cpp
+++ b/lldb/tools/lldb-dap/Handler/DataBreakpointInfoRequestHandler.cpp
@@ -118,7 +118,10 @@ void DataBreakpointInfoRequestHandler::operator()(
const auto variablesReference =
GetInteger<uint64_t>(arguments, "variablesReference").value_or(0);
llvm::StringRef name = GetString(arguments, "name").value_or("");
- lldb::SBFrame frame = dap.GetLLDBFrame(*arguments);
+
+ const uint64_t frame_id =
+ GetInteger<uint64_t>(arguments, "frameId").value_or(UINT64_MAX);
+ lldb::SBFrame frame = dap.GetLLDBFrame(frame_id);
lldb::SBValue variable = dap.variables.FindVariable(variablesReference, name);
std::string addr, size;
diff --git a/lldb/tools/lldb-dap/Handler/EvaluateRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/EvaluateRequestHandler.cpp
index 5ce133c33b7e1..01b5f956ba953 100644
--- a/lldb/tools/lldb-dap/Handler/EvaluateRequestHandler.cpp
+++ b/lldb/tools/lldb-dap/Handler/EvaluateRequestHandler.cpp
@@ -144,7 +144,10 @@ void EvaluateRequestHandler::operator()(
FillResponse(request, response);
llvm::json::Object body;
const auto *arguments = request.getObject("arguments");
- lldb::SBFrame frame = dap.GetLLDBFrame(*arguments);
+
+ const uint64_t frame_id =
+ GetInteger<uint64_t>(arguments, "frameId").value_or(UINT64_MAX);
+ lldb::SBFrame frame = dap.GetLLDBFrame(frame_id);
std::string expression =
GetString(arguments, "expression").value_or("").str();
const llvm::StringRef context = GetString(arguments, "context").value_or("");
diff --git a/lldb/tools/lldb-dap/Handler/RequestHandler.h b/lldb/tools/lldb-dap/Handler/RequestHandler.h
index fa3d76ed4a125..00eeb66f10670 100644
--- a/lldb/tools/lldb-dap/Handler/RequestHandler.h
+++ b/lldb/tools/lldb-dap/Handler/RequestHandler.h
@@ -423,11 +423,15 @@ class PauseRequestHandler : public LegacyRequestHandler {
void operator()(const llvm::json::Object &request) const override;
};
-class ScopesRequestHandler : public LegacyRequestHandler {
+class ScopesRequestHandler final
+ : public RequestHandler<protocol::ScopesArguments,
+ llvm::Expected<protocol::ScopesResponseBody>> {
public:
- using LegacyRequestHandler::LegacyRequestHandler;
+ using RequestHandler::RequestHandler;
static llvm::StringLiteral GetCommand() { return "scopes"; }
- void operator()(const llvm::json::Object &request) const override;
+
+ llvm::Expected<protocol::ScopesResponseBody>
+ Run(const protocol::ScopesArguments &args) const override;
};
class SetVariableRequestHandler : public LegacyRequestHandler {
diff --git a/lldb/tools/lldb-dap/Handler/ScopesRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/ScopesRequestHandler.cpp
index 7d1608f59f9a4..d9dd29f7269f2 100644
--- a/lldb/tools/lldb-dap/Handler/ScopesRequestHandler.cpp
+++ b/lldb/tools/lldb-dap/Handler/ScopesRequestHandler.cpp
@@ -7,69 +7,55 @@
//===----------------------------------------------------------------------===//
#include "DAP.h"
-#include "EventHelper.h"
-#include "JSONUtils.h"
#include "RequestHandler.h"
+using namespace lldb_dap::protocol;
namespace lldb_dap {
-// "ScopesRequest": {
-// "allOf": [ { "$ref": "#/definitions/Request" }, {
-// "type": "object",
-// "description": "Scopes request; value of command field is 'scopes'. The
-// request returns the variable scopes for a given stackframe ID.",
-// "properties": {
-// "command": {
-// "type": "string",
-// "enum": [ "scopes" ]
-// },
-// "arguments": {
-// "$ref": "#/definitions/ScopesArguments"
-// }
-// },
-// "required": [ "command", "arguments" ]
-// }]
-// },
-// "ScopesArguments": {
-// "type": "object",
-// "description": "Arguments for 'scopes' request.",
-// "properties": {
-// "frameId": {
-// "type": "integer",
-// "description": "Retrieve the scopes for this stackframe."
-// }
-// },
-// "required": [ "frameId" ]
-// },
-// "ScopesResponse": {
-// "allOf": [ { "$ref": "#/definitions/Response" }, {
-// "type": "object",
-// "description": "Response to 'scopes' request.",
-// "properties": {
-// "body": {
-// "type": "object",
-// "properties": {
-// "scopes": {
-// "type": "array",
-// "items": {
-// "$ref": "#/definitions/Scope"
-// },
-// "description": "The scopes of the stackframe. If the array has
-// length zero, there are no scopes available."
-// }
-// },
-// "required": [ "scopes" ]
-// }
-// },
-// "required": [ "body" ]
-// }]
-// }
-void ScopesRequestHandler::operator()(const llvm::json::Object &request) const {
- llvm::json::Object response;
- FillResponse(request, response);
- llvm::json::Object body;
- const auto *arguments = request.getObject("arguments");
- lldb::SBFrame frame = dap.GetLLDBFrame(*arguments);
+/// Creates a `protocol::Scope` struct.
+///
+///
+/// \param[in] name
+/// The value to place into the "name" key
+///
+/// \param[in] variablesReference
+/// The value to place into the "variablesReference" key
+///
+/// \param[in] namedVariables
+/// The value to place into the "namedVariables" key
+///
+/// \param[in] expensive
+/// The value to place into the "expensive" key
+///
+/// \return
+/// A `protocol::Scope`
+static Scope CreateScope(const llvm::StringRef name, int64_t variablesReference,
+ int64_t namedVariables, bool expensive) {
+ Scope scope;
+ scope.name = name;
+
+ // TODO: Support "arguments" and "return value" scope.
+ // At the moment lldb-dap includes the arguments and return_value into the
+ // "locals" scope. add presentation hint;
+ // vscode only expands the first non-expensive scope, this causes friction
+ // as the locals scope will not be expanded. It becomes more annoying when
+ // the scope has arguments, return_value and locals.
+ if (variablesReference == VARREF_LOCALS)
+ scope.presentationHint = Scope::ePresentationHintLocals;
+ else if (variablesReference == VARREF_REGS)
+ scope.presentationHint = Scope::ePresentationHintRegisters;
+
+ scope.variablesReference = variablesReference;
+ scope.namedVariables = namedVariables;
+ scope.expensive = expensive;
+
+ return scope;
+}
+
+llvm::Expected<ScopesResponseBody>
+ScopesRequestHandler::Run(const ScopesArguments &args) const {
+ lldb::SBFrame frame = dap.GetLLDBFrame(args.frameId);
+
// As the user selects different stack frames in the GUI, a "scopes" request
// will be sent to the DAP. This is the only way we know that the user has
// selected a frame in a thread. There are no other notifications that are
@@ -78,9 +64,9 @@ void ScopesRequestHandler::operator()(const llvm::json::Object &request) const {
// are sent, this allows users to type commands in the debugger console
// with a backtick character to run lldb commands and these lldb commands
// will now have the right context selected as they are run. If the user
- // types "`bt" into the debugger console and we had another thread selected
+ // types "`bt" into the debugger console, and we had another thread selected
// in the LLDB library, we would show the wrong thing to the user. If the
- // users switches threads with a lldb command like "`thread select 14", the
+ // users switch threads with a lldb command like "`thread select 14", the
// GUI will not update as there are no "event" notification packets that
// allow us to change the currently selected thread or frame in the GUI that
// I am aware of.
@@ -88,7 +74,6 @@ void ScopesRequestHandler::operator()(const llvm::json::Object &request) const {
frame.GetThread().GetProcess().SetSelectedThread(frame.GetThread());
frame.GetThread().SetSelectedFrame(frame.GetFrameID());
}
-
dap.variables.locals = frame.GetVariables(/*arguments=*/true,
/*locals=*/true,
/*statics=*/false,
@@ -98,9 +83,15 @@ void ScopesRequestHandler::operator()(const llvm::json::Object &request) const {
/*statics=*/true,
/*in_scope_only=*/true);
dap.variables.registers = frame.GetRegisters();
- body.try_emplace("scopes", dap.CreateTopLevelScopes());
- response.try_emplace("body", std::move(body));
- dap.SendJSON(llvm::json::Value(std::move(response)));
+
+ std::vector scopes = {CreateScope("Locals", VARREF_LOCALS,
+ dap.variables.locals.GetSize(), false),
+ CreateScope("Globals", VARREF_GLOBALS,
+ dap.variables.globals.GetSize(), false),
+ CreateScope("Registers", VARREF_REGS,
+ dap.variables.registers.GetSize(), false)};
+
+ return ScopesResponseBody{std::move(scopes)};
}
} // namespace lldb_dap
diff --git a/lldb/tools/lldb-dap/Handler/StepInTargetsRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/StepInTargetsRequestHandler.cpp
index 9b99791599f82..e7c568e9e9e60 100644
--- a/lldb/tools/lldb-dap/Handler/StepInTargetsRequestHandler.cpp
+++ b/lldb/tools/lldb-dap/Handler/StepInTargetsRequestHandler.cpp
@@ -74,7 +74,10 @@ void StepInTargetsRequestHandler::operator()(
const auto *arguments = request.getObject("arguments");
dap.step_in_targets.clear();
- lldb::SBFrame frame = dap.GetLLDBFrame(*arguments);
+
+ const uint64_t frame_id =
+ GetInteger<uint64_t>(arguments, "frameId").value_or(UINT64_MAX);
+ lldb::SBFrame frame = dap.GetLLDBFrame(frame_id);
if (frame.IsValid()) {
lldb::SBAddress pc_addr = frame.GetPCAddress();
lldb::SBAddress line_end_addr =
diff --git a/lldb/tools/lldb-dap/JSONUtils.cpp b/lldb/tools/lldb-dap/JSONUtils.cpp
index 4409cf5b27e5b..5e6f1f3942dc5 100644
--- a/lldb/tools/lldb-dap/JSONUtils.cpp
+++ b/lldb/tools/lldb-dap/JSONUtils.cpp
@@ -269,91 +269,6 @@ void FillResponse(const llvm::json::Object &request,
response.try_emplace("success", true);
}
-// "Scope": {
-// "type": "object",
-// "description": "A Scope is a named container for variables. Optionally
-// a scope can map to a source or a range within a source.",
-// "properties": {
-// "name": {
-// "type": "string",
-// "description": "Name of the scope such as 'Arguments', 'Locals'."
-// },
-// "presentationHint": {
-// "type": "string",
-// "description": "An optional hint for how to present this scope in the
-// UI. If this attribute is missing, the scope is shown
-// with a generic UI.",
-// "_enum": [ "arguments", "locals", "registers" ],
-// },
-// "variablesReference": {
-// "type": "integer",
-// "description": "The variables of this scope can be retrieved by
-// passing the value of variablesReference to the
-// VariablesRequest."
-// },
-// "namedVariables": {
-// "type": "integer",
-// "description": "The number of named variables in this scope. The
-// client can use this optional information to present
-// the variables in a paged UI and fetch them in chunks."
-// },
-// "indexedVariables": {
-// "type": "integer",
-// "description": "The number of indexed variables in this scope. The
-// client can use this optional information to present
-// the variables in a paged UI and fetch them in chunks."
-// },
-// "expensive": {
-// "type": "boolean",
-// "description": "If true, the number of variables in this scope is
-// large or expensive to retrieve."
-// },
-// "source": {
-// "$ref": "#/definitions/Source",
-// "description": "Optional source for this scope."
-// },
-// "line": {
-// "type": "integer",
-// "description": "Optional start line of the range covered by this
-// scope."
-// },
-// "column": {
-// "type": "integer",
-// "description": "Optional start column of the range covered by this
-// scope."
-// },
-// "endLine": {
-// "type": "integer",
-// "description": "Optional end line of the range covered by this scope."
-// },
-// "endColumn": {
-// "type": "integer",
-// "description": "Optional end column of the range covered by this
-// scope."
-// }
-// },
-// "required": [ "name", "variablesReference", "expensive" ]
-// }
-llvm::json::Value CreateScope(const llvm::StringRef name,
- int64_t variablesReference,
- int64_t namedVariables, bool expensive) {
- llvm::json::Object object;
- EmplaceSafeString(object, "name", name.str());
-
- // TODO: Support "arguments" scope. At the moment lldb-dap includes the
- // arguments into the "locals" scope.
- if (variablesReference == VARREF_LOCALS) {
- object.try_emplace("presentationHint", "locals");
- } else if (variablesReference == VARREF_REGS) {
- object.try_emplace("presentationHint", "registers");
- }
-
- object.try_emplace("variablesReference", variablesReference);
- object.try_emplace("expensive", expensive);
- object.try_emplace("namedVariables", namedVariables);
- return llvm::json::Value(std::move(object));
-}
-
// "Breakpoint": {
// "type": "object",
// "description": "Information about a Breakpoint created in setBreakpoints
diff --git a/lldb/tools/lldb-dap/JSONUtils.h b/lldb/tools/lldb-dap/JSONUtils.h
index d0e20729f4ed9..1d9f72c3763c9 100644
--- a/lldb/tools/lldb-dap/JSONUtils.h
+++ b/lldb/tools/lldb-dap/JSONUtils.h
@@ -294,27 +294,6 @@ llvm::json::Object CreateEventObject(const llvm::StringRef event_name);
protocol::ExceptionBreakpointsFilter
CreateExceptionBreakpointFilter(const ExceptionBreakpoint &bp);
-/// Create a "Scope" JSON object as described in the debug adapter definition.
-///
-/// \param[in] name
-/// The value to place into the "name" key
-//
-/// \param[in] variablesReference
-/// The value to place into the "variablesReference" key
-//
-/// \param[in] namedVariables
-/// The value to place into the "namedVariables" key
-//
-/// \param[in] expensive
-/// The value to place into the "expensive" key
-///
-/// \return
-/// A "Scope" JSON object with that follows the formal JSON
-/// definition outlined by Microsoft.
-llvm::json::Value CreateScope(const llvm::StringRef name,
- int64_t variablesReference,
- int64_t namedVariables, bool expensive);
-
/// Create a "Source" JSON object as described in the debug adapter definition.
///
/// \param[in] file
diff --git a/lldb/tools/lldb-dap/Protocol/ProtocolRequests.cpp b/lldb/tools/lldb-dap/Protocol/ProtocolRequests.cpp
index 61fea66490c30..aa8f193140d7a 100644
--- a/lldb/tools/lldb-dap/Protocol/ProtocolRequests.cpp
+++ b/lldb/tools/lldb-dap/Protocol/ProtocolRequests.cpp
@@ -260,6 +260,20 @@ bool fromJSON(const json::Value &Params, LaunchRequestArguments &LRA,
O.mapOptional("runInTerminal", LRA.runInTerminal) &&
parseEnv(Params, LRA.env, P) && parseTimeout(Params, LRA.timeout, P);
}
+bool fromJSON(const llvm::json::Value &Params, ScopesArguments &SCA,
+ llvm::json::Path P) {
+ json::ObjectMapper O(Params, P);
+ return O && O.map("frameId", SCA.frameId);
+}
+
+llvm::json::Value toJSON(const ScopesResponseBody &SCR) {
+ llvm::json::Array scopes;
+ for (const Scope &scope : SCR.scopes) {
+ scopes.emplace_back(toJSON(scope));
+ }
+
+ return llvm::json::Object{{"scopes", std::move(scopes)}};
+}
bool fromJSON(const json::Value &Params, SourceArguments &SA, json::Path P) {
json::ObjectMapper O(Params, P);
diff --git a/lldb/tools/lldb-dap/Protocol/ProtocolRequests.h b/lldb/tools/lldb-dap/Protocol/ProtocolRequests.h
index 33f93cc38799a..9ac5468608e1e 100644
--- a/lldb/tools/lldb-dap/Protocol/ProtocolRequests.h
+++ b/lldb/tools/lldb-dap/Protocol/ProtocolRequests.h
@@ -294,6 +294,19 @@ bool fromJSON(const llvm::json::Value &, LaunchRequestArguments &,
/// field is required.
using LaunchResponseBody = VoidResponse;
+struct ScopesArguments {
+ /// Retrieve the scopes for the stack frame identified by `frameId`. The
+ /// `frameId` must have been obtained in the current suspended state. See
+ /// 'Lifetime of Object References' in the Overview section for details.
+ uint64_t frameId;
+};
+bool fromJSON(const llvm::json::Value &, ScopesArguments &, llvm::json::Path);
+
+struct ScopesResponseBody {
+ std::vector<Scope> scopes;
+};
+llvm::json::Value toJSON(const ScopesResponseBody &);
+
/// Arguments for `source` request.
struct SourceArguments {
/// Specifies the source content to load. Either `source.path` or
diff --git a/lldb/tools/lldb-dap/Protocol/ProtocolTypes.cpp b/lldb/tools/lldb-dap/Protocol/ProtocolTypes.cpp
index e64998c4ca488..0f382a8dc66c3 100644
--- a/lldb/tools/lldb-dap/Protocol/ProtocolTypes.cpp
+++ b/lldb/tools/lldb-dap/Protocol/ProtocolTypes.cpp
@@ -16,17 +16,18 @@ using name...
[truncated]
|
ashgti
reviewed
May 8, 2025
| CreateScope("Registers", VARREF_REGS, | ||
| dap.variables.registers.GetSize(), false)}; | ||
|
|
||
| return ScopesResponseBody{std::move(scopes)}; |
There was a problem hiding this comment.
I almost wonder if it would make more sense for some of this to move into the Variables helper?
Maybe llvm::Expected<std::vector<protocol::Scope>> Variables::SetFrame(SBFrame frame); or a SetFrame and GetScopes helper separately.
I think the intention is for the Variables helper to be able to handle caching and managing variable lookups. We can make it a bit more aware of its own state by moving this over there.
How does that sound to you?
There was a problem hiding this comment.
I think it should be only in the scoped request since nothing outside is using the functionality.
would be worth it to move it to the variables if there was another request that depends on getting all the scopes.
d539169 to
1782831
Compare
ashgti
approved these changes
May 12, 2025
Comment on lines
278
to
296
| enum PresentationHint : unsigned { | ||
| ePresentationHintNormal, | ||
| ePresentationHintEmphasize, | ||
| ePresentationHintDeemphasize, | ||
| }; |
There was a problem hiding this comment.
style nit: I think in lldb enums are usually kept in the same namespace and we should make the type more specific like ScopePresentationHint and eScopePresentationHintNormal, but I'll defer @JDevlieghere this.
There was a problem hiding this comment.
I'm slightly leaning towards that too, because that would be most consistent with the other enums which are potentially shared across types.
JDevlieghere
approved these changes
May 12, 2025
8112ecd to
6fcf3c0
Compare
|
This means also adding serialisation or de-serialisation for types that do not use it except for test. |
b9f7369 to
5d488dc
Compare
5d488dc to
6519da9
Compare
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.
No description provided.