Skip to content

[lldb-dap] assembly breakpoints #139969

New issue

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

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

Already on GitHub? Sign in to your account

Merged
merged 19 commits into from
May 21, 2025
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions lldb/include/lldb/API/SBFileSpec.h
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#define LLDB_API_SBFILESPEC_H

#include "lldb/API/SBDefines.h"
#include "lldb/API/SBStream.h"

namespace lldb {

Expand Down Expand Up @@ -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;
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved to LLDBUtils


static int ResolvePath(const char *src_path, char *dst_path, size_t dst_len);

bool GetDescription(lldb::SBStream &description) const;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

💯

args_dict = {
"source": source_dict,
"sourceModified": False,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
8 changes: 8 additions & 0 deletions lldb/source/API/SBFileSpec.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

#include <cinttypes>
#include <climits>
#include <string>

using namespace lldb;
using namespace lldb_private;
Expand Down Expand Up @@ -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();
}
Expand Down
3 changes: 3 additions & 0 deletions lldb/test/API/tools/lldb-dap/breakpoint-assembly/Makefile
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
C_SOURCES := main.c

include Makefile.rules
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
"""
Test lldb-dap setBreakpoints request
Copy link
Contributor

Choose a reason for hiding this comment

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

Test lldb-dap setBreakpoints in assembly source references?

"""


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):
Copy link
Contributor

Choose a reason for hiding this comment

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

test_can_break_in_source_references?

"""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)
14 changes: 14 additions & 0 deletions lldb/test/API/tools/lldb-dap/breakpoint-assembly/main.c
Original file line number Diff line number Diff line change
@@ -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;
}
33 changes: 26 additions & 7 deletions lldb/tools/lldb-dap/Breakpoint.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Breakpoint made by assembly
// Assembly breakpoint.

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;
Expand Down
5 changes: 5 additions & 0 deletions lldb/tools/lldb-dap/DAP.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Copy link
Member

Choose a reason for hiding this comment

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

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;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

💯


/// Creates a new DAP sessions.
///
/// \param[in] log
Expand Down
68 changes: 51 additions & 17 deletions lldb/tools/lldb-dap/Handler/BreakpointLocationsHandler.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
//===----------------------------------------------------------------------===//

#include "DAP.h"
#include "JSONUtils.h"
#include "RequestHandler.h"
#include <vector>

Expand All @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Find all relevant lines & columns
// Find all relevant lines & columns.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

💯

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));
}
Copy link
Member

Choose a reason for hiding this comment

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

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});

Copy link
Contributor Author

@eronnen eronnen May 18, 2025

Choose a reason for hiding this comment

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

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 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 =
Expand Down Expand Up @@ -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,
Copy link
Member

Choose a reason for hiding this comment

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

You can avoid the template by taking a reference to a SmallVectorImpl<std::pair<uint32_t, uint32_t>>, which SmallVector inherits from.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreem chganged to return std::vector

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
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// start_line is relative to the symbol's start address
// 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
20 changes: 20 additions & 0 deletions lldb/tools/lldb-dap/Handler/RequestHandler.h
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down
Loading
Loading