lldb-dap: Stop using replicated variable ids

lldb/packages/Python/lldbsuite/test/tools/lldb-dap/lldbdap_testcase.py

@@ -351,11 +351,41 @@ def get_local_as_int(self, name, threadId=None):
 
     def set_local(self, name, value, id=None):
         """Set a top level local variable only."""
-        return self.dap_server.request_setVariable(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.dap_server.request_setVariable(locals_ref, name, str(value), id=id)
 
     def set_global(self, name, value, id=None):
         """Set a top level global variable only."""
-        return self.dap_server.request_setVariable(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.dap_server.request_setVariable(
+                    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,

lldb/test/API/tools/lldb-dap/databreakpoint/TestDAP_setDataBreakpoints.py

@@ -106,8 +106,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]"

lldb/test/API/tools/lldb-dap/memory/TestDAP_memory.py

@@ -70,9 +70,11 @@ def test_memory_refs_set_variable(self):
         self.continue_to_next_stop()
 
         ptr_value = self.get_local_as_int("rawptr")
+        locals_ref = self.get_locals_scope_reference()
+        self.assertIsNotNone(locals_ref, "Failed to get locals scope reference")
         self.assertIn(
             "memoryReference",
-            self.dap_server.request_setVariable(1, "rawptr", ptr_value + 2)[
+            self.dap_server.request_setVariable(locals_ref, "rawptr", ptr_value + 2)[
                 "body"
             ].keys(),
         )

lldb/test/API/tools/lldb-dap/variables/TestDAP_variables.py

@@ -341,26 +341,37 @@ def do_test_scopes_variables_setVariable_evaluate(
         self.verify_variables(verify_locals, self.dap_server.get_local_variables())
 
         # Now we verify that we correctly change the name of a variable with and without differentiator suffix
-        self.assertFalse(self.dap_server.request_setVariable(1, "x2", 9)["success"])
+        local_scope_ref = self.get_locals_scope_reference()
         self.assertFalse(
-            self.dap_server.request_setVariable(1, "x @ main.cpp:0", 9)["success"]
+            self.dap_server.request_setVariable(local_scope_ref, "x2", 9)["success"]
+        )
+        self.assertFalse(
+            self.dap_server.request_setVariable(local_scope_ref, "x @ main.cpp:0", 9)[
+                "success"
+            ]
         )
 
         self.assertTrue(
-            self.dap_server.request_setVariable(1, "x @ main.cpp:19", 19)["success"]
+            self.dap_server.request_setVariable(local_scope_ref, "x @ main.cpp:19", 19)[
+                "success"
+            ]
         )
         self.assertTrue(
-            self.dap_server.request_setVariable(1, "x @ main.cpp:21", 21)["success"]
+            self.dap_server.request_setVariable(local_scope_ref, "x @ main.cpp:21", 21)[
+                "success"
+            ]
         )
         self.assertTrue(
-            self.dap_server.request_setVariable(1, "x @ main.cpp:23", 23)["success"]
+            self.dap_server.request_setVariable(local_scope_ref, "x @ main.cpp:23", 23)[
+                "success"
+            ]
         )
 
         # The following should have no effect
         self.assertFalse(
-            self.dap_server.request_setVariable(1, "x @ main.cpp:23", "invalid")[
-                "success"
-            ]
+            self.dap_server.request_setVariable(
+                local_scope_ref, "x @ main.cpp:23", "invalid"
+            )["success"]
         )
 
         verify_locals["x @ main.cpp:19"]["equals"]["value"] = "19"
@@ -370,7 +381,9 @@ def do_test_scopes_variables_setVariable_evaluate(
         self.verify_variables(verify_locals, self.dap_server.get_local_variables())
 
         # The plain x variable shold refer to the innermost x
-        self.assertTrue(self.dap_server.request_setVariable(1, "x", 22)["success"])
+        self.assertTrue(
+            self.dap_server.request_setVariable(local_scope_ref, "x", 22)["success"]
+        )
         verify_locals["x @ main.cpp:23"]["equals"]["value"] = "22"
 
         self.verify_variables(verify_locals, self.dap_server.get_local_variables())
@@ -709,7 +722,9 @@ def test_return_variables(self):
                 break
 
         self.assertFalse(
-            self.dap_server.request_setVariable(1, "(Return Value)", 20)["success"]
+            self.dap_server.request_setVariable(
+                self.get_locals_scope_reference(), "(Return Value)", 20
+            )["success"]
         )
 
     @skipIfWindows
@@ -831,3 +846,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}")

lldb/tools/lldb-dap/Handler/ScopesRequestHandler.cpp

@@ -8,6 +8,7 @@
 
 #include "DAP.h"
 #include "RequestHandler.h"
+#include "Variables.h"
 
 using namespace lldb_dap::protocol;
 namespace lldb_dap {
@@ -29,10 +30,9 @@ namespace lldb_dap {
 ///
 /// \return
 ///     A `protocol::Scope`
-static Scope CreateScope(const llvm::StringRef name, int64_t variablesReference,
+static Scope CreateScope(const ScopeKind kind, 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
@@ -41,10 +41,19 @@ static Scope CreateScope(const llvm::StringRef name, int64_t variablesReference,
   // 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)
+  switch (kind) {
+  case ScopeKind::Locals:
     scope.presentationHint = Scope::eScopePresentationHintLocals;
-  else if (variablesReference == VARREF_REGS)
+    scope.name = "Locals";
+    break;
+  case ScopeKind::Globals:
+    scope.name = "Globals";
+    break;
+  case ScopeKind::Registers:
     scope.presentationHint = Scope::eScopePresentationHintRegisters;
+    scope.name = "Registers";
+    break;
+  }
 
   scope.variablesReference = variablesReference;
   scope.namedVariables = namedVariables;
@@ -75,22 +84,34 @@ 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)};
+
+  uint32_t frame_id = frame.GetFrameID();
+
+  dap.variables.ReadyFrame(frame_id, frame);
+
+  std::vector<protocol::Scope> scopes = {};
+
+  int64_t variable_reference = dap.variables.GetNewVariableReference(false);
+  scopes.push_back(CreateScope(
+      ScopeKind::Locals, variable_reference,
+      dap.variables.GetScope(frame_id, ScopeKind::Locals)->GetSize(), false));
+
+  dap.variables.AddScopeKind(variable_reference, ScopeKind::Locals, frame_id);
+
+  variable_reference = dap.variables.GetNewVariableReference(false);
+  scopes.push_back(CreateScope(
+      ScopeKind::Globals, variable_reference,
+      dap.variables.GetScope(frame_id, ScopeKind::Globals)->GetSize(), false));
+  dap.variables.AddScopeKind(variable_reference, ScopeKind::Globals, frame_id);
+
+  variable_reference = dap.variables.GetNewVariableReference(false);
+  scopes.push_back(CreateScope(
+      ScopeKind::Registers, variable_reference,
+      dap.variables.GetScope(frame_id, ScopeKind::Registers)->GetSize(),
+      false));
+
+  dap.variables.AddScopeKind(variable_reference, ScopeKind::Registers,
+                             frame_id);
 
   return ScopesResponseBody{std::move(scopes)};
 }

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;
@@ -38,14 +39,16 @@ VariablesRequestHandler::Run(const VariablesArguments &arguments) const {
     int64_t start_idx = 0;
     int64_t num_children = 0;
 
-    if (var_ref == VARREF_REGS) {
+    std::optional<ScopeData> scope_data = dap.variables.GetScopeKind(var_ref);
+    if (scope_data.has_value() && scope_data->kind == ScopeKind::Registers) {
+
       // 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);
@@ -58,7 +61,8 @@ VariablesRequestHandler::Run(const VariablesArguments &arguments) const {
     }
 
     num_children = top_scope->GetSize();
-    if (num_children == 0 && var_ref == VARREF_LOCALS) {
+    if (num_children == 0 && scope_data &&
+        scope_data->kind == ScopeKind::Locals) {
       // Check for an error in the SBValueList that might explain why we don't
       // have locals. If we have an error display it as the sole value in the
       // the locals.
@@ -94,7 +98,7 @@ VariablesRequestHandler::Run(const VariablesArguments &arguments) const {
     }
 
     // Show return value if there is any ( in the local top frame )
-    if (var_ref == VARREF_LOCALS) {
+    if (scope_data && scope_data->kind == ScopeKind::Locals) {
       auto process = dap.target.GetProcess();
       auto selected_thread = process.GetSelectedThread();
       lldb::SBValue stop_return_value = selected_thread.GetStopReturnValue();