[lldb][lldb-dap] Implement jump to cursor #130503
Conversation
|
@llvm/pr-subscribers-lldb Author: Ebuka Ezike (da-viper) ChangesFixes #109335 Depends on #130435 Full diff: https://github.com/llvm/llvm-project/pull/130503.diff 13 Files Affected:
diff --git a/lldb/cmake/modules/LLDBConfig.cmake b/lldb/cmake/modules/LLDBConfig.cmake
index 747f7e6038181..8d02088548634 100644
--- a/lldb/cmake/modules/LLDBConfig.cmake
+++ b/lldb/cmake/modules/LLDBConfig.cmake
@@ -57,7 +57,7 @@ add_optional_dependency(LLDB_ENABLE_CURSES "Enable curses support in LLDB" Curse
add_optional_dependency(LLDB_ENABLE_LZMA "Enable LZMA compression support in LLDB" LibLZMA LIBLZMA_FOUND)
add_optional_dependency(LLDB_ENABLE_LUA "Enable Lua scripting support in LLDB" LuaAndSwig LUAANDSWIG_FOUND)
add_optional_dependency(LLDB_ENABLE_PYTHON "Enable Python scripting support in LLDB" PythonAndSwig PYTHONANDSWIG_FOUND)
-add_optional_dependency(LLDB_ENABLE_LIBXML2 "Enable Libxml 2 support in LLDB" LibXml2 LIBXML2_FOUND VERSION 2.8)
+add_optional_dependency(LLDB_ENABLE_LIBXML2 "Enable Libxml 2 support in LLDB" LibXml2 LIBXML2_FOUND VERSION)
add_optional_dependency(LLDB_ENABLE_FBSDVMCORE "Enable libfbsdvmcore support in LLDB" FBSDVMCore FBSDVMCore_FOUND QUIET)
option(LLDB_USE_ENTITLEMENTS "When codesigning, use entitlements if available" ON)
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 9471594b66012..d6c3bd0551cd7 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
@@ -753,6 +753,33 @@ def request_exceptionInfo(self, threadId=None):
}
return self.send_recv(command_dict)
+ def request_goto(self, threadId: int, targetId: int):
+ command_dict = {
+ "command": "goto",
+ "type": "request",
+ "arguments": {
+ "threadId": threadId,
+ "targetId": targetId,
+ },
+ }
+ return self.send_recv(command_dict)
+
+ def request_gotoTargets(self, filename: str, path: str, line: int, column: int):
+ arguments = {
+ "source": {
+ "name": filename,
+ "path": path,
+ },
+ "line": line,
+ "column": column,
+ }
+ command_dict = {
+ "command": "gotoTargets",
+ "type": "request",
+ "arguments": arguments,
+ }
+ return self.send_recv(command_dict)
+
def request_initialize(self, sourceInitFile):
command_dict = {
"command": "initialize",
diff --git a/lldb/test/API/tools/lldb-dap/gotoTarget/Makefile b/lldb/test/API/tools/lldb-dap/gotoTarget/Makefile
new file mode 100644
index 0000000000000..10495940055b6
--- /dev/null
+++ b/lldb/test/API/tools/lldb-dap/gotoTarget/Makefile
@@ -0,0 +1,3 @@
+C_SOURCES := main.c
+
+include Makefile.rules
diff --git a/lldb/test/API/tools/lldb-dap/gotoTarget/TestDAP_gotoTarget.py b/lldb/test/API/tools/lldb-dap/gotoTarget/TestDAP_gotoTarget.py
new file mode 100644
index 0000000000000..6d0f9ae478f33
--- /dev/null
+++ b/lldb/test/API/tools/lldb-dap/gotoTarget/TestDAP_gotoTarget.py
@@ -0,0 +1,60 @@
+"""
+Test lldb-dap gotoTarget request
+"""
+
+from lldbsuite.test.lldbtest import line_number
+import lldbdap_testcase
+import os
+
+
+class TestDAP_gotoTarget(lldbdap_testcase.DAPTestCaseBase):
+
+ def test_default(self):
+ """
+ Tests the jump to cursor of a simple program. No arguments,
+ environment, or anything else is specified.
+ This does not run any statement between the current breakpoint
+ and the jump line location.
+ """
+ program = self.getBuildArtifact("a.out")
+ self.build_and_launch(program)
+
+ source_file = "main.c"
+ self.source_path = os.path.join(os.getcwd(), source_file)
+ self.set_source_breakpoints(
+ source_file, [line_number(source_file, "// breakpoint 1")]
+ )
+ self.continue_to_next_stop()
+
+ first_var_1_object = self.dap_server.get_local_variable("var_1")
+ self.assertEqual(first_var_1_object["value"], "10")
+
+ goto_line = line_number(source_file, "// goto 1")
+ goto_column = 1
+ response = self.dap_server.request_gotoTargets(
+ source_file, self.source_path, goto_line, goto_column
+ )
+
+ self.assertEqual(
+ response["success"], True, "expects success when request for targets"
+ )
+ target = response["body"]["targets"][0]
+ self.assertGreaterEqual(
+ target["id"], 0, "targetId should be greater than or equal to zero"
+ )
+
+ target_id = target["id"]
+ thread_id = self.dap_server.get_thread_id()
+ self.assertIsNotNone(thread_id, "thread Id should not be none")
+
+ response = self.dap_server.request_goto(thread_id, target_id)
+
+ self.assertEqual(
+ response["success"], True, "expects success to go to a target id"
+ )
+
+ var_1_object = self.dap_server.get_local_variable("var_1")
+ self.assertEqual(first_var_1_object["value"], var_1_object["value"])
+
+ self.continue_to_next_stop() # a stop event is sent after a successful goto response
+ self.continue_to_exit()
diff --git a/lldb/test/API/tools/lldb-dap/gotoTarget/main.c b/lldb/test/API/tools/lldb-dap/gotoTarget/main.c
new file mode 100644
index 0000000000000..74210e5877369
--- /dev/null
+++ b/lldb/test/API/tools/lldb-dap/gotoTarget/main.c
@@ -0,0 +1,11 @@
+
+int main() {
+
+ int var_1 = 10;
+
+ var_1 = 20; // breakpoint 1
+
+ int var_2 = 40; // goto 1
+
+ return 0;
+}
\ No newline at end of file
diff --git a/lldb/tools/lldb-dap/CMakeLists.txt b/lldb/tools/lldb-dap/CMakeLists.txt
index 9a2d604f4d573..ff7e413c4bb1c 100644
--- a/lldb/tools/lldb-dap/CMakeLists.txt
+++ b/lldb/tools/lldb-dap/CMakeLists.txt
@@ -50,6 +50,8 @@ add_lldb_tool(lldb-dap
Handler/DisconnectRequestHandler.cpp
Handler/EvaluateRequestHandler.cpp
Handler/ExceptionInfoRequestHandler.cpp
+ Handler/GoToRequestHandler.cpp
+ Handler/GoToTargetsRequestHandler.cpp
Handler/InitializeRequestHandler.cpp
Handler/LaunchRequestHandler.cpp
Handler/LocationsRequestHandler.cpp
diff --git a/lldb/tools/lldb-dap/DAP.cpp b/lldb/tools/lldb-dap/DAP.cpp
index 1f7b25e7c5bcc..f72bc34d52b53 100644
--- a/lldb/tools/lldb-dap/DAP.cpp
+++ b/lldb/tools/lldb-dap/DAP.cpp
@@ -76,7 +76,7 @@ DAP::DAP(std::string name, llvm::StringRef path, std::ofstream *log,
configuration_done_sent(false), waiting_for_run_in_terminal(false),
progress_event_reporter(
[&](const ProgressEvent &event) { SendJSON(event.ToJSON()); }),
- reverse_request_seq(0), repl_mode(repl_mode) {}
+ reverse_request_seq(0), repl_mode(repl_mode), goto_id_map() {}
DAP::~DAP() = default;
@@ -899,6 +899,27 @@ lldb::SBError DAP::WaitForProcessToStop(uint32_t seconds) {
return error;
}
+std::optional<lldb::SBLineEntry> Gotos::GetLineEntry(uint64_t id) const {
+ const auto iter = line_entries.find(id);
+ if (iter != line_entries.end())
+ return iter->second;
+
+ return std::nullopt;
+}
+
+uint64_t Gotos::InsertLineEntry(lldb::SBLineEntry line_entry) {
+ const auto spec_id = this->NewSpecId();
+ line_entries.insert(std::make_pair(spec_id, line_entry));
+ return spec_id;
+}
+
+void Gotos::Clear() {
+ new_id = 0UL;
+ line_entries.clear();
+}
+
+uint64_t Gotos::NewSpecId() { return new_id++; }
+
void Variables::Clear() {
locals.Clear();
globals.Clear();
diff --git a/lldb/tools/lldb-dap/DAP.h b/lldb/tools/lldb-dap/DAP.h
index 8b2e498a28c95..693908016fdc9 100644
--- a/lldb/tools/lldb-dap/DAP.h
+++ b/lldb/tools/lldb-dap/DAP.h
@@ -79,6 +79,27 @@ enum class PacketStatus {
enum class ReplMode { Variable = 0, Command, Auto };
+class Gotos {
+public:
+ /// \return the line_entry corresponding with \p id
+ ///
+ /// If \p id is invalid std::nullopt is returned.
+ std::optional<lldb::SBLineEntry> GetLineEntry(uint64_t id) const;
+
+ /// Insert a new \p line_entry.
+ /// \return id assigned to this line_entry.
+ uint64_t InsertLineEntry(lldb::SBLineEntry line_entry);
+
+ /// clears all line entries and reset the generated ids.
+ void Clear();
+
+private:
+ uint64_t NewSpecId();
+
+ llvm::DenseMap<uint64_t, lldb::SBLineEntry> line_entries;
+ uint64_t new_id = 0ul;
+};
+
struct Variables {
/// Variable_reference start index of permanent expandable variable.
static constexpr int64_t PermanentVariableStartIndex = (1ll << 32);
@@ -209,6 +230,7 @@ struct DAP {
// empty; if the previous expression was a variable expression, this string
// will contain that expression.
std::string last_nonempty_var_expression;
+ Gotos goto_id_map;
DAP(std::string name, llvm::StringRef path, std::ofstream *log,
lldb::IOObjectSP input, lldb::IOObjectSP output, ReplMode repl_mode,
@@ -352,7 +374,10 @@ struct DAP {
}
/// Debuggee will continue from stopped state.
- void WillContinue() { variables.Clear(); }
+ void WillContinue() {
+ variables.Clear();
+ goto_id_map.Clear();
+ }
/// Poll the process to wait for it to reach the eStateStopped state.
///
diff --git a/lldb/tools/lldb-dap/Handler/GoToRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/GoToRequestHandler.cpp
new file mode 100644
index 0000000000000..06a50eb939828
--- /dev/null
+++ b/lldb/tools/lldb-dap/Handler/GoToRequestHandler.cpp
@@ -0,0 +1,103 @@
+//===-- GoToRequestHandler.cpp --------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "DAP.h"
+#include "EventHelper.h"
+#include "JSONUtils.h"
+
+namespace lldb_dap {
+
+// "GotoRequest": {
+// "allOf": [ { "$ref": "#/definitions/Request" }, {
+// "type": "object",
+// "description": "The request sets the location where the debuggee will
+// continue to run.\nThis makes it possible to skip the execution of code or
+// to execute code again.\nThe code between the current location and the
+// goto target is not executed but skipped.\nThe debug adapter first sends
+// the response and then a `stopped` event with reason `goto`.\nClients
+// should only call this request if the corresponding capability
+// `supportsGotoTargetsRequest` is true (because only then goto targets
+// exist that can be passed as arguments).", "properties": {
+// "command": {
+// "type": "string",
+// "enum": [ "goto" ]
+// },
+// "arguments": {
+// "$ref": "#/definitions/GotoArguments"
+// }
+// },
+// "required": [ "command", "arguments" ]
+// }]
+// }
+// "GotoArguments": {
+// "type": "object",
+// "description": "Arguments for `goto` request.",
+// "properties": {
+// "threadId": {
+// "type": "integer",
+// "description": "Set the goto target for this thread."
+// },
+// "targetId": {
+// "type": "integer",
+// "description": "The location where the debuggee will continue to run."
+// }
+// },
+// "required": [ "threadId", "targetId" ]
+// }
+// "GotoResponse": {
+// "allOf": [ { "$ref": "#/definitions/Response" }, {
+// "type": "object",
+// "description": "Response to `goto` request. This is just an
+// acknowledgement, so no body field is required."
+// }]
+// }
+void GoToRequestHandler::operator()(const llvm::json::Object &request) const {
+ llvm::json::Object response;
+ FillResponse(request, response);
+
+ auto SendError = [&](auto &&message) {
+ response["success"] = false;
+ response["message"] = message;
+ dap.SendJSON(llvm::json::Value(std::move(response)));
+ };
+
+ const auto *goto_arguments = request.getObject("arguments");
+ if (goto_arguments == nullptr) {
+ SendError("Arguments is empty");
+ return;
+ }
+
+ lldb::SBThread current_thread = dap.GetLLDBThread(*goto_arguments);
+ if (!current_thread.IsValid()) {
+ SendError(llvm::formatv("Thread id `{0}` is not valid",
+ current_thread.GetThreadID()));
+ return;
+ }
+
+ const auto target_id = GetInteger<uint64_t>(goto_arguments, "targetId");
+ const auto line_entry = dap.goto_id_map.GetLineEntry(target_id.value());
+ if (!target_id || !line_entry) {
+ SendError(llvm::formatv("Target id `{0}` is not valid",
+ current_thread.GetThreadID()));
+ return;
+ }
+
+ auto file_spec = line_entry->GetFileSpec();
+ const auto error =
+ current_thread.JumpToLine(file_spec, line_entry->GetLine());
+ if (error.Fail()) {
+ SendError(error.GetCString());
+ return;
+ }
+
+ dap.SendJSON(llvm::json::Value(std::move(response)));
+
+ SendThreadStoppedEvent(dap);
+}
+
+} // namespace lldb_dap
\ No newline at end of file
diff --git a/lldb/tools/lldb-dap/Handler/GoToTargetsRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/GoToTargetsRequestHandler.cpp
new file mode 100644
index 0000000000000..9481055ee0119
--- /dev/null
+++ b/lldb/tools/lldb-dap/Handler/GoToTargetsRequestHandler.cpp
@@ -0,0 +1,120 @@
+//===-- GoToTargetsRequestHandler.cpp
+//--------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "DAP.h"
+
+#include "JSONUtils.h"
+
+#include <lldb/API/SBStream.h>
+
+namespace lldb_dap {
+
+// "GotoTargetsRequest": {
+// "allOf": [ { "$ref": "#/definitions/Request" }, {
+// "type": "object",
+// "description": "This request retrieves the possible goto targets for the
+// specified source location.\nThese targets can be used in the `goto`
+// request.\nClients should only call this request if the corresponding
+// capability `supportsGotoTargetsRequest` is true.", "properties": {
+// "command": {
+// "type": "string",
+// "enum": [ "gotoTargets" ]
+// },
+// "arguments": {
+// "$ref": "#/definitions/GotoTargetsArguments"
+// }
+// },
+// "required": [ "command", "arguments" ]
+// }]
+// },
+// "GotoTargetsArguments": {
+// "type": "object",
+// "description": "Arguments for `gotoTargets` request.",
+// "properties": {
+// "source": {
+// "$ref": "#/definitions/Source",
+// "description": "The source location for which the goto targets are
+// determined."
+// },
+// "line": {
+// "type": "integer",
+// "description": "The line location for which the goto targets are
+// determined."
+// },
+// "column": {
+// "type": "integer",
+// "description": "The position within `line` for which the goto targets
+// are determined. It is measured in UTF-16 code units and the client
+// capability `columnsStartAt1` determines whether it is 0- or 1-based."
+// }
+// },
+// "required": [ "source", "line" ]
+// },
+// "GotoTargetsResponse": {
+// "allOf": [ { "$ref": "#/definitions/Response" }, {
+// "type": "object",
+// "description": "Response to `gotoTargets` request.",
+// "properties": {
+// "body": {
+// "type": "object",
+// "properties": {
+// "targets": {
+// "type": "array",
+// "items": {
+// "$ref": "#/definitions/GotoTarget"
+// },
+// "description": "The possible goto targets of the specified
+// location."
+// }
+// },
+// "required": [ "targets" ]
+// }
+// },
+// "required": [ "body" ]
+// }]
+// },
+void GoToTargetsRequestHandler::operator()(
+ const llvm::json::Object &request) const {
+ llvm::json::Object response;
+ FillResponse(request, response);
+ const auto *arguments = request.getObject("arguments");
+ const auto *source = arguments->getObject("source");
+ const std::string path = GetString(source, "path").str();
+
+ const auto goto_line = GetInteger<uint64_t>(arguments, "line").value_or(0u);
+ const auto goto_column =
+ GetInteger<uint64_t>(arguments, "column").value_or(0u);
+
+ lldb::SBLineEntry line_entry{};
+ const lldb::SBFileSpec file_spec(path.c_str(), true);
+ line_entry.SetFileSpec(file_spec);
+ line_entry.SetLine(goto_line);
+ line_entry.SetColumn(goto_column);
+
+ const auto target_id = dap.goto_id_map.InsertLineEntry(line_entry);
+ llvm::json::Array response_targets;
+ const auto target_line = line_entry.GetLine();
+ const auto target_column = line_entry.GetColumn();
+ auto target = llvm::json::Object();
+ target.try_emplace("id", target_id);
+
+ lldb::SBStream stream;
+ line_entry.GetDescription(stream);
+ target.try_emplace("label",
+ llvm::StringRef(stream.GetData(), stream.GetSize()));
+ target.try_emplace("column", target_column);
+ target.try_emplace("line", target_line);
+
+ response_targets.push_back(std::move(target));
+ llvm::json::Object body;
+ body.try_emplace("targets", std::move(response_targets));
+ response.try_emplace("body", std::move(body));
+ dap.SendJSON(llvm::json::Value(std::move(response)));
+}
+} // namespace lldb_dap
diff --git a/lldb/tools/lldb-dap/Handler/InitializeRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/InitializeRequestHandler.cpp
index 5bb73a7ec0d85..12f292a612e6c 100644
--- a/lldb/tools/lldb-dap/Handler/InitializeRequestHandler.cpp
+++ b/lldb/tools/lldb-dap/Handler/InitializeRequestHandler.cpp
@@ -406,7 +406,7 @@ void InitializeRequestHandler::operator()(
// The debug adapter supports restarting a frame.
body.try_emplace("supportsRestartFrame", false);
// The debug adapter supports the gotoTargetsRequest.
- body.try_emplace("supportsGotoTargetsRequest", false);
+ body.try_emplace("supportsGotoTargetsRequest", true);
// The debug adapter supports the stepInTargetsRequest.
body.try_emplace("supportsStepInTargetsRequest", true);
// The debug adapter supports the completions request.
diff --git a/lldb/tools/lldb-dap/Handler/RequestHandler.h b/lldb/tools/lldb-dap/Handler/RequestHandler.h
index b44367518bcb9..c0f33684da7c4 100644
--- a/lldb/tools/lldb-dap/Handler/RequestHandler.h
+++ b/lldb/tools/lldb-dap/Handler/RequestHandler.h
@@ -113,6 +113,20 @@ class ExceptionInfoRequestHandler : public RequestHandler {
void operator()(const llvm::json::Object &request) const override;
};
+class GoToRequestHandler : public RequestHandler {
+public:
+ using RequestHandler::RequestHandler;
+ static llvm::StringLiteral getCommand() { return "goto"; }
+ void operator()(const llvm::json::Object &request) const override;
+};
+
+class GoToTargetsRequestHandler : public RequestHandler {
+public:
+ using RequestHandler::RequestHandler;
+ static llvm::StringLiteral getCommand() { return "gotoTargets"; }
+ void operator()(const llvm::json::Object &request) const override;
+};
+
class InitializeRequestHandler : public RequestHandler {
public:
using RequestHandler::RequestHandler;
diff --git a/lldb/tools/lldb-dap/lldb-dap.cpp b/lldb/tools/lldb-dap/lldb-dap.cpp
index a5d9978e30248..ef2a1d92010ca 100644
--- a/lldb/tools/lldb-dap/lldb-dap.cpp
+++ b/lldb/tools/lldb-dap/lldb-dap.cpp
@@ -124,6 +124,8 @@ static void RegisterRequestCallbacks(DAP &dap) {
dap.RegisterRequest<EvaluateRequestHandler>();
dap.RegisterRequest<ExceptionInfoRequestHandler>();
dap.RegisterRequest<InitializeRequestHandler>();
+ dap.RegisterRequest<GoToRequestHandler>();
+ dap.RegisterRequest<GoToTargetsRequestHandler>();
dap.RegisterRequest<LaunchRequestHandler>();
dap.RegisterRequest<LocationsRequestHandler>();
dap.RegisterRequest<NextRequestHandler>();
|
|
✅ With the latest revision this PR passed the Python code formatter. |
DavidSpickett
changed the title
da-viper
force-pushed
the
implement-jump-to-cursor
branch
from
1cd1abe to
07ec2d6
Compare
| lldb::SBLineEntry line_entry{}; | ||
| const lldb::SBFileSpec file_spec(path.c_str(), true); | ||
| line_entry.SetFileSpec(file_spec); | ||
| line_entry.SetLine(goto_line); | ||
| line_entry.SetColumn(goto_column); |
There was a problem hiding this comment.
Instead of simply forwarding the line + column as provided by the user, I think we should check where we can actually go-to.
E.g., if clicking on an empty line where we cannot continue, we might want to provide the two options "previous line" and "next line" to allow the user to specify more precisely where to go to. What are other debug adapters doing here?
There was a problem hiding this comment.
SBThread currently only provides jump to line and no way to check if you can jump to a line. If the current line is empty it correctly jump to the next line with statements.
I wanted to try validating it the line_entry with SBLineEntyry::IsValid but we cannot guarantee if the previous / next line is in the current scope or function.
There was a problem hiding this comment.
SBThread currently only provides jump to line
Does it actually jump to the line and ignore column information? Or does it also take the column information from the SBFileSpec into account?
no way to check if you can jump to a line
Could we use a similar approach to BreakpointLocationsRequestHandler, i.e. iterate through the line table? Potentially even factor out some logic from BreakpointLocationsRequestHandler to make it reusable also for Go-To?
There was a problem hiding this comment.
Does it actually jump to the line and ignore column information ?
Yes It ignores the column information. see
llvm-project/lldb/source/Target/Thread.cpp
Lines 1585 to 1599 in c337e2d
Could we use a similar approach to BreakpointLocationsRequestHandler
Will check it
There was a problem hiding this comment.
Somehow I think we shouldn't support jump based on columns. You might find yourself breaking the semantics of the code.
There was a problem hiding this comment.
@walter-erquinigo @vogelsgesang
I was thinking instead we only support jumps within a function for the same reason as above.
it would be something like this
def is_line_in_func(fn, line) ->:
# cannot use function_line_entry as it may not have a valid end entry
s_addr = fn.addr
e_addr = fn.end_addr
return (s_addr <= line.addr )and (line.addr <= e_addr)
def get_line_valid_entry(file_spec, line, target):
result = None
for units in target.compile_unit:
for line_entry in units.reverse():
result = line_entry
if line.GetLine() < line:
break
return SBLineEntry
line_entry = get_line_valid_entry()
// if it is not valid it is not in that file spec
return is_line_in_func(...) There was a problem hiding this comment.
I used breakpoints instead because they are able to resolve if the line location is valid. then use it to create targets for the goto request
There was a problem hiding this comment.
Somehow I think we shouldn't support jump based on columns. You might find yourself breaking the semantics of the code.
I think in an ideal world we would allow "go to" for all line entries marked as is_stmt. Currenlty, is_stmt is set also based on line numbers. There are also some discussions on improving the is_stmt on Discourse, though https://discourse.llvm.org/t/rfc-improving-is-stmt-placement-for-better-interactive-debugging/82668/13.
CC @labath, since you were also active on that discourse thread
lldb/test/API/tools/lldb-dap/gotoTargets/TestDAP_gotoTargets.py
Outdated
Show resolved
Hide resolved
lldb/test/API/tools/lldb-dap/gotoTargets/TestDAP_gotoTargets.py
Outdated
Show resolved
Hide resolved
lldb/test/API/tools/lldb-dap/gotoTargets/TestDAP_gotoTargets.py
Outdated
Show resolved
Hide resolved
| lldb::SBLineEntry line_entry{}; | ||
| const lldb::SBFileSpec file_spec(path.c_str(), true); | ||
| line_entry.SetFileSpec(file_spec); | ||
| line_entry.SetLine(goto_line); | ||
| line_entry.SetColumn(goto_column); |
There was a problem hiding this comment.
Somehow I think we shouldn't support jump based on columns. You might find yourself breaking the semantics of the code.
| lldb::SBListener listener = dap.debugger.GetListener(); | ||
| lldb::SBBroadcaster broadcaster = dap.target.GetBroadcaster(); | ||
| constexpr auto event_mask = lldb::SBTarget::eBroadcastBitBreakpointChanged; | ||
| listener.StopListeningForEvents(broadcaster, event_mask); |
There was a problem hiding this comment.
This seems somewhat iffy. It's tacitly synchronizing with the even thread which is going to be hard to reason about if we ever have to debug a breakpoint event not getting broadcast.
I wonder if we can avoid this complexity by using an internal breakpoint that doesn't have the dap label. I'm not sure if we broadcast breakpoint changed events for internal breakpoints (I would expected we don't but I didn't look at the code to confirm), but even if we do, we should ignore breakpoints that weren't created through lldb-dap (by checking the label).
There was a problem hiding this comment.
At the moment we do not broadcast breakpoint that is not owned by dap. But I think we should as it will make setting breakpoint from the debug console possible.
There was a problem hiding this comment.
yes, I think we shuold do that with broadcasting. The IDE should transparently behave well after cli commands.
I don't see clearly how broadcasting would simplify this code, but I trust Jonas. Having to stop listening for events for some time open the door to some hard-to-catch bugs and we should avoid that whenever possible.
There was a problem hiding this comment.
Seems like we are using the breakpoint API just so we can answer the questions "tell me all of the locations that map to source file and line". I would prefer to expose the source file + line resolving code in a SBTarget API so we can avoid having to use breakpoints just so we can get all of the locations. The API I am thinking would be:
lldb::SBSymbolContextList lldb::SBTarget::ResolveContexts(lldb::SBFileSpec &file_spec, uint32_t line);
And this returns the same info as the breakpoint location contexts. LLDB breakpoint code uses address resolvers and search filters, and we can just use the same APIs as the breakpoints.
There was a problem hiding this comment.
Using breakpoints and disabling notifications seems like a bad side affect of using the Breakpoint APIs to work around a missing and needed public API.
There was a problem hiding this comment.
A patch that adds the above APIs could be done in a separate patch. It can easily to unit tested.
There was a problem hiding this comment.
We might also want to add a parameter that specifies if we want to catch inline line entries, or just match line entries for compile units:
/// Find all locations for a file and line in this lldb::SBModule
lldb::SBSymbolContextList lldb::SBModule::ResolveContexts(lldb::SBFileSpec &file_spec, uint32_t line, bool find_inlines);
/// Find all locations for a file and line in all lldb::SBModule in the lldb::SBTarget
lldb::SBSymbolContextList lldb::SBTarget::ResolveContexts(lldb::SBFileSpec &file_spec, uint32_t line, bool find_inlines);
@JDevlieghere @jimingham let me know what you think of the above APIs? I think it would be very useful to expose the breakpoint location search features as an API.
There was a problem hiding this comment.
This seems generally useful, and it is silly to have to actually set breakpoints to do this kind of symbol search.
I think at this point we should always allow column information when we do file & line type queries along with the file & line. I almost wonder if we should have an SBLineEntrySpec to use for these sorts of queries so we don't have to repeat all the parameters. But regardless, we should allow users to also pass in column info here.
In the case of breakpoints we will allow inexact matches if the user asks for them. That's useful for setting breakpoints, but I'm not sure it's all that useful for a "ResolveContexts" type API. So I don't think you need to add a bool exact_match but you should document the behavior since people might be surprised if it doesn't match what break set of the same file & line returns.
There was a problem hiding this comment.
There are other types of queries that might be interesting here, like "file and line contained in another file or function" - when you're looking for inlining that's handy. I'm not saying that we should support those up front, but if we can think of a couple more things like that that we'd want to do, it might be worth coming up with a container (like SBLineEntrySpec) that we can then add to as we go w/o having to change the API's.
There was a problem hiding this comment.
If we add a new API, we should make sure it also covers the "get column breakpoints" use case. See the current source code which iterates over the line tables manually. In the PR which introduced column-breakpoint support for lldb-dap, we already had a similar discussion on exposing a new SB-API.
For the API, this would mean that:
- We should be able to search for a line-range and not only for a single line
- We should be able to restrict the column numbers
- We need a way to return all column positions within the line(s) on which we could potentially stop
da-viper
force-pushed
the
implement-jump-to-cursor
branch
from
a2a2cb8 to
bacb624
Compare
Co-authored-by: Adrian Vogelsgesang <adrian.vogelsgesang@tum.de>
Co-authored-by: Adrian Vogelsgesang <adrian.vogelsgesang@tum.de>
Co-authored-by: Adrian Vogelsgesang <adrian.vogelsgesang@tum.de>
LLDB currently does not have a `StopReason` for goto so I cannot use `SendThreadStoppedEvent` as I need to put the goto reason. Signed-off-by: Ebuka Ezike <yerimyah1@gmail.com>
…idation. Renamed test files, directories, and references from `gotoTarget` to `gotoTargets` for consistency. Added validation to confirm stopped events include a "goto" reason after goto requests in the test suite. Signed-off-by: Ebuka Ezike <yerimyah1@gmail.com>
Co-authored-by: Adrian Vogelsgesang <adrian.vogelsgesang@tum.de>
Co-authored-by: Adrian Vogelsgesang <adrian.vogelsgesang@tum.de>
Co-authored-by: Adrian Vogelsgesang <adrian.vogelsgesang@tum.de>
Co-authored-by: Adrian Vogelsgesang <adrian.vogelsgesang@tum.de>
da-viper
force-pushed
the
implement-jump-to-cursor
branch
from
a10b5a1 to
3ff3281
Compare
Remove disabling the breakpoint event listener. Some of the error are not meant to be user facing as they cannot do anything with the message received.
| lldb::SBListener listener = dap.debugger.GetListener(); | ||
| lldb::SBBroadcaster broadcaster = dap.target.GetBroadcaster(); | ||
| constexpr auto event_mask = lldb::SBTarget::eBroadcastBitBreakpointChanged; | ||
| listener.StopListeningForEvents(broadcaster, event_mask); |
There was a problem hiding this comment.
I think we can use the lldb.target.modules[0].FindCompileUnits() to find this information without using a breakpoint.
Trying this out on the stepInTargets binary:
$ lldb lldb-dap/stepInTargets/TestDAP_stepInTargets.test_basic/a.out
(lldb) script
>>>[line_entry for line_entry in lldb.target.FindCompileUnits(lldb.SBFileSpec("lldb/test/API/tools/lldb-dap/stepInTargets/main.cpp"))[0].compile_unit]
[
lldb/test/API/tools/lldb-dap/stepInTargets/main.cpp:2,
lldb/test/API/tools/lldb-dap/stepInTargets/main.cpp:2:38,
lldb/test/API/tools/lldb-dap/stepInTargets/main.cpp:2:44,
lldb/test/API/tools/lldb-dap/stepInTargets/main.cpp:2:42,
lldb/test/API/tools/lldb-dap/stepInTargets/main.cpp:2:31,
lldb/test/API/tools/lldb-dap/stepInTargets/main.cpp:4:15,
lldb/test/API/tools/lldb-dap/stepInTargets/main.cpp:6:15,
lldb/test/API/tools/lldb-dap/stepInTargets/main.cpp:8,
lldb/test/API/tools/lldb-dap/stepInTargets/main.cpp:9:7,
lldb/test/API/tools/lldb-dap/stepInTargets/main.cpp:9:16,
lldb/test/API/tools/lldb-dap/stepInTargets/main.cpp:9:3,
lldb/test/API/tools/lldb-dap/stepInTargets/main.cpp:10:3,
]
I'm not sure how we should scope those. We could use the block to scope the targets to the same block as the requested file:line:column.
Fixes #109335
Depends on #130435