From a20b2bcb6b68cbe08cd5c3c56e5b9555d3c9d792 Mon Sep 17 00:00:00 2001 From: John Harrison Date: Wed, 12 Nov 2025 09:07:12 -0800 Subject: [PATCH 1/3] [lldb-dap] Migrating 'evaluate' to structured types. Adding structured types for the evaluate request handler. This should be mostly a non-functional change. I did catch some spelling mistakes in our tests ('variable' vs 'variables'). --- .../test/tools/lldb-dap/dap_server.py | 3 +- .../lldb-dap/evaluate/TestDAP_evaluate.py | 6 +- .../Handler/EvaluateRequestHandler.cpp | 273 +++++------------- lldb/tools/lldb-dap/Handler/RequestHandler.h | 9 +- lldb/tools/lldb-dap/JSONUtils.cpp | 5 +- lldb/tools/lldb-dap/JSONUtils.h | 6 +- .../lldb-dap/Protocol/ProtocolRequests.cpp | 51 ++++ .../lldb-dap/Protocol/ProtocolRequests.h | 117 ++++++++ lldb/tools/lldb-dap/Protocol/ProtocolTypes.h | 3 +- lldb/unittests/DAP/ProtocolRequestsTest.cpp | 51 ++++ 10 files changed, 317 insertions(+), 207 deletions(-) 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 ac550962cfb85..a4ca090021f3f 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 @@ -978,9 +978,10 @@ def request_evaluate(self, expression, frameIndex=0, threadId=None, context=None return [] args_dict = { "expression": expression, - "context": context, "frameId": stackFrame["id"], } + if context: + args_dict["context"] = context command_dict = { "command": "evaluate", "type": "request", diff --git a/lldb/test/API/tools/lldb-dap/evaluate/TestDAP_evaluate.py b/lldb/test/API/tools/lldb-dap/evaluate/TestDAP_evaluate.py index 20a75f4076e42..d620cddbebcc1 100644 --- a/lldb/test/API/tools/lldb-dap/evaluate/TestDAP_evaluate.py +++ b/lldb/test/API/tools/lldb-dap/evaluate/TestDAP_evaluate.py @@ -1,5 +1,5 @@ """ -Test lldb-dap completions request +Test lldb-dap evaluate request """ import re @@ -245,4 +245,6 @@ def test_hover_evaluate_expressions(self): @skipIfWindows def test_variable_evaluate_expressions(self): # Tests expression evaluations that are triggered in the variable explorer - self.run_test_evaluate_expressions("variable", enableAutoVariableSummaries=True) + self.run_test_evaluate_expressions( + "variables", enableAutoVariableSummaries=True + ) diff --git a/lldb/tools/lldb-dap/Handler/EvaluateRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/EvaluateRequestHandler.cpp index e1556846dff19..57b2f621865f1 100644 --- a/lldb/tools/lldb-dap/Handler/EvaluateRequestHandler.cpp +++ b/lldb/tools/lldb-dap/Handler/EvaluateRequestHandler.cpp @@ -10,148 +10,36 @@ #include "EventHelper.h" #include "JSONUtils.h" #include "LLDBUtils.h" +#include "Protocol/ProtocolRequests.h" +#include "Protocol/ProtocolTypes.h" #include "RequestHandler.h" +#include "lldb/API/SBCommandInterpreter.h" +#include "lldb/API/SBCommandReturnObject.h" +#include "lldb/lldb-enumerations.h" +#include "llvm/ADT/StringRef.h" +#include "llvm/Support/Error.h" +#include +#include +#include + +using namespace llvm; +using namespace lldb_dap; +using namespace lldb_dap::protocol; namespace lldb_dap { -// "EvaluateRequest": { -// "allOf": [ { "$ref": "#/definitions/Request" }, { -// "type": "object", -// "description": "Evaluate request; value of command field is 'evaluate'. -// Evaluates the given expression in the context of the -// top most stack frame. The expression has access to any -// variables and arguments that are in scope.", -// "properties": { -// "command": { -// "type": "string", -// "enum": [ "evaluate" ] -// }, -// "arguments": { -// "$ref": "#/definitions/EvaluateArguments" -// } -// }, -// "required": [ "command", "arguments" ] -// }] -// }, -// "EvaluateArguments": { -// "type": "object", -// "description": "Arguments for 'evaluate' request.", -// "properties": { -// "expression": { -// "type": "string", -// "description": "The expression to evaluate." -// }, -// "frameId": { -// "type": "integer", -// "description": "Evaluate the expression in the scope of this stack -// frame. If not specified, the expression is evaluated -// in the global scope." -// }, -// "context": { -// "type": "string", -// "_enum": [ "watch", "repl", "hover" ], -// "enumDescriptions": [ -// "evaluate is run in a watch.", -// "evaluate is run from REPL console.", -// "evaluate is run from a data hover." -// ], -// "description": "The context in which the evaluate request is run." -// }, -// "format": { -// "$ref": "#/definitions/ValueFormat", -// "description": "Specifies details on how to format the Evaluate -// result." -// } -// }, -// "required": [ "expression" ] -// }, -// "EvaluateResponse": { -// "allOf": [ { "$ref": "#/definitions/Response" }, { -// "type": "object", -// "description": "Response to 'evaluate' request.", -// "properties": { -// "body": { -// "type": "object", -// "properties": { -// "result": { -// "type": "string", -// "description": "The result of the evaluate request." -// }, -// "type": { -// "type": "string", -// "description": "The optional type of the evaluate result." -// }, -// "presentationHint": { -// "$ref": "#/definitions/VariablePresentationHint", -// "description": "Properties of a evaluate result that can be -// used to determine how to render the result in -// the UI." -// }, -// "variablesReference": { -// "type": "number", -// "description": "If variablesReference is > 0, the evaluate -// result is structured and its children can be -// retrieved by passing variablesReference to the -// VariablesRequest." -// }, -// "namedVariables": { -// "type": "number", -// "description": "The number of named child variables. The -// client can use this optional information to -// present the variables in a paged UI and fetch -// them in chunks." -// }, -// "indexedVariables": { -// "type": "number", -// "description": "The number of indexed child variables. The -// client can use this optional information to -// present the variables in a paged UI and fetch -// them in chunks." -// }, -// "valueLocationReference": { -// "type": "integer", -// "description": "A reference that allows the client to request -// the location where the returned value is -// declared. For example, if a function pointer is -// returned, 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." -// } -// "memoryReference": { -// "type": "string", -// "description": "A memory reference to a location appropriate -// for this result. For pointer type eval -// results, this is generally a reference to the -// memory address contained in the pointer. This -// attribute may be returned by a debug adapter -// if corresponding capability -// `supportsMemoryReferences` is true." -// }, -// }, -// "required": [ "result", "variablesReference" ] -// } -// }, -// "required": [ "body" ] -// }] -// } -void EvaluateRequestHandler::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); - std::string expression = - GetString(arguments, "expression").value_or("").str(); - const llvm::StringRef context = GetString(arguments, "context").value_or(""); +/// Evaluates the given expression in the context of a stack frame. +/// +/// The expression has access to any variables and arguments that are in scope. +Expected +EvaluateRequestHandler::Run(const EvaluateArguments &arguments) const { + EvaluateResponseBody body; + lldb::SBFrame frame = dap.GetLLDBFrame(arguments.frameId); + std::string expression = arguments.expression; bool repeat_last_command = expression.empty() && dap.last_nonempty_var_expression.empty(); - if (context == "repl" && + if (arguments.context == protocol::eEvaluateContextRepl && (repeat_last_command || (!expression.empty() && dap.DetectReplMode(frame, expression, false) == ReplMode::Command))) { @@ -169,66 +57,61 @@ void EvaluateRequestHandler::operator()( dap.debugger, llvm::StringRef(), {expression}, required_command_failed, /*parse_command_directives=*/false, /*echo_commands=*/false); - EmplaceSafeString(body, "result", result); - body.try_emplace("variablesReference", (int64_t)0); - } else { - if (context == "repl") { - // If the expression is empty and the last expression was for a - // variable, set the expression to the previous expression (repeat the - // evaluation); otherwise save the current non-empty expression for the - // next (possibly empty) variable expression. - if (expression.empty()) - expression = dap.last_nonempty_var_expression; - else - dap.last_nonempty_var_expression = expression; - } - // Always try to get the answer from the local variables if possible. If - // this fails, then if the context is not "hover", actually evaluate an - // expression using the expression parser. - // - // "frame variable" is more reliable than the expression parser in - // many cases and it is faster. - lldb::SBValue value = frame.GetValueForVariablePath( - expression.data(), lldb::eDynamicDontRunTarget); - - // Freeze dry the value in case users expand it later in the debug console - if (value.GetError().Success() && context == "repl") - value = value.Persist(); - - if (value.GetError().Fail() && context != "hover") - value = frame.EvaluateExpression(expression.data()); - - if (value.GetError().Fail()) { - response["success"] = llvm::json::Value(false); - // This error object must live until we're done with the pointer returned - // by GetCString(). - lldb::SBError error = value.GetError(); - const char *error_cstr = error.GetCString(); - if (error_cstr && error_cstr[0]) - EmplaceSafeString(response, "message", error_cstr); - else - EmplaceSafeString(response, "message", "evaluate failed"); - } else { - VariableDescription desc(value, - dap.configuration.enableAutoVariableSummaries); - EmplaceSafeString(body, "result", desc.GetResult(context)); - EmplaceSafeString(body, "type", desc.display_type_name); - int64_t var_ref = 0; - if (value.MightHaveChildren() || ValuePointsToCode(value)) - var_ref = dap.variables.InsertVariable( - value, /*is_permanent=*/context == "repl"); - if (value.MightHaveChildren()) - body.try_emplace("variablesReference", var_ref); - else - body.try_emplace("variablesReference", (int64_t)0); - if (lldb::addr_t addr = value.GetLoadAddress(); - addr != LLDB_INVALID_ADDRESS) - body.try_emplace("memoryReference", EncodeMemoryReference(addr)); - if (ValuePointsToCode(value)) - body.try_emplace("valueLocationReference", var_ref); - } + body.result = result; + return body; + } + + if (arguments.context == eEvaluateContextRepl) { + // If the expression is empty and the last expression was for a + // variable, set the expression to the previous expression (repeat the + // evaluation); otherwise save the current non-empty expression for the + // next (possibly empty) variable expression. + if (expression.empty()) + expression = dap.last_nonempty_var_expression; + else + dap.last_nonempty_var_expression = expression; + } + + // Always try to get the answer from the local variables if possible. If + // this fails, then if the context is not "hover", actually evaluate an + // expression using the expression parser. + // + // "frame variable" is more reliable than the expression parser in + // many cases and it is faster. + lldb::SBValue value = frame.GetValueForVariablePath( + expression.data(), lldb::eDynamicDontRunTarget); + + // Freeze dry the value in case users expand it later in the debug console + if (value.GetError().Success() && arguments.context == eEvaluateContextRepl) + value = value.Persist(); + + if (value.GetError().Fail() && arguments.context != eEvaluateContextHover) + value = frame.EvaluateExpression(expression.data()); + + if (value.GetError().Fail()) { + // This error object must live until we're done with the pointer returned + // by GetCString(). + lldb::SBError error = value.GetError(); + const char *error_cstr = error.GetCString(); + return make_error( + error_cstr && error_cstr[0] ? error_cstr : "evaluate failed"); } - response.try_emplace("body", std::move(body)); - dap.SendJSON(llvm::json::Value(std::move(response))); + + VariableDescription desc(value, + dap.configuration.enableAutoVariableSummaries); + body.result = desc.GetResult(arguments.context); + body.type = desc.display_type_name; + if (value.MightHaveChildren() || ValuePointsToCode(value)) + body.variablesReference = dap.variables.InsertVariable( + value, /*is_permanent=*/arguments.context == eEvaluateContextRepl); + if (lldb::addr_t addr = value.GetLoadAddress(); addr != LLDB_INVALID_ADDRESS) + body.memoryReference = EncodeMemoryReference(addr); + + if (ValuePointsToCode(value) && + body.variablesReference != LLDB_DAP_INVALID_VARRERF) + body.valueLocationReference = body.variablesReference; + + return body; } + } // namespace lldb_dap diff --git a/lldb/tools/lldb-dap/Handler/RequestHandler.h b/lldb/tools/lldb-dap/Handler/RequestHandler.h index bc22133d92453..65a52075ebd79 100644 --- a/lldb/tools/lldb-dap/Handler/RequestHandler.h +++ b/lldb/tools/lldb-dap/Handler/RequestHandler.h @@ -292,11 +292,14 @@ class DisconnectRequestHandler Run(const std::optional &args) const override; }; -class EvaluateRequestHandler : public LegacyRequestHandler { +class EvaluateRequestHandler + : public RequestHandler> { public: - using LegacyRequestHandler::LegacyRequestHandler; + using RequestHandler::RequestHandler; static llvm::StringLiteral GetCommand() { return "evaluate"; } - void operator()(const llvm::json::Object &request) const override; + llvm::Expected + Run(const protocol::EvaluateArguments &) const override; FeatureSet GetSupportedFeatures() const override { return {protocol::eAdapterFeatureEvaluateForHovers}; } diff --git a/lldb/tools/lldb-dap/JSONUtils.cpp b/lldb/tools/lldb-dap/JSONUtils.cpp index 1a3a6701b194d..81eadae03bb48 100644 --- a/lldb/tools/lldb-dap/JSONUtils.cpp +++ b/lldb/tools/lldb-dap/JSONUtils.cpp @@ -11,6 +11,7 @@ #include "ExceptionBreakpoint.h" #include "LLDBUtils.h" #include "Protocol/ProtocolBase.h" +#include "Protocol/ProtocolRequests.h" #include "ProtocolUtils.h" #include "lldb/API/SBAddress.h" #include "lldb/API/SBCompileUnit.h" @@ -817,10 +818,10 @@ VariableDescription::VariableDescription(lldb::SBValue v, evaluate_name = llvm::StringRef(evaluateStream.GetData()).str(); } -std::string VariableDescription::GetResult(llvm::StringRef context) { +std::string VariableDescription::GetResult(protocol::EvaluateContext context) { // In repl context, the results can be displayed as multiple lines so more // detailed descriptions can be returned. - if (context != "repl") + if (context != protocol::eEvaluateContextRepl) return display_value; if (!v.IsValid()) diff --git a/lldb/tools/lldb-dap/JSONUtils.h b/lldb/tools/lldb-dap/JSONUtils.h index 0c865a33a6ce4..329dc8ab02f99 100644 --- a/lldb/tools/lldb-dap/JSONUtils.h +++ b/lldb/tools/lldb-dap/JSONUtils.h @@ -10,7 +10,7 @@ #define LLDB_TOOLS_LLDB_DAP_JSONUTILS_H #include "DAPForward.h" -#include "Protocol/ProtocolTypes.h" +#include "Protocol/ProtocolRequests.h" #include "lldb/API/SBCompileUnit.h" #include "lldb/API/SBFormat.h" #include "lldb/API/SBType.h" @@ -28,7 +28,7 @@ namespace lldb_dap { -/// Emplace a StringRef in a json::Object after enusring that the +/// Emplace a StringRef in a json::Object after ensuring that the /// string is valid UTF8. If not, first call llvm::json::fixUTF8 /// before emplacing. /// @@ -351,7 +351,7 @@ struct VariableDescription { std::optional custom_name = {}); /// Returns a description of the value appropriate for the specified context. - std::string GetResult(llvm::StringRef context); + std::string GetResult(protocol::EvaluateContext context); }; /// Does the given variable have an associated value location? diff --git a/lldb/tools/lldb-dap/Protocol/ProtocolRequests.cpp b/lldb/tools/lldb-dap/Protocol/ProtocolRequests.cpp index 44ae79f8b9f43..82a461dad4451 100644 --- a/lldb/tools/lldb-dap/Protocol/ProtocolRequests.cpp +++ b/lldb/tools/lldb-dap/Protocol/ProtocolRequests.cpp @@ -8,6 +8,7 @@ #include "Protocol/ProtocolRequests.h" #include "JSONUtils.h" +#include "Protocol/ProtocolTypes.h" #include "lldb/lldb-defines.h" #include "llvm/ADT/DenseMap.h" #include "llvm/ADT/StringMap.h" @@ -639,6 +640,56 @@ json::Value toJSON(const ExceptionInfoResponseBody &ERB) { result.insert({"description", ERB.description}); if (ERB.details.has_value()) result.insert({"details", *ERB.details}); + return result; +} + +static bool fromJSON(const llvm::json::Value &Params, EvaluateContext &C, + llvm::json::Path P) { + auto rawContext = Params.getAsString(); + if (!rawContext) { + P.report("expected a string"); + return false; + } + C = StringSwitch(*rawContext) + .Case("watch", EvaluateContext::eEvaluateContextWatch) + .Case("repl", EvaluateContext::eEvaluateContextRepl) + .Case("hover", EvaluateContext::eEvaluateContextHover) + .Case("clipboard", EvaluateContext::eEvaluateContextClipboard) + .Case("variables", EvaluateContext::eEvaluateContextVariables) + .Default(eEvaluateContextUnknown); + return true; +} + +bool fromJSON(const llvm::json::Value &Params, EvaluateArguments &Args, + llvm::json::Path P) { + json::ObjectMapper O(Params, P); + return O && O.map("expression", Args.expression) && + O.mapOptional("frameId", Args.frameId) && + O.mapOptional("line", Args.line) && + O.mapOptional("column", Args.column) && + O.mapOptional("source", Args.source) && + O.mapOptional("context", Args.context) && + O.mapOptional("format", Args.format); +} + +llvm::json::Value toJSON(const EvaluateResponseBody &Body) { + json::Object result{{"result", Body.result}}; + + if (!Body.type.empty()) + result.insert({"type", Body.type}); + if (Body.presentationHint) + result.insert({"presentationHint", Body.presentationHint}); + if (Body.variablesReference != 0 && + Body.variablesReference != LLDB_DAP_INVALID_VARRERF) + result.insert({"variablesReference", Body.variablesReference}); + if (Body.namedVariables) + result.insert({"namedVariables", Body.namedVariables}); + if (Body.indexedVariables) + result.insert({"indexedVariables", Body.indexedVariables}); + if (!Body.memoryReference.empty()) + result.insert({"memoryReference", Body.memoryReference}); + if (Body.valueLocationReference != LLDB_DAP_INVALID_VALUE_LOC) + result.insert({"valueLocationReference", Body.valueLocationReference}); return result; } diff --git a/lldb/tools/lldb-dap/Protocol/ProtocolRequests.h b/lldb/tools/lldb-dap/Protocol/ProtocolRequests.h index b894f2b4ed44d..36517c926dbda 100644 --- a/lldb/tools/lldb-dap/Protocol/ProtocolRequests.h +++ b/lldb/tools/lldb-dap/Protocol/ProtocolRequests.h @@ -1061,6 +1061,123 @@ struct ExceptionInfoResponseBody { }; llvm::json::Value toJSON(const ExceptionInfoResponseBody &); +/// The context in which the evaluate request is used. +enum EvaluateContext : unsigned { + /// An unspecified or unknown evaluate context. + eEvaluateContextUnknown = 0, + /// 'watch': evaluate is called from a watch view context. + eEvaluateContextWatch = 1, + /// 'repl': evaluate is called from a REPL context. + eEvaluateContextRepl = 2, + /// 'hover': evaluate is called to generate the debug hover contents. + /// This value should only be used if the corresponding capability + /// `supportsEvaluateForHovers` is true. + eEvaluateContextHover = 3, + /// 'clipboard': evaluate is called to generate clipboard contents. + /// This value should only be used if the corresponding capability + /// `supportsClipboardContext` is true. + eEvaluateContextClipboard = 4, + /// 'variables': evaluate is called from a variables view context. + eEvaluateContextVariables = 5, +}; + +/// Arguments for `evaluate` request. +struct EvaluateArguments { + /// The expression to evaluate. + std::string expression; + + /// Evaluate the expression in the scope of this stack frame. If not + /// specified, the expression is evaluated in the global scope. + uint64_t frameId = LLDB_DAP_INVALID_FRAME_ID; + + /// The contextual line where the expression should be evaluated. In the + /// 'hover' context, this should be set to the start of the expression being + /// hovered. + uint32_t line = LLDB_INVALID_LINE_NUMBER; + + /// The contextual column where the expression should be evaluated. This may + /// be provided if `line` is also provided. + /// + /// It is measured in UTF-16 code units and the client capability + /// `columnsStartAt1` determines whether it is 0- or 1-based. + uint32_t column = LLDB_INVALID_COLUMN_NUMBER; + + /// The contextual source in which the `line` is found. This must be provided + /// if `line` is provided. + std::optional source; + + /// The context in which the evaluate request is used. + /// Values: + /// 'watch': evaluate is called from a watch view context. + /// 'repl': evaluate is called from a REPL context. + /// 'hover': evaluate is called to generate the debug hover contents. + /// This value should only be used if the corresponding capability + /// `supportsEvaluateForHovers` is true. + /// 'clipboard': evaluate is called to generate clipboard contents. + /// This value should only be used if the corresponding capability + /// `supportsClipboardContext` is true. + /// 'variables': evaluate is called from a variables view context. + /// etc. + EvaluateContext context = eEvaluateContextUnknown; + + /// Specifies details on how to format the result. + /// The attribute is only honored by a debug adapter if the corresponding + /// capability `supportsValueFormattingOptions` is true. + std::optional format; +}; +bool fromJSON(const llvm::json::Value &, EvaluateArguments &, llvm::json::Path); + +/// Response to 'evaluate' request. +struct EvaluateResponseBody { + /// The result of the evaluate request. + std::string result; + + /// The type of the evaluate result. + /// This attribute should only be returned by a debug adapter if the + /// corresponding capability `supportsVariableType` is true. + std::string type; + + /// Properties of an evaluate result that can be used to determine how to + /// render the result in the UI. + std::optional presentationHint; + + /// If `variablesReference` is > 0, the evaluate result is structured and its + /// children can be retrieved by passing `variablesReference` to the + /// `variables` request as long as execution remains suspended. See 'Lifetime + /// of Object References' in the Overview section for details. + int64_t variablesReference = LLDB_DAP_INVALID_VARRERF; + + /// The number of named child variables. + /// The client can use this information to present the variables in a paged + /// UI and fetch them in chunks. + /// The value should be less than or equal to 2147483647 (2^31-1). + uint32_t namedVariables = 0; + + /// The number of indexed child variables. + /// The client can use this information to present the variables in a paged + /// UI and fetch them in chunks. + /// The value should be less than or equal to 2147483647 (2^31-1). + uint32_t indexedVariables = 0; + + /// A memory reference to a location appropriate for this result. + /// For pointer type eval results, this is generally a reference to the + /// memory address contained in the pointer. + /// This attribute may be returned by a debug adapter if corresponding + /// capability `supportsMemoryReferences` is true. + std::string memoryReference; + + /// A reference that allows the client to request the location where the + /// returned value is declared. For example, if a function pointer is + /// returned, 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. + /// + /// This reference shares the same lifetime as the `variablesReference`. See + /// 'Lifetime of Object References' in the Overview section for details. + uint32_t valueLocationReference = LLDB_DAP_INVALID_VALUE_LOC; +}; +llvm::json::Value toJSON(const EvaluateResponseBody &); + } // namespace lldb_dap::protocol #endif diff --git a/lldb/tools/lldb-dap/Protocol/ProtocolTypes.h b/lldb/tools/lldb-dap/Protocol/ProtocolTypes.h index 6d85c74377bd3..690a1d684d0e9 100644 --- a/lldb/tools/lldb-dap/Protocol/ProtocolTypes.h +++ b/lldb/tools/lldb-dap/Protocol/ProtocolTypes.h @@ -28,8 +28,9 @@ #include #include -#define LLDB_DAP_INVALID_VARRERF UINT64_MAX +#define LLDB_DAP_INVALID_VARRERF INT64_MAX #define LLDB_DAP_INVALID_SRC_REF 0 +#define LLDB_DAP_INVALID_VALUE_LOC 0 namespace lldb_dap::protocol { diff --git a/lldb/unittests/DAP/ProtocolRequestsTest.cpp b/lldb/unittests/DAP/ProtocolRequestsTest.cpp index 498195dc09325..ba9aef1e5fcc5 100644 --- a/lldb/unittests/DAP/ProtocolRequestsTest.cpp +++ b/lldb/unittests/DAP/ProtocolRequestsTest.cpp @@ -67,3 +67,54 @@ TEST(ProtocolRequestsTest, ExceptionInfoResponseBody) { ASSERT_THAT_EXPECTED(expected_opt, llvm::Succeeded()); EXPECT_EQ(PrettyPrint(*expected_opt), PrettyPrint(body)); } + +TEST(ProtocolRequestsTest, EvaluateArguments) { + llvm::Expected expected = parse(R"({ + "expression": "hello world", + "context": "repl" + })"); + ASSERT_THAT_EXPECTED(expected, llvm::Succeeded()); + EXPECT_EQ(expected->expression, "hello world"); + EXPECT_EQ(expected->context, eEvaluateContextRepl); + + // Check required keys; + EXPECT_THAT_EXPECTED(parse(R"({})"), + FailedWithMessage("missing value at (root).expression")); +} + +TEST(ProtocolRequestsTest, EvaluateResponseBody) { + EvaluateResponseBody body; + body.result = "hello world"; + body.variablesReference = 7; + + // Check required keys. + Expected expected = parse(R"({ + "result": "hello world", + "variablesReference": 7 + })"); + + ASSERT_THAT_EXPECTED(expected, llvm::Succeeded()); + EXPECT_EQ(PrettyPrint(*expected), PrettyPrint(body)); + + // Check optional keys. + body.result = "'abc'"; + body.type = "string"; + body.variablesReference = 42; + body.namedVariables = 1; + body.indexedVariables = 2; + body.memoryReference = "0x123"; + body.valueLocationReference = 22; + + Expected expected_opt = parse(R"({ + "result": "'abc'", + "type": "string", + "variablesReference": 42, + "namedVariables": 1, + "indexedVariables": 2, + "memoryReference": "0x123", + "valueLocationReference": 22 + })"); + + ASSERT_THAT_EXPECTED(expected_opt, llvm::Succeeded()); + EXPECT_EQ(PrettyPrint(*expected_opt), PrettyPrint(body)); +} From dd9232e2b9e7b68a7f2e9d342ba48b74ca0888e0 Mon Sep 17 00:00:00 2001 From: John Harrison Date: Wed, 12 Nov 2025 11:41:46 -0800 Subject: [PATCH 2/3] Expanding the unit tests for 'evaluate' to cover more than the 'result' field. --- .../lldb-dap/evaluate/TestDAP_evaluate.py | 188 ++++++++++++++---- .../Handler/EvaluateRequestHandler.cpp | 5 +- .../lldb-dap/Protocol/ProtocolRequests.cpp | 6 +- .../lldb-dap/Protocol/ProtocolRequests.h | 4 +- 4 files changed, 155 insertions(+), 48 deletions(-) diff --git a/lldb/test/API/tools/lldb-dap/evaluate/TestDAP_evaluate.py b/lldb/test/API/tools/lldb-dap/evaluate/TestDAP_evaluate.py index d620cddbebcc1..3c233a5b43ebb 100644 --- a/lldb/test/API/tools/lldb-dap/evaluate/TestDAP_evaluate.py +++ b/lldb/test/API/tools/lldb-dap/evaluate/TestDAP_evaluate.py @@ -7,16 +7,67 @@ import lldbdap_testcase from lldbsuite.test.decorators import skipIfWindows from lldbsuite.test.lldbtest import line_number +from typing import TypedDict, Optional + + +class EvaluateResponseBody(TypedDict, total=False): + result: str + variablesReference: int + type: Optional[str] + memoryReference: Optional[str] + valueLocationReference: Optional[int] class TestDAP_evaluate(lldbdap_testcase.DAPTestCaseBase): - def assertEvaluate(self, expression, regex): + def assertEvaluate( + self, + expression, + result: str, + want_type="", + want_varref=False, + want_memref=True, + want_locref=False, + ): + resp = self.dap_server.request_evaluate(expression, context=self.context) + self.assertTrue( + resp["success"], f"Failed to evaluate expression {expression!r}" + ) + body: EvaluateResponseBody = resp["body"] self.assertRegex( - self.dap_server.request_evaluate(expression, context=self.context)["body"][ - "result" - ], - regex, + body["result"], + result, + f"Unexpected 'result' for expression {expression!r} in response body {body}", ) + if want_varref: + self.assertNotEqual( + body["variablesReference"], + 0, + f"Unexpected 'variablesReference' for expression {expression!r} in response body {body}", + ) + else: + self.assertEqual( + body["variablesReference"], + 0, + f"Unexpected 'variablesReference' for expression {expression!r} in response body {body}", + ) + if want_type: + self.assertEqual( + body["type"], + want_type, + f"Unexpected 'type' for expression {expression!r} in response body {body}", + ) + if want_memref: + self.assertIn( + "memoryReference", + body, + f"Unexpected 'memoryReference' for expression {expression!r} in response body {body}", + ) + if want_locref: + self.assertIn( + "valueLocationReference", + body, + f"Unexpected 'valueLocationReference' for expression {expression!r} in response body {body}", + ) def assertEvaluateFailure(self, expression): self.assertNotIn( @@ -71,29 +122,39 @@ def run_test_evaluate_expressions( self.continue_to_breakpoint(breakpoint_1) # Expressions at breakpoint 1, which is in main - self.assertEvaluate("var1", "20") + self.assertEvaluate("var1", "20", want_type="int") # Empty expression should equate to the previous expression. if context == "repl": self.assertEvaluate("", "20") else: self.assertEvaluateFailure("") - self.assertEvaluate("var2", "21") + self.assertEvaluate("var2", "21", want_type="int") if context == "repl": - self.assertEvaluate("", "21") - self.assertEvaluate("", "21") - self.assertEvaluate("static_int", "42") - self.assertEvaluate("non_static_int", "43") - self.assertEvaluate("struct1.foo", "15") - self.assertEvaluate("struct2->foo", "16") + self.assertEvaluate("", "21", want_type="int") + self.assertEvaluate("", "21", want_type="int") + self.assertEvaluate("static_int", "42", want_type="int") + self.assertEvaluate("non_static_int", "43", want_type="int") + self.assertEvaluate("struct1.foo", "15", want_type="int") + self.assertEvaluate("struct2->foo", "16", want_type="int") if self.isResultExpandedDescription(): self.assertEvaluate( "struct1", r"\(my_struct\) (struct1|\$\d+) = \(foo = 15\)", + want_type="my_struct", + want_varref=True, + ) + self.assertEvaluate( + "struct2", + r"\(my_struct \*\) (struct2|\$\d+) = 0x.*", + want_type="my_struct *", + want_varref=True, ) - self.assertEvaluate("struct2", r"\(my_struct \*\) (struct2|\$\d+) = 0x.*") self.assertEvaluate( - "struct3", r"\(my_struct \*\) (struct3|\$\d+) = nullptr" + "struct3", + r"\(my_struct \*\) (struct3|\$\d+) = nullptr", + want_type="my_struct *", + want_varref=True, ) else: self.assertEvaluate( @@ -103,16 +164,22 @@ def run_test_evaluate_expressions( if enableAutoVariableSummaries else "my_struct @ 0x" ), + want_varref=True, ) self.assertEvaluate( - "struct2", "0x.* {foo:16}" if enableAutoVariableSummaries else "0x.*" + "struct2", + "0x.* {foo:16}" if enableAutoVariableSummaries else "0x.*", + want_varref=True, + want_type="my_struct *", + ) + self.assertEvaluate( + "struct3", "0x.*0", want_varref=True, want_type="my_struct *" ) - self.assertEvaluate("struct3", "0x.*0") if context == "repl": # In the repl context expressions may be interpreted as lldb # commands since no variables have the same name as the command. - self.assertEvaluate("list", r".*") + self.assertEvaluate("list", r".*", want_memref=False) else: self.assertEvaluateFailure("list") # local variable of a_function @@ -121,10 +188,26 @@ def run_test_evaluate_expressions( self.assertEvaluateFailure("foo") # member of my_struct if self.isExpressionParsedExpected(): - self.assertEvaluate("a_function", "0x.*a.out`a_function.*") - self.assertEvaluate("a_function(1)", "1") - self.assertEvaluate("var2 + struct1.foo", "36") - self.assertEvaluate("foo_func", "0x.*a.out`foo_func.*") + self.assertEvaluate( + "a_function", + "0x.*a.out`a_function.*", + want_type="int (*)(int)", + want_varref=True, + want_memref=False, + want_locref=True, + ) + self.assertEvaluate( + "a_function(1)", "1", want_memref=False, want_type="int" + ) + self.assertEvaluate("var2 + struct1.foo", "36", want_memref=False) + self.assertEvaluate( + "foo_func", + "0x.*a.out`foo_func.*", + want_type="int (*)()", + want_varref=True, + want_memref=False, + want_locref=True, + ) self.assertEvaluate("foo_var", "44") else: self.assertEvaluateFailure("a_function") @@ -145,6 +228,8 @@ def run_test_evaluate_expressions( self.assertEvaluate( "struct1", r"\(my_struct\) (struct1|\$\d+) = \(foo = 15\)", + want_type="my_struct", + want_varref=True, ) else: self.assertEvaluate( @@ -154,15 +239,26 @@ def run_test_evaluate_expressions( if enableAutoVariableSummaries else "my_struct @ 0x" ), + want_type="my_struct", + want_varref=True, ) self.assertEvaluate("struct1.foo", "15") self.assertEvaluate("struct2->foo", "16") if self.isExpressionParsedExpected(): - self.assertEvaluate("a_function", "0x.*a.out`a_function.*") - self.assertEvaluate("a_function(1)", "1") - self.assertEvaluate("var2 + struct1.foo", "17") - self.assertEvaluate("foo_func", "0x.*a.out`foo_func.*") + self.assertEvaluate( + "a_function", + "0x.*a.out`a_function.*", + want_type="int (*)(int)", + want_varref=True, + want_memref=False, + want_locref=True, + ) + self.assertEvaluate("a_function(1)", "1", want_memref=False) + self.assertEvaluate("var2 + struct1.foo", "17", want_memref=False) + self.assertEvaluate( + "foo_func", "0x.*a.out`foo_func.*", want_varref=True, want_memref=False + ) self.assertEvaluate("foo_var", "44") else: self.assertEvaluateFailure("a_function") @@ -185,10 +281,18 @@ def run_test_evaluate_expressions( self.assertEvaluateFailure("var2 + struct1.foo") if self.isExpressionParsedExpected(): - self.assertEvaluate("a_function", "0x.*a.out`a_function.*") - self.assertEvaluate("a_function(1)", "1") - self.assertEvaluate("list + 1", "43") - self.assertEvaluate("foo_func", "0x.*a.out`foo_func.*") + self.assertEvaluate( + "a_function", + "0x.*a.out`a_function.*", + want_varref=True, + want_memref=False, + want_locref=True, + ) + self.assertEvaluate("a_function(1)", "1", want_memref=False) + self.assertEvaluate("list + 1", "43", want_memref=False) + self.assertEvaluate( + "foo_func", "0x.*a.out`foo_func.*", want_varref=True, want_memref=False + ) self.assertEvaluate("foo_var", "44") else: self.assertEvaluateFailure("a_function") @@ -199,26 +303,28 @@ def run_test_evaluate_expressions( # Now we check that values are updated after stepping self.continue_to_breakpoint(breakpoint_4) - self.assertEvaluate("my_vec", "size=2") + self.assertEvaluate("my_vec", "size=2", want_varref=True) self.continue_to_breakpoint(breakpoint_5) - self.assertEvaluate("my_vec", "size=3") + self.assertEvaluate("my_vec", "size=3", want_varref=True) - self.assertEvaluate("my_map", "size=2") + self.assertEvaluate("my_map", "size=2", want_varref=True) self.continue_to_breakpoint(breakpoint_6) - self.assertEvaluate("my_map", "size=3") + self.assertEvaluate("my_map", "size=3", want_varref=True) - self.assertEvaluate("my_bool_vec", "size=1") + self.assertEvaluate("my_bool_vec", "size=1", want_varref=True) self.continue_to_breakpoint(breakpoint_7) - self.assertEvaluate("my_bool_vec", "size=2") + self.assertEvaluate("my_bool_vec", "size=2", want_varref=True) self.continue_to_breakpoint(breakpoint_8) # Test memory read, especially with 'empty' repeat commands. if context == "repl": - self.assertEvaluate("memory read -c 1 &my_ints", ".* 05 .*\n") - self.assertEvaluate("", ".* 0a .*\n") - self.assertEvaluate("", ".* 0f .*\n") - self.assertEvaluate("", ".* 14 .*\n") - self.assertEvaluate("", ".* 19 .*\n") + self.assertEvaluate( + "memory read -c 1 &my_ints", ".* 05 .*\n", want_memref=False + ) + self.assertEvaluate("", ".* 0a .*\n", want_memref=False) + self.assertEvaluate("", ".* 0f .*\n", want_memref=False) + self.assertEvaluate("", ".* 14 .*\n", want_memref=False) + self.assertEvaluate("", ".* 19 .*\n", want_memref=False) self.continue_to_exit() diff --git a/lldb/tools/lldb-dap/Handler/EvaluateRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/EvaluateRequestHandler.cpp index 57b2f621865f1..3f306a9489e32 100644 --- a/lldb/tools/lldb-dap/Handler/EvaluateRequestHandler.cpp +++ b/lldb/tools/lldb-dap/Handler/EvaluateRequestHandler.cpp @@ -99,17 +99,20 @@ EvaluateRequestHandler::Run(const EvaluateArguments &arguments) const { VariableDescription desc(value, dap.configuration.enableAutoVariableSummaries); + body.result = desc.GetResult(arguments.context); body.type = desc.display_type_name; + if (value.MightHaveChildren() || ValuePointsToCode(value)) body.variablesReference = dap.variables.InsertVariable( value, /*is_permanent=*/arguments.context == eEvaluateContextRepl); + if (lldb::addr_t addr = value.GetLoadAddress(); addr != LLDB_INVALID_ADDRESS) body.memoryReference = EncodeMemoryReference(addr); if (ValuePointsToCode(value) && body.variablesReference != LLDB_DAP_INVALID_VARRERF) - body.valueLocationReference = body.variablesReference; + body.valueLocationReference = PackLocation(body.variablesReference, true); return body; } diff --git a/lldb/tools/lldb-dap/Protocol/ProtocolRequests.cpp b/lldb/tools/lldb-dap/Protocol/ProtocolRequests.cpp index 82a461dad4451..ac01cfb95dd41 100644 --- a/lldb/tools/lldb-dap/Protocol/ProtocolRequests.cpp +++ b/lldb/tools/lldb-dap/Protocol/ProtocolRequests.cpp @@ -673,15 +673,13 @@ bool fromJSON(const llvm::json::Value &Params, EvaluateArguments &Args, } llvm::json::Value toJSON(const EvaluateResponseBody &Body) { - json::Object result{{"result", Body.result}}; + json::Object result{{"result", Body.result}, + {"variablesReference", Body.variablesReference}}; if (!Body.type.empty()) result.insert({"type", Body.type}); if (Body.presentationHint) result.insert({"presentationHint", Body.presentationHint}); - if (Body.variablesReference != 0 && - Body.variablesReference != LLDB_DAP_INVALID_VARRERF) - result.insert({"variablesReference", Body.variablesReference}); if (Body.namedVariables) result.insert({"namedVariables", Body.namedVariables}); if (Body.indexedVariables) diff --git a/lldb/tools/lldb-dap/Protocol/ProtocolRequests.h b/lldb/tools/lldb-dap/Protocol/ProtocolRequests.h index 36517c926dbda..c1e1e93f1e44a 100644 --- a/lldb/tools/lldb-dap/Protocol/ProtocolRequests.h +++ b/lldb/tools/lldb-dap/Protocol/ProtocolRequests.h @@ -1145,7 +1145,7 @@ struct EvaluateResponseBody { /// children can be retrieved by passing `variablesReference` to the /// `variables` request as long as execution remains suspended. See 'Lifetime /// of Object References' in the Overview section for details. - int64_t variablesReference = LLDB_DAP_INVALID_VARRERF; + int64_t variablesReference = 0; /// The number of named child variables. /// The client can use this information to present the variables in a paged @@ -1174,7 +1174,7 @@ struct EvaluateResponseBody { /// /// This reference shares the same lifetime as the `variablesReference`. See /// 'Lifetime of Object References' in the Overview section for details. - uint32_t valueLocationReference = LLDB_DAP_INVALID_VALUE_LOC; + uint64_t valueLocationReference = LLDB_DAP_INVALID_VALUE_LOC; }; llvm::json::Value toJSON(const EvaluateResponseBody &); From 3ea64a44e0c1e2ddbfe9bbd8a9568ccfdcff2381 Mon Sep 17 00:00:00 2001 From: John Harrison Date: Thu, 13 Nov 2025 14:47:54 -0800 Subject: [PATCH 3/3] Addressing reviewer comments. --- .../Handler/EvaluateRequestHandler.cpp | 19 +++---------------- lldb/tools/lldb-dap/LLDBUtils.cpp | 11 +++++++---- lldb/tools/lldb-dap/LLDBUtils.h | 2 +- 3 files changed, 11 insertions(+), 21 deletions(-) diff --git a/lldb/tools/lldb-dap/Handler/EvaluateRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/EvaluateRequestHandler.cpp index 3f306a9489e32..ea8c3a2a4a296 100644 --- a/lldb/tools/lldb-dap/Handler/EvaluateRequestHandler.cpp +++ b/lldb/tools/lldb-dap/Handler/EvaluateRequestHandler.cpp @@ -13,14 +13,9 @@ #include "Protocol/ProtocolRequests.h" #include "Protocol/ProtocolTypes.h" #include "RequestHandler.h" -#include "lldb/API/SBCommandInterpreter.h" -#include "lldb/API/SBCommandReturnObject.h" #include "lldb/lldb-enumerations.h" #include "llvm/ADT/StringRef.h" #include "llvm/Support/Error.h" -#include -#include -#include using namespace llvm; using namespace lldb_dap; @@ -53,11 +48,9 @@ EvaluateRequestHandler::Run(const EvaluateArguments &arguments) const { } bool required_command_failed = false; - std::string result = RunLLDBCommands( + body.result = RunLLDBCommands( dap.debugger, llvm::StringRef(), {expression}, required_command_failed, /*parse_command_directives=*/false, /*echo_commands=*/false); - - body.result = result; return body; } @@ -88,14 +81,8 @@ EvaluateRequestHandler::Run(const EvaluateArguments &arguments) const { if (value.GetError().Fail() && arguments.context != eEvaluateContextHover) value = frame.EvaluateExpression(expression.data()); - if (value.GetError().Fail()) { - // This error object must live until we're done with the pointer returned - // by GetCString(). - lldb::SBError error = value.GetError(); - const char *error_cstr = error.GetCString(); - return make_error( - error_cstr && error_cstr[0] ? error_cstr : "evaluate failed"); - } + if (value.GetError().Fail()) + return ToError(value.GetError(), /*show_user=*/false); VariableDescription desc(value, dap.configuration.enableAutoVariableSummaries); diff --git a/lldb/tools/lldb-dap/LLDBUtils.cpp b/lldb/tools/lldb-dap/LLDBUtils.cpp index 4db6caa1af38b..e2ba2ee64103d 100644 --- a/lldb/tools/lldb-dap/LLDBUtils.cpp +++ b/lldb/tools/lldb-dap/LLDBUtils.cpp @@ -7,6 +7,7 @@ //===----------------------------------------------------------------------===// #include "LLDBUtils.h" +#include "DAPError.h" #include "JSONUtils.h" #include "lldb/API/SBCommandInterpreter.h" #include "lldb/API/SBCommandReturnObject.h" @@ -17,6 +18,7 @@ #include "lldb/API/SBThread.h" #include "lldb/lldb-enumerations.h" #include "llvm/ADT/ArrayRef.h" +#include "llvm/Support/Error.h" #include "llvm/Support/JSON.h" #include "llvm/Support/raw_ostream.h" @@ -214,13 +216,14 @@ GetStopDisassemblyDisplay(lldb::SBDebugger &debugger) { return result; } -llvm::Error ToError(const lldb::SBError &error) { +llvm::Error ToError(const lldb::SBError &error, bool show_user) { if (error.Success()) return llvm::Error::success(); - return llvm::createStringError( - std::error_code(error.GetError(), std::generic_category()), - error.GetCString()); + return llvm::make_error( + /*message=*/error.GetCString(), + /*EC=*/std::error_code(error.GetError(), std::generic_category()), + /*show_user=*/show_user); } std::string GetStringValue(const lldb::SBStructuredData &data) { diff --git a/lldb/tools/lldb-dap/LLDBUtils.h b/lldb/tools/lldb-dap/LLDBUtils.h index 9db721a47ccf7..a29d3d88789a0 100644 --- a/lldb/tools/lldb-dap/LLDBUtils.h +++ b/lldb/tools/lldb-dap/LLDBUtils.h @@ -243,7 +243,7 @@ class ScopeSyncMode { lldb::StopDisassemblyType GetStopDisassemblyDisplay(lldb::SBDebugger &debugger); /// Take ownership of the stored error. -llvm::Error ToError(const lldb::SBError &error); +llvm::Error ToError(const lldb::SBError &error, bool show_user = true); /// Provides the string value if this data structure is a string type. std::string GetStringValue(const lldb::SBStructuredData &data);