llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-lldb Author: Adrian Vogelsgesang (vogelsgesang) <details> <summary>Changes</summary> 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 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] `````````` </details> https://github.com/llvm/llvm-project/pull/104589 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
