Skip to content

[lldb-dap] Correctly report breakpoints as resolved on macOS #129589

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.

Sign up for GitHub

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 2 commits into from
Mar 4, 2025
Merged

[lldb-dap] Correctly report breakpoints as resolved on macOS #129589

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2706528
[lldb-dap] Correctly report breakpoints as resolved on macOS
JDevlieghere Mar 3, 2025
427c132
Check the event mask before the label
JDevlieghere Mar 3, 2025
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
110 changes: 55 additions & 55 deletions lldb/test/API/tools/lldb-dap/breakpoint-events/TestDAP_breakpointEvents.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
Test lldb-dap setBreakpoints request
"""


import dap_server
from lldbsuite.test.decorators import *
from lldbsuite.test.lldbtest import *
Expand All @@ -12,9 +11,7 @@


class TestDAP_breakpointEvents(lldbdap_testcase.DAPTestCaseBase):
@skipIfWindows
@skipUnlessDarwin
@expectedFailureAll(macos_version=[">=", "13.0"])
def test_breakpoint_events(self):
"""
This test sets a breakpoint in a shared library and runs and stops
Expand Down Expand Up @@ -63,70 +60,73 @@ def test_breakpoint_events(self):
response = self.dap_server.request_setBreakpoints(
main_source_path, [main_bp_line]
)
if response:
breakpoints = response["body"]["breakpoints"]
for breakpoint in breakpoints:
main_bp_id = breakpoint["id"]
dap_breakpoint_ids.append("%i" % (main_bp_id))
# line = breakpoint['line']
self.assertTrue(
breakpoint["verified"], "expect main breakpoint to be verified"
)
self.assertTrue(response)
breakpoints = response["body"]["breakpoints"]
for breakpoint in breakpoints:
main_bp_id = breakpoint["id"]
dap_breakpoint_ids.append("%i" % (main_bp_id))
self.assertTrue(
breakpoint["verified"], "expect main breakpoint to be verified"
)

response = self.dap_server.request_setBreakpoints(
foo_source_path, [foo_bp1_line]
)
if response:
breakpoints = response["body"]["breakpoints"]
for breakpoint in breakpoints:
foo_bp_id = breakpoint["id"]
dap_breakpoint_ids.append("%i" % (foo_bp_id))
self.assertFalse(
breakpoint["verified"], "expect foo breakpoint to not be verified"
)
self.assertTrue(response)
breakpoints = response["body"]["breakpoints"]
for breakpoint in breakpoints:
foo_bp_id = breakpoint["id"]
dap_breakpoint_ids.append("%i" % (foo_bp_id))
self.assertFalse(
breakpoint["verified"], "expect foo breakpoint to not be verified"
)

# Get the stop at the entry point
self.continue_to_next_stop()

# We are now stopped at the entry point to the program. Shared
# libraries are not loaded yet (at least on macOS they aren't) and any
# breakpoints set in foo.cpp should not be resolved.
# libraries are not loaded yet (at least on macOS they aren't) and only
# the breakpoint in the main executable should be resolved.
self.assertEqual(len(self.dap_server.breakpoint_events), 1)
event = self.dap_server.breakpoint_events[0]
body = event["body"]
self.assertEqual(
len(self.dap_server.breakpoint_events),
0,
"no breakpoint events when stopped at entry point",
body["reason"], "changed", "breakpoint event should say changed"
)
breakpoint = body["breakpoint"]
self.assertEqual(breakpoint["id"], main_bp_id)
self.assertTrue(breakpoint["verified"], "main breakpoint should be resolved")

# Clear the list of breakpoint events so we don't see this one again.
self.dap_server.breakpoint_events.clear()

# Continue to the breakpoint
self.continue_to_breakpoints(dap_breakpoint_ids)

