-
Notifications
You must be signed in to change notification settings - Fork 14.6k
[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.
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
Conversation
Currently, the "stopped" event returned when a breakpoint is hit will always return only the ID of first breakpoint returned from `GetStopReasonDataAtIndex`. This is slightly different from the behaviour in `lldb`, where multiple breakpoints can exist at a single instruction address and all are returned as part of the stop reason when that address is hit. This patch allows all multiple hit breakpoints to be returned in the "stopped" event, both in the hitBreakpointIds field and in the description, using the same formatting as lldb e.g. "breakpoint 1.1 2.1".
@llvm/pr-subscribers-lldb Author: Stephen Tozer (SLTozer) ChangesCurrently, the "stopped" event returned when a breakpoint is hit will always return only the ID of first breakpoint returned from This patch allows all multiple hit breakpoints to be returned in the "stopped" event, both in the hitBreakpointIds field and in the description, using the same formatting as lldb e.g. "breakpoint 1.1 2.1". I'm not aware of any effect this will have on debugger plugins; as far as I can tell, it makes no difference within the VS Code UI - this just fixes a minor issue encountered while writing an Full diff: https://github.com/llvm/llvm-project/pull/149133.diff 4 Files Affected:
diff --git a/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/lldbdap_testcase.py b/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/lldbdap_testcase.py
index d823126e3e2fd..9b227c67bb859 100644
--- a/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/lldbdap_testcase.py
+++ b/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/lldbdap_testcase.py
@@ -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"])."""
+ 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
diff --git a/lldb/test/API/tools/lldb-dap/breakpoint/TestDAP_setBreakpoints.py b/lldb/test/API/tools/lldb-dap/breakpoint/TestDAP_setBreakpoints.py
index 831edd6494c1e..121933c672bf7 100644
--- a/lldb/test/API/tools/lldb-dap/breakpoint/TestDAP_setBreakpoints.py
+++ b/lldb/test/API/tools/lldb-dap/breakpoint/TestDAP_setBreakpoints.py
@@ -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)
diff --git a/lldb/test/API/tools/lldb-dap/breakpoint/main.cpp b/lldb/test/API/tools/lldb-dap/breakpoint/main.cpp
index a84546a95af15..a9a16f240e37d 100644
--- a/lldb/test/API/tools/lldb-dap/breakpoint/main.cpp
+++ b/lldb/test/API/tools/lldb-dap/breakpoint/main.cpp
@@ -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
foo(12); // before loop
for (int i = 0; i < 10; ++i) {
diff --git a/lldb/tools/lldb-dap/JSONUtils.cpp b/lldb/tools/lldb-dap/JSONUtils.cpp
index 41ca29a405ac9..30e6ea900d919 100644
--- a/lldb/tools/lldb-dap/JSONUtils.cpp
+++ b/lldb/tools/lldb-dap/JSONUtils.cpp
@@ -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;
|
✅ With the latest revision this PR passed the Python code formatter. |
@@ -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 |
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.
} // end of foo check | |
} |
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.
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.
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.
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.
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 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!
"breakpoint_ids" should be a list of breakpoint ID strings | ||
(["1", "2"]).""" |
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.
Why not numbers, to match the protocol?
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 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.
✅ With the latest revision this PR passed the C/C++ code formatter. |
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.
LGTM!
…lvm#149133) Currently, the "stopped" event returned when a breakpoint is hit will always return only the ID of first breakpoint returned from `GetStopReasonDataAtIndex`. This is slightly different from the behaviour in `lldb`, where multiple breakpoints can exist at a single instruction address and all are returned as part of the stop reason when that address is hit. This patch allows all multiple hit breakpoints to be returned in the "stopped" event, both in the hitBreakpointIds field and in the description, using the same formatting as lldb e.g. "breakpoint 1.1 2.1". I'm not aware of any effect this will have on debugger plugins; as far as I can tell, it makes no difference within the VS Code UI - this just fixes a minor issue encountered while writing an `lldb-dap` backend for Dexter.
…lvm#149133) Currently, the "stopped" event returned when a breakpoint is hit will always return only the ID of first breakpoint returned from `GetStopReasonDataAtIndex`. This is slightly different from the behaviour in `lldb`, where multiple breakpoints can exist at a single instruction address and all are returned as part of the stop reason when that address is hit. This patch allows all multiple hit breakpoints to be returned in the "stopped" event, both in the hitBreakpointIds field and in the description, using the same formatting as lldb e.g. "breakpoint 1.1 2.1". I'm not aware of any effect this will have on debugger plugins; as far as I can tell, it makes no difference within the VS Code UI - this just fixes a minor issue encountered while writing an `lldb-dap` backend for Dexter.
…lvm#149133) Currently, the "stopped" event returned when a breakpoint is hit will always return only the ID of first breakpoint returned from `GetStopReasonDataAtIndex`. This is slightly different from the behaviour in `lldb`, where multiple breakpoints can exist at a single instruction address and all are returned as part of the stop reason when that address is hit. This patch allows all multiple hit breakpoints to be returned in the "stopped" event, both in the hitBreakpointIds field and in the description, using the same formatting as lldb e.g. "breakpoint 1.1 2.1". I'm not aware of any effect this will have on debugger plugins; as far as I can tell, it makes no difference within the VS Code UI - this just fixes a minor issue encountered while writing an `lldb-dap` backend for Dexter.
Currently, the "stopped" event returned when a breakpoint is hit will always return only the ID of first breakpoint returned from
GetStopReasonDataAtIndex
. This is slightly different from the behaviour inlldb
, where multiple breakpoints can exist at a single instruction address and all are returned as part of the stop reason when that address is hit.This patch allows all multiple hit breakpoints to be returned in the "stopped" event, both in the hitBreakpointIds field and in the description, using the same formatting as lldb e.g. "breakpoint 1.1 2.1". I'm not aware of any effect this will have on debugger plugins; as far as I can tell, it makes no difference within the VS Code UI - this just fixes a minor issue encountered while writing an
lldb-dap
backend for Dexter.