[lldb-dap] Emit declarations along with variables #74865

walter-erquinigo · 2023-12-08T16:56:00Z

This is an extension to the protocol that emits the declaration information along with the metadata of each variable. This can be used by vscode extensions to implement, for example, a "goToDefinition" action in the debug tab, or for showing the value of a variable right next to where it's declared during a debug session.
As this is cheap, I'm not gating this information under any setting.

This is an extension to the protocol that emits the declaration information along with the metadata of each variable. This can be used by vscode extensions to implement, for example, a "goToDefinition" action in the debug tab, or for showing the value of a variable right next to where it's declared during a debug session. As this is cheap, I'm not gating this information under any setting.

llvmbot · 2023-12-08T16:56:29Z

@llvm/pr-subscribers-lldb

Author: Walter Erquinigo (walter-erquinigo)

Changes

This is an extension to the protocol that emits the declaration information along with the metadata of each variable. This can be used by vscode extensions to implement, for example, a "goToDefinition" action in the debug tab, or for showing the value of a variable right next to where it's declared during a debug session.
As this is cheap, I'm not gating this information under any setting.

Full diff: https://github.com/llvm/llvm-project/pull/74865.diff

2 Files Affected:

(modified) lldb/test/API/tools/lldb-dap/variables/TestDAP_variables.py (+8-2)
(modified) lldb/tools/lldb-dap/JSONUtils.cpp (+41)

diff --git a/lldb/test/API/tools/lldb-dap/variables/TestDAP_variables.py b/lldb/test/API/tools/lldb-dap/variables/TestDAP_variables.py
index d2a21ad3cd1d4..9b0755eea7d3e 100644
--- a/lldb/test/API/tools/lldb-dap/variables/TestDAP_variables.py
+++ b/lldb/test/API/tools/lldb-dap/variables/TestDAP_variables.py
@@ -4,8 +4,8 @@
 
 import os
 
-import lldbdap_testcase
 import dap_server
+import lldbdap_testcase
 from lldbsuite.test import lldbutil
 from lldbsuite.test.decorators import *
 from lldbsuite.test.lldbtest import *
