From e9799e51ed32cf5238be03aadd4d8615a6c2cb67 Mon Sep 17 00:00:00 2001 From: John Harrison Date: Wed, 18 Mar 2026 11:39:22 -0700 Subject: [PATCH] [lldb-dap] Improve support for variables with anonymous fields and types (#186482) While looking at the '[raw]' value of a std::vector I noticed we didn't handle the anonymous inner struct very well. The 'evaluateName' was incorrect (e.g. the evaluateName would return `.` for the anonymous struct). This improves support for variables with anonymous fields and anonymous types. * Changed the name of anonymous fields from `` to `(anonymous)`, which matches other tooling like clangd's representation and how types are presented if the field is not defined. * Adjusts variables to not return an 'evaluateName' for anonymous fields. * Adjusted '[raw]' values to be marked as 'internal' which deemphasizes them in the UI. While working in this area, I also consolidated some helpers that are only used within Variables.cpp. Before my changes: before After my changes: after --- .../lldb-dap/variables/TestDAP_variables.py | 177 ++++++++-- .../API/tools/lldb-dap/variables/main.cpp | 43 ++- lldb/tools/lldb-dap/DAP.cpp | 2 +- .../Handler/EvaluateRequestHandler.cpp | 4 +- .../Handler/SetVariableRequestHandler.cpp | 4 +- .../Handler/VariablesRequestHandler.cpp | 3 +- lldb/tools/lldb-dap/JSONUtils.cpp | 15 +- lldb/tools/lldb-dap/ProtocolUtils.cpp | 73 ----- lldb/tools/lldb-dap/ProtocolUtils.h | 42 --- lldb/tools/lldb-dap/Variables.cpp | 303 +++++++++++------- lldb/tools/lldb-dap/Variables.h | 43 ++- lldb/unittests/DAP/VariablesTest.cpp | 29 +- 12 files changed, 440 insertions(+), 298 deletions(-) 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 1216cfd73282..66c8d7f720ae 100644 --- a/lldb/test/API/tools/lldb-dap/variables/TestDAP_variables.py +++ b/lldb/test/API/tools/lldb-dap/variables/TestDAP_variables.py @@ -351,9 +351,9 @@ class TestDAP_variables(lldbdap_testcase.DAPTestCaseBase): verify_locals["argc"]["equals"]["value"] = "123" verify_locals["pt"]["children"]["x"]["equals"]["value"] = "111" - verify_locals["x @ main.cpp:23"] = {"equals": {"type": "int", "value": "89"}} - verify_locals["x @ main.cpp:25"] = {"equals": {"type": "int", "value": "42"}} - verify_locals["x @ main.cpp:27"] = {"equals": {"type": "int", "value": "72"}} + verify_locals["x @ main.cpp:27"] = {"equals": {"type": "int", "value": "89"}} + verify_locals["x @ main.cpp:29"] = {"equals": {"type": "int", "value": "42"}} + verify_locals["x @ main.cpp:31"] = {"equals": {"type": "int", "value": "72"}} self.verify_variables(verify_locals, self.dap_server.get_local_variables()) @@ -361,22 +361,22 @@ class TestDAP_variables(lldbdap_testcase.DAPTestCaseBase): self.assertFalse(self.set_local("x2", 9)["success"]) self.assertFalse(self.set_local("x @ main.cpp:0", 9)["success"]) - self.assertTrue(self.set_local("x @ main.cpp:23", 19)["success"]) - self.assertTrue(self.set_local("x @ main.cpp:25", 21)["success"]) - self.assertTrue(self.set_local("x @ main.cpp:27", 23)["success"]) + self.assertTrue(self.set_local("x @ main.cpp:27", 19)["success"]) + self.assertTrue(self.set_local("x @ main.cpp:29", 21)["success"]) + self.assertTrue(self.set_local("x @ main.cpp:31", 23)["success"]) # The following should have no effect - self.assertFalse(self.set_local("x @ main.cpp:27", "invalid")["success"]) + self.assertFalse(self.set_local("x @ main.cpp:31", "invalid")["success"]) - verify_locals["x @ main.cpp:23"]["equals"]["value"] = "19" - verify_locals["x @ main.cpp:25"]["equals"]["value"] = "21" - verify_locals["x @ main.cpp:27"]["equals"]["value"] = "23" + verify_locals["x @ main.cpp:27"]["equals"]["value"] = "19" + verify_locals["x @ main.cpp:29"]["equals"]["value"] = "21" + verify_locals["x @ main.cpp:31"]["equals"]["value"] = "23" self.verify_variables(verify_locals, self.dap_server.get_local_variables()) # The plain x variable shold refer to the innermost x self.assertTrue(self.set_local("x", 22)["success"]) - verify_locals["x @ main.cpp:27"]["equals"]["value"] = "22" + verify_locals["x @ main.cpp:31"]["equals"]["value"] = "22" self.verify_variables(verify_locals, self.dap_server.get_local_variables()) @@ -393,9 +393,9 @@ class TestDAP_variables(lldbdap_testcase.DAPTestCaseBase): names = [var["name"] for var in locals] # The first shadowed x shouldn't have a suffix anymore verify_locals["x"] = {"equals": {"type": "int", "value": "19"}} - self.assertNotIn("x @ main.cpp:23", names) - self.assertNotIn("x @ main.cpp:25", names) self.assertNotIn("x @ main.cpp:27", names) + self.assertNotIn("x @ main.cpp:29", names) + self.assertNotIn("x @ main.cpp:31", names) self.verify_variables(verify_locals, locals) @@ -422,14 +422,19 @@ class TestDAP_variables(lldbdap_testcase.DAPTestCaseBase): program, enableAutoVariableSummaries=enableAutoVariableSummaries ) source = "main.cpp" - breakpoint1_line = line_number(source, "// breakpoint 1") - lines = [breakpoint1_line] - # Set breakpoint in the thread function so we can step the threads + lines = [ + line_number(source, "// breakpoint 1"), + line_number(source, "// breakpoint 3"), + line_number(source, "// breakpoint 6"), + line_number(source, "// breakpoint 7"), + line_number(source, "// breakpoint 8"), + ] breakpoint_ids = self.set_source_breakpoints(source, lines) self.assertEqual( len(breakpoint_ids), len(lines), "expect correct number of breakpoints" ) - self.continue_to_breakpoints(breakpoint_ids) + [bp1, bp3, bp6, bp7, bp8] = breakpoint_ids + self.continue_to_breakpoint(bp1) # Verify locals locals = self.dap_server.get_local_variables() @@ -602,15 +607,139 @@ class TestDAP_variables(lldbdap_testcase.DAPTestCaseBase): expandable_expression["children"], response["body"]["variables"] ) + self.continue_to_breakpoint(bp6) + + locals = self.dap_server.get_local_variables() + self.verify_variables( + { + "my_var": { + "equals": { + "type": "(unnamed struct)", + "value": '{name:"hello world!", x:42, y:7}' + if enableAutoVariableSummaries + else "(unnamed struct)", + "evaluateName": "my_var", + }, + "readOnly": True, + }, + }, + locals, + ) + + self.continue_to_breakpoint(bp7) + + expr_varref_dict = {} + locals = self.dap_server.get_local_variables() + self.verify_variables( + { + "home": { + "equals": { + "type": "MySock", + "value": "MySock", + "evaluateName": "home", + }, + "readOnly": True, + }, + }, + locals, + expr_varref_dict, + ) + + inner_var_ref = expr_varref_dict["home"] + anon_vars = self.dap_server.request_variables(inner_var_ref)["body"][ + "variables" + ] + self.verify_variables( + { + "(anonymous)": { + "equals": {"type": "MySock::(anonymous union)"}, + "matches": { + "value": r"{ipv4:.*, ipv6:.*}" + if enableAutoVariableSummaries + else r"MySock::\(anonymous union\)", + }, + "readOnly": True, + "missing": ["evaluateName"], + } + }, + anon_vars, + ) + inner_union_vars = self.dap_server.request_variables( + anon_vars[0]["variablesReference"] + )["body"]["variables"] + self.verify_variables( + { + "ipv4": { + "equals": { + "type": "unsigned char[4]", + "evaluateName": "home.ipv4", + }, + "readOnly": True, + }, + "ipv6": { + "equals": { + "type": "unsigned char[6]", + "evaluateName": "home.ipv6", + }, + "readOnly": True, + }, + }, + inner_union_vars, + ) + + self.continue_to_breakpoint(bp8) + + locals = self.dap_server.get_local_variables() + self.verify_variables( + { + "e": { + "equals": { + "type": "example", + "value": "{lo:10, hi:11}" + if enableAutoVariableSummaries + else "example", + "evaluateName": "e", + }, + "readOnly": True, + }, + }, + locals, + ) + inner_bitfields_struct = self.dap_server.request_variables( + locals[0]["variablesReference"] + )["body"]["variables"] + self.verify_variables( + { + "lo": { + "equals": { + "type": "unsigned int", + "value": "10", + "evaluateName": "e.lo", + "variablesReference": 0, + } + }, + "(anonymous)": { + "equals": { + "type": "int", + "value": "0", + "variablesReference": 0, + } + }, + "hi": { + "equals": { + "type": "unsigned int", + "value": "11", + "evaluateName": "e.hi", + "variablesReference": 0, + } + }, + }, + inner_bitfields_struct, + ) + # Continue to breakpoint 3, permanent variable should still exist # after resume. - breakpoint3_line = line_number(source, "// breakpoint 3") - lines = [breakpoint3_line] - breakpoint_ids = self.set_source_breakpoints(source, lines) - self.assertEqual( - len(breakpoint_ids), len(lines), "expect correct number of breakpoints" - ) - self.continue_to_breakpoints(breakpoint_ids) + self.continue_to_breakpoint(bp3) var_ref = permanent_expandable_ref response = self.dap_server.request_variables(var_ref) diff --git a/lldb/test/API/tools/lldb-dap/variables/main.cpp b/lldb/test/API/tools/lldb-dap/variables/main.cpp index 04fc62f02c22..6e17ebadc246 100644 --- a/lldb/test/API/tools/lldb-dap/variables/main.cpp +++ b/lldb/test/API/tools/lldb-dap/variables/main.cpp @@ -7,10 +7,14 @@ struct PointType { }; #include #include +extern int g_global; int g_global = 123; static int s_global = 234; int test_indexedVariables(); int test_return_variable(); +int test_anonymous_types(); +int test_anonymous_fields(); +void test_unnamed_bitfields(); int main(int argc, char const *argv[]) { static float s_local = 2.25; @@ -31,6 +35,9 @@ int main(int argc, char const *argv[]) { { int return_result = test_return_variable(); } + test_anonymous_types(); + test_anonymous_fields(); + test_unnamed_bitfields(); return test_indexedVariables(); // breakpoint 3 } @@ -46,4 +53,38 @@ int test_indexedVariables() { int test_return_variable() { return 300; // breakpoint 5 -} \ No newline at end of file +} + +int test_anonymous_types() { + struct { + char name[16]; + int x, y; + } my_var = {"hello world!", 42, 7}; + return my_var.x + my_var.y; // breakpoint 6 +} + +struct MySock { + union { + unsigned char ipv4[4]; + unsigned char ipv6[6]; + }; +}; + +int test_anonymous_fields() { + MySock home = {{{0}}}; + home.ipv4[0] = 127; + home.ipv4[1] = 0; + home.ipv4[2] = 0; + home.ipv4[1] = 1; + return 1; // breakpoint 7 +} + +void test_unnamed_bitfields() { + struct example { + unsigned int lo : 4; + unsigned int : 0; + unsigned int hi : 4; + }; + example e = {0xA, 0xB}; + printf("lo: %u, hi: %u\n", e.lo, e.hi); // breakpoint 8 +} diff --git a/lldb/tools/lldb-dap/DAP.cpp b/lldb/tools/lldb-dap/DAP.cpp index f482fe5e7aa9..f1ebe9b31649 100644 --- a/lldb/tools/lldb-dap/DAP.cpp +++ b/lldb/tools/lldb-dap/DAP.cpp @@ -126,7 +126,7 @@ llvm::StringRef DAP::debug_adapter_path = ""; DAP::DAP(Log &log, const ReplMode default_repl_mode, const std::vector &pre_init_commands, bool no_lldbinit, llvm::StringRef client_name, DAPTransport &transport, MainLoop &loop) - : log(log), transport(transport), reference_storage(log), + : log(log), transport(transport), reference_storage(log, configuration), broadcaster("lldb-dap"), progress_event_reporter( [&](const ProgressEvent &event) { SendJSON(event.ToJSON()); }), diff --git a/lldb/tools/lldb-dap/Handler/EvaluateRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/EvaluateRequestHandler.cpp index f2117d81f315..906f262779e1 100644 --- a/lldb/tools/lldb-dap/Handler/EvaluateRequestHandler.cpp +++ b/lldb/tools/lldb-dap/Handler/EvaluateRequestHandler.cpp @@ -133,8 +133,8 @@ EvaluateRequestHandler::Run(const EvaluateArguments &arguments) const { body.type = desc.display_type_name; if (value.MightHaveChildren() || ValuePointsToCode(value)) - body.variablesReference = - dap.reference_storage.Insert(value, /*is_permanent=*/is_repl_context); + body.variablesReference = dap.reference_storage.Insert( + value, /*is_permanent=*/is_repl_context, /*is_internal=*/false); if (lldb::addr_t addr = value.GetLoadAddress(); addr != LLDB_INVALID_ADDRESS) body.memoryReference = EncodeMemoryReference(addr); diff --git a/lldb/tools/lldb-dap/Handler/SetVariableRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/SetVariableRequestHandler.cpp index 8189f5181b1b..47a45ab80785 100644 --- a/lldb/tools/lldb-dap/Handler/SetVariableRequestHandler.cpp +++ b/lldb/tools/lldb-dap/Handler/SetVariableRequestHandler.cpp @@ -83,8 +83,8 @@ SetVariableRequestHandler::Run(const SetVariableArguments &args) const { // so always insert a new one to get its variablesReference. // is_permanent is false because debug console does not support // setVariable request. - const var_ref_t new_var_ref = - dap.reference_storage.Insert(variable, /*is_permanent=*/false); + const var_ref_t new_var_ref = dap.reference_storage.Insert( + variable, /*is_permanent=*/false, /*is_internal=*/false); if (variable.MightHaveChildren()) { body.variablesReference = new_var_ref; if (desc.type_obj.IsArrayType()) diff --git a/lldb/tools/lldb-dap/Handler/VariablesRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/VariablesRequestHandler.cpp index 7918be4983cc..841e0ec2e497 100644 --- a/lldb/tools/lldb-dap/Handler/VariablesRequestHandler.cpp +++ b/lldb/tools/lldb-dap/Handler/VariablesRequestHandler.cpp @@ -37,8 +37,7 @@ VariablesRequestHandler::Run(const VariablesArguments &arguments) const { llvm::formatv("invalid variablesReference: {}.", var_ref.AsUInt32()), /*error_code=*/llvm::inconvertibleErrorCode(), /*show_user=*/false); - Expected> variables = - store->GetVariables(dap.reference_storage, dap.configuration, arguments); + Expected> variables = store->GetVariables(arguments); if (llvm::Error err = variables.takeError()) return err; diff --git a/lldb/tools/lldb-dap/JSONUtils.cpp b/lldb/tools/lldb-dap/JSONUtils.cpp index d75b4e23d0e4..5cc37917f4b2 100644 --- a/lldb/tools/lldb-dap/JSONUtils.cpp +++ b/lldb/tools/lldb-dap/JSONUtils.cpp @@ -244,7 +244,7 @@ llvm::json::Object CreateEventObject(const llvm::StringRef event_name) { llvm::StringRef GetNonNullVariableName(lldb::SBValue &v) { const llvm::StringRef name = v.GetName(); - return !name.empty() ? name : ""; + return !name.empty() ? name : "(anonymous)"; } std::string CreateUniqueVariableNameForDisplay(lldb::SBValue &v, @@ -311,9 +311,16 @@ VariableDescription::VariableDescription( } } - lldb::SBStream evaluateStream; - val.GetExpressionPath(evaluateStream); - evaluate_name = llvm::StringRef(evaluateStream.GetData()).str(); + // Only include the evaluation name if the name is not empty. If the name is + // empty then 'GetExpressionPath' will return an empty string like 'foo.', + // which does not actually work in expression evaluation. + if (!llvm::StringRef{val.GetName()}.empty()) { + lldb::SBStream evaluateStream; + val.GetExpressionPath(evaluateStream); + evaluate_name = + llvm::StringRef{evaluateStream.GetData(), evaluateStream.GetSize()} + .str(); + } } std::string VariableDescription::GetResult(protocol::EvaluateContext context) { diff --git a/lldb/tools/lldb-dap/ProtocolUtils.cpp b/lldb/tools/lldb-dap/ProtocolUtils.cpp index bc997b1da9bd..d94a31f554e2 100644 --- a/lldb/tools/lldb-dap/ProtocolUtils.cpp +++ b/lldb/tools/lldb-dap/ProtocolUtils.cpp @@ -238,77 +238,4 @@ CreateExceptionBreakpointFilter(const ExceptionBreakpoint &bp) { return filter; } -Variable CreateVariable(lldb::SBValue v, var_ref_t var_ref, bool format_hex, - bool auto_variable_summaries, - bool synthetic_child_debugging, bool is_name_duplicated, - std::optional custom_name) { - VariableDescription desc(v, auto_variable_summaries, format_hex, - is_name_duplicated, custom_name); - Variable var; - var.name = desc.name; - var.value = desc.display_value; - var.type = desc.display_type_name; - - if (!desc.evaluate_name.empty()) - var.evaluateName = desc.evaluate_name; - - // If we have a type with many children, we would like to be able to - // give a hint to the IDE that the type has indexed children so that the - // request can be broken up in grabbing only a few children at a time. We - // want to be careful and only call "v.GetNumChildren()" if we have an array - // type or if we have a synthetic child provider producing indexed children. - // We don't want to call "v.GetNumChildren()" on all objects as class, struct - // and union types don't need to be completed if they are never expanded. So - // we want to avoid calling this to only cases where we it makes sense to keep - // performance high during normal debugging. - - // If we have an array type, say that it is indexed and provide the number - // of children in case we have a huge array. If we don't do this, then we - // might take a while to produce all children at onces which can delay your - // debug session. - if (desc.type_obj.IsArrayType()) { - var.indexedVariables = v.GetNumChildren(); - } else if (v.IsSynthetic()) { - // For a type with a synthetic child provider, the SBType of "v" won't tell - // us anything about what might be displayed. Instead, we check if the first - // child's name is "[0]" and then say it is indexed. We call - // GetNumChildren() only if the child name matches to avoid a potentially - // expensive operation. - if (lldb::SBValue first_child = v.GetChildAtIndex(0)) { - llvm::StringRef first_child_name = first_child.GetName(); - if (first_child_name == "[0]") { - size_t num_children = v.GetNumChildren(); - // If we are creating a "[raw]" fake child for each synthetic type, we - // have to account for it when returning indexed variables. - if (synthetic_child_debugging) - ++num_children; - var.indexedVariables = num_children; - } - } - } - - if (v.MightHaveChildren()) - var.variablesReference = var_ref; - - if (v.GetDeclaration().IsValid()) - var.declarationLocationReference = PackLocation(var_ref.AsUInt32(), false); - - if (ValuePointsToCode(v)) - var.valueLocationReference = PackLocation(var_ref.AsUInt32(), true); - - if (lldb::addr_t addr = v.GetLoadAddress(); addr != LLDB_INVALID_ADDRESS) - var.memoryReference = addr; - - bool is_readonly = v.GetType().IsAggregateType() || - v.GetValueType() == lldb::eValueTypeRegisterSet || - var.name == "(Return Value)"; - if (is_readonly) { - if (!var.presentationHint) - var.presentationHint = {VariablePresentationHint()}; - var.presentationHint->attributes.push_back("readOnly"); - } - - return var; -} - } // namespace lldb_dap diff --git a/lldb/tools/lldb-dap/ProtocolUtils.h b/lldb/tools/lldb-dap/ProtocolUtils.h index 376a8d6b7c02..f3ce2f22ac1c 100644 --- a/lldb/tools/lldb-dap/ProtocolUtils.h +++ b/lldb/tools/lldb-dap/ProtocolUtils.h @@ -107,48 +107,6 @@ CreateExceptionBreakpointFilter(const ExceptionBreakpoint &bp); /// "2 MB"). std::string ConvertDebugInfoSizeToString(uint64_t debug_size); -/// Create a protocol Variable for the given value. -/// -/// \param[in] v -/// The LLDB value to use when populating out the "Variable" -/// object. -/// -/// \param[in] var_ref -/// The variable reference. Used to identify the value, e.g. -/// in the `variablesReference` or `declarationLocationReference` -/// properties. -/// -/// \param[in] format_hex -/// If set to true the variable will be formatted as hex in -/// the "value" key value pair for the value of the variable. -/// -/// \param[in] auto_variable_summaries -/// If set to true the variable will create an automatic variable summary. -/// -/// \param[in] synthetic_child_debugging -/// Whether to include synthetic children when listing properties of the -/// value. -/// -/// \param[in] is_name_duplicated -/// Whether the same variable name appears multiple times within the same -/// context (e.g. locals). This can happen due to shadowed variables in -/// nested blocks. -/// -/// As VSCode doesn't render two of more variables with the same name, we -/// apply a suffix to distinguish duplicated variables. -/// -/// \param[in] custom_name -/// A provided custom name that is used instead of the SBValue's when -/// creating the JSON representation. -/// -/// \return -/// A Variable representing the given value. -protocol::Variable -CreateVariable(lldb::SBValue v, var_ref_t var_ref, bool format_hex, - bool auto_variable_summaries, bool synthetic_child_debugging, - bool is_name_duplicated, - std::optional custom_name = {}); - } // namespace lldb_dap #endif diff --git a/lldb/tools/lldb-dap/Variables.cpp b/lldb/tools/lldb-dap/Variables.cpp index 0053926aeeea..6ec2548ccb39 100644 --- a/lldb/tools/lldb-dap/Variables.cpp +++ b/lldb/tools/lldb-dap/Variables.cpp @@ -7,19 +7,18 @@ //===----------------------------------------------------------------------===// #include "Variables.h" -#include "DAPLog.h" #include "JSONUtils.h" #include "LLDBUtils.h" #include "Protocol/DAPTypes.h" #include "Protocol/ProtocolRequests.h" #include "Protocol/ProtocolTypes.h" -#include "ProtocolUtils.h" #include "SBAPIExtras.h" #include "lldb/API/SBDeclaration.h" #include "lldb/API/SBFrame.h" #include "lldb/API/SBValue.h" #include "lldb/API/SBValueList.h" #include "llvm/ADT/Sequence.h" +#include "llvm/ADT/StringExtras.h" #include "llvm/ADT/StringMap.h" #include "llvm/Support/ErrorHandling.h" #include @@ -47,73 +46,185 @@ template StringMap DistinctNames(T &container) { return variable_name_counts; } -template -std::vector MakeVariables( - VariableReferenceStorage &storage, const Configuration &config, - const VariablesArguments &args, T &container, bool is_permanent, - const std::map &name_overrides = {}) { - std::vector variables; +protocol::Scope MakeScope(ScopeKind kind, var_ref_t variablesReference, + bool expensive) { + protocol::Scope scope; + scope.variablesReference = variablesReference; + scope.expensive = expensive; - // We first find out which variable names are duplicated. - StringMap variable_name_counts = DistinctNames(container); - - const bool format_hex = args.format ? args.format->hex : false; - auto start_it = begin(container) + args.start; - auto end_it = args.count == 0 ? end(container) : start_it + args.count; - - // Now we construct the result with unique display variable names. - for (; start_it != end_it; start_it++) { - lldb::SBValue variable = *start_it; - if (!variable.IsValid()) - break; - - const var_ref_t var_ref = - HasInnerVarref(variable) - ? storage.Insert(variable, /*is_permanent=*/is_permanent) - : var_ref_t(var_ref_t::k_no_child); - if (LLVM_UNLIKELY(var_ref.AsUInt32() >= - var_ref_t::k_variables_reference_threshold)) { - DAP_LOG(storage.log, - "warning: variablesReference threshold reached. " - "current: {} threshold: {}, maximum {}.", - var_ref.AsUInt32(), var_ref_t::k_variables_reference_threshold, - var_ref_t::k_max_variables_references); - break; - } - - if (LLVM_UNLIKELY(var_ref.Kind() == eReferenceKindInvalid)) - break; - - std::optional custom_name; - auto name_it = name_overrides.find(variable.GetID()); - if (name_it != name_overrides.end()) - custom_name = name_it->second; - - variables.emplace_back(CreateVariable( - variable, var_ref, format_hex, config.enableAutoVariableSummaries, - config.enableSyntheticChildDebugging, - variable_name_counts[GetNonNullVariableName(variable)] > 1, - custom_name)); + switch (kind) { + case eScopeKindLocals: + scope.presentationHint = protocol::Scope::eScopePresentationHintLocals; + scope.name = "Locals"; + break; + case eScopeKindGlobals: + scope.name = "Globals"; + break; + case eScopeKindRegisters: + scope.presentationHint = protocol::Scope::eScopePresentationHintRegisters; + scope.name = "Registers"; + break; } - return variables; + return scope; } +std::optional +MakeVariablePresentationHints(bool is_readonly, bool is_internal) { + if (!is_readonly && !is_internal) + return std::nullopt; + + VariablePresentationHint hint; + + if (is_readonly) + hint.attributes.push_back("readOnly"); + if (is_internal) + hint.visibility = "internal"; + + return hint; +} + +bool IsReservedName(llvm::StringRef name) { + if (name == "[raw]" || name.starts_with("std::__")) + return true; + auto last_namespace_component = name.rfind("::"); + if (last_namespace_component != llvm::StringRef::npos) + name = name.substr(last_namespace_component + 2); + return /* c/c++ std reserves prefixes __ or _[A-Z] for internal use */ + name.starts_with("__") || + (name.size() >= 2 && name[0] == '_' && isUpper(name[1])); +} + +class VariableStoreImpl : public VariableStore { +public: + using VariableStore::VariableStore; + Variable CreateVariable(lldb::SBValue v, bool format_hex, + bool is_name_duplicated, + std::optional custom_name) { + VariableDescription desc(v, m_storage.config.enableAutoVariableSummaries, + format_hex, is_name_duplicated, custom_name); + Variable var; + var.name = std::move(desc.name); + var.value = std::move(desc.display_value); + var.type = std::move(desc.display_type_name); + + if (!desc.evaluate_name.empty()) + var.evaluateName = std::move(desc.evaluate_name); + + // If we have a type with many children, we would like to be able to + // give a hint to the IDE that the type has indexed children so that the + // request can be broken up in grabbing only a few children at a time. We + // want to be careful and only call "v.GetNumChildren()" if we have an array + // type or if we have a synthetic child provider producing indexed children. + // We don't want to call "v.GetNumChildren()" on all objects as class, + // struct and union types don't need to be completed if they are never + // expanded. So we want to avoid calling this to only cases where we it + // makes sense to keep performance high during normal debugging. + + // If we have an array type, say that it is indexed and provide the number + // of children in case we have a huge array. If we don't do this, then we + // might take a while to produce all children at onces which can delay your + // debug session. + if (desc.type_obj.IsArrayType()) { + var.indexedVariables = v.GetNumChildren(); + } else if (v.IsSynthetic()) { + // For a type with a synthetic child provider, the SBType of "v" won't + // tell us anything about what might be displayed. Instead, we check if + // the first child's name is "[0]" and then say it is indexed. We call + // GetNumChildren() only if the child name matches to avoid a potentially + // expensive operation. + if (lldb::SBValue first_child = v.GetChildAtIndex(0)) { + llvm::StringRef first_child_name = first_child.GetName(); + if (first_child_name == "[0]") { + size_t num_children = v.GetNumChildren(); + // If we are creating a "[raw]" fake child for each synthetic type, we + // have to account for it when returning indexed variables. + if (m_storage.config.enableSyntheticChildDebugging) + ++num_children; + var.indexedVariables = num_children; + } + } + } + + const bool is_internal = IsReservedName(var.name) || m_is_internal; + const bool is_readonly = is_internal || v.GetType().IsAggregateType() || + v.GetValueType() == lldb::eValueTypeRegisterSet || + var.name == "(Return Value)"; + + var.presentationHint = + MakeVariablePresentationHints(is_readonly, is_internal); + + const var_ref_t var_ref = + HasInnerVarref(v) + ? m_storage.Insert(v, /*is_permanent=*/m_is_permanent, is_internal) + : var_ref_t(var_ref_t::k_no_child); + + if (var.indexedVariables || v.MightHaveChildren()) + var.variablesReference = var_ref; + + if (v.GetDeclaration().IsValid()) + var.declarationLocationReference = + PackLocation(var_ref.AsUInt32(), false); + + if (ValuePointsToCode(v)) + var.valueLocationReference = PackLocation(var_ref.AsUInt32(), true); + + if (lldb::addr_t addr = v.GetLoadAddress(); addr != LLDB_INVALID_ADDRESS) + var.memoryReference = addr; + + return var; + } + + template + std::vector MakeVariables( + const VariablesArguments &args, T &container, + const std::map &name_overrides = {}) { + std::vector variables; + + // We first find out which variable names are duplicated. + StringMap variable_name_counts = DistinctNames(container); + + const bool format_hex = args.format ? args.format->hex : false; + auto start_it = begin(container) + args.start; + auto end_it = args.count == 0 ? end(container) : start_it + args.count; + + // Now we construct the result with unique display variable names. + for (; start_it != end_it; start_it++) { + lldb::SBValue variable = *start_it; + if (!variable.IsValid()) + break; + + std::optional custom_name; + auto name_it = name_overrides.find(variable.GetID()); + if (name_it != name_overrides.end()) + custom_name = name_it->second; + + variables.emplace_back(CreateVariable( + variable, format_hex, + variable_name_counts[GetNonNullVariableName(variable)] > 1, + custom_name)); + } + + return variables; + } +}; + /// A Variable store for fetching variables within a specific scope (locals, /// globals, or registers) for a given stack frame. -class ScopeStore final : public VariableStore { +class ScopeStore final : public VariableStoreImpl { public: - explicit ScopeStore(ScopeKind kind, const lldb::SBFrame &frame) - : m_frame(frame), m_kind(kind) {} + explicit ScopeStore(VariableReferenceStorage &storage, ScopeKind kind, + const lldb::SBFrame &frame) + : VariableStoreImpl(storage, /*is_permanent=*/false, + /*is_internal=*/frame.IsArtificial()), + m_frame(frame), m_kind(kind) {} Expected> - GetVariables(VariableReferenceStorage &storage, const Configuration &config, - const VariablesArguments &args) override { + GetVariables(const VariablesArguments &args) override { LoadVariables(); if (m_error.Fail()) return ToError(m_error); - return MakeVariables(storage, config, args, m_children, - /*is_permanent=*/false, m_names); + return MakeVariables(args, m_children, m_names); } lldb::SBValue FindVariable(llvm::StringRef name) override { @@ -138,8 +249,6 @@ public: return variable; } - lldb::SBValue GetVariable() const override { return lldb::SBValue(); } - private: void LoadVariables() { if (m_variables_loaded) @@ -213,15 +322,16 @@ private: /// /// Manages children variables of complex types (structs, arrays, pointers, /// etc.) that can be expanded in the debugger UI. -class ExpandableValueStore final : public VariableStore { +class ExpandableValueStore final : public VariableStoreImpl { public: - explicit ExpandableValueStore(const lldb::SBValue &value) : m_value(value) {} + explicit ExpandableValueStore(VariableReferenceStorage &storage, + bool is_permanent, bool is_internal, + const lldb::SBValue &value) + : VariableStoreImpl(storage, is_permanent, is_internal), m_value(value) {} llvm::Expected> - GetVariables(VariableReferenceStorage &storage, - const protocol::Configuration &config, - const protocol::VariablesArguments &args) override { + GetVariables(const protocol::VariablesArguments &args) override { std::map name_overrides; lldb::SBValueList list; for (auto inner : m_value) @@ -230,7 +340,8 @@ public: // We insert a new "[raw]" child that can be used to inspect the raw version // of a synthetic member. That eliminates the need for the user to go to the // debug console and type `frame var to get these values. - if (config.enableSyntheticChildDebugging && m_value.IsSynthetic()) { + if (m_storage.config.enableSyntheticChildDebugging && + m_value.IsSynthetic()) { lldb::SBValue synthetic_value = m_value.GetNonSyntheticValue(); name_overrides[synthetic_value.GetID()] = "[raw]"; // FIXME: Cloning the value seems to affect the type summary, see @@ -239,10 +350,7 @@ public: list.Append(synthetic_value); } - const bool is_permanent = - args.variablesReference.Kind() == eReferenceKindPermanent; - return MakeVariables(storage, config, args, list, is_permanent, - name_overrides); + return MakeVariables(args, list, name_overrides); } lldb::SBValue FindVariable(llvm::StringRef name) override { @@ -269,18 +377,18 @@ private: lldb::SBValue m_value; }; -class ExpandableValueListStore final : public VariableStore { +class ExpandableValueListStore final : public VariableStoreImpl { public: - explicit ExpandableValueListStore(const lldb::SBValueList &list) - : m_value_list(list) {} + explicit ExpandableValueListStore(VariableReferenceStorage &storage, + bool is_permanent, bool is_internal, + const lldb::SBValueList &list) + : VariableStoreImpl(storage, is_permanent, is_internal), + m_value_list(list) {} llvm::Expected> - GetVariables(VariableReferenceStorage &storage, - const protocol::Configuration &config, - const protocol::VariablesArguments &args) override { - return MakeVariables(storage, config, args, m_value_list, - /*is_permanent=*/true); + GetVariables(const protocol::VariablesArguments &args) override { + return MakeVariables(args, m_value_list); } lldb::SBValue FindVariable(llvm::StringRef name) override { @@ -291,10 +399,6 @@ public: return lldb::SBValue(); } - [[nodiscard]] lldb::SBValue GetVariable() const override { - return lldb::SBValue(); - } - private: lldb::SBValueList m_value_list; }; @@ -303,29 +407,6 @@ private: namespace lldb_dap { -protocol::Scope CreateScope(ScopeKind kind, var_ref_t variablesReference, - bool expensive) { - protocol::Scope scope; - scope.variablesReference = variablesReference; - scope.expensive = expensive; - - switch (kind) { - case eScopeKindLocals: - scope.presentationHint = protocol::Scope::eScopePresentationHintLocals; - scope.name = "Locals"; - break; - case eScopeKindGlobals: - scope.name = "Globals"; - break; - case eScopeKindRegisters: - scope.presentationHint = protocol::Scope::eScopePresentationHintRegisters; - scope.name = "Registers"; - break; - } - - return scope; -} - lldb::SBValue VariableReferenceStorage::GetVariable(var_ref_t var_ref) { const ReferenceKind kind = var_ref.Kind(); @@ -343,24 +424,28 @@ lldb::SBValue VariableReferenceStorage::GetVariable(var_ref_t var_ref) { } var_ref_t VariableReferenceStorage::Insert(const lldb::SBValue &variable, - bool is_permanent) { + bool is_permanent, + bool is_internal) { if (is_permanent) - return m_permanent_kind_pool.Add(variable); + return m_permanent_kind_pool.Add( + *this, is_permanent, is_internal, variable); - return m_temporary_kind_pool.Add(variable); + return m_temporary_kind_pool.Add(*this, is_permanent, + is_internal, variable); } var_ref_t VariableReferenceStorage::Insert(const lldb::SBValueList &values) { - return m_permanent_kind_pool.Add(values); + return m_permanent_kind_pool.Add( + *this, /*is_permanent=*/true, /*is_internal=*/false, values); } std::vector VariableReferenceStorage::Insert(const lldb::SBFrame &frame) { auto create_scope = [&](ScopeKind kind) { const var_ref_t var_ref = - m_temporary_kind_pool.Add(kind, frame); + m_temporary_kind_pool.Add(*this, kind, frame); const bool is_expensive = kind != eScopeKindLocals; - return CreateScope(kind, var_ref, is_expensive); + return MakeScope(kind, var_ref, is_expensive); }; return {create_scope(eScopeKindLocals), create_scope(eScopeKindGlobals), diff --git a/lldb/tools/lldb-dap/Variables.h b/lldb/tools/lldb-dap/Variables.h index bc6c4637fac8..eda7103aaa78 100644 --- a/lldb/tools/lldb-dap/Variables.h +++ b/lldb/tools/lldb-dap/Variables.h @@ -28,44 +28,35 @@ enum ScopeKind : unsigned { eScopeKindRegisters }; -/// Creates a `protocol::Scope` struct. -/// -/// \param[in] kind -/// The kind of scope to create -/// -/// \param[in] variablesReference -/// The value to place into the "variablesReference" key -/// -/// \param[in] expensive -/// The value to place into the "expensive" key -/// -/// \return -/// A `protocol::Scope` -protocol::Scope CreateScope(ScopeKind kind, var_ref_t variablesReference, - bool expensive); - /// An Interface to get or find specific variables by name. class VariableStore { public: - explicit VariableStore() = default; + explicit VariableStore(VariableReferenceStorage &storage, bool is_permanent, + bool is_internal) + : m_storage(storage), m_is_permanent(is_permanent), + m_is_internal(is_internal) {} virtual ~VariableStore() = default; virtual llvm::Expected> - GetVariables(VariableReferenceStorage &storage, - const protocol::Configuration &config, - const protocol::VariablesArguments &args) = 0; + GetVariables(const protocol::VariablesArguments &args) = 0; virtual lldb::SBValue FindVariable(llvm::StringRef name) = 0; - virtual lldb::SBValue GetVariable() const = 0; + virtual lldb::SBValue GetVariable() const { return {}; } // Not copyable. VariableStore(const VariableStore &) = delete; VariableStore &operator=(const VariableStore &) = delete; - VariableStore(VariableStore &&) = default; - VariableStore &operator=(VariableStore &&) = default; + VariableStore(VariableStore &&) = delete; + VariableStore &operator=(VariableStore &&) = delete; + +protected: + VariableReferenceStorage &m_storage; + bool m_is_permanent; + bool m_is_internal; }; struct VariableReferenceStorage { - explicit VariableReferenceStorage(Log &log) : log(log) {} + explicit VariableReferenceStorage(Log &log, protocol::Configuration &config) + : log(log), config(config) {} /// \return a new variableReference. /// Specify is_permanent as true for variable that should persist entire /// debug session. @@ -78,7 +69,8 @@ struct VariableReferenceStorage { /// Insert a new \p variable. /// \return variableReference assigned to this expandable variable. - var_ref_t Insert(const lldb::SBValue &variable, bool is_permanent); + var_ref_t Insert(const lldb::SBValue &variable, bool is_permanent, + bool is_internal); /// Insert a value list. Used to store references to lldb repl command /// outputs. @@ -93,6 +85,7 @@ struct VariableReferenceStorage { VariableStore *GetVariableStore(var_ref_t var_ref); Log &log; + protocol::Configuration &config; private: /// Template class for managing pools of variable stores. diff --git a/lldb/unittests/DAP/VariablesTest.cpp b/lldb/unittests/DAP/VariablesTest.cpp index fe85e92a7fee..7a27bf0445f3 100644 --- a/lldb/unittests/DAP/VariablesTest.cpp +++ b/lldb/unittests/DAP/VariablesTest.cpp @@ -27,7 +27,7 @@ using namespace lldb_dap::protocol; class VariablesTest : public ::testing::Test { public: - VariablesTest() : log(llvm::nulls(), mutex), vars(log) {} + VariablesTest() : log(llvm::nulls(), mutex), vars(log, config) {} static void SetUpTestSuite() { lldb::SBError error = SBDebugger::InitializeWithErrorHandling(); @@ -48,6 +48,7 @@ protected: enum : bool { Permanent = true, Temporary = false }; Log::Mutex mutex; Log log; + protocol::Configuration config; VariableReferenceStorage vars; lldb::SBDebugger debugger; lldb::SBTarget target; @@ -104,10 +105,10 @@ TEST_F(VariablesTest, GetNewVariableReference_UniqueAndRanges) { auto y42 = target.CreateValueFromExpression("y", "42"); auto gzero = target.CreateValueFromExpression("$0", "42"); auto gone = target.CreateValueFromExpression("$1", "7"); - const var_ref_t temp1 = vars.Insert(x15, Temporary); - const var_ref_t temp2 = vars.Insert(y42, Temporary); - const var_ref_t perm1 = vars.Insert(gzero, Permanent); - const var_ref_t perm2 = vars.Insert(gone, Permanent); + const var_ref_t temp1 = vars.Insert(x15, Temporary, /*is_internal=*/false); + const var_ref_t temp2 = vars.Insert(y42, Temporary, /*is_internal=*/false); + const var_ref_t perm1 = vars.Insert(gzero, Permanent, /*is_internal=*/false); + const var_ref_t perm2 = vars.Insert(gone, Permanent, /*is_internal=*/false); EXPECT_NE(temp1.AsUInt32(), temp2.AsUInt32()); EXPECT_NE(perm1.AsUInt32(), perm2.AsUInt32()); EXPECT_LT(temp1.AsUInt32(), perm1.AsUInt32()); @@ -116,7 +117,7 @@ TEST_F(VariablesTest, GetNewVariableReference_UniqueAndRanges) { TEST_F(VariablesTest, InsertAndGetVariable_Temporary) { lldb::SBValue dummy; - const var_ref_t ref = vars.Insert(dummy, Temporary); + const var_ref_t ref = vars.Insert(dummy, Temporary, /*is_internal=*/false); lldb::SBValue out = vars.GetVariable(ref); EXPECT_EQ(out.IsValid(), dummy.IsValid()); @@ -124,15 +125,17 @@ TEST_F(VariablesTest, InsertAndGetVariable_Temporary) { TEST_F(VariablesTest, InsertAndGetVariable_Permanent) { lldb::SBValue dummy; - const var_ref_t ref = vars.Insert(dummy, Permanent); + const var_ref_t ref = vars.Insert(dummy, Permanent, /*is_internal=*/false); lldb::SBValue out = vars.GetVariable(ref); EXPECT_EQ(out.IsValid(), dummy.IsValid()); } TEST_F(VariablesTest, IsPermanentVariableReference) { - const var_ref_t perm = vars.Insert(lldb::SBValue(), Permanent); - const var_ref_t temp = vars.Insert(lldb::SBValue(), Temporary); + const var_ref_t perm = + vars.Insert(lldb::SBValue(), Permanent, /*is_internal=*/false); + const var_ref_t temp = + vars.Insert(lldb::SBValue(), Temporary, /*is_internal=*/false); EXPECT_EQ(perm.Kind(), eReferenceKindPermanent); EXPECT_EQ(temp.Kind(), eReferenceKindTemporary); @@ -140,8 +143,8 @@ TEST_F(VariablesTest, IsPermanentVariableReference) { TEST_F(VariablesTest, Clear_RemovesTemporaryKeepsPermanent) { lldb::SBValue dummy; - const var_ref_t temp = vars.Insert(dummy, Temporary); - const var_ref_t perm = vars.Insert(dummy, Permanent); + const var_ref_t temp = vars.Insert(dummy, Temporary, /*is_internal=*/false); + const var_ref_t perm = vars.Insert(dummy, Permanent, /*is_internal=*/false); vars.Clear(); EXPECT_FALSE(vars.GetVariable(temp).IsValid()); @@ -184,7 +187,7 @@ TEST_F(VariablesTest, VariablesStore) { ASSERT_TRUE(vars.FindVariable(local_ref, "rect").IsValid()); - auto variables = locals_store->GetVariables(vars, {}, {}); + auto variables = locals_store->GetVariables({}); ASSERT_THAT_EXPECTED(variables, Succeeded()); ASSERT_EQ(variables->size(), 1u); auto rect = variables->at(0); @@ -196,7 +199,7 @@ TEST_F(VariablesTest, VariablesStore) { auto *store = vars.GetVariableStore(args.variablesReference); ASSERT_NE(store, nullptr); - variables = store->GetVariables(vars, {}, args); + variables = store->GetVariables(args); ASSERT_THAT_EXPECTED(variables, Succeeded()); ASSERT_EQ(variables->size(), 4u); EXPECT_EQ(variables->at(0).name, "x");