# Make sure we only get an event for the breakpoint we set via a call
# to self.dap_server.request_setBreakpoints(...), not the breakpoint
# we set with with a LLDB command in preRunCommands.
self.assertEqual(
len(self.dap_server.breakpoint_events),
1,
"make sure we got a breakpoint event",
)
event = self.dap_server.breakpoint_events[0]
# Verify the details of the breakpoint changed notification.
body = event["body"]
self.assertEqual(
body["reason"], "changed", "breakpoint event is says breakpoint is changed"
)
breakpoint = body["breakpoint"]
self.assertTrue(
breakpoint["verified"], "breakpoint event is says it is verified"
)
self.assertEqual(
breakpoint["id"],
foo_bp_id,
"breakpoint event is for breakpoint %i" % (foo_bp_id),
)
self.assertTrue(
"line" in breakpoint and breakpoint["line"] > 0,
"breakpoint event is has a line number",
)
self.assertNotIn(
"source", breakpoint, "breakpoint event should not return a source object"
)
# When the process launches, we first expect to see both the main and
# foo breakpoint as unresolved.
for event in self.dap_server.breakpoint_events[:2]:
body = event["body"]
self.assertEqual(
body["reason"], "changed", "breakpoint event should say changed"
)
breakpoint = body["breakpoint"]
self.assertIn(str(breakpoint["id"]), dap_breakpoint_ids)
self.assertFalse(breakpoint["verified"], "breakpoint should be unresolved")

# Then, once the dynamic loader has given us a load address, they
# should show up as resolved again.
for event in self.dap_server.breakpoint_events[3:]:
body = event["body"]
self.assertEqual(
body["reason"], "changed", "breakpoint event should say changed"
)
breakpoint = body["breakpoint"]
self.assertIn(str(breakpoint["id"]), dap_breakpoint_ids)
self.assertTrue(breakpoint["verified"], "breakpoint should be resolved")
self.assertNotIn(
"source",
breakpoint,
"breakpoint event should not return a source object",
)
self.assertIn("line", breakpoint, "breakpoint event should have line")
31 changes: 17 additions & 14 deletions lldb/tools/lldb-dap/Handler/InitializeRequestHandler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,27 +137,30 @@ static void EventThreadFunction(DAP &dap) {
lldb::SBBreakpoint::GetBreakpointEventTypeFromEvent(event);
auto bp = Breakpoint(
dap, lldb::SBBreakpoint::GetBreakpointFromEvent(event));
// If the breakpoint was originated from the IDE, it will have the
// If the breakpoint was originated from DAP, it will have the
Copy link
Contributor

@Jlalond Jlalond Mar 3, 2025

Choose a reason for hiding this comment

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

Is the first sentence redundant because we're in LLDB-DAP?

Copy link
Member Author

@JDevlieghere JDevlieghere Mar 3, 2025

Choose a reason for hiding this comment

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

No, it distinguishes from a breakpoint that was set through a command in the console or for example through your ~/.lldbinit.

// BreakpointBase::GetBreakpointLabel() label attached. Regardless
// of wether the locations were added or removed, the breakpoint
// ins't going away, so we the reason is always "changed".
if ((event_type & lldb::eBreakpointEventTypeLocationsAdded ||
event_type & lldb::eBreakpointEventTypeLocationsRemoved) &&
bp.MatchesName(BreakpointBase::GetBreakpointLabel())) {
auto bp_event = CreateEventObject("breakpoint");
llvm::json::Object body;
// As VSCode already knows the path of this breakpoint, we don't
// need to send it back as part of a "changed" event. This
// prevent us from sending to VSCode paths that should be source
// mapped. Note that CreateBreakpoint doesn't apply source mapping.
// Besides, the current implementation of VSCode ignores the
// "source" element of breakpoint events.
// of whether locations were added, removed, or resolved, the
// breakpoint isn't going away and the reason is always "changed".
if (bp.MatchesName(BreakpointBase::GetBreakpointLabel()) &&
(event_type & lldb::eBreakpointEventTypeLocationsAdded ||
event_type & lldb::eBreakpointEventTypeLocationsRemoved ||
event_type & lldb::eBreakpointEventTypeLocationsResolved)) {
// As the DAP client already knows the path of this breakpoint, we
// don't need to send it back as part of the "changed" event. This
// prevent us from sending paths that should be source
// mapped. Note that CreateBreakpoint doesn't apply source mapping
// and certain implementation ignore the source part of this event
// anyway.
llvm::json::Value source_bp = CreateBreakpoint(&bp);
source_bp.getAsObject()->erase("source");

llvm::json::Object body;
body.try_emplace("breakpoint", source_bp);
body.try_emplace("reason", "changed");

llvm::json::Object bp_event = CreateEventObject("breakpoint");
bp_event.try_emplace("body", std::move(body));

dap.SendJSON(llvm::json::Value(std::move(bp_event)));
}
}
Expand Down