llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-lldb Author: None (llvmbot) <details> <summary>Changes</summary> Backport 6ca8f79fc88971412e6e5692ea86d680cd438fe3 Requested by: @<!-- -->da-viper --- Patch is 29.84 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/180031.diff 10 Files Affected: - (modified) lldb/packages/Python/lldbsuite/test/tools/lldb-dap/lldbdap_testcase.py (+30-2) - (modified) lldb/test/API/tools/lldb-dap/databreakpoint/TestDAP_setDataBreakpoints.py (+3-1) - (modified) lldb/test/API/tools/lldb-dap/variables/TestDAP_variables.py (+36) - (modified) lldb/tools/lldb-dap/Handler/ScopesRequestHandler.cpp (+3-56) - (modified) lldb/tools/lldb-dap/Handler/VariablesRequestHandler.cpp (+12-9) - (modified) lldb/tools/lldb-dap/JSONUtils.cpp (-85) - (modified) lldb/tools/lldb-dap/SourceBreakpoint.cpp (-1) - (modified) lldb/tools/lldb-dap/Variables.cpp (+116-16) - (modified) lldb/tools/lldb-dap/Variables.h (+72-10) - (modified) lldb/unittests/DAP/VariablesTest.cpp (+47-10) ``````````diff diff --git a/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/lldbdap_testcase.py b/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/lldbdap_testcase.py index e4a3e1c786ffe..5dd2405a3389b 100644 --- a/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/lldbdap_testcase.py +++ b/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/lldbdap_testcase.py @@ -379,11 +379,39 @@ def set_variable(self, varRef, name, value, id=None): def set_local(self, name, value, id=None): """Set a top level local variable only.""" - return self.set_variable(1, name, str(value), id=id) + # Get the locals scope reference dynamically + locals_ref = self.get_locals_scope_reference() + if locals_ref is None: + return None + return self.set_variable(locals_ref, name, str(value), id=id) def set_global(self, name, value, id=None): """Set a top level global variable only.""" - return self.set_variable(2, name, str(value), id=id) + # Get the globals scope reference dynamically + stackFrame = self.dap_server.get_stackFrame() + if stackFrame is None: + return None + frameId = stackFrame["id"] + scopes_response = self.dap_server.request_scopes(frameId) + frame_scopes = scopes_response["body"]["scopes"] + for scope in frame_scopes: + if scope["name"] == "Globals": + varRef = scope["variablesReference"] + return self.set_variable(varRef, name, str(value), id=id) + return None + + def get_locals_scope_reference(self): + """Get the variablesReference for the locals scope.""" + stackFrame = self.dap_server.get_stackFrame() + if stackFrame is None: + return None + frameId = stackFrame["id"] + scopes_response = self.dap_server.request_scopes(frameId) + frame_scopes = scopes_response["body"]["scopes"] + for scope in frame_scopes: + if scope["name"] == "Locals": + return scope["variablesReference"] + return None def stepIn( self, diff --git a/lldb/test/API/tools/lldb-dap/databreakpoint/TestDAP_setDataBreakpoints.py b/lldb/test/API/tools/lldb-dap/databreakpoint/TestDAP_setDataBreakpoints.py index df029ca16d667..0c8ba4e5d07af 100644 --- a/lldb/test/API/tools/lldb-dap/databreakpoint/TestDAP_setDataBreakpoints.py +++ b/lldb/test/API/tools/lldb-dap/databreakpoint/TestDAP_setDataBreakpoints.py @@ -111,8 +111,10 @@ def test_functionality(self): self.set_source_breakpoints(source, [first_loop_break_line]) self.continue_to_next_stop() self.dap_server.get_local_variables() + locals_ref = self.get_locals_scope_reference() + self.assertIsNotNone(locals_ref, "Failed to get locals scope reference") # Test write watchpoints on x, arr[2] - response_x = self.dap_server.request_dataBreakpointInfo(1, "x") + response_x = self.dap_server.request_dataBreakpointInfo(locals_ref, "x") arr = self.dap_server.get_local_variable("arr") response_arr_2 = self.dap_server.request_dataBreakpointInfo( arr["variablesReference"], "[2]" 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 05445af40aea5..1dbb0143e7a55 100644 --- a/lldb/test/API/tools/lldb-dap/variables/TestDAP_variables.py +++ b/lldb/test/API/tools/lldb-dap/variables/TestDAP_variables.py @@ -834,3 +834,39 @@ def test_value_format(self): self.assertEqual(var_pt_x["value"], "11") var_pt_y = self.dap_server.get_local_variable_child("pt", "y", is_hex=is_hex) self.assertEqual(var_pt_y["value"], "22") + + @skipIfWindows + def test_variable_id_uniqueness_simple(self): + """ + Simple regression test for variable ID uniqueness across frames. + Ensures variable IDs are not reused between different scopes/frames. + """ + program = self.getBuildArtifact("a.out") + self.build_and_launch(program) + source = "main.cpp" + + bp_line = line_number(source, "// breakpoint 3") + self.set_source_breakpoints(source, [bp_line]) + self.continue_to_next_stop() + + frames = self.get_stackFrames() + self.assertGreaterEqual(len(frames), 2, "Need at least 2 frames") + + all_refs = set() + + for i in range(min(3, len(frames))): + frame_id = frames[i]["id"] + scopes = self.dap_server.request_scopes(frame_id)["body"]["scopes"] + + for scope in scopes: + ref = scope["variablesReference"] + if ref != 0: + self.assertNotIn( + ref, all_refs, f"Variable reference {ref} was reused!" + ) + all_refs.add(ref) + + self.assertGreater(len(all_refs), 0, "Should have found variable references") + for ref in all_refs: + response = self.dap_server.request_variables(ref) + self.assertTrue(response["success"], f"Failed to access reference {ref}") diff --git a/lldb/tools/lldb-dap/Handler/ScopesRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/ScopesRequestHandler.cpp index aaad0e20f9c21..251711ca62406 100644 --- a/lldb/tools/lldb-dap/Handler/ScopesRequestHandler.cpp +++ b/lldb/tools/lldb-dap/Handler/ScopesRequestHandler.cpp @@ -8,51 +8,11 @@ #include "DAP.h" #include "RequestHandler.h" +#include "Variables.h" using namespace lldb_dap::protocol; namespace lldb_dap { -/// Creates a `protocol::Scope` struct. -/// -/// -/// \param[in] name -/// The value to place into the "name" key -/// -/// \param[in] variablesReference -/// The value to place into the "variablesReference" key -/// -/// \param[in] namedVariables -/// The value to place into the "namedVariables" key -/// -/// \param[in] expensive -/// The value to place into the "expensive" key -/// -/// \return -/// A `protocol::Scope` -static Scope CreateScope(const llvm::StringRef name, int64_t variablesReference, - int64_t namedVariables, bool expensive) { - Scope scope; - scope.name = name; - - // TODO: Support "arguments" and "return value" scope. - // At the moment lldb-dap includes the arguments and return_value into the - // "locals" scope. - // vscode only expands the first non-expensive scope, this causes friction - // if we add the arguments above the local scope as the locals scope will not - // be expanded if we enter a function with arguments. It becomes more - // annoying when the scope has arguments, return_value and locals. - if (variablesReference == VARREF_LOCALS) - scope.presentationHint = Scope::eScopePresentationHintLocals; - else if (variablesReference == VARREF_REGS) - scope.presentationHint = Scope::eScopePresentationHintRegisters; - - scope.variablesReference = variablesReference; - scope.namedVariables = namedVariables; - scope.expensive = expensive; - - return scope; -} - llvm::Expected<ScopesResponseBody> ScopesRequestHandler::Run(const ScopesArguments &args) const { lldb::SBFrame frame = dap.GetLLDBFrame(args.frameId); @@ -75,22 +35,9 @@ ScopesRequestHandler::Run(const ScopesArguments &args) const { frame.GetThread().GetProcess().SetSelectedThread(frame.GetThread()); frame.GetThread().SetSelectedFrame(frame.GetFrameID()); } - dap.variables.locals = frame.GetVariables(/*arguments=*/true, - /*locals=*/true, - /*statics=*/false, - /*in_scope_only=*/true); - dap.variables.globals = frame.GetVariables(/*arguments=*/false, - /*locals=*/false, - /*statics=*/true, - /*in_scope_only=*/true); - dap.variables.registers = frame.GetRegisters(); - std::vector scopes = {CreateScope("Locals", VARREF_LOCALS, - dap.variables.locals.GetSize(), false), - CreateScope("Globals", VARREF_GLOBALS, - dap.variables.globals.GetSize(), false), - CreateScope("Registers", VARREF_REGS, - dap.variables.registers.GetSize(), false)}; + std::vector<protocol::Scope> scopes = + dap.variables.CreateScopes(args.frameId, frame); return ScopesResponseBody{std::move(scopes)}; } diff --git a/lldb/tools/lldb-dap/Handler/VariablesRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/VariablesRequestHandler.cpp index 0177df8b754ab..ee7cb525d9c29 100644 --- a/lldb/tools/lldb-dap/Handler/VariablesRequestHandler.cpp +++ b/lldb/tools/lldb-dap/Handler/VariablesRequestHandler.cpp @@ -11,6 +11,7 @@ #include "Handler/RequestHandler.h" #include "JSONUtils.h" #include "ProtocolUtils.h" +#include "Variables.h" using namespace llvm; using namespace lldb_dap::protocol; @@ -30,20 +31,22 @@ VariablesRequestHandler::Run(const VariablesArguments &arguments) const { std::vector<Variable> variables; - if (lldb::SBValueList *top_scope = dap.variables.GetTopLevelScope(var_ref)) { + std::optional<ScopeData> scope_data = dap.variables.GetTopLevelScope(var_ref); + if (scope_data) { // 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 (var_ref == VARREF_REGS) { + if (scope_data->kind == eScopeKindRegisters) { + // 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 // format was set to the default format or if it was hex as some registers // have formats set for them. const uint32_t addr_size = dap.target.GetProcess().GetAddressByteSize(); - lldb::SBValue reg_set = dap.variables.registers.GetValueAtIndex(0); + lldb::SBValue reg_set = scope_data->scope.GetValueAtIndex(0); const uint32_t num_regs = reg_set.GetNumChildren(); for (uint32_t reg_idx = 0; reg_idx < num_regs; ++reg_idx) { lldb::SBValue reg = reg_set.GetChildAtIndex(reg_idx); @@ -55,15 +58,15 @@ VariablesRequestHandler::Run(const VariablesArguments &arguments) const { } } - num_children = top_scope->GetSize(); - if (num_children == 0 && var_ref == VARREF_LOCALS) { + num_children = scope_data->scope.GetSize(); + if (num_children == 0 && scope_data->kind == eScopeKindLocals) { // 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. // "error" owns the error string so we must keep it alive as long as we // want to use the returns "const char *" - lldb::SBError error = top_scope->GetError(); + lldb::SBError error = scope_data->scope.GetError(); const char *var_err = error.GetCString(); if (var_err) { // Create a fake variable named "error" to explain why variables were @@ -85,14 +88,14 @@ VariablesRequestHandler::Run(const VariablesArguments &arguments) const { // We first find out which variable names are duplicated std::map<llvm::StringRef, int> variable_name_counts; for (auto i = start_idx; i < end_idx; ++i) { - lldb::SBValue variable = top_scope->GetValueAtIndex(i); + lldb::SBValue variable = scope_data->scope.GetValueAtIndex(i); if (!variable.IsValid()) break; variable_name_counts[GetNonNullVariableName(variable)]++; } // Show return value if there is any ( in the local top frame ) - if (var_ref == VARREF_LOCALS) { + if (scope_data && scope_data->kind == eScopeKindLocals) { auto process = dap.target.GetProcess(); auto selected_thread = process.GetSelectedThread(); lldb::SBValue stop_return_value = selected_thread.GetStopReturnValue(); @@ -116,7 +119,7 @@ VariablesRequestHandler::Run(const VariablesArguments &arguments) const { // Now we construct the result with unique display variable names for (auto i = start_idx; i < end_idx; ++i) { - lldb::SBValue variable = top_scope->GetValueAtIndex(i); + lldb::SBValue variable = scope_data->scope.GetValueAtIndex(i); if (!variable.IsValid()) break; diff --git a/lldb/tools/lldb-dap/JSONUtils.cpp b/lldb/tools/lldb-dap/JSONUtils.cpp index 8851af65d1c4f..530927a4a9ef5 100644 --- a/lldb/tools/lldb-dap/JSONUtils.cpp +++ b/lldb/tools/lldb-dap/JSONUtils.cpp @@ -299,91 +299,6 @@ void FillResponse(const llvm::json::Object &request, response.try_emplace("success", true); } -// "Scope": { -// "type": "object", -// "description": "A Scope is a named container for variables. Optionally -// a scope can map to a source or a range within a source.", -// "properties": { -// "name": { -// "type": "string", -// "description": "Name of the scope such as 'Arguments', 'Locals'." -// }, -// "presentationHint": { -// "type": "string", -// "description": "An optional hint for how to present this scope in the -// UI. If this attribute is missing, the scope is shown -// with a generic UI.", -// "_enum": [ "arguments", "locals", "registers" ], -// }, -// "variablesReference": { -// "type": "integer", -// "description": "The variables of this scope can be retrieved by -// passing the value of variablesReference to the -// VariablesRequest." -// }, -// "namedVariables": { -// "type": "integer", -// "description": "The number of named variables in this scope. The -// client can use this optional information to present -// the variables in a paged UI and fetch them in chunks." -// }, -// "indexedVariables": { -// "type": "integer", -// "description": "The number of indexed variables in this scope. The -// client can use this optional information to present -// the variables in a paged UI and fetch them in chunks." -// }, -// "expensive": { -// "type": "boolean", -// "description": "If true, the number of variables in this scope is -// large or expensive to retrieve." -// }, -// "source": { -// "$ref": "#/definitions/Source", -// "description": "Optional source for this scope." -// }, -// "line": { -// "type": "integer", -// "description": "Optional start line of the range covered by this -// scope." -// }, -// "column": { -// "type": "integer", -// "description": "Optional start column of the range covered by this -// scope." -// }, -// "endLine": { -// "type": "integer", -// "description": "Optional end line of the range covered by this scope." -// }, -// "endColumn": { -// "type": "integer", -// "description": "Optional end column of the range covered by this -// scope." -// } -// }, -// "required": [ "name", "variablesReference", "expensive" ] -// } -llvm::json::Value CreateScope(const llvm::StringRef name, - int64_t variablesReference, - int64_t namedVariables, bool expensive) { - llvm::json::Object object; - EmplaceSafeString(object, "name", name.str()); - - // TODO: Support "arguments" scope. At the moment lldb-dap includes the - // arguments into the "locals" scope. - if (variablesReference == VARREF_LOCALS) { - object.try_emplace("presentationHint", "locals"); - } else if (variablesReference == VARREF_REGS) { - object.try_emplace("presentationHint", "registers"); - } - - object.try_emplace("variablesReference", variablesReference); - object.try_emplace("expensive", expensive); - object.try_emplace("namedVariables", namedVariables); - return llvm::json::Value(std::move(object)); -} - // "Event": { // "allOf": [ { "$ref": "#/definitions/ProtocolMessage" }, { // "type": "object", diff --git a/lldb/tools/lldb-dap/SourceBreakpoint.cpp b/lldb/tools/lldb-dap/SourceBreakpoint.cpp index 843a5eb09c7ae..6f776f4d5f429 100644 --- a/lldb/tools/lldb-dap/SourceBreakpoint.cpp +++ b/lldb/tools/lldb-dap/SourceBreakpoint.cpp @@ -10,7 +10,6 @@ #include "BreakpointBase.h" #include "DAP.h" #include "JSONUtils.h" -#include "ProtocolUtils.h" #include "lldb/API/SBBreakpoint.h" #include "lldb/API/SBFileSpec.h" #include "lldb/API/SBFileSpecList.h" diff --git a/lldb/tools/lldb-dap/Variables.cpp b/lldb/tools/lldb-dap/Variables.cpp index 777e3183d8c0d..6d49113243799 100644 --- a/lldb/tools/lldb-dap/Variables.cpp +++ b/lldb/tools/lldb-dap/Variables.cpp @@ -8,27 +8,77 @@ #include "Variables.h" #include "JSONUtils.h" +#include "Protocol/ProtocolTypes.h" +#include "lldb/API/SBFrame.h" +#include "lldb/API/SBValue.h" +#include "lldb/API/SBValueList.h" +#include <cstdint> +#include <optional> +#include <vector> using namespace lldb_dap; -lldb::SBValueList *Variables::GetTopLevelScope(int64_t variablesReference) { - switch (variablesReference) { - case VARREF_LOCALS: - return &locals; - case VARREF_GLOBALS: - return &globals; - case VARREF_REGS: - return ®isters; - default: - return nullptr; +namespace lldb_dap { + +protocol::Scope CreateScope(const ScopeKind kind, int64_t variablesReference, + int64_t namedVariables, bool expensive) { + protocol::Scope scope; + scope.variablesReference = variablesReference; + scope.namedVariables = namedVariables; + scope.expensive = expensive; + + // TODO: Support "arguments" and "return value" scope. + // At the moment lldb-dap includes the arguments and return_value into the + // "locals" scope. + // VS Code only expands the first non-expensive scope. This causes friction + // if we add the arguments above the local scope, as the locals scope will not + // be expanded if we enter a function with arguments. It becomes more + // annoying when the scope has arguments, return_value and locals. + 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; +} + +std::optional<ScopeData> +Variables::GetTopLevelScope(int64_t variablesReference) { + auto scope_kind_iter = m_scope_kinds.find(variablesReference); + if (scope_kind_iter == m_scope_kinds.end()) + return std::nullopt; + + ScopeKind scope_kind = scope_kind_iter->second.first; + uint64_t dap_frame_id = scope_kind_iter->second.second; + + auto frame_iter = m_frames.find(dap_frame_id); + if (frame_iter == m_frames.end()) + return std::nullopt; + + lldb::SBValueList *scope = frame_iter->second.GetScope(scope_kind); + if (scope == nullptr) + return std::nullopt; + + ScopeData scope_data; + scope_data.kind = scope_kind; + scope_data.scope = *scope; + return scope_data; } void Variables::Clear() { - locals.Clear(); - globals.Clear(); - registers.Clear(); m_referencedvariables.clear(); + m_scope_kinds.clear(); + m_frames.clear(); + m_next_temporary_var_ref = TemporaryVariableStartIndex; } int64_t Variables::GetNewVariableReference(bool is_permanent) { @@ -66,15 +116,16 @@ int64_t Variables::InsertVariable(lldb::SBValue variable, bool is_permanent) { lldb::SBValue Variables::FindVariable(uint64_t variablesReference, llvm::StringRef name) { lldb::SBValue variable; - if (lldb::SBValueList *top_scope = GetTopLevelScope(variablesReference)) { + if (std::optional<ScopeData> scope_data = + GetTopLevelScope(variablesReference)) { bool is_duplicated_variable_name = name.contains(" @"); // variablesReference is one ... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/180031 _______________________________________________ llvm-branch-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
