-
Notifications
You must be signed in to change notification settings - Fork 14.3k
[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
[lldb-dap] assembly breakpoints #139969
Changes from 6 commits
d6325b3
ee49203
dbb2f9b
3699524
61623de
f5fd76d
40f68e4
199512f
dbad33b
2e9edca
0515c6d
9dbca55
396970d
7a0e0ee
71dce6c
15e7294
044fd90
ef75e4d
f9d9024
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we keep the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💯 |
||
args_dict = { | ||
"source": source_dict, | ||
"sourceModified": False, | ||
|
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
""" | ||
|
||
|
||
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): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
"""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) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
#include <stddef.h> | ||
eronnen marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
__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; | ||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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 | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
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; | ||||||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this a constant?
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💯 |
||||||
|
||||||
/// Creates a new DAP sessions. | ||||||
/// | ||||||
/// \param[in] log | ||||||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -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 | ||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)); | ||||||||||||||||||
} | ||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For some reason got 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 = | ||||||||||||||||||
|
@@ -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, | ||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can avoid the template by taking a reference to a There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreem chganged to return |
||||||||||||||||||
int64_t sourceReference, uint32_t start_line, uint32_t end_line) const { | ||||||||||||||||||
da-viper marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||
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(); | ||||||||||||||||||
da-viper marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||
if (!symbol.IsValid()) | ||||||||||||||||||
return; | ||||||||||||||||||
|
||||||||||||||||||
return protocol::BreakpointLocationsResponseBody{ | ||||||||||||||||||
/*breakpoints=*/std::move(breakpoint_locations)}; | ||||||||||||||||||
// start_line is relative to the symbol's start address | ||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||
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); | ||||||||||||||||||
eronnen marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||
} | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
} // namespace lldb_dap |
There was a problem hiding this comment.
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?There was a problem hiding this comment.
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 usingGetFilename
andGetDirectory
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved to LLDBUtils