[lldb-dap] Implement value locations for function pointers

[vogelsgesang]

This commit adds valueLocationReference to function pointers and function references. Thereby, users can navigate directly to the pointed-to function from within the "variables" pane.

In general, it would be useful to also a add similar location references also to member function pointers, std::source_location, std::function, and many more. Doing so would require extending the formatters to provide such a source code location.

There were two RFCs about this a while ago:
https://discourse.llvm.org/t/rfc-extending-formatters-with-a-source-code-reference/68375
https://discourse.llvm.org/t/rfc-sbvalue-metadata-provider/68377/26

However, both RFCs ended without a conclusion. As such, this commit now implements the lowest-hanging fruit, i.e. function pointers. If people find it useful, I will revive the RFC afterwards.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Adrian Vogelsgesang (vogelsgesang)

Changes

Note to reviewers: This commit builds on top of the not-yet-merged PR #102928. When reviewing, ignore the first commit, it is part of the over PR. I will rebase and turn this into a non-draft PR after #102928 landed.

---

This commit adds valueLocationReference to function pointers and function references. Thereby, users can navigate directly to the pointed-to function from within the "variables" pane.

In general, it would be useful to also a add similar location references also to member function pointers, std::source_location, std::function, and many more. Doing so would require extending the formatters to provide such a source code location.

There were two RFCs about this a while ago:
https://discourse.llvm.org/t/rfc-extending-formatters-with-a-source-code-reference/68375
https://discourse.llvm.org/t/rfc-sbvalue-metadata-provider/68377/26

However, both RFCs ended without a conclusion. As such, this commit now solve the lowest-hanging fruit, i.e. function pointers. If people find it useful, I will revive the RFC afterwards.

---

Patch is 33.92 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/104589.diff

9 Files Affected:

