[lldb-dap] Provide declarationLocation for variables (llvm#102928)

vogelsgesang · JDevlieghere · commit b9a721200c6a · 2024-10-16T15:51:36.000-07:00
This commit implements support for the "declaration location" recently added by microsoft/debug-adapter-protocol#494 to the debug adapter protocol. For the `declarationLocationReference` we need a variable ID similar to the `variablesReference`. I decided to simply reuse the `variablesReference` here and renamed `Variables::expandable_variables` and friends accordingly. Given that almost all variables have a declaration location, we now assign those variable ids to all variables. While `declarationLocationReference` effectively supersedes `$__lldb_extensions.declaration`, I did not remove this extension, yet, since I assume that there are some closed-source extensions which rely on it. I tested this against VS-Code Insiders. However, VS-Code Insiders currently only supports `valueLoctionReference` and not `declarationLocationReference`, yet. Locally, I hence published the declaration locations as value locations, and VS Code Insiders navigated to the expected places. Looking forward to proper VS Code support for `declarationLocationReference`. (cherry picked from commit 0cc2cd7)
diff --git a/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py b/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py
@@ -1109,6 +1109,17 @@ def request_setVariable(self, containingVarRef, name, value, id=None):
         }
         return self.send_recv(command_dict)
 
+    def request_locations(self, locationReference):
+        args_dict = {
+            "locationReference": locationReference,
+        }
+        command_dict = {
+            "command": "locations",
+            "type": "request",
+            "arguments": args_dict,
+        }
+        return self.send_recv(command_dict)
+
     def request_testGetTargetBreakpoints(self):
         """A request packet used in the LLDB test suite to get all currently
         set breakpoint infos for all breakpoints currently set in the
diff --git a/lldb/test/API/tools/lldb-dap/locations/Makefile b/lldb/test/API/tools/lldb-dap/locations/Makefile
@@ -0,0 +1,3 @@
+C_SOURCES := main.c
+
+include Makefile.rules
diff --git a/lldb/test/API/tools/lldb-dap/locations/TestDAP_locations.py b/lldb/test/API/tools/lldb-dap/locations/TestDAP_locations.py
@@ -0,0 +1,40 @@
+"""
+Test lldb-dap locations request
+"""
+
+
+import dap_server
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+import lldbdap_testcase
+import os
+
+
+class TestDAP_locations(lldbdap_testcase.DAPTestCaseBase):
+    @skipIfWindows
+    def test_locations(self):
+        """
+        Tests the 'locations' request.
+        """
+        program = self.getBuildArtifact("a.out")
+        self.build_and_launch(program)
+        source = "main.c"
+        self.source_path = os.path.join(os.getcwd(), source)
+        self.set_source_breakpoints(
+            source,
+            [line_number(source, "// BREAK HERE")],
+        )
+        self.continue_to_next_stop()
+
+        locals = {l["name"]: l for l in self.dap_server.get_local_variables()}
+
+        # var1 has a declarationLocation but no valueLocation
+        self.assertIn("declarationLocationReference", locals["var1"].keys())
+        self.assertNotIn("valueLocationReference", locals["var1"].keys())
+        loc_var1 = self.dap_server.request_locations(
+            locals["var1"]["declarationLocationReference"]
+        )
+        self.assertTrue(loc_var1["success"])
+        self.assertTrue(loc_var1["body"]["source"]["path"].endswith("main.c"))
+        self.assertEqual(loc_var1["body"]["line"], 2)
diff --git a/lldb/test/API/tools/lldb-dap/locations/main.c b/lldb/test/API/tools/lldb-dap/locations/main.c
@@ -0,0 +1,5 @@
+int main(void) {
+  int var1 = 1;
+  // BREAK HERE
+  return 0;
+}
diff --git a/lldb/tools/lldb-dap/DAP.cpp b/lldb/tools/lldb-dap/DAP.cpp
@@ -824,7 +824,7 @@ void Variables::Clear() {
   locals.Clear();
   globals.Clear();
   registers.Clear();
-  expandable_variables.clear();
+  referenced_variables.clear();
 }
 
 int64_t Variables::GetNewVariableReference(bool is_permanent) {
@@ -839,24 +839,23 @@ bool Variables::IsPermanentVariableReference(int64_t var_ref) {
 
 lldb::SBValue Variables::GetVariable(int64_t var_ref) const {
   if (IsPermanentVariableReference(var_ref)) {
-    auto pos = expandable_permanent_variables.find(var_ref);
-    if (pos != expandable_permanent_variables.end())
+    auto pos = referenced_permanent_variables.find(var_ref);
+    if (pos != referenced_permanent_variables.end())
       return pos->second;
   } else {
-    auto pos = expandable_variables.find(var_ref);
-    if (pos != expandable_variables.end())
+    auto pos = referenced_variables.find(var_ref);
+    if (pos != referenced_variables.end())
       return pos->second;
   }
   return lldb::SBValue();
 }
 
-int64_t Variables::InsertExpandableVariable(lldb::SBValue variable,
-                                            bool is_permanent) {
+int64_t Variables::InsertVariable(lldb::SBValue variable, bool is_permanent) {
   int64_t var_ref = GetNewVariableReference(is_permanent);
   if (is_permanent)
-    expandable_permanent_variables.insert(std::make_pair(var_ref, variable));
+    referenced_permanent_variables.insert(std::make_pair(var_ref, variable));
   else
-    expandable_variables.insert(std::make_pair(var_ref, variable));
+    referenced_variables.insert(std::make_pair(var_ref, variable));
   return var_ref;
 }
 
diff --git a/lldb/tools/lldb-dap/DAP.h b/lldb/tools/lldb-dap/DAP.h
@@ -111,12 +111,12 @@ struct Variables {
   int64_t next_temporary_var_ref{VARREF_FIRST_VAR_IDX};
   int64_t next_permanent_var_ref{PermanentVariableStartIndex};
 
-  /// Expandable variables that are alive in this stop state.
+  /// Variables that are alive in this stop state.
   /// Will be cleared when debuggee resumes.
-  llvm::DenseMap<int64_t, lldb::SBValue> expandable_variables;
-  /// Expandable variables that persist across entire debug session.
+  llvm::DenseMap<int64_t, lldb::SBValue> referenced_variables;
+  /// Variables that persist across entire debug session.
   /// These are the variables evaluated from debug console REPL.
-  llvm::DenseMap<int64_t, lldb::SBValue> expandable_permanent_variables;
+  llvm::DenseMap<int64_t, lldb::SBValue> referenced_permanent_variables;
 
   /// Check if \p var_ref points to a variable that should persist for the
   /// entire duration of the debug session, e.g. repl expandable variables
@@ -134,7 +134,7 @@ struct Variables {
 
   /// Insert a new \p variable.
   /// \return variableReference assigned to this expandable variable.
-  int64_t InsertExpandableVariable(lldb::SBValue variable, bool is_permanent);
+  int64_t InsertVariable(lldb::SBValue variable, bool is_permanent);
 
   /// Clear all scope variables and non-permanent expandable variables.
   void Clear();
diff --git a/lldb/tools/lldb-dap/JSONUtils.cpp b/lldb/tools/lldb-dap/JSONUtils.cpp
@@ -630,9 +630,8 @@ CreateExceptionBreakpointFilter(const ExceptionBreakpoint &bp) {
 //     }
 //   }
 // }
-llvm::json::Value CreateSource(lldb::SBLineEntry &line_entry) {
+llvm::json::Value CreateSource(const lldb::SBFileSpec &file) {
   llvm::json::Object object;
-  lldb::SBFileSpec file = line_entry.GetFileSpec();
   if (file.IsValid()) {
     const char *name = file.GetFilename();
     if (name)
@@ -646,6 +645,10 @@ llvm::json::Value CreateSource(lldb::SBLineEntry &line_entry) {
   return llvm::json::Value(std::move(object));
 }
 
+llvm::json::Value CreateSource(const lldb::SBLineEntry &line_entry) {
+  return CreateSource(line_entry.GetFileSpec());
+}
+
 llvm::json::Value CreateSource(llvm::StringRef source_path) {
   llvm::json::Object source;
   llvm::StringRef name = llvm::sys::path::filename(source_path);
@@ -1250,7 +1253,7 @@ std::string VariableDescription::GetResult(llvm::StringRef context) {
 //       "description": "The number of indexed child variables. The client
 //                       can use this optional information to present the
 //                       children in a paged UI and fetch them in chunks."
-//     }
+//     },
 //     "memoryReference": {
 //        "type": "string",
 //        "description": "A memory reference associated with this variable.
@@ -1260,63 +1263,75 @@ std::string VariableDescription::GetResult(llvm::StringRef context) {
 //                        be used in a `disassemble` request. This attribute may
 //                        be returned by a debug adapter if corresponding
 //                        capability `supportsMemoryReferences` is true."
-//      },
+//     },
+//     "declarationLocationReference": {
+//       "type": "integer",
+//       "description": "A reference that allows the client to request the
+//                       location where the variable is declared. This should be
+//                       present only if the adapter is likely to be able to
+//                       resolve the location.\n\nThis reference shares the same
+//                       lifetime as the `variablesReference`. See 'Lifetime of
+//                       Object References' in the Overview section for
+//                       details."
+//     },
+//
 //     "$__lldb_extensions": {
 //       "description": "Unofficial extensions to the protocol",
 //       "properties": {
 //         "declaration": {
-//         "type": "object",
-//         "description": "The source location where the variable was declared.
-//                         This value won't be present if no declaration is
-//                         available.",
-//         "properties": {
-//           "path": {
-//             "type": "string",
-//             "description": "The source file path where the variable was
-//                            declared."
-//           },
-//           "line": {
-//             "type": "number",
-//             "description": "The 1-indexed source line where the variable was
-//                             declared."
-//           },
-//           "column": {
-//             "type": "number",
-//             "description": "The 1-indexed source column where the variable
-//                             was declared."
+//           "type": "object",
+//           "description": "The source location where the variable was
+//                           declared. This value won't be present if no
+//                           declaration is available.
+//                           Superseded by `declarationLocationReference`",
+//           "properties": {
+//             "path": {
+//               "type": "string",
+//               "description": "The source file path where the variable was
+//                              declared."
+//             },
+//             "line": {
+//               "type": "number",
+//               "description": "The 1-indexed source line where the variable
+//                               was declared."
+//             },
+//             "column": {
+//               "type": "number",
+//               "description": "The 1-indexed source column where the variable
+//                               was declared."
+//             }
 //           }
+//         },
+//         "value": {
+//           "type": "string",
+//           "description": "The internal value of the variable as returned by
+//                            This is effectively SBValue.GetValue(). The other
+//                            `value` entry in the top-level variable response
+//                            is, on the other hand, just a display string for
+//                            the variable."
+//         },
+//         "summary": {
+//           "type": "string",
+//           "description": "The summary string of the variable. This is
+//                           effectively SBValue.GetSummary()."
+//         },
+//         "autoSummary": {
+//           "type": "string",
+//           "description": "The auto generated summary if using
+//                           `enableAutoVariableSummaries`."
+//         },
+//         "error": {
+//           "type": "string",
+//           "description": "An error message generated if LLDB couldn't inspect
+//                           the variable."
 //         }
-//       },
-//       "value":
-//         "type": "string",
-//         "description": "The internal value of the variable as returned by
-//                         This is effectively SBValue.GetValue(). The other
-//                         `value` entry in the top-level variable response is,
-//                          on the other hand, just a display string for the
-//                          variable."
-//       },
-//       "summary":
-//         "type": "string",
-//         "description": "The summary string of the variable. This is
-//                         effectively SBValue.GetSummary()."
-//       },
-//       "autoSummary":
-//         "type": "string",
-//         "description": "The auto generated summary if using
-//                         `enableAutoVariableSummaries`."
-//       },
-//       "error":
-//         "type": "string",
-//         "description": "An error message generated if LLDB couldn't inspect
-//                         the variable."
 //       }
 //     }
 //   },
 //   "required": [ "name", "value", "variablesReference" ]
 // }
-llvm::json::Value CreateVariable(lldb::SBValue v, int64_t variablesReference,
-                                 int64_t varID, bool format_hex,
-                                 bool is_name_duplicated,
+llvm::json::Value CreateVariable(lldb::SBValue v, int64_t var_ref,
+                                 bool format_hex, bool is_name_duplicated,
                                  std::optional<std::string> custom_name) {
   VariableDescription desc(v, format_hex, is_name_duplicated, custom_name);
   llvm::json::Object object;
@@ -1361,12 +1376,18 @@ llvm::json::Value CreateVariable(lldb::SBValue v, int64_t variablesReference,
     }
   }
   EmplaceSafeString(object, "type", desc.display_type_name);
-  if (varID != INT64_MAX)
-    object.try_emplace("id", varID);
+
+  // A unique variable identifier to help in properly identifying variables with
+  // the same name. This is an extension to the VS protocol.
+  object.try_emplace("id", var_ref);
+
   if (v.MightHaveChildren())
-    object.try_emplace("variablesReference", variablesReference);
+    object.try_emplace("variablesReference", var_ref);
   else
-    object.try_emplace("variablesReference", (int64_t)0);
+    object.try_emplace("variablesReference", 0);
+
+  if (v.GetDeclaration().IsValid())
+    object.try_emplace("declarationLocationReference", var_ref);
 
   if (lldb::addr_t addr = v.GetLoadAddress(); addr != LLDB_INVALID_ADDRESS)
     object.try_emplace("memoryReference", EncodeMemoryReference(addr));
diff --git a/lldb/tools/lldb-dap/JSONUtils.h b/lldb/tools/lldb-dap/JSONUtils.h
@@ -289,16 +289,26 @@ llvm::json::Value CreateScope(const llvm::StringRef name,
                               int64_t variablesReference,
                               int64_t namedVariables, bool expensive);
 
+/// Create a "Source" JSON object as described in the debug adaptor definition.
+///
+/// \param[in] file
+///     The SBFileSpec to use when populating out the "Source" object
+///
+/// \return
+///     A "Source" JSON object that follows the formal JSON
+///     definition outlined by Microsoft.
+llvm::json::Value CreateSource(const lldb::SBFileSpec &file);
+
 /// Create a "Source" JSON object as described in the debug adaptor definition.
 ///
 /// \param[in] line_entry
 ///     The LLDB line table to use when populating out the "Source"
 ///     object
 ///
 /// \return
-///     A "Source" JSON object with that follows the formal JSON
+///     A "Source" JSON object that follows the formal JSON
 ///     definition outlined by Microsoft.
-llvm::json::Value CreateSource(lldb::SBLineEntry &line_entry);
+llvm::json::Value CreateSource(const lldb::SBLineEntry &line_entry);
 
 /// Create a "Source" object for a given source path.
 ///
@@ -470,15 +480,10 @@ struct VariableDescription {
 ///     The LLDB value to use when populating out the "Variable"
 ///     object.
 ///
-/// \param[in] variablesReference
-///     The variable reference. Zero if this value isn't structured
-///     and has no children, non-zero if it does have children and
-///     might be asked to expand itself.
-///
-/// \param[in] varID
-///     A unique variable identifier to help in properly identifying
-///     variables with the same name. This is an extension to the
-///     VS protocol.
+/// \param[in] var_ref
+///     The variable reference. Used to identify the value, e.g.
+///     in the `variablesReference` or `declarationLocationReference`
+///     properties.
 ///
 /// \param[in] format_hex
 ///     It set to true the variable will be formatted as hex in
@@ -499,8 +504,8 @@ struct VariableDescription {
 /// \return
 ///     A "Variable" JSON object with that follows the formal JSON
 ///     definition outlined by Microsoft.
-llvm::json::Value CreateVariable(lldb::SBValue v, int64_t variablesReference,
-                                 int64_t varID, bool format_hex,
+llvm::json::Value CreateVariable(lldb::SBValue v, int64_t var_ref,
+                                 bool format_hex,
                                  bool is_name_duplicated = false,
                                  std::optional<std::string> custom_name = {});
 
diff --git a/lldb/tools/lldb-dap/lldb-dap.cpp b/lldb/tools/lldb-dap/lldb-dap.cpp

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,3 @@`
	`1`	`+C_SOURCES := main.c`
	`2`	`+`
	`3`	`+include Makefile.rules`