[lldb-dap] assembly breakpoints #139969
Merged
[lldb-dap] assembly breakpoints #139969
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
eronnen
force-pushed
the
lldb-dap-assembly-breakpoint
branch
2 times, most recently
from
2263631 to
3807a12
Compare
eronnen
force-pushed
the
lldb-dap-assembly-breakpoint
branch
from
0f2ed75 to
f5fd76d
Compare
|
@llvm/pr-subscribers-lldb Author: Ely Ronnen (eronnen) Changes
Screencast.From.2025-05-17.23-57-30.webmPatch is 29.52 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/139969.diff 19 Files Affected:
diff --git a/lldb/include/lldb/API/SBFileSpec.h b/lldb/include/lldb/API/SBFileSpec.h
index 36641843aabeb..303cb7d712cbf 100644
--- a/lldb/include/lldb/API/SBFileSpec.h
+++ b/lldb/include/lldb/API/SBFileSpec.h
@@ -10,6 +10,7 @@
#define LLDB_API_SBFILESPEC_H
#include "lldb/API/SBDefines.h"
+#include "lldb/API/SBStream.h"
namespace lldb {
@@ -53,6 +54,8 @@ class LLDB_API SBFileSpec {
uint32_t GetPath(char *dst_path, size_t dst_len) const;
+ bool GetPath(lldb::SBStream &dst_path) const;
+
static int ResolvePath(const char *src_path, char *dst_path, size_t dst_len);
bool GetDescription(lldb::SBStream &description) const;
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 70fd0b0c419db..4a907a5e36901 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
@@ -955,6 +955,13 @@ def request_setBreakpoints(self, file_path, line_array, data=None):
"""
(dir, base) = os.path.split(file_path)
source_dict = {"name": base, "path": file_path}
+ return self.request_setBreakpoints_with_source(source_dict, line_array, data)
+
+ def request_setBreakpointsAssembly(self, sourceReference, line_array, data=None):
+ source_dict = {"sourceReference": sourceReference}
+ return self.request_setBreakpoints_with_source(source_dict, line_array, data)
+
+ def request_setBreakpoints_with_source(self, source_dict, line_array, data=None):
args_dict = {
"source": source_dict,
"sourceModified": False,
diff --git a/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/lldbdap_testcase.py b/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/lldbdap_testcase.py
index afdc746ed0d0d..427f66a7da0c8 100644
--- a/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/lldbdap_testcase.py
+++ b/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/lldbdap_testcase.py
@@ -63,6 +63,16 @@ def set_source_breakpoints(self, source_path, lines, data=None):
for breakpoint in breakpoints:
breakpoint_ids.append("%i" % (breakpoint["id"]))
return breakpoint_ids
+
+ def set_source_breakpoints_assembly(self, source_reference, lines, data=None):
+ response = self.dap_server.request_setBreakpointsAssembly(source_reference, lines, data)
+ if response is None:
+ return []
+ breakpoints = response["body"]["breakpoints"]
+ breakpoint_ids = []
+ for breakpoint in breakpoints:
+ breakpoint_ids.append("%i" % (breakpoint["id"]))
+ return breakpoint_ids
def set_function_breakpoints(self, functions, condition=None, hitCondition=None):
"""Sets breakpoints by function name given an array of function names
diff --git a/lldb/source/API/SBFileSpec.cpp b/lldb/source/API/SBFileSpec.cpp
index a7df9afc4b8eb..cb44dac1d4fcc 100644
--- a/lldb/source/API/SBFileSpec.cpp
+++ b/lldb/source/API/SBFileSpec.cpp
@@ -19,6 +19,7 @@
#include <cinttypes>
#include <climits>
+#include <string>
using namespace lldb;
using namespace lldb_private;
@@ -147,6 +148,13 @@ uint32_t SBFileSpec::GetPath(char *dst_path, size_t dst_len) const {
return result;
}
+bool SBFileSpec::GetPath(SBStream &dst_path) const {
+ LLDB_INSTRUMENT_VA(this, dst_path);
+
+ std::string path = m_opaque_up->GetPath();
+ return dst_path->PutCString(path.c_str()) > 0;
+}
+
const lldb_private::FileSpec *SBFileSpec::operator->() const {
return m_opaque_up.get();
}
diff --git a/lldb/test/API/tools/lldb-dap/breakpoint-assembly/Makefile b/lldb/test/API/tools/lldb-dap/breakpoint-assembly/Makefile
new file mode 100644
index 0000000000000..10495940055b6
--- /dev/null
+++ b/lldb/test/API/tools/lldb-dap/breakpoint-assembly/Makefile
@@ -0,0 +1,3 @@
+C_SOURCES := main.c
+
+include Makefile.rules
diff --git a/lldb/test/API/tools/lldb-dap/breakpoint-assembly/TestDAP_breakpointAssembly.py b/lldb/test/API/tools/lldb-dap/breakpoint-assembly/TestDAP_breakpointAssembly.py
new file mode 100644
index 0000000000000..ba9df3a18590b
--- /dev/null
+++ b/lldb/test/API/tools/lldb-dap/breakpoint-assembly/TestDAP_breakpointAssembly.py
@@ -0,0 +1,42 @@
+"""
+Test lldb-dap setBreakpoints request
+"""
+
+
+import dap_server
+import shutil
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+import lldbdap_testcase
+import os
+
+
+class TestDAP_setBreakpointsAssembly(lldbdap_testcase.DAPTestCaseBase):
+ # @skipIfWindows
+ def test_functionality(self):
+ """Tests hitting assembly source breakpoints"""
+ program = self.getBuildArtifact("a.out")
+ self.build_and_launch(program)
+
+ self.dap_server.request_evaluate(
+ "`settings set stop-disassembly-display no-debuginfo", context="repl"
+ )
+
+ assmebly_func_breakpoints = self.set_function_breakpoints(["assembly_func"])
+ self.continue_to_breakpoints(assmebly_func_breakpoints)
+
+ assembly_func_frame = self.get_stackFrames()[0]
+ self.assertIn(
+ "sourceReference",
+ assembly_func_frame.get("source"),
+ "Expected assembly source frame",
+ )
+
+ line = assembly_func_frame["line"]
+
+ # Set an assembly breakpoint in the next line and check that it's hit
+ assembly_breakpoint_ids = self.set_source_breakpoints_assembly(
+ assembly_func_frame["source"]["sourceReference"], [line + 1]
+ )
+ self.continue_to_breakpoints(assembly_breakpoint_ids)
diff --git a/lldb/test/API/tools/lldb-dap/breakpoint-assembly/main.c b/lldb/test/API/tools/lldb-dap/breakpoint-assembly/main.c
new file mode 100644
index 0000000000000..350739006f903
--- /dev/null
+++ b/lldb/test/API/tools/lldb-dap/breakpoint-assembly/main.c
@@ -0,0 +1,14 @@
+#include <stddef.h>
+
+__attribute__((nodebug)) int assembly_func(int n) {
+ n += 1;
+ n += 2;
+ n += 3;
+
+ return n;
+}
+
+int main(int argc, char const *argv[]) {
+ assembly_func(10);
+ return 0;
+}
diff --git a/lldb/tools/lldb-dap/Breakpoint.cpp b/lldb/tools/lldb-dap/Breakpoint.cpp
index 26d633d1d172e..a54a34e0f936d 100644
--- a/lldb/tools/lldb-dap/Breakpoint.cpp
+++ b/lldb/tools/lldb-dap/Breakpoint.cpp
@@ -9,10 +9,12 @@
#include "Breakpoint.h"
#include "DAP.h"
#include "JSONUtils.h"
+#include "LLDBUtils.h"
#include "lldb/API/SBAddress.h"
#include "lldb/API/SBBreakpointLocation.h"
#include "lldb/API/SBLineEntry.h"
#include "lldb/API/SBMutex.h"
+#include "lldb/lldb-enumerations.h"
#include "llvm/ADT/StringExtras.h"
#include <cstddef>
#include <cstdint>
@@ -63,14 +65,31 @@ protocol::Breakpoint Breakpoint::ToProtocolBreakpoint() {
std::string formatted_addr =
"0x" + llvm::utohexstr(bp_addr.GetLoadAddress(m_bp.GetTarget()));
breakpoint.instructionReference = formatted_addr;
+
+ lldb::StopDisassemblyType stop_disassembly_display =
+ GetStopDisassemblyDisplay(m_dap.debugger);
auto line_entry = bp_addr.GetLineEntry();
- const auto line = line_entry.GetLine();
- if (line != UINT32_MAX)
- breakpoint.line = line;
- const auto column = line_entry.GetColumn();
- if (column != 0)
- breakpoint.column = column;
- breakpoint.source = CreateSource(line_entry);
+ if (!ShouldDisplayAssemblySource(line_entry, stop_disassembly_display)) {
+ const auto line = line_entry.GetLine();
+ if (line != UINT32_MAX)
+ breakpoint.line = line;
+ const auto column = line_entry.GetColumn();
+ if (column != 0)
+ breakpoint.column = column;
+ breakpoint.source = CreateSource(line_entry);
+ } else {
+ // Breakpoint made by assembly
+ auto symbol = bp_addr.GetSymbol();
+ if (symbol.IsValid()) {
+ breakpoint.line =
+ m_bp.GetTarget()
+ .ReadInstructions(symbol.GetStartAddress(), bp_addr, nullptr)
+ .GetSize() +
+ 1;
+
+ breakpoint.source = CreateAssemblySource(m_dap.target, bp_addr);
+ }
+ }
}
return breakpoint;
diff --git a/lldb/tools/lldb-dap/DAP.h b/lldb/tools/lldb-dap/DAP.h
index 8f24c6cf82924..b0fe265b7bca1 100644
--- a/lldb/tools/lldb-dap/DAP.h
+++ b/lldb/tools/lldb-dap/DAP.h
@@ -169,6 +169,8 @@ struct DAP {
Variables variables;
lldb::SBBroadcaster broadcaster;
llvm::StringMap<SourceBreakpointMap> source_breakpoints;
+ llvm::DenseMap<int64_t, llvm::DenseMap<uint32_t, SourceBreakpoint>>
+ assembly_breakpoints;
FunctionBreakpointMap function_breakpoints;
InstructionBreakpointMap instruction_breakpoints;
std::optional<std::vector<ExceptionBreakpoint>> exception_breakpoints;
@@ -219,6 +221,9 @@ struct DAP {
llvm::StringSet<> modules;
/// @}
+ /// Number of lines of assembly code to show when no debug info is available.
+ uint32_t number_of_assembly_lines_for_nodebug = 32;
+
/// Creates a new DAP sessions.
///
/// \param[in] log
diff --git a/lldb/tools/lldb-dap/Handler/BreakpointLocationsHandler.cpp b/lldb/tools/lldb-dap/Handler/BreakpointLocationsHandler.cpp
index 2ac886c3a5d2c..c4d658caeee2d 100644
--- a/lldb/tools/lldb-dap/Handler/BreakpointLocationsHandler.cpp
+++ b/lldb/tools/lldb-dap/Handler/BreakpointLocationsHandler.cpp
@@ -7,7 +7,6 @@
//===----------------------------------------------------------------------===//
#include "DAP.h"
-#include "JSONUtils.h"
#include "RequestHandler.h"
#include <vector>
@@ -19,19 +18,50 @@ namespace lldb_dap {
llvm::Expected<protocol::BreakpointLocationsResponseBody>
BreakpointLocationsRequestHandler::Run(
const protocol::BreakpointLocationsArguments &args) const {
- std::string path = args.source.path.value_or("");
uint32_t start_line = args.line;
uint32_t start_column = args.column.value_or(LLDB_INVALID_COLUMN_NUMBER);
uint32_t end_line = args.endLine.value_or(start_line);
uint32_t end_column =
args.endColumn.value_or(std::numeric_limits<uint32_t>::max());
+ // Find all relevant lines & columns
+ llvm::SmallVector<std::pair<uint32_t, uint32_t>, 8> locations;
+ if (args.source.sourceReference) {
+ AddAssemblyBreakpointLocations(locations, *args.source.sourceReference,
+ start_line, end_line);
+ } else {
+ std::string path = args.source.path.value_or("");
+ AddSourceBreakpointLocations(locations, std::move(path), start_line,
+ start_column, end_line, end_column);
+ }
+
+ // The line entries are sorted by addresses, but we must return the list
+ // ordered by line / column position.
+ std::sort(locations.begin(), locations.end());
+ locations.erase(llvm::unique(locations), locations.end());
+
+ std::vector<protocol::BreakpointLocation> breakpoint_locations;
+ for (auto &l : locations) {
+ protocol::BreakpointLocation lc;
+ lc.line = l.first;
+ lc.column = l.second;
+ breakpoint_locations.push_back(std::move(lc));
+ }
+
+ return protocol::BreakpointLocationsResponseBody{
+ /*breakpoints=*/std::move(breakpoint_locations)};
+}
+
+template <unsigned N>
+void BreakpointLocationsRequestHandler::AddSourceBreakpointLocations(
+ llvm::SmallVector<std::pair<uint32_t, uint32_t>, N> &locations,
+ std::string path, uint32_t start_line, uint32_t start_column,
+ uint32_t end_line, uint32_t end_column) const {
+
lldb::SBFileSpec file_spec(path.c_str(), true);
lldb::SBSymbolContextList compile_units =
dap.target.FindCompileUnits(file_spec);
- // Find all relevant lines & columns
- llvm::SmallVector<std::pair<uint32_t, uint32_t>, 8> locations;
for (uint32_t c_idx = 0, c_limit = compile_units.GetSize(); c_idx < c_limit;
++c_idx) {
const lldb::SBCompileUnit &compile_unit =
@@ -71,22 +101,26 @@ BreakpointLocationsRequestHandler::Run(
locations.emplace_back(line, column);
}
}
+}
- // The line entries are sorted by addresses, but we must return the list
- // ordered by line / column position.
- std::sort(locations.begin(), locations.end());
- locations.erase(llvm::unique(locations), locations.end());
+template <unsigned N>
+void BreakpointLocationsRequestHandler::AddAssemblyBreakpointLocations(
+ llvm::SmallVector<std::pair<uint32_t, uint32_t>, N> &locations,
+ int64_t sourceReference, uint32_t start_line, uint32_t end_line) const {
+ lldb::SBAddress address(sourceReference, dap.target);
+ if (!address.IsValid())
+ return;
- std::vector<protocol::BreakpointLocation> breakpoint_locations;
- for (auto &l : locations) {
- protocol::BreakpointLocation lc;
- lc.line = l.first;
- lc.column = l.second;
- breakpoint_locations.push_back(std::move(lc));
- }
+ lldb::SBSymbol symbol = address.GetSymbol();
+ if (!symbol.IsValid())
+ return;
- return protocol::BreakpointLocationsResponseBody{
- /*breakpoints=*/std::move(breakpoint_locations)};
+ // start_line is relative to the symbol's start address
+ lldb::SBInstructionList insts = symbol.GetInstructions(dap.target);
+ for (uint32_t i = start_line - 1; i < insts.GetSize() && i <= (end_line - 1);
+ ++i) {
+ locations.emplace_back(i, 1);
+ }
}
} // namespace lldb_dap
diff --git a/lldb/tools/lldb-dap/Handler/RequestHandler.h b/lldb/tools/lldb-dap/Handler/RequestHandler.h
index e6bccfe12f402..80898d1ee5ef1 100644
--- a/lldb/tools/lldb-dap/Handler/RequestHandler.h
+++ b/lldb/tools/lldb-dap/Handler/RequestHandler.h
@@ -16,6 +16,7 @@
#include "Protocol/ProtocolRequests.h"
#include "Protocol/ProtocolTypes.h"
#include "llvm/ADT/DenseSet.h"
+#include "llvm/ADT/SmallVector.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/Support/Error.h"
#include "llvm/Support/JSON.h"
@@ -232,6 +233,16 @@ class BreakpointLocationsRequestHandler
}
llvm::Expected<protocol::BreakpointLocationsResponseBody>
Run(const protocol::BreakpointLocationsArguments &args) const override;
+
+ template <unsigned N>
+ void AddSourceBreakpointLocations(
+ llvm::SmallVector<std::pair<uint32_t, uint32_t>, N> &locations,
+ std::string path, uint32_t start_line, uint32_t start_column,
+ uint32_t end_line, uint32_t end_column) const;
+ template <unsigned N>
+ void AddAssemblyBreakpointLocations(
+ llvm::SmallVector<std::pair<uint32_t, uint32_t>, N> &locations,
+ int64_t sourceReference, uint32_t start_line, uint32_t end_line) const;
};
class CompletionsRequestHandler : public LegacyRequestHandler {
@@ -378,6 +389,15 @@ class SetBreakpointsRequestHandler
}
llvm::Expected<protocol::SetBreakpointsResponseBody>
Run(const protocol::SetBreakpointsArguments &args) const override;
+
+ std::vector<protocol::Breakpoint> SetSourceBreakpoints(
+ const protocol::Source &source,
+ const std::optional<std::vector<protocol::SourceBreakpoint>> &breakpoints)
+ const;
+ std::vector<protocol::Breakpoint> SetAssemblyBreakpoints(
+ const protocol::Source &source,
+ const std::optional<std::vector<protocol::SourceBreakpoint>> &breakpoints)
+ const;
};
class SetExceptionBreakpointsRequestHandler : public LegacyRequestHandler {
diff --git a/lldb/tools/lldb-dap/Handler/SetBreakpointsRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/SetBreakpointsRequestHandler.cpp
index 86e090b66afe9..7b401f06e9a85 100644
--- a/lldb/tools/lldb-dap/Handler/SetBreakpointsRequestHandler.cpp
+++ b/lldb/tools/lldb-dap/Handler/SetBreakpointsRequestHandler.cpp
@@ -23,15 +23,29 @@ llvm::Expected<protocol::SetBreakpointsResponseBody>
SetBreakpointsRequestHandler::Run(
const protocol::SetBreakpointsArguments &args) const {
const auto &source = args.source;
- const auto path = source.path.value_or("");
std::vector<protocol::Breakpoint> response_breakpoints;
+ if (source.sourceReference)
+ response_breakpoints = SetAssemblyBreakpoints(source, args.breakpoints);
+ else if (source.path)
+ response_breakpoints = SetSourceBreakpoints(source, args.breakpoints);
+
+ return protocol::SetBreakpointsResponseBody{std::move(response_breakpoints)};
+}
+
+std::vector<protocol::Breakpoint>
+SetBreakpointsRequestHandler::SetSourceBreakpoints(
+ const protocol::Source &source,
+ const std::optional<std::vector<protocol::SourceBreakpoint>> &breakpoints)
+ const {
+ std::vector<protocol::Breakpoint> response_breakpoints;
+ std::string path = source.path.value_or("");
// Decode the source breakpoint infos for this "setBreakpoints" request
SourceBreakpointMap request_bps;
// "breakpoints" may be unset, in which case we treat it the same as being set
// to an empty array.
- if (args.breakpoints) {
- for (const auto &bp : *args.breakpoints) {
+ if (breakpoints) {
+ for (const auto &bp : *breakpoints) {
SourceBreakpoint src_bp(dap, bp);
std::pair<uint32_t, uint32_t> bp_pos(src_bp.GetLine(),
src_bp.GetColumn());
@@ -73,7 +87,64 @@ SetBreakpointsRequestHandler::Run(
}
}
- return protocol::SetBreakpointsResponseBody{std::move(response_breakpoints)};
+ return response_breakpoints;
+}
+
+std::vector<protocol::Breakpoint>
+SetBreakpointsRequestHandler::SetAssemblyBreakpoints(
+ const protocol::Source &source,
+ const std::optional<std::vector<protocol::SourceBreakpoint>> &breakpoints)
+ const {
+ std::vector<protocol::Breakpoint> response_breakpoints;
+ int64_t sourceReference = source.sourceReference.value_or(0);
+
+ lldb::SBAddress address(sourceReference, dap.target);
+ if (!address.IsValid())
+ return response_breakpoints;
+
+ lldb::SBSymbol symbol = address.GetSymbol();
+ if (!symbol.IsValid())
+ return response_breakpoints; // Not yet supporting breakpoints in assembly
+ // without a valid symbol
+
+ llvm::DenseMap<uint32_t, SourceBreakpoint> request_bps;
+ if (breakpoints) {
+ for (const auto &bp : *breakpoints) {
+ SourceBreakpoint src_bp(dap, bp);
+ request_bps.try_emplace(src_bp.GetLine(), src_bp);
+ const auto [iv, inserted] =
+ dap.assembly_breakpoints[sourceReference].try_emplace(
+ src_bp.GetLine(), src_bp);
+ // We check if this breakpoint already exists to update it
+ if (inserted)
+ iv->getSecond().SetBreakpoint(symbol);
+ else
+ iv->getSecond().UpdateBreakpoint(src_bp);
+
+ protocol::Breakpoint response_bp = iv->getSecond().ToProtocolBreakpoint();
+ response_bp.source = source;
+ if (!response_bp.line)
+ response_bp.line = src_bp.GetLine();
+ if (bp.column)
+ response_bp.column = *bp.column;
+ response_breakpoints.push_back(response_bp);
+ }
+ }
+
+ // Delete existing breakpoints for this sourceReference that are not in the
+ // request_bps set.
+ auto old_src_bp_pos = dap.assembly_breakpoints.find(sourceReference);
+ if (old_src_bp_pos != dap.assembly_breakpoints.end()) {
+ for (auto &old_bp : old_src_bp_pos->second) {
+ auto request_pos = request_bps.find(old_bp.first);
+ if (request_pos == request_bps.end()) {
+ dap.target.BreakpointDelete(old_bp.second.GetID());
+ old_src_bp_pos->second.erase(old_bp.first);
+ }
+ }
+ }
+
+ return response_breakpoints;
}
} // namespace lldb_dap
diff --git a/lldb/tools/lldb-dap/Handler/SourceRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/SourceRequestHandler.cpp
index 0ddd87881a164..9249e2aa6fef7 100644
--- a/lldb/tools/lldb-dap/Handler/SourceRequestHandler.cpp
+++ b/lldb/tools/lldb-dap/Handler/SourceRequestHandler.cpp
@@ -11,6 +11,7 @@
#include "LLDBUtils.h"
#include "Protocol/ProtocolRequests.h"
#include "Protocol/ProtocolTypes.h"
+#include "lldb/API/SBAddress.h"
#include "lldb/API/SBExecutionContext.h"
#include "lldb/API/SBFrame.h"
#include "lldb/API/SBInstructionList.h"
@@ -19,6 +20,7 @@
#include "lldb/API/SBSymbol.h"
#include "lldb/API/SBTarget.h"
#include "lldb/API/SBThread.h"
+#include "lldb/lldb-types.h"
#include "llvm/Support/Error.h"
namespace lldb_dap {
@@ -34,26 +36,22 @@ SourceRequestHandler::Run(const protocol::SourceArguments &args) const {
return llvm::make_error<DAPError>(
"invalid arguments, expected source.sourceReference to be set");
- lldb::SBProcess process = dap.target.GetProcess();
- // Upper 32 bits is the thread index ID
- lldb::SBThread thread =
- process.GetThreadByIndexID(GetLLDBThreadIndexID(source));
- ...
[truncated]
|
|
✅ With the latest revision this PR passed the Python code formatter. |
lldb/include/lldb/API/SBFileSpec.h
Outdated
|
|
||
| uint32_t GetPath(char *dst_path, size_t dst_len) const; | ||
|
|
||
| bool GetPath(lldb::SBStream &dst_path) const; |
There was a problem hiding this comment.
I have mixed feelings about adding this API. Generally, we usually use streams when we have potentially multiline output which isn't the case. Here I'm also worried about setting a precedent that will lead to having more methods overloaded with streams. If this were private API I wouldn't mind, but since we guarantee ABI stability for the SB API, once an API has been added, we have to support it forever.
How about adding a helper to LLDBUtils that takes a SBFileSpec and returns the path as a std::string?
There was a problem hiding this comment.
makes sense, I was trying to avoid the potential problem of the path being longer than MAX_PATH but it's still possible to get the real size using GetFilename and GetDirectory
lldb/tools/lldb-dap/DAP.h
Outdated
| /// @} | ||
|
|
||
| /// Number of lines of assembly code to show when no debug info is available. | ||
| uint32_t number_of_assembly_lines_for_nodebug = 32; |
There was a problem hiding this comment.
Is this a constant?
Suggested change
| uint32_t number_of_assembly_lines_for_nodebug = 32; | |
| static constexpr uint32_t number_of_assembly_lines_for_nodebug = 32; |
Comment on lines
106
to
108
| template <unsigned N> | ||
| void BreakpointLocationsRequestHandler::AddAssemblyBreakpointLocations( | ||
| llvm::SmallVector<std::pair<uint32_t, uint32_t>, N> &locations, |
There was a problem hiding this comment.
You can avoid the template by taking a reference to a SmallVectorImpl<std::pair<uint32_t, uint32_t>>, which SmallVector inherits from.
There was a problem hiding this comment.
Looking at how this is called, maybe you don't even need to take this by reference, and you can have this return a llvm::SmallVector<std::pair<uint32_t, uint32_t>, 8>. Honestly I don't even know if the SmallVector optimization is worth it here, personally I would've just returned a regular std::vector. If you change the return type, you can also change the name of the method from Add to Get. Now it looks like there's potentially multiple calls adding values to the location list.
There was a problem hiding this comment.
Agreem chganged to return std::vector
Comment on lines
106
to
108
| if (!symbol.IsValid()) | ||
| return response_breakpoints; // Not yet supporting breakpoints in assembly | ||
| // without a valid symbol |
There was a problem hiding this comment.
Don't worry about adding the braces when you have a comment.
Suggested change
| if (!symbol.IsValid()) | |
| return response_breakpoints; // Not yet supporting breakpoints in assembly | |
| // without a valid symbol | |
| if (!symbol.IsValid()) { | |
| // Not yet supporting breakpoints in assembly without a valid symbol. | |
| return response_breakpoints; | |
| } |
| const auto [iv, inserted] = | ||
| dap.assembly_breakpoints[sourceReference].try_emplace( | ||
| src_bp.GetLine(), src_bp); | ||
| // We check if this breakpoint already exists to update it |
There was a problem hiding this comment.
Suggested change
| // We check if this breakpoint already exists to update it | |
| // We check if this breakpoint already exists to update it. |
| uint32_t end_column = | ||
| args.endColumn.value_or(std::numeric_limits<uint32_t>::max()); | ||
|
|
||
| // Find all relevant lines & columns |
There was a problem hiding this comment.
Suggested change
| // Find all relevant lines & columns | |
| // Find all relevant lines & columns. |
Comment on lines
44
to
49
| for (auto &l : locations) { | ||
| protocol::BreakpointLocation lc; | ||
| lc.line = l.first; | ||
| lc.column = l.second; | ||
| breakpoint_locations.push_back(std::move(lc)); | ||
| } |
There was a problem hiding this comment.
I think you should be able to create the locations in place like this:
Suggested change
| for (auto &l : locations) { | |
| protocol::BreakpointLocation lc; | |
| lc.line = l.first; | |
| lc.column = l.second; | |
| breakpoint_locations.push_back(std::move(lc)); | |
| } | |
| for (auto &l : locations) | |
| breakpoint_locations.emplace_back({l.first, l.second}); |
There was a problem hiding this comment.
For some reason got no matching constructor for initialization, but the following works
breakpoint_locations.push_back({l.first, l.second, std::nullopt, std::nullopt});| return response_breakpoints; // Not yet supporting breakpoints in assembly | ||
| // without a valid symbol | ||
|
|
||
| llvm::DenseMap<uint32_t, SourceBreakpoint> request_bps; |
There was a problem hiding this comment.
Let's add a comment saying what the key is, or better change the variable name to make it obvious: line_to_requested_source_bps or something.
There was a problem hiding this comment.
Moved this to the DAP abstraction
Comment on lines
115
to
116
| const auto [iv, inserted] = | ||
| dap.assembly_breakpoints[sourceReference].try_emplace( |
There was a problem hiding this comment.
This together with the logic to either add or update a breakpoint seems like it could be abstracted away behind a function in the dap class. Something like dap.SetAssemblyBreakpoint(uint32_t line, SourceBreakpoint& source_bp). Same thing with the delete below and then we can make assembly_breakpoints a member and provide some more abstraction.
There was a problem hiding this comment.
💯 Great idea, moved this implementation to a DAP function using the same function for both source and assembly breakpoints
eronnen
commented
May 18, 2025
| namespace lldb_dap { | ||
|
|
||
| typedef llvm::DenseMap<std::pair<uint32_t, uint32_t>, SourceBreakpoint> | ||
| typedef std::map<std::pair<uint32_t, uint32_t>, SourceBreakpoint> |
There was a problem hiding this comment.
changed to std::map because I'm not sure how to remove from the map while iterating it with DenseMap
da-viper
requested changes
May 19, 2025
lldb/test/API/tools/lldb-dap/breakpoint-assembly/TestDAP_breakpointAssembly.py
Outdated
Show resolved
Hide resolved
ashgti
reviewed
May 19, 2025
Comment on lines
958
to
964
| return self.request_setBreakpoints_with_source(source_dict, line_array, data) | ||
|
|
||
| def request_setBreakpointsAssembly(self, sourceReference, line_array, data=None): | ||
| source_dict = {"sourceReference": sourceReference} | ||
| return self.request_setBreakpoints_with_source(source_dict, line_array, data) | ||
|
|
||
| def request_setBreakpoints_with_source(self, source_dict, line_array, data=None): |
There was a problem hiding this comment.
Could we keep the request_* names just the DAP requests and make a helper without the request_* prefix for this? I like the consistency of having request_* being able to map to specific request and I think its valuable to keep that.
| @@ -0,0 +1,55 @@ | |||
| """ | |||
| Test lldb-dap setBreakpoints request | |||
There was a problem hiding this comment.
Test lldb-dap setBreakpoints in assembly source references?
|
|
||
| class TestDAP_setBreakpointsAssembly(lldbdap_testcase.DAPTestCaseBase): | ||
| # @skipIfWindows | ||
| def test_functionality(self): |
There was a problem hiding this comment.
test_can_break_in_source_references?
| if (source.sourceReference) { | ||
| // breakpoint set by assembly source. | ||
| lldb::SBAddress source_address(*source.sourceReference, m_dap.target); | ||
| if (!source_address.IsValid()) |
There was a problem hiding this comment.
Should we return an error? Or maybe log that this is invalid?
There was a problem hiding this comment.
Good idea, seems that the DAP client should even display an error if we return it as an invalid breakpoint with a message
Comment on lines
+479
to
+480
| llvm::StringMap<SourceBreakpointMap> m_source_breakpoints; | ||
| llvm::DenseMap<int64_t, SourceBreakpointMap> m_source_assembly_breakpoints; |
There was a problem hiding this comment.
If we made protocol::Source work in a std::map (or llvm::DenseMap) as a key, could we merge these? That could simplify things and make this more consistent between the two.
There was a problem hiding this comment.
I think it should be pretty simplified already since the only access to these maps is abstracted by DAP::SetSourceBreakpoints
It should be possible to use Source as a key but I think it might introduce new complications like implementing a hash / ordering for the struct, and there are extra fields in Source like name and presentationHint
…oints in python tests to support both paths and source_reference
da-viper
reviewed
May 19, 2025
ashgti
approved these changes
May 19, 2025
| return self.send_recv(command_dict) | ||
|
|
||
| def request_setBreakpoints(self, file_path, line_array, data=None): | ||
| def request_setBreakpoints(self, source_dict, line_array, data=None): |
There was a problem hiding this comment.
What about:
from typing import TypedDict, Optional
class Source(TypedDict, total=False): # At runtime, this is just a dict with well known keys
name: str
path: str
sourceReference: int
class SourceBreakpoint(TypedDict, total=False):
line: int
column: str
condition: str
hitCondition: str
logMessage: str
def request_setBreakpoints(
self,
# For backwards compatibility, leaving these as positional args
sourcePath: Optional[str] = None,
lines: Optional[list[int]] = None,
breakpoints: Optional[list[SourceBreakpoint]] = None,
*,
source: Optional[Source] = None,
sourceReference: Optional[int] = None,
):
source_dict = {}
if source is not None:
source_dict = source
elif sourcePath is not None:
source_dict["name"] = os.path.basename(sourcePath)
source_dict["pah"] = sourcePath
elif sourceReference is not None:
source_dict["sourceReference"] = sourceReference
else:
raise ValueError("'source', 'sourcePath' or 'sourceReference' must be set')
args_dict = {
"source": source_dict,
"sourceModified": False,
}
...Then we wouldn't need the get_source_for_path and get_source_for_source_reference helpers, since those would be built based on the args of the request.
I could also look at adding this as a follow up to this.
There was a problem hiding this comment.
Nice idea, I added a simple Source class for this, let me know what you think
ashgti
approved these changes
May 20, 2025
lldb/tools/lldb-dap/Breakpoint.cpp
Outdated
| breakpoint.column = column; | ||
| breakpoint.source = CreateSource(line_entry); | ||
| } else { | ||
| // Breakpoint made by assembly |
There was a problem hiding this comment.
Suggested change
| // Breakpoint made by assembly | |
| // Assembly breakpoint. |
lldb/tools/lldb-dap/DAP.cpp
Outdated
| const std::optional<std::vector<protocol::SourceBreakpoint>> &breakpoints) { | ||
| std::vector<protocol::Breakpoint> response_breakpoints; | ||
| if (source.sourceReference) { | ||
| // breakpoint set by assembly source. |
There was a problem hiding this comment.
Suggested change
| // breakpoint set by assembly source. | |
| // Breakpoint set by assembly source. |
lldb/tools/lldb-dap/DAP.cpp
Outdated
| response_breakpoints = | ||
| SetSourceBreakpoints(source, breakpoints, existing_breakpoints); | ||
| } else { | ||
| // breakpoint set by a regular source file. |
There was a problem hiding this comment.
Suggested change
| // breakpoint set by a regular source file. | |
| // Breakpoint set by a regular source file. |
lldb/tools/lldb-dap/DAP.cpp
Outdated
Comment on lines
1670
to
1671
| } else | ||
| iv->second.UpdateBreakpoint(src_bp); |
There was a problem hiding this comment.
Suggested change
| } else | |
| iv->second.UpdateBreakpoint(src_bp); | |
| } else { | |
| iv->second.UpdateBreakpoint(src_bp); | |
| } |
lldb/tools/lldb-dap/DAP.cpp
Outdated
| existing_breakpoints.try_emplace(bp_pos, src_bp); | ||
| // We check if this breakpoint already exists to update it. | ||
| if (inserted) { | ||
| if (auto error = iv->second.SetBreakpoint(source)) { |
There was a problem hiding this comment.
Don't use auto: the type isn't obvious from the RHS.
Suggested change
| if (auto error = iv->second.SetBreakpoint(source)) { | |
| if (llvm::Error error = iv->second.SetBreakpoint(source)) { |
lldb/tools/lldb-dap/DAP.h
Outdated
| /// @} | ||
|
|
||
| /// Number of lines of assembly code to show when no debug info is available. | ||
| static constexpr uint32_t number_of_assembly_lines_for_nodebug = 32; |
There was a problem hiding this comment.
Constants use a k_ prefix.
Suggested change
| static constexpr uint32_t number_of_assembly_lines_for_nodebug = 32; | |
| static constexpr uint32_t k_number_of_assembly_lines_for_nodebug = 32; |
Comment on lines
30
to
33
| if (args.source.sourceReference) | ||
| locations = GetAssemblyBreakpointLocations(*args.source.sourceReference, | ||
| start_line, end_line); | ||
| else { |
There was a problem hiding this comment.
Suggested change
| if (args.source.sourceReference) | |
| locations = GetAssemblyBreakpointLocations(*args.source.sourceReference, | |
| start_line, end_line); | |
| else { | |
| if (args.source.sourceReference) { | |
| locations = GetAssemblyBreakpointLocations(*args.source.sourceReference, | |
| start_line, end_line); | |
| } else { |
| if (!symbol.IsValid()) | ||
| return locations; | ||
|
|
||
| // start_line is relative to the symbol's start address |
There was a problem hiding this comment.
Suggested change
| // start_line is relative to the symbol's start address | |
| // start_line is relative to the symbol's start address. |
|
|
||
| lldb::SBSymbol symbol = source_address.GetSymbol(); | ||
| if (!symbol.IsValid()) { | ||
| // Not yet supporting breakpoints in assembly without a valid symbol. |
There was a problem hiding this comment.
Suggested change
| // Not yet supporting breakpoints in assembly without a valid symbol. | |
| // FIXME: Support assembly breakpoints without a valid symbol. |
|
|
||
| m_bp = m_dap.target.BreakpointCreateBySBAddress(address); | ||
| } else { | ||
| // breakpoint set by a regular source file. |
There was a problem hiding this comment.
Suggested change
| // breakpoint set by a regular source file. | |
| // Breakpoint set by a regular source file. |
|
@JDevlieghere Fixed comments, sorry for the trouble. I'll try to pay extra attention next time :) |
|
This is failing on Windows on Arm, started here: https://lab.llvm.org/buildbot/#/builders/141/builds/8891 Have any fixes been attempted already? I'm going to reproduce locally and see if I can fix it. |
|
I Haven't attempted, unfortunately I only own a x64 laptop, I can attempt to reproduce on a VM later |
|
Let's see what I find first, sometimes these things are quite simple to fix. |
DavidSpickett
added a commit
that referenced
this pull request
May 22, 2025
New test added by #139969. On Windows we need debug information to be able to break on the function (we don't need it for main, but I assume that's a special case). So disable the test on Windows. I tried a few tricks like making a global label in assembly, but that doesn't show up without debug info either.
|
Looks like a Windows/PDB/COFF vs. Linux/DWARF/ELF difference. We can't break on a function when there's no debug information. It's not due to the architecture. So if you feel like coming up with a hack to make it work, the failure should reproduce on x64 Windows as well. Personally I'm fine with this just being tested on Linux. The principles are generic enough. |
|
Such a hack might be breaking on a place that does have debug information and stepping into one that doesn't, then placing a breakpoint on subsequent assembly lines. Might be. Didn't try it because it seemed like it might defeat the point of the test as it wouldn't place the initial breakpoint on a place without debug info. |
llvm-sync bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
May 22, 2025
New test added by llvm/llvm-project#139969. On Windows we need debug information to be able to break on the function (we don't need it for main, but I assume that's a special case). So disable the test on Windows. I tried a few tricks like making a global label in assembly, but that doesn't show up without debug info either.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
sourceReferenceto be the symbol load address for simplicity and consistency across threads/framesScreencast.From.2025-05-17.23-57-30.webm