Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Dec 11, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions lldb/test/API/tools/lldb-dap/variables/TestDAP_variables.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 *
Expand Down Expand Up @@ -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"},
Expand Down
41 changes: 41 additions & 0 deletions lldb/tools/lldb-dap/JSONUtils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// "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."
// }
// }
// }
Comment on lines +1104 to +1126
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// },
// "required": [ "name", "value", "variablesReference" ]
// }
Expand Down Expand Up @@ -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));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
return llvm::json::Value(std::move(object));
}

Expand Down