• (modified) lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py (+11)
• (added) lldb/test/API/tools/lldb-dap/locations/Makefile (+3)
• (added) lldb/test/API/tools/lldb-dap/locations/TestDAP_locations.py (+77)
• (added) lldb/test/API/tools/lldb-dap/locations/main.cpp (+13)
• (modified) lldb/tools/lldb-dap/DAP.cpp (+8-9)
• (modified) lldb/tools/lldb-dap/DAP.h (+5-5)
• (modified) lldb/tools/lldb-dap/JSONUtils.cpp (+101-55)
• (modified) lldb/tools/lldb-dap/JSONUtils.h (+21-13)
• (modified) lldb/tools/lldb-dap/lldb-dap.cpp (+192-30)

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
index a324af57b61df3..9879a34ed2020c 100644
--- a/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py
+++ b/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py
@@ -1079,6 +1079,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
new file mode 100644
index 00000000000000..99998b20bcb050
--- /dev/null
+++ b/lldb/test/API/tools/lldb-dap/locations/Makefile
@@ -0,0 +1,3 @@
+CXX_SOURCES := main.cpp
+
+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
new file mode 100644
index 00000000000000..5a755ee44d12c2
--- /dev/null
+++ b/lldb/test/API/tools/lldb-dap/locations/TestDAP_locations.py
@@ -0,0 +1,77 @@
+"""
+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.cpp"
+        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.cpp"))
+        self.assertEqual(loc_var1["body"]["line"], 8)
+
+        # func_ptr has both a declaration and a valueLocation
+        self.assertIn("declarationLocationReference", locals["func_ptr"].keys())
+        self.assertIn("valueLocationReference", locals["func_ptr"].keys())
+        decl_loc_func_ptr = self.dap_server.request_locations(
+            locals["func_ptr"]["declarationLocationReference"]
+        )
+        self.assertTrue(decl_loc_func_ptr["success"])
+        self.assertTrue(decl_loc_func_ptr["body"]["source"]["path"].endswith("main.cpp"))
+        self.assertEqual(decl_loc_func_ptr["body"]["line"], 9)
+        val_loc_func_ptr = self.dap_server.request_locations(
+            locals["func_ptr"]["valueLocationReference"]
+        )
+        self.assertTrue(val_loc_func_ptr["success"])
+        self.assertTrue(val_loc_func_ptr["body"]["source"]["path"].endswith("main.cpp"))
+        self.assertEqual(val_loc_func_ptr["body"]["line"], 3)
+
+        # func_ref has both a declaration and a valueLocation
+        self.assertIn("declarationLocationReference", locals["func_ref"].keys())
+        self.assertIn("valueLocationReference", locals["func_ref"].keys())
+        decl_loc_func_ref = self.dap_server.request_locations(
+            locals["func_ref"]["declarationLocationReference"]
+        )
+        self.assertTrue(decl_loc_func_ref["success"])
+        self.assertTrue(decl_loc_func_ref["body"]["source"]["path"].endswith("main.cpp"))
+        self.assertEqual(decl_loc_func_ref["body"]["line"], 10)
+        val_loc_func_ref = self.dap_server.request_locations(
+            locals["func_ref"]["valueLocationReference"]
+        )
+        self.assertTrue(val_loc_func_ref["success"])
+        self.assertTrue(val_loc_func_ref["body"]["source"]["path"].endswith("main.cpp"))
+        self.assertEqual(val_loc_func_ref["body"]["line"], 3)
+
+        # `evaluate` responses for function pointers also have locations associated
+        eval_res = self.dap_server.request_evaluate("greet")
+        self.assertTrue(decl_loc_func_ref["success"])
+        self.assertIn("valueLocationReference", eval_res["body"].keys())
diff --git a/lldb/test/API/tools/lldb-dap/locations/main.cpp b/lldb/test/API/tools/lldb-dap/locations/main.cpp
new file mode 100644
index 00000000000000..8eb5a7636d188c
--- /dev/null
+++ b/lldb/test/API/tools/lldb-dap/locations/main.cpp
@@ -0,0 +1,13 @@
+#include <cstdio>
+
+void greet() {
+   printf("Hello");
+}
+
+int main(void) {
+  int var1 = 1;
+  void (*func_ptr)() = &greet;
+  void (&func_ref)() = greet;
+  // BREAK HERE
+  return 0;
+}
diff --git a/lldb/tools/lldb-dap/DAP.cpp b/lldb/tools/lldb-dap/DAP.cpp
index c3c70e9d739846..a773c2df28ed25 100644
--- a/lldb/tools/lldb-dap/DAP.cpp
+++ b/lldb/tools/lldb-dap/DAP.cpp
@@ -813,7 +813,7 @@ void Variables::Clear() {
   locals.Clear();
   globals.Clear();
   registers.Clear();
-  expandable_variables.clear();
+  referenced_variables.clear();
 }
 
 int64_t Variables::GetNewVariableReference(bool is_permanent) {
@@ -828,24 +828,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
index 57562a14983519..848e69145da4f6 100644
--- a/lldb/tools/lldb-dap/DAP.h
+++ b/lldb/tools/lldb-dap/DAP.h
@@ -104,12 +104,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
@@ -127,7 +127,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
index a8b85f55939e17..b543355fa6fba8 100644
--- a/lldb/tools/lldb-dap/JSONUtils.cpp
+++ b/lldb/tools/lldb-dap/JSONUtils.cpp
@@ -614,9 +614,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)
@@ -630,6 +629,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);
@@ -1085,6 +1088,18 @@ std::string VariableDescription::GetResult(llvm::StringRef context) {
   return description.trim().str();
 }
 
+bool HasValueLocation(lldb::SBValue v) {
+  if (!v.GetType().IsPointerType() && !v.GetType().IsReferenceType()) {
+    return false;
+  }
+
+  lldb::addr_t addr = v.GetValueAsAddress();
+  lldb::SBLineEntry line_entry =
+      g_dap.target.ResolveLoadAddress(addr).GetLineEntry();
+
+  return line_entry.IsValid();
+}
+
 // "Variable": {
 //   "type": "object",
 //   "description": "A Variable is a name/value pair. Optionally a variable
@@ -1143,65 +1158,87 @@ 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."
-//     }
-//
+//     },
+//     "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."
+//     },
+//     "valueLocationReference": {
+//       "type": "integer",
+//       "description": "A reference that allows the client to request the
+//                       location where the variable's value is declared. For
+//                       example, if the variable contains a function pointer,
+//                       the adapter may be able to look up the function's
+//                       location. 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;
@@ -1246,12 +1283,21 @@ 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 << 1);
+
+  if (HasValueLocation(v))
+    object.try_emplace("valueLocationReference", (var_ref << 1) | 1);
 
   object.try_emplace("$__lldb_extensions", desc.GetVariableExtensionsJSON());
   return llvm::json::Value(std::move(object));
@@ -1274,8 +1320,8 @@ CreateRunInTerminalReverseRequest(const llvm::json::Object &launch_request,
                                   llvm::StringRef comm_file,
                                   lldb::pid_t debugger_pid) {
   llvm::json::Object run_in_terminal_args;
-  // This indicates the IDE to open an embedded terminal, instead of opening the
-  // terminal in a new window.
+  // This indicates the IDE to open an embedded terminal, instead of opening
+  // the terminal in a new window.
   run_in_terminal_args.try_emplace("kind", "integrated");
 
   auto launch_request_arguments = launch_request.getObject("arguments");
diff --git a/lldb/tools/lldb-dap/JSONUtils.h b/lldb/tools/lldb-dap/JSONUtils.h
index 1515f5ba2e5f4d..861951e488b6b8 100644
--- a/lldb/tools/lldb-dap/JSONUtils.h
+++ b/lldb/tools/lldb-dap/JSONUtils.h
@@ -282,6 +282,16 @@ 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
@@ -289,9 +299,9 @@ llvm::json::Value CreateScope(const llvm::StringRef name,
 ///     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.
 ///
@@ -411,6 +421,9 @@ struct VariableDescription {
   std::string GetResult(llvm::StringRef context);
 };
 
+/// Does the given variable have an associated value location?
+bool HasValueLocation(lldb::SBValue v);
+
 /// Create a "Variable" object for a LLDB thread object.
 ///
 /// This function will fill in the following keys in the returned
@@ -433,15 +446,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 ...
[truncated]

[github-actions]

✅ With the latest revision this PR passed the Python code formatter.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[walter-erquinigo]

could you share a screenshot or something like that? :)

[vogelsgesang]

In the below screen recording, you can see:

• The function pointers are shown in the variables view as usual
• The corresponding values are linked, as indicated by the underline when hovering the value
• When Cmd+Clicking on the value, the link is followed
• Currently, this still leads to an error message. Afaict, this is due to a bug in VS-Code Insiders. I suspect that it does not correctly resolve the file path provided by the debug adapter. Probably this fails because I am using a remote, SSH-based editing session here.
• However, from the DAP logs (visible towards the bottom of the screen), we can see that the debug adapter returns the correct file name and also the correct line

Update: This was fixed in VS-Code by now. Clicking on the function pointer opens the correct source code location now

value-location.mov

[vogelsgesang]

Here the screen recording of a fully functional, non-remote session:

value-location2.mov

[walter-erquinigo]

Overall looks very nice, and I want to make sure that this works for the Mojo language after this gets merged.
Please rebase this change so that I have a better time reviewing it.
Also, happy to chime in on discourse to make sure that the RFC gets approved. I really love this kind of functionality.

[vogelsgesang]

Now that #102928 landed, this commit is also ready for review

[clayborg]

My only question here is do we really want to do an address resolution for any pointer or reference or can we limit that for only function pointers? I worry a bit about the extra cost for any variable view that has tons of pointers or references as this will cause LLDB to lookup their address all the time. If this should be just for function pointers, it would be good to limit the extra searches to only those.

[clayborg]

no parens for single line if statements per llvm coding guidelines. The name isn't quite clear for what this is doing. Maybe ValuePointsToCode would be better? This is also adding some extra work for every variable that is a pointer or reference. Is there no way to limit this to function pointers only? It would be great to skip the resolving of the load address if we can help it, right now it will do an address resolving on every pointer and reference.

[vogelsgesang]

should be fixed in the updated PR

[vogelsgesang]

@walter-erquinigo @clayborg afaict, I adressed all your review comments. Could you take another look?

[vogelsgesang]

friendly ping, @walter-erquinigo @clayborg