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,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,10 @@ 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();
+  std::vector<protocol::Scope> scopes =
+      dap.variables.ReadyFrame(frame_id, frame);
 
   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();

lldb/tools/lldb-dap/JSONUtils.cpp

@@ -292,91 +292,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",