@@ -152,7 +152,13 @@ def do_test_scopes_variables_setVariable_evaluate(
         globals = self.dap_server.get_global_variables()
         buffer_children = make_buffer_verify_dict(0, 32)
         verify_locals = {
-            "argc": {"equals": {"type": "int", "value": "1"}},
+            "argc": {
+                "equals": {"type": "int", "value": "1"},
+                "declaration": {
+                    "equals": {"line": 12, "column": 14},
+                    "contains": {"path": ["lldb-dap", "variables", "main.cpp"]},
+                },
+            },
             "argv": {
                 "equals": {"type": "const char **"},
                 "startswith": {"value": "0x"},
diff --git a/lldb/tools/lldb-dap/JSONUtils.cpp b/lldb/tools/lldb-dap/JSONUtils.cpp
index 03a43f9da87f2..d8b5636fd03a9 100644
--- a/lldb/tools/lldb-dap/JSONUtils.cpp
+++ b/lldb/tools/lldb-dap/JSONUtils.cpp
@@ -1101,6 +1101,29 @@ std::string CreateUniqueVariableNameForDisplay(lldb::SBValue v,
 //                       can use this optional information to present the
 //                       children in a paged UI and fetch them in chunks."
 //     }
+//     "declaration": {
+//       "type": "object | undefined",
+//       "description": "Extension to the protocol that indicates the source
+//                       location where the variable was declared. This value
+//                       might not be present if no declaration is available.",
+//       "properties": {
+//         "path": {
+//           "type": "string | undefined",
+//           "description": "The source file path where the variable was
+//                           declared."
+//         },
+//         "line": {
+//           "type": "number | undefined",
+//           "description": "The 1-indexed source line where the variable was
+//                          declared."
+//         },
+//         "column": {
+//           "type": "number | undefined",
+//           "description": "The 1-indexed source column where the variable was
+//                          declared."
+//         }
+//       }
+//     }
 //   },
 //   "required": [ "name", "value", "variablesReference" ]
 // }
@@ -1165,6 +1188,24 @@ llvm::json::Value CreateVariable(lldb::SBValue v, int64_t variablesReference,
   const char *evaluateName = evaluateStream.GetData();
   if (evaluateName && evaluateName[0])
     EmplaceSafeString(object, "evaluateName", std::string(evaluateName));
+
+  if (lldb::SBDeclaration decl = v.GetDeclaration(); decl.IsValid()) {
+    llvm::json::Object decl_obj;
+    if (lldb::SBFileSpec file = decl.GetFileSpec(); file.IsValid()) {
+      char path[PATH_MAX] = "";
+      if (file.GetPath(path, sizeof(path)) &&
+          lldb::SBFileSpec::ResolvePath(path, path, PATH_MAX)) {
+        decl_obj.try_emplace("path", std::string(path));
+      }
+    }
+
+    if (int line = decl.GetLine())
+      decl_obj.try_emplace("line", line);
+    if (int column = decl.GetColumn())
+      decl_obj.try_emplace("column", column);
+
+    object.try_emplace("declaration", std::move(decl_obj));
+  }
   return llvm::json::Value(std::move(object));
 }

bulbazord · 2023-12-09T00:04:07Z

lldb/tools/lldb-dap/JSONUtils.cpp

@@ -1101,6 +1101,29 @@ std::string CreateUniqueVariableNameForDisplay(lldb::SBValue v,
 //                       can use this optional information to present the
 //                       children in a paged UI and fetch them in chunks."
 //     }
+//     "declaration": {
+//       "type": "object | undefined",


What is "type"? I also don't see you setting it in the diff below this one.

this "type" is a JSON schema attribute, not an actual thing being returned

bulbazord · 2023-12-11T18:56:39Z

Is this extension to the protocol documented anywhere I can see? Or is this an ad-hoc thing? I think it would be useful to write down somewhere other than a commit message (in docs, comments, or otherwise) that this is an extension.

walter-erquinigo · 2023-12-11T19:11:10Z

Is this extension to the protocol documented anywhere I can see? Or is this an ad-hoc thing? I think it would be useful to write down somewhere other than a commit message (in docs, comments, or otherwise) that this is an extension.

@bulbazord , it's an ad hoc thing. And I actually documented it in the description of this field above the function :)

bulbazord · 2023-12-11T19:13:33Z

Is this extension to the protocol documented anywhere I can see? Or is this an ad-hoc thing? I think it would be useful to write down somewhere other than a commit message (in docs, comments, or otherwise) that this is an extension.

@bulbazord , it's an ad hoc thing. And I actually documented it in the description of this field above the function :)

Oh! So you did, I see, in the description field. LGTM, thanks!

clayborg

Might be good idea to only add "declaration" if and only if we have both a file and line at least?

clayborg · 2023-12-11T20:23:25Z

lldb/tools/lldb-dap/JSONUtils.cpp

+    if (int column = decl.GetColumn())
+      decl_obj.try_emplace("column", column);
+
+    object.try_emplace("declaration", std::move(decl_obj));


So above we might or might not get any of the info, do we want to only emplace this into "declaration" if we at least have both a valid path and a valid line?

This one makes sense. I'll make that fix.

walter-erquinigo · 2023-12-11T20:34:19Z

Might be good idea to only add "declaration" if and only if we have both a file and line at least?

I think it's better to emit it if there's at least one of them. We can leave it to the client the decision of what to do if only one of them is available.

clayborg

I am ok with this patch. I would just like to document what key/value pairs are extensions because the spec description in the comment was directly from the DAP Protocol website and it would be nice to let people know what are extensions.

clayborg · 2023-12-12T01:38:06Z

lldb/tools/lldb-dap/JSONUtils.cpp

+//     "declaration": {
+//       "type": "object | undefined",
+//       "description": "Extension to the protocol that indicates the source
+//                       location where the variable was declared. This value
+//                       might not be present if no declaration is available.",
+//       "properties": {
+//         "path": {
+//           "type": "string | undefined",
+//           "description": "The source file path where the variable was
+//                           declared."
+//         },
+//         "line": {
+//           "type": "number | undefined",
+//           "description": "The 1-indexed source line where the variable was
+//                          declared."
+//         },
+//         "column": {
+//           "type": "number | undefined",
+//           "description": "The 1-indexed source column where the variable was
+//                          declared."
+//         }
+//       }
+//     }


Can we document that is is an extension if it actually isn't part of the DAP protocol from the Microsoft website?

In order to allow smarter vscode extensions, it's useful to send additional structured information of SBValues to the client. Specifically, I'm now sending error, summary, autoSummary and inMemoryValue in addition to the existing properties being sent. This is cheap because these properties have to be calculated anyway to generate the display value of the variable, but they are now available for extensions to better analyze variables. For example, if the error field is not present, the extension might be able to provide cool features, and the current way to do that is to look for the `"<error: "` prefix, which is error-prone. This also incorporates a tiny feedback from #74865 (comment)

This is an extension to the protocol that emits the declaration information along with the metadata of each variable. This can be used by vscode extensions to implement, for example, a "goToDefinition" action in the debug tab, or for showing the value of a variable right next to where it's declared during a debug session. As this is cheap, I'm not gating this information under any setting. (cherry picked from commit 0ea19bd)

vogelsgesang · 2024-08-12T15:55:30Z

FYI @walter-erquinigo: There is a proposal under discussion to add first-class support for declarationLocation (and also valueLocation) to the debug adapter protocol. See microsoft/debug-adapter-protocol#343

walter-erquinigo · 2024-08-12T16:08:07Z

FYI @walter-erquinigo: There is a proposal under discussion to add first-class support for declarationLocation (and also valueLocation) to the debug adapter protocol. See microsoft/debug-adapter-protocol#343

Man, that's amazing. I really hope that gets merged

walter-erquinigo requested a review from JDevlieghere as a code owner December 8, 2023 16:56

walter-erquinigo requested a review from clayborg December 8, 2023 16:56

llvmbot added the lldb label Dec 8, 2023

bulbazord reviewed Dec 9, 2023

View reviewed changes

bulbazord approved these changes Dec 11, 2023

View reviewed changes

walter-erquinigo merged commit 0ea19bd into llvm:main Dec 11, 2023
5 checks passed

clayborg reviewed Dec 11, 2023

View reviewed changes

clayborg reviewed Dec 12, 2023

View reviewed changes

walter-erquinigo mentioned this pull request Dec 12, 2023

[lldb-dap] Emit more structured info along with variables #75244

Merged

vogelsgesang mentioned this pull request Aug 10, 2024

Add an optional source and line+column range for a Variable microsoft/debug-adapter-protocol#343

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[lldb-dap] Emit declarations along with variables #74865

[lldb-dap] Emit declarations along with variables #74865

walter-erquinigo commented Dec 8, 2023

llvmbot commented Dec 8, 2023

bulbazord Dec 9, 2023

walter-erquinigo Dec 9, 2023

bulbazord commented Dec 11, 2023

walter-erquinigo commented Dec 11, 2023

bulbazord commented Dec 11, 2023

clayborg left a comment

clayborg Dec 11, 2023

walter-erquinigo Dec 11, 2023

walter-erquinigo commented Dec 11, 2023

clayborg left a comment

clayborg Dec 12, 2023

vogelsgesang commented Aug 12, 2024

walter-erquinigo commented Aug 12, 2024 •

edited

Loading

[lldb-dap] Emit declarations along with variables #74865

[lldb-dap] Emit declarations along with variables #74865

Conversation

walter-erquinigo commented Dec 8, 2023

llvmbot commented Dec 8, 2023

bulbazord Dec 9, 2023

Choose a reason for hiding this comment

walter-erquinigo Dec 9, 2023

Choose a reason for hiding this comment

bulbazord commented Dec 11, 2023

walter-erquinigo commented Dec 11, 2023

bulbazord commented Dec 11, 2023

clayborg left a comment

Choose a reason for hiding this comment

clayborg Dec 11, 2023

Choose a reason for hiding this comment

walter-erquinigo Dec 11, 2023

Choose a reason for hiding this comment

walter-erquinigo commented Dec 11, 2023

clayborg left a comment

Choose a reason for hiding this comment

clayborg Dec 12, 2023

Choose a reason for hiding this comment

vogelsgesang commented Aug 12, 2024

walter-erquinigo commented Aug 12, 2024 • edited Loading

walter-erquinigo commented Aug 12, 2024 •

edited

Loading