Skip to content
Merged
Show file tree
Hide file tree
Changes from 15 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
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 @@ -64,6 +64,18 @@ def set_source_breakpoints(self, source_path, lines, data=None):
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
and returns an array of strings containing the breakpoint IDs
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,55 @@
"""
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 line_number
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"
)

finish_line = line_number("main.c", "// Break here")
finish_breakpoints = self.set_source_breakpoints("main.c", [finish_line])

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
source_reference = assembly_func_frame["source"]["sourceReference"]
assembly_breakpoint_ids = self.set_source_breakpoints_assembly(
source_reference, [line + 1]
)
self.continue_to_breakpoints(assembly_breakpoint_ids)

# Continue again and verify it hits in the next function call
self.continue_to_breakpoints(assmebly_func_breakpoints)
self.continue_to_breakpoints(assembly_breakpoint_ids)

# Clear the breakpoint and then check that the assembly breakpoint does not hit next time
self.set_source_breakpoints_assembly(source_reference, [])
self.continue_to_breakpoints(assmebly_func_breakpoints)
self.continue_to_breakpoints(finish_breakpoints)
16 changes: 16 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,16 @@
#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);
assembly_func(20);
assembly_func(30);
return 0; // Break here
}
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 != LLDB_INVALID_LINE_NUMBER)
breakpoint.line = line;
const auto column = line_entry.GetColumn();
if (column != LLDB_INVALID_COLUMN_NUMBER)
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
75 changes: 75 additions & 0 deletions lldb/tools/lldb-dap/DAP.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1620,6 +1620,81 @@ void DAP::EventThread() {
}
}

std::vector<protocol::Breakpoint> DAP::SetSourceBreakpoints(
const protocol::Source &source,
const std::optional<std::vector<protocol::SourceBreakpoint>> &breakpoints) {
std::vector<protocol::Breakpoint> response_breakpoints;
if (source.sourceReference) {
// breakpoint set by assembly source.
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 set by assembly source.
// Breakpoint set by assembly source.

auto &existing_breakpoints =
m_source_assembly_breakpoints[*source.sourceReference];
response_breakpoints =
SetSourceBreakpoints(source, breakpoints, existing_breakpoints);
} else {
// breakpoint set by a regular source file.
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 set by a regular source file.
// Breakpoint set by a regular source file.

const auto path = source.path.value_or("");
auto &existing_breakpoints = m_source_breakpoints[path];
response_breakpoints =
SetSourceBreakpoints(source, breakpoints, existing_breakpoints);
}

return response_breakpoints;
}

std::vector<protocol::Breakpoint> DAP::SetSourceBreakpoints(
const protocol::Source &source,
const std::optional<std::vector<protocol::SourceBreakpoint>> &breakpoints,
SourceBreakpointMap &existing_breakpoints) {
std::vector<protocol::Breakpoint> response_breakpoints;

SourceBreakpointMap request_breakpoints;
if (breakpoints) {
for (const auto &bp : *breakpoints) {
SourceBreakpoint src_bp(*this, bp);
std::pair<uint32_t, uint32_t> bp_pos(src_bp.GetLine(),
src_bp.GetColumn());
request_breakpoints.try_emplace(bp_pos, src_bp);

const auto [iv, inserted] =
existing_breakpoints.try_emplace(bp_pos, src_bp);
// We check if this breakpoint already exists to update it.
if (inserted)
iv->second.SetBreakpoint(source);
else
iv->second.UpdateBreakpoint(src_bp);

protocol::Breakpoint response_breakpoint =
iv->second.ToProtocolBreakpoint();
response_breakpoint.source = source;

if (!response_breakpoint.line &&
src_bp.GetLine() != LLDB_INVALID_LINE_NUMBER)
response_breakpoint.line = src_bp.GetLine();
if (!response_breakpoint.column &&
src_bp.GetColumn() != LLDB_INVALID_COLUMN_NUMBER)
response_breakpoint.column = src_bp.GetColumn();
response_breakpoints.push_back(response_breakpoint);
}
}

// Delete any breakpoints in this source file that aren't in the
// request_bps set. There is no call to remove breakpoints other than
// calling this function with a smaller or empty "breakpoints" list.
for (auto it = existing_breakpoints.begin();
it != existing_breakpoints.end();) {
auto request_pos = request_breakpoints.find(it->first);
if (request_pos == request_breakpoints.end()) {
// This breakpoint no longer exists in this source file, delete it
target.BreakpointDelete(it->second.GetID());
it = existing_breakpoints.erase(it);
} else {
++it;
}
}

return response_breakpoints;
}

void DAP::RegisterRequests() {
RegisterRequest<AttachRequestHandler>();
RegisterRequest<BreakpointLocationsRequestHandler>();
Expand Down
30 changes: 28 additions & 2 deletions lldb/tools/lldb-dap/DAP.h
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@

namespace lldb_dap {

typedef llvm::DenseMap<std::pair<uint32_t, uint32_t>, SourceBreakpoint>
typedef std::map<std::pair<uint32_t, uint32_t>, SourceBreakpoint>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed to std::map because I'm not sure how to remove from the map while iterating it with DenseMap

SourceBreakpointMap;
typedef llvm::StringMap<FunctionBreakpoint> FunctionBreakpointMap;
typedef llvm::DenseMap<lldb::addr_t, InstructionBreakpoint>
Expand Down Expand Up @@ -168,7 +168,6 @@ struct DAP {
lldb::SBTarget target;
Variables variables;
lldb::SBBroadcaster broadcaster;
llvm::StringMap<SourceBreakpointMap> source_breakpoints;
FunctionBreakpointMap function_breakpoints;
InstructionBreakpointMap instruction_breakpoints;
std::optional<std::vector<ExceptionBreakpoint>> exception_breakpoints;
Expand Down Expand Up @@ -219,6 +218,9 @@ struct DAP {
llvm::StringSet<> modules;
/// @}

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

Choose a reason for hiding this comment

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

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;


/// Creates a new DAP sessions.
///
/// \param[in] log
Expand Down Expand Up @@ -423,7 +425,28 @@ struct DAP {
void StartEventThread();
void StartProgressEventThread();

/// Sets the given protocol `breakpoints` in the given `source`, while
/// removing any existing breakpoints in the given source if they are not in
/// `breakpoint`.
///
/// \param[in] source
/// The relevant source of the breakpoints.
///
/// \param[in] breakpoints
/// The breakpoints to set.
///
/// \return a vector of the breakpoints that were set.
std::vector<protocol::Breakpoint> SetSourceBreakpoints(
const protocol::Source &source,
const std::optional<std::vector<protocol::SourceBreakpoint>>
&breakpoints);

private:
std::vector<protocol::Breakpoint> SetSourceBreakpoints(
const protocol::Source &source,
const std::optional<std::vector<protocol::SourceBreakpoint>> &breakpoints,
SourceBreakpointMap &existing_breakpoints);

/// Registration of request handler.
/// @{
void RegisterRequests();
Expand Down Expand Up @@ -452,6 +475,9 @@ struct DAP {

std::mutex m_active_request_mutex;
const protocol::Request *m_active_request;

llvm::StringMap<SourceBreakpointMap> m_source_breakpoints;
llvm::DenseMap<int64_t, SourceBreakpointMap> m_source_assembly_breakpoints;
Comment on lines +479 to +480
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

};

} // namespace lldb_dap
Expand Down
Loading
Loading