[lldb-dap] Migrate variables request protocol types. #147611
Open
+548
−442
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
This adds new protocol types for the 'variables' request. While implementing this, I removed the '$__lldb_extension' field we returned on the 'variables' request, since I think all the data can be retrieved from other DAP requests.
ashgti
force-pushed
the
lldb-dap-variables-request
branch
from
43ca23b to
f39b3a4
Compare
|
@llvm/pr-subscribers-lldb Author: John Harrison (ashgti) ChangesThis adds new protocol types for the 'variables' request. While implementing this, I removed the '$__lldb_extension' field we returned on the 'variables' request, since I think all the data can be retrieved from other DAP requests. Patch is 46.00 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/147611.diff 12 Files Affected:
diff --git a/lldb/test/API/tools/lldb-dap/optimized/TestDAP_optimized.py b/lldb/test/API/tools/lldb-dap/optimized/TestDAP_optimized.py
index 9cfa9b20f6051..3b769d2dd89ce 100644
--- a/lldb/test/API/tools/lldb-dap/optimized/TestDAP_optimized.py
+++ b/lldb/test/API/tools/lldb-dap/optimized/TestDAP_optimized.py
@@ -28,7 +28,7 @@ def test_stack_frame_name(self):
parent_frame = self.dap_server.get_stackFrame(frameIndex=1)
self.assertTrue(parent_frame["name"].endswith(" [opt]"))
- @skipIfAsan # On ASAN builds this test intermittently fails https://github.com/llvm/llvm-project/issues/111061
+ @skipIfAsan # On ASAN builds this test intermittently fails https://github.com/llvm/llvm-project/issues/111061
@skipIfWindows
def test_optimized_variable(self):
"""Test optimized variable value contains error."""
@@ -50,9 +50,8 @@ def test_optimized_variable(self):
value.startswith("<error:"),
f"expect error for value: '{value}'",
)
- error_msg = optimized_variable["$__lldb_extensions"]["error"]
self.assertTrue(
- ("could not evaluate DW_OP_entry_value: no parent function" in error_msg)
- or ("variable not available" in error_msg)
+ ("could not evaluate DW_OP_entry_value: no parent function" in value)
+ or ("variable not available" in value)
)
self.continue_to_exit()
diff --git a/lldb/test/API/tools/lldb-dap/variables/TestDAP_variables.py b/lldb/test/API/tools/lldb-dap/variables/TestDAP_variables.py
index 340be0b39010d..69d4eab0b6fcd 100644
--- a/lldb/test/API/tools/lldb-dap/variables/TestDAP_variables.py
+++ b/lldb/test/API/tools/lldb-dap/variables/TestDAP_variables.py
@@ -1,12 +1,10 @@
"""
-Test lldb-dap setBreakpoints request
+Test lldb-dap variables request
"""
import os
-import dap_server
import lldbdap_testcase
-from lldbsuite.test import lldbutil
from lldbsuite.test.decorators import *
from lldbsuite.test.lldbtest import *
@@ -168,15 +166,6 @@ def do_test_scopes_variables_setVariable_evaluate(
"type": "int",
"value": "1",
},
- "$__lldb_extensions": {
- "equals": {
- "value": "1",
- },
- "declaration": {
- "equals": {"line": 12, "column": 14},
- "contains": {"path": ["lldb-dap", "variables", "main.cpp"]},
- },
- },
},
"argv": {
"equals": {"type": "const char **"},
@@ -196,10 +185,6 @@ def do_test_scopes_variables_setVariable_evaluate(
},
"x": {"equals": {"type": "int"}},
}
- if enableAutoVariableSummaries:
- verify_locals["pt"]["$__lldb_extensions"] = {
- "equals": {"autoSummary": "{x:11, y:22}"}
- }
verify_globals = {
"s_local": {"equals": {"type": "float", "value": "2.25"}},
diff --git a/lldb/tools/lldb-dap/Handler/RequestHandler.h b/lldb/tools/lldb-dap/Handler/RequestHandler.h
index 3b910abe81992..2d141a2704112 100644
--- a/lldb/tools/lldb-dap/Handler/RequestHandler.h
+++ b/lldb/tools/lldb-dap/Handler/RequestHandler.h
@@ -540,11 +540,14 @@ class ThreadsRequestHandler
Run(const protocol::ThreadsArguments &) const override;
};
-class VariablesRequestHandler : public LegacyRequestHandler {
+class VariablesRequestHandler
+ : public RequestHandler<protocol::VariablesArguments,
+ llvm::Expected<protocol::VariablesResponseBody>> {
public:
- using LegacyRequestHandler::LegacyRequestHandler;
+ using RequestHandler::RequestHandler;
static llvm::StringLiteral GetCommand() { return "variables"; }
- void operator()(const llvm::json::Object &request) const override;
+ llvm::Expected<protocol::VariablesResponseBody>
+ Run(const protocol::VariablesArguments &) const override;
};
class LocationsRequestHandler : public LegacyRequestHandler {
diff --git a/lldb/tools/lldb-dap/Handler/VariablesRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/VariablesRequestHandler.cpp
index 19bcca2b22b9b..f80a69a5f5ee5 100644
--- a/lldb/tools/lldb-dap/Handler/VariablesRequestHandler.cpp
+++ b/lldb/tools/lldb-dap/Handler/VariablesRequestHandler.cpp
@@ -8,107 +8,37 @@
#include "DAP.h"
#include "EventHelper.h"
+#include "Handler/RequestHandler.h"
#include "JSONUtils.h"
-#include "RequestHandler.h"
+#include "ProtocolUtils.h"
+
+using namespace llvm;
+using namespace lldb_dap::protocol;
namespace lldb_dap {
-// "VariablesRequest": {
-// "allOf": [ { "$ref": "#/definitions/Request" }, {
-// "type": "object",
-// "description": "Variables request; value of command field is 'variables'.
-// Retrieves all child variables for the given variable reference. An
-// optional filter can be used to limit the fetched children to either named
-// or indexed children.", "properties": {
-// "command": {
-// "type": "string",
-// "enum": [ "variables" ]
-// },
-// "arguments": {
-// "$ref": "#/definitions/VariablesArguments"
-// }
-// },
-// "required": [ "command", "arguments" ]
-// }]
-// },
-// "VariablesArguments": {
-// "type": "object",
-// "description": "Arguments for 'variables' request.",
-// "properties": {
-// "variablesReference": {
-// "type": "integer",
-// "description": "The Variable reference."
-// },
-// "filter": {
-// "type": "string",
-// "enum": [ "indexed", "named" ],
-// "description": "Optional filter to limit the child variables to either
-// named or indexed. If ommited, both types are fetched."
-// },
-// "start": {
-// "type": "integer",
-// "description": "The index of the first variable to return; if omitted
-// children start at 0."
-// },
-// "count": {
-// "type": "integer",
-// "description": "The number of variables to return. If count is missing
-// or 0, all variables are returned."
-// },
-// "format": {
-// "$ref": "#/definitions/ValueFormat",
-// "description": "Specifies details on how to format the Variable
-// values."
-// }
-// },
-// "required": [ "variablesReference" ]
-// },
-// "VariablesResponse": {
-// "allOf": [ { "$ref": "#/definitions/Response" }, {
-// "type": "object",
-// "description": "Response to 'variables' request.",
-// "properties": {
-// "body": {
-// "type": "object",
-// "properties": {
-// "variables": {
-// "type": "array",
-// "items": {
-// "$ref": "#/definitions/Variable"
-// },
-// "description": "All (or a range) of variables for the given
-// variable reference."
-// }
-// },
-// "required": [ "variables" ]
-// }
-// },
-// "required": [ "body" ]
-// }]
-// }
-void VariablesRequestHandler::operator()(
- const llvm::json::Object &request) const {
- llvm::json::Object response;
- FillResponse(request, response);
- llvm::json::Array variables;
- const auto *arguments = request.getObject("arguments");
- const auto variablesReference =
- GetInteger<uint64_t>(arguments, "variablesReference").value_or(0);
- const auto start = GetInteger<int64_t>(arguments, "start").value_or(0);
- const auto count = GetInteger<int64_t>(arguments, "count").value_or(0);
+/// Retrieves all child variables for the given variable reference.
+///
+/// A filter can be used to limit the fetched children to either named or
+/// indexed children.
+Expected<VariablesResponseBody>
+VariablesRequestHandler::Run(const VariablesArguments &arguments) const {
+ uint64_t var_ref = arguments.variablesReference;
+ uint64_t count = arguments.count;
+ uint64_t start = arguments.start;
bool hex = false;
- const auto *format = arguments->getObject("format");
- if (format)
- hex = GetBoolean(format, "hex").value_or(false);
+ if (arguments.format)
+ hex = arguments.format->hex.value_or(false);
+
+ std::vector<Variable> variables;
- if (lldb::SBValueList *top_scope =
- dap.variables.GetTopLevelScope(variablesReference)) {
+ if (lldb::SBValueList *top_scope = dap.variables.GetTopLevelScope(var_ref)) {
// variablesReference is one of our scopes, not an actual variable it is
// asking for the list of args, locals or globals.
int64_t start_idx = 0;
int64_t num_children = 0;
- if (variablesReference == VARREF_REGS) {
+ if (var_ref == VARREF_REGS) {
// Change the default format of any pointer sized registers in the first
// register set to be the lldb::eFormatAddressInfo so we show the pointer
// and resolve what the pointer resolves to. Only change the format if the
@@ -128,7 +58,7 @@ void VariablesRequestHandler::operator()(
}
num_children = top_scope->GetSize();
- if (num_children == 0 && variablesReference == VARREF_LOCALS) {
+ if (num_children == 0 && var_ref == VARREF_LOCALS) {
// Check for an error in the SBValueList that might explain why we don't
// have locals. If we have an error display it as the sole value in the
// the locals.
@@ -145,12 +75,11 @@ void VariablesRequestHandler::operator()(
// errors are only set when there is a problem that the user could
// fix, so no error will show up when you have no debug info, only when
// we do have debug info and something that is fixable can be done.
- llvm::json::Object object;
- EmplaceSafeString(object, "name", "<error>");
- EmplaceSafeString(object, "type", "const char *");
- EmplaceSafeString(object, "value", var_err);
- object.try_emplace("variablesReference", (int64_t)0);
- variables.emplace_back(std::move(object));
+ Variable var;
+ var.name = "<error>";
+ var.type = "const char *";
+ var.value = var_err;
+ variables.emplace_back(var);
}
}
const int64_t end_idx = start_idx + ((count == 0) ? num_children : count);
@@ -165,7 +94,7 @@ void VariablesRequestHandler::operator()(
}
// Show return value if there is any ( in the local top frame )
- if (variablesReference == VARREF_LOCALS) {
+ if (var_ref == VARREF_LOCALS) {
auto process = dap.target.GetProcess();
auto selected_thread = process.GetSelectedThread();
lldb::SBValue stop_return_value = selected_thread.GetStopReturnValue();
@@ -204,14 +133,13 @@ void VariablesRequestHandler::operator()(
} else {
// We are expanding a variable that has children, so we will return its
// children.
- lldb::SBValue variable = dap.variables.GetVariable(variablesReference);
+ lldb::SBValue variable = dap.variables.GetVariable(var_ref);
if (variable.IsValid()) {
auto addChild = [&](lldb::SBValue child,
std::optional<std::string> custom_name = {}) {
if (!child.IsValid())
return;
- bool is_permanent =
- dap.variables.IsPermanentVariableReference(variablesReference);
+ bool is_permanent = dap.variables.IsPermanentVariableReference(var_ref);
int64_t var_ref = dap.variables.InsertVariable(child, is_permanent);
variables.emplace_back(CreateVariable(
child, var_ref, hex, dap.configuration.enableAutoVariableSummaries,
@@ -233,10 +161,8 @@ void VariablesRequestHandler::operator()(
addChild(variable.GetNonSyntheticValue(), "[raw]");
}
}
- llvm::json::Object body;
- body.try_emplace("variables", std::move(variables));
- response.try_emplace("body", std::move(body));
- dap.SendJSON(llvm::json::Value(std::move(response)));
+
+ return VariablesResponseBody{variables};
}
} // namespace lldb_dap
diff --git a/lldb/tools/lldb-dap/JSONUtils.cpp b/lldb/tools/lldb-dap/JSONUtils.cpp
index 553c52605c998..311e3fb9198ae 100644
--- a/lldb/tools/lldb-dap/JSONUtils.cpp
+++ b/lldb/tools/lldb-dap/JSONUtils.cpp
@@ -768,38 +768,6 @@ VariableDescription::VariableDescription(lldb::SBValue v,
evaluate_name = llvm::StringRef(evaluateStream.GetData()).str();
}
-llvm::json::Object VariableDescription::GetVariableExtensionsJSON() {
- llvm::json::Object extensions;
- if (error)
- EmplaceSafeString(extensions, "error", *error);
- if (!value.empty())
- EmplaceSafeString(extensions, "value", value);
- if (!summary.empty())
- EmplaceSafeString(extensions, "summary", summary);
- if (auto_summary)
- EmplaceSafeString(extensions, "autoSummary", *auto_summary);
-
- if (lldb::SBDeclaration decl = v.GetDeclaration(); decl.IsValid()) {
- llvm::json::Object decl_obj;
- if (lldb::SBFileSpec file = decl.GetFileSpec(); file.IsValid()) {
- char path[PATH_MAX] = "";
- if (file.GetPath(path, sizeof(path)) &&
- lldb::SBFileSpec::ResolvePath(path, path, PATH_MAX)) {
- decl_obj.try_emplace("path", std::string(path));
- }
- }
-
- if (int line = decl.GetLine())
- decl_obj.try_emplace("line", line);
- if (int column = decl.GetColumn())
- decl_obj.try_emplace("column", column);
-
- if (!decl_obj.empty())
- extensions.try_emplace("declaration", std::move(decl_obj));
- }
- return extensions;
-}
-
std::string VariableDescription::GetResult(llvm::StringRef context) {
// In repl context, the results can be displayed as multiple lines so more
// detailed descriptions can be returned.
@@ -836,226 +804,6 @@ std::pair<int64_t, bool> UnpackLocation(int64_t location_id) {
return std::pair{location_id >> 1, location_id & 1};
}
-// "Variable": {
-// "type": "object",
-// "description": "A Variable is a name/value pair. Optionally a variable
-// can have a 'type' that is shown if space permits or when
-// hovering over the variable's name. An optional 'kind' is
-// used to render additional properties of the variable,
-// e.g. different icons can be used to indicate that a
-// variable is public or private. If the value is
-// structured (has children), a handle is provided to
-// retrieve the children with the VariablesRequest. If
-// the number of named or indexed children is large, the
-// numbers should be returned via the optional
-// 'namedVariables' and 'indexedVariables' attributes. The
-// client can use this optional information to present the
-// children in a paged UI and fetch them in chunks.",
-// "properties": {
-// "name": {
-// "type": "string",
-// "description": "The variable's name."
-// },
-// "value": {
-// "type": "string",
-// "description": "The variable's value. This can be a multi-line text,
-// e.g. for a function the body of a function."
-// },
-// "type": {
-// "type": "string",
-// "description": "The type of the variable's value. Typically shown in
-// the UI when hovering over the value."
-// },
-// "presentationHint": {
-// "$ref": "#/definitions/VariablePresentationHint",
-// "description": "Properties of a variable that can be used to determine
-// how to render the variable in the UI."
-// },
-// "evaluateName": {
-// "type": "string",
-// "description": "Optional evaluatable name of this variable which can
-// be passed to the 'EvaluateRequest' to fetch the
-// variable's value."
-// },
-// "variablesReference": {
-// "type": "integer",
-// "description": "If variablesReference is > 0, the variable is
-// structured and its children can be retrieved by
-// passing variablesReference to the VariablesRequest."
-// },
-// "namedVariables": {
-// "type": "integer",
-// "description": "The number of named child variables. The client can
-// use this optional information to present the children
-// in a paged UI and fetch them in chunks."
-// },
-// "indexedVariables": {
-// "type": "integer",
-// "description": "The number of indexed child variables. The client
-// can use this optional information to present the
-// children in a paged UI and fetch them in chunks."
-// },
-// "memoryReference": {
-// "type": "string",
-// "description": "A memory reference associated with this variable.
-// For pointer type variables, this is generally a
-// reference to the memory address contained in the
-// pointer. For executable data, this reference may later
-// be used in a `disassemble` request. This attribute may
-// be returned by a debug adapter if corresponding
-// capability `supportsMemoryReferences` is true."
-// },
-// "declarationLocationReference": {
-// "type": "integer",
-// "description": "A reference that allows the client to request the
-// location where the variable is declared. This should be
-// present only if the adapter is likely to be able to
-// resolve the location.\n\nThis reference shares the same
-// lifetime as the `variablesReference`. See 'Lifetime of
-// Object References' in the Overview section for
-// details."
-// },
-// "valueLocationReference": {
-// "type": "integer",
-// "description": "A reference that allows the client to request the
-// location where the variable's value is declared. For
-// example, if the variable contains a function pointer,
-// the adapter may be able to look up the function's
-// location. This should be present only if the adapter
-// is likely to be able to resolve the location.\n\nThis
-// reference shares the same lifetime as the
-// `variablesReference`. See 'Lifetime of Object
-// References' in the Overview section for details."
-// },
-//
-// "$__lldb_extensions": {
-// "description": "Unofficial extensions to the protocol",
-// "properties": {
-// "declaration": {
-// "type": "object",
-// "description": "The source location where the variable was
-// declared. This value won't be present if no
-// declaration is available.
-// Superseded by `declarationLocationReference`",
-// "properties": {
-// "path": {
-// "type": "string",
-// "description": "The source file path where the variable was
-// declared."
-// },
-// "line": {
-// "type": "number",
-// "description": "The 1-indexed source line where the variable
-// was declared."
-// },
-// "column": {
-// "type": "number",
-// "description": "The 1-indexed source column where the variable
-// was declared."
-// }
-// }
-// },
-// "value": {
-// "type": "string",
-// "description": "The internal value of the variable as returned by
-// This is effectively SBValue.GetValue(). The other
-// `value` entry in the top-level variable response
-// is, on the other hand, just a display string for
-// the variable."
-// },
-// "summary": {
-// "type": "string",
-// "description": "The summary string of the variable. This is
-// effectively SBValue.GetSummary()."
-// ...
[truncated]
|
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 new protocol types for the 'variables' request.
While implementing this, I removed the '$__lldb_extension' field we returned on the 'variables' request, since I think all the data can be retrieved from other DAP requests.