lldb-dap: Stop using replicated variable ids

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

@@ -351,11 +351,40 @@ 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,24 +341,25 @@ 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(local_scope_ref, "x2", 9)["success"])
         self.assertFalse(
-            self.dap_server.request_setVariable(1, "x @ main.cpp:0", 9)["success"]
+            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")[
+            self.dap_server.request_setVariable(local_scope_ref, "x @ main.cpp:23", "invalid")[
                 "success"
             ]
         )
@@ -370,7 +371,7 @@ 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 +710,7 @@ 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 +832,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/EvaluateRequestHandler.cpp

@@ -162,6 +162,7 @@ void EvaluateRequestHandler::operator()(
     // focus_tid to the current frame for any thread related events.
     if (frame.IsValid()) {
       dap.focus_tid = frame.GetThread().GetThreadID();
+      dap.variables.SwitchFrame(frame.GetFrameID());
     }
 
     bool required_command_failed = false;

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

@@ -29,8 +29,8 @@ namespace lldb_dap {
 ///
 /// \return
 ///     A `protocol::Scope`
-static Scope CreateScope(const llvm::StringRef name, int64_t variablesReference,
-                         int64_t namedVariables, bool expensive) {
+Scope CreateScope(const llvm::StringRef name, int64_t variablesReference,
+                  int64_t namedVariables, bool expensive) {
   Scope scope;
   scope.name = name;
 
@@ -41,9 +41,9 @@ 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)
+  if (name == "Locals")
     scope.presentationHint = Scope::eScopePresentationHintLocals;
-  else if (variablesReference == VARREF_REGS)
+  else if (name == "Registers")
     scope.presentationHint = Scope::eScopePresentationHintRegisters;
 
   scope.variablesReference = variablesReference;
@@ -75,22 +75,31 @@ 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);
+  dap.variables.SwitchFrame(frame_id);
+
+  std::vector<Scope> scopes = {};
+
+  int64_t variable_reference = dap.variables.GetNewVariableReference(false);
+  scopes.push_back(CreateScope("Locals", variable_reference,
+                               dap.variables.locals.GetSize(), false));
+
+  dap.variables.AddScopeKind(variable_reference, ScopeKind::Locals, frame_id);
+
+  variable_reference = dap.variables.GetNewVariableReference(false);
+  scopes.push_back(CreateScope("Globals", variable_reference,
+                               dap.variables.globals.GetSize(), false));
+  dap.variables.AddScopeKind(variable_reference, ScopeKind::Globals, frame_id);
+
+  variable_reference = dap.variables.GetNewVariableReference(false);
+  scopes.push_back(CreateScope("Registers", variable_reference,
+                               dap.variables.registers.GetSize(), false));
+
+  dap.variables.AddScopeKind(variable_reference, ScopeKind::Registers,
+                             frame_id);
 
   return ScopesResponseBody{std::move(scopes)};
 }

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

@@ -38,7 +38,8 @@ VariablesRequestHandler::Run(const VariablesArguments &arguments) const {
     int64_t start_idx = 0;
     int64_t num_children = 0;
 
-    if (var_ref == VARREF_REGS) {
+    std::optional<ScopeKind> scope_kind = dap.variables.GetScopeKind(var_ref);
+    if (scope_kind && *scope_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
@@ -58,7 +59,7 @@ VariablesRequestHandler::Run(const VariablesArguments &arguments) const {
     }
 
     num_children = top_scope->GetSize();
-    if (num_children == 0 && var_ref == VARREF_LOCALS) {
+    if (num_children == 0 && scope_kind && *scope_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 +95,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_kind && *scope_kind == ScopeKind::Locals) {
       auto process = dap.target.GetProcess();
       auto selected_thread = process.GetSelectedThread();
       lldb::SBValue stop_return_value = selected_thread.GetStopReturnValue();

lldb/tools/lldb-dap/JSONUtils.cpp

@@ -11,6 +11,7 @@
 #include "ExceptionBreakpoint.h"
 #include "LLDBUtils.h"
 #include "ProtocolUtils.h"
+#include "Variables.h"
 #include "lldb/API/SBAddress.h"
 #include "lldb/API/SBCompileUnit.h"
 #include "lldb/API/SBDeclaration.h"
@@ -292,91 +293,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",

lldb/tools/lldb-dap/JSONUtils.h

@@ -9,6 +9,7 @@
 #ifndef LLDB_TOOLS_LLDB_DAP_JSONUTILS_H
 #define LLDB_TOOLS_LLDB_DAP_JSONUTILS_H
 
+#include "DAP.h"
 #include "DAPForward.h"
 #include "Protocol/ProtocolTypes.h"
 #include "lldb/API/SBCompileUnit.h"