llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-lldb Author: Adrian Vogelsgesang (vogelsgesang) <details> <summary>Changes</summary> 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 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 tried to publish the declaration locations as value locations. However, it seems that VS-Code still has issues with correctly resolving file paths, as reported in https://github.com/microsoft/vscode/pull/225546#issuecomment-2292428591 --- Patch is 24.84 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/102928.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 (+38) - (added) lldb/test/API/tools/lldb-dap/locations/main.c (+5) - (modified) lldb/tools/lldb-dap/DAP.cpp (+9-9) - (modified) lldb/tools/lldb-dap/DAP.h (+5-5) - (modified) lldb/tools/lldb-dap/JSONUtils.cpp (+72-53) - (modified) lldb/tools/lldb-dap/JSONUtils.h (+18-13) - (modified) lldb/tools/lldb-dap/lldb-dap.cpp (+124-22) ``````````diff 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..10495940055b63 --- /dev/null +++ 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 new file mode 100644 index 00000000000000..26feb789db39d6 --- /dev/null +++ b/lldb/test/API/tools/lldb-dap/locations/TestDAP_locations.py @@ -0,0 +1,38 @@ +""" +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 new file mode 100644 index 00000000000000..6a8c86d00cb562 --- /dev/null +++ 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 index c3c70e9d739846..baf02e5b35683a 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,24 @@ 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..10e6b1fc6d9abe 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); @@ -1143,65 +1146,75 @@ 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." +// }, // // "$__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 +1259,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 << 1); object.try_emplace("$__lldb_extensions", desc.GetVariableExtensionsJSON()); return llvm::json::Value(std::move(object)); diff --git a/lldb/tools/lldb-dap/JSONUtils.h b/lldb/tools/lldb-dap/JSONUtils.h index 1515f5ba2e5f4d..0a88dd269bfbdf 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. /// @@ -433,15 +443,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 @@ -462,8 +467,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 index ea84f31aec3a6c..c203514fc3d395 100644 --- a/lldb/tools/lldb-dap/lldb-dap.cpp +++ b/lldb/tools/lldb-dap/lldb-dap.cpp @@ -8,6 +8,7 @@ #include "DAP.h" #include "Watchpoint.h" +#include "lldb/API/SBDeclaration.h" #include "lldb/API/SBMemoryRegionInfo.h" #include <cassert> @@ -1405,7 +1406,7 @@ void request_evaluate(const llvm::json::Object &request) { EmplaceSafeString(body, "result", desc.GetResult(context)); EmplaceSafeString(body, "type", desc.display_type_name); if (value.MightHaveChildren()) { - auto variableReference = g_dap.variables.InsertExpandableVariable( + auto variableReference = g_dap.variables.InsertVariable( value, /*is_permanent=*/context == "repl"); body.try_emplace("variablesReference", variableReference); } else { @@ -3598,8 +3599,8 @@ void request_setVariable(const llvm::json::Object &request) { // is_permanent is false because debug console does not support // setVariable request. if (variable.MightHaveChildren()) - newVariablesReference = g_dap.variables.InsertExpandableVariable( - variable, /*is_permanent=*/false); + newVariablesReference = + g_dap.variables.InsertVariable(variable, /*is_permanent=*/false); body.try_emplace("variablesReference", newVariablesReference); } else { @@ -3770,13 +3771,10 @@ void request_variables(const llvm::json::Object &request) { if (!variable.IsValid()) break; - int64_t var_ref = 0; - if (variable.MightHaveChildren() || variable.IsSynthetic()) { - var_ref = g_dap.variables.InsertExpandableVariable( - variable, /*is_permanent=*/false); - } + int64_t var_ref = + g_dap.variables.InsertVariable(variable, /*is_permanent=*/false); variables.emplace_back(CreateVariable( - variable, var_ref, var_ref != 0 ? var_ref : UINT64_MAX, hex, + variable, var_ref, hex, variable_name_counts[GetNonNullVariableName(variable)] > 1)); } } else { @@ -3788,19 +3786,11 @@ void request_variables(const llvm::json::Object &request) { std::optional<std::string> custom_name = {}) { if (!child.IsValid()) return; - if (child.MightHaveChildren()) { - auto is_permanent = - g_dap.variables.IsPermanentVariableReference(variablesReference); - auto childVariablesReferences = - g_dap.variables.InsertExpandableVariable(child, is_permanent); - variables.emplace_back(CreateVariable( - child, childVariablesReferences, childVariablesReferences, hex, - /*is_name_duplicated=*/false, custom_name)); - } else { - variables.emplace_back(CreateVariable(child, 0, INT64_MAX, hex, - /*is_name_duplicated=*/false, - custom_name)); - } + bool is_permanent = + g_dap.variables.IsPermanentVariableReference(variablesReference); + int64_t var_ref = g_dap.variables.InsertVariable(child, is_permanent); + variables.emplace_back(CreateVariable( + child, var_ref, hex, /*is_name_duplicated=*/false, custom_name)); }; const int64_t num_children = variable.GetNumChildren(); int64_t end_idx = start + ((count == 0) ? num_children : count); @@ -3823,6 +3813,117 @@ void request_variables(const llvm::... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/102928 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
