Skip to content

[lldb-dap] Allow returning multiple breakpoints in "stopped" event #149133

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 4 commits into from
Jul 22, 2025
Merged

[lldb-dap] Allow returning multiple breakpoints in "stopped" event #149133

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
773cff4
[lldb-dap] Allow returning multiple breakpoints in "stopped" event
SLTozer Jul 16, 2025
71a817e
Some review comments
SLTozer Jul 16, 2025
c789b42
Address further review comments
SLTozer Jul 17, 2025
83ad725
crush clang-format
SLTozer Jul 17, 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
23 changes: 23 additions & 0 deletions lldb/packages/Python/lldbsuite/test/tools/lldb-dap/lldbdap_testcase.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,29 @@ def verify_breakpoint_hit(self, breakpoint_ids, timeout=DEFAULT_TIMEOUT):
return
self.assertTrue(False, f"breakpoint not hit, stopped_events={stopped_events}")

def verify_all_breakpoints_hit(self, breakpoint_ids, timeout=DEFAULT_TIMEOUT):
"""Wait for the process we are debugging to stop, and verify we hit
all of the breakpoint locations in the "breakpoint_ids" array.
"breakpoint_ids" should be a list of breakpoint ID strings
(["1", "2"])."""
Copy link
Member

@JDevlieghere JDevlieghere Jul 16, 2025

Choose a reason for hiding this comment

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

Why not numbers, to match the protocol?

Copy link
Contributor Author

@SLTozer SLTozer Jul 16, 2025

Choose a reason for hiding this comment

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

I chose to have it accept the output from a setBreakpoints response as with verify_breakpoint_hit, but converting to ints in the test and passing those also makes sense.

stopped_events = self.dap_server.wait_for_stopped(timeout)
for stopped_event in stopped_events:
if "body" in stopped_event:
body = stopped_event["body"]
if "reason" not in body:
continue
if (
body["reason"] != "breakpoint"
and body["reason"] != "instruction breakpoint"
):
continue
if "hitBreakpointIds" not in body:
continue
hit_bps = body["hitBreakpointIds"]
if all(int(breakpoint_id) in hit_bps for breakpoint_id in breakpoint_ids):
return
self.assertTrue(False, f"breakpoints not hit, stopped_events={stopped_events}")

def verify_stop_exception_info(self, expected_description, timeout=DEFAULT_TIMEOUT):
"""Wait for the process we are debugging to stop, and verify the stop
reason is 'exception' and that the description matches
Expand Down
20 changes: 20 additions & 0 deletions lldb/test/API/tools/lldb-dap/breakpoint/TestDAP_setBreakpoints.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -398,3 +398,23 @@ def test_column_breakpoints(self):
self.stepIn()
func_name = self.get_stackFrames()[0]["name"]
self.assertEqual(func_name, "a::fourteen(int)")

@skipIfWindows
def test_hit_multiple_breakpoints(self):
"""Test that if we hit multiple breakpoints at the same address, they
all appear in the stop reason."""
breakpoint_lines = [
line_number("main.cpp", "// end of foo check"),
line_number("main.cpp", "// before loop")
]

program = self.getBuildArtifact("a.out")
self.build_and_launch(program)

# Set a pair of breakpoints that will both resolve to the same address.
breakpoint_ids = self.set_source_breakpoints(self.main_path, breakpoint_lines)
self.assertEqual(len(breakpoint_ids), 2, "expected two breakpoints")
self.dap_server.request_continue()
print(breakpoint_ids)
# Verify we hit both of the breakpoints we just set
self.verify_all_breakpoints_hit(breakpoint_ids)
2 changes: 1 addition & 1 deletion lldb/test/API/tools/lldb-dap/breakpoint/main.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ int main(int argc, char const *argv[]) {
if (foo == nullptr) {
fprintf(stderr, "%s\n", dlerror());
exit(2);
}
} // end of foo check
Copy link
Member

@JDevlieghere JDevlieghere Jul 16, 2025

Choose a reason for hiding this comment

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

Suggested change
} // end of foo check
}

Copy link
Contributor Author

@SLTozer SLTozer Jul 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This comment is being used to set the breakpoint from here in the test - thinking about it, the only real requirement for where the second breakpoint appears is that it be on a line that is not breakpoint-able that precedes a breakpoint-able line, so I could also move the comment to an empty line before any other line.

Copy link
Member

@JDevlieghere JDevlieghere Jul 16, 2025

Choose a reason for hiding this comment

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

Ah, I missed that you were using that to set the breakpoint. In that case, maybe say something like "Break here for non-breakpointable line" or something.

Copy link
Contributor Author

@SLTozer SLTozer Jul 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I can make this change, but other comments used for setting breakpoints in the same file follow this "minimal" convention, e.g.

  return 12 + i; // break 12
  return 14 + i; // break 14
  foo(12); // before loop
    int x = twelve(i) + thirteen(i) + a::fourteen(i); // break loop

I'll add break as a prefix to match most of the other cases, though I'm also happy to just rewrite the string following your suggestion if you think that would be better.
Edit: Actually, "non-breakpointable" does explain better the purpose for the test, so I've used that!

foo(12); // before loop

for (int i = 0; i < 10; ++i) {
Expand Down
17 changes: 11 additions & 6 deletions lldb/tools/lldb-dap/JSONUtils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -654,12 +654,17 @@ llvm::json::Value CreateThreadStopped(DAP &dap, lldb::SBThread &thread,
} else {
body.try_emplace("reason", "breakpoint");
}
lldb::break_id_t bp_id = thread.GetStopReasonDataAtIndex(0);
lldb::break_id_t bp_loc_id = thread.GetStopReasonDataAtIndex(1);
std::string desc_str =
llvm::formatv("breakpoint {0}.{1}", bp_id, bp_loc_id);
body.try_emplace("hitBreakpointIds",
llvm::json::Array{llvm::json::Value(bp_id)});
std::vector<llvm::json::Value> bp_ids;
std::ostringstream desc_sstream;
desc_sstream << "breakpoint";
for (size_t idx = 0; idx < thread.GetStopReasonDataCount(); idx += 2) {
lldb::break_id_t bp_id = thread.GetStopReasonDataAtIndex(idx);
lldb::break_id_t bp_loc_id = thread.GetStopReasonDataAtIndex(idx + 1);
bp_ids.push_back(llvm::json::Value(bp_id));
desc_sstream << " " << bp_id << "." << bp_loc_id;
}
std::string desc_str = desc_sstream.str();
body.try_emplace("hitBreakpointIds", llvm::json::Array(bp_ids));
EmplaceSafeString(body, "description", desc_str);
}
} break;
Expand Down
Loading