[Lldb-commits] [lldb] a515fd0 - [lldb-vscode] fix breakpoint result ordering
Walter Erquinigo via lldb-commits
lldb-commits at lists.llvm.org
Fri Mar 27 08:43:08 PDT 2020
Author: Walter Erquinigo
Date: 2020-03-27T08:42:58-07:00
New Revision: a515fd01a4f2355291ab424d093e9de4f8acd4dc
URL: https://github.com/llvm/llvm-project/commit/a515fd01a4f2355291ab424d093e9de4f8acd4dc
DIFF: https://github.com/llvm/llvm-project/commit/a515fd01a4f2355291ab424d093e9de4f8acd4dc.diff
LOG: [lldb-vscode] fix breakpoint result ordering
Summary:
The DAP specifies the following for the SetBreakpoints request:
The breakpoints returned are in the same order as the elements of the 'breakpoints' arguments
This was not followed, as lldb-vscode was returning the breakpoints in a different order, because they were first stored into a map, and then traversed. Of course, maps normally don't preserve ordering.
See this log I captured:
-->
{"command":"setBreakpoints",
"arguments":{
"source":{
"name":"main.cpp",
"path":"/Users/wallace/fbsource/xplat/sand/test-projects/buck-cpp/main.cpp"
},
"lines":[6,10,11],
"breakpoints":[{"line":6},{"line":10},{"line":11}],
"sourceModified":false
},
"type":"request",
"seq":3
}
<--
{"body":{
"breakpoints":[
{"id":1, "line":11,"source":{"name":"main.cpp","path":"xplat/sand/test-projects/buck-cpp/main.cpp"},"verified":true},
{"id":2,"line":6,"source":{"name":"main.cpp","path":"xplat/sand/test-projects/buck-cpp/main.cpp"},"verified":true},
{"id":3,"line":10,"source":{"name":"main.cpp","path":"xplat/sand/test-projects/buck-cpp/main.cpp"},"verified":true}]},
"command":"setBreakpoints",
"request_seq":3,
"seq":0,
"success":true,
"type":"response"
}
As you can see, the order was not respected. This was causing the IDE not to be able to disable/enable breakpoints by clicking on them in the breakpoint view in the lower corner of the Debug tab.
This diff fixes the ordering problem. The traversal + querying was done very fast in O(nlogn) time. I'm keeping the same complexity.
I also updated a couple of tests to account for the ordering.
Reviewers: clayborg, aadsm, kusmour, labath
Subscribers: lldb-commits
Tags: #lldb
Differential Revision: https://reviews.llvm.org/D76891
Added:
Modified:
lldb/test/API/tools/lldb-vscode/breakpoint/TestVSCode_setBreakpoints.py
lldb/tools/lldb-vscode/lldb-vscode.cpp
Removed:
################################################################################
diff --git a/lldb/test/API/tools/lldb-vscode/breakpoint/TestVSCode_setBreakpoints.py b/lldb/test/API/tools/lldb-vscode/breakpoint/TestVSCode_setBreakpoints.py
index aff210d9f8ac..dde9989703fb 100644
--- a/lldb/test/API/tools/lldb-vscode/breakpoint/TestVSCode_setBreakpoints.py
+++ b/lldb/test/API/tools/lldb-vscode/breakpoint/TestVSCode_setBreakpoints.py
@@ -36,7 +36,7 @@ def test_set_and_clear(self):
first_line = line_number('main.cpp', 'break 12')
second_line = line_number('main.cpp', 'break 13')
third_line = line_number('main.cpp', 'break 14')
- lines = [first_line, second_line, third_line]
+ lines = [first_line, third_line, second_line]
# Visual Studio Code Debug Adaptors have no way to specify the file
# without launching or attaching to a process, so we must start a
@@ -51,8 +51,9 @@ def test_set_and_clear(self):
breakpoints = response['body']['breakpoints']
self.assertEquals(len(breakpoints), len(lines),
"expect %u source breakpoints" % (len(lines)))
- for breakpoint in breakpoints:
+ for (breakpoint, index) in zip(breakpoints, range(len(lines))):
line = breakpoint['line']
+ self.assertTrue(line, lines[index])
# Store the "id" of the breakpoint that was set for later
line_to_id[line] = breakpoint['id']
self.assertTrue(line in lines, "line expected in lines array")
@@ -74,8 +75,9 @@ def test_set_and_clear(self):
breakpoints = response['body']['breakpoints']
self.assertEquals(len(breakpoints), len(lines),
"expect %u source breakpoints" % (len(lines)))
- for breakpoint in breakpoints:
+ for (breakpoint, index) in zip(breakpoints, range(len(lines))):
line = breakpoint['line']
+ self.assertTrue(line, lines[index])
# Verify the same breakpoints are still set within LLDB by
# making sure the breakpoint ID didn't change
self.assertEquals(line_to_id[line], breakpoint['id'],
diff --git a/lldb/tools/lldb-vscode/lldb-vscode.cpp b/lldb/tools/lldb-vscode/lldb-vscode.cpp
index ec8000c264ae..2403bc0d73ac 100644
--- a/lldb/tools/lldb-vscode/lldb-vscode.cpp
+++ b/lldb/tools/lldb-vscode/lldb-vscode.cpp
@@ -1733,70 +1733,45 @@ void request_setBreakpoints(const llvm::json::Object &request) {
const auto path = GetString(source, "path");
auto breakpoints = arguments->getArray("breakpoints");
llvm::json::Array response_breakpoints;
+
// Decode the source breakpoint infos for this "setBreakpoints" request
SourceBreakpointMap request_bps;
for (const auto &bp : *breakpoints) {
auto bp_obj = bp.getAsObject();
if (bp_obj) {
SourceBreakpoint src_bp(*bp_obj);
- request_bps[src_bp.line] = std::move(src_bp);
+ request_bps[src_bp.line] = src_bp;
+
+ // We check if this breakpoint already exists to update it
+ auto existing_source_bps = g_vsc.source_breakpoints.find(path);
+ if (existing_source_bps != g_vsc.source_breakpoints.end()) {
+ const auto &existing_bp = existing_source_bps->second.find(src_bp.line);
+ if (existing_bp != existing_source_bps->second.end()) {
+ existing_bp->second.UpdateBreakpoint(src_bp);
+ AppendBreakpoint(existing_bp->second.bp, response_breakpoints);
+ continue;
+ }
+ }
+ // At this point the breakpoint is new
+ src_bp.SetBreakpoint(path.data());
+ AppendBreakpoint(src_bp.bp, response_breakpoints);
+ g_vsc.source_breakpoints[path][src_bp.line] = std::move(src_bp);
}
}
- // See if we already have breakpoints set for this source file from a
- // previous "setBreakpoints" request
+ // 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.
auto old_src_bp_pos = g_vsc.source_breakpoints.find(path);
if (old_src_bp_pos != g_vsc.source_breakpoints.end()) {
-
- // We have already set breakpoints in this source file and they are giving
- // use a new list of lines to set breakpoints on. Some breakpoints might
- // already be set, and some might not. We need to remove any breakpoints
- // whose lines are not contained in the any breakpoints lines in in the
- // "breakpoints" array.
-
- // 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.
- std::vector<uint32_t> remove_lines;
- for (auto &pair: old_src_bp_pos->second) {
- auto request_pos = request_bps.find(pair.first);
+ for (auto &old_bp : old_src_bp_pos->second) {
+ auto request_pos = request_bps.find(old_bp.first);
if (request_pos == request_bps.end()) {
// This breakpoint no longer exists in this source file, delete it
- g_vsc.target.BreakpointDelete(pair.second.bp.GetID());
- remove_lines.push_back(pair.first);
- } else {
- pair.second.UpdateBreakpoint(request_pos->second);
- // Remove this breakpoint from the request breakpoints since we have
- // handled it here and we don't need to set a new breakpoint below.
- request_bps.erase(request_pos);
- // Add this breakpoint info to the response
- AppendBreakpoint(pair.second.bp, response_breakpoints);
+ g_vsc.target.BreakpointDelete(old_bp.second.bp.GetID());
+ old_src_bp_pos->second.erase(old_bp.first);
}
}
- // Remove any lines from this existing source breakpoint map
- for (auto line: remove_lines)
- old_src_bp_pos->second.erase(line);
-
- // Now add any breakpoint infos left over in request_bps are the
- // breakpoints that weren't set in this source file yet. We need to update
- // thread source breakpoint info for the source file in the variable
- // "old_src_bp_pos->second" so the info for this source file is up to date.
- for (auto &pair : request_bps) {
- pair.second.SetBreakpoint(path.data());
- // Add this breakpoint info to the response
- AppendBreakpoint(pair.second.bp, response_breakpoints);
- old_src_bp_pos->second[pair.first] = std::move(pair.second);
- }
- } else {
- // No breakpoints were set for this source file yet. Set all breakpoints
- // for each line and add them to the response and create an entry in
- // g_vsc.source_breakpoints for this source file.
- for (auto &pair : request_bps) {
- pair.second.SetBreakpoint(path.data());
- // Add this breakpoint info to the response
- AppendBreakpoint(pair.second.bp, response_breakpoints);
- }
- g_vsc.source_breakpoints[path] = std::move(request_bps);
}
llvm::json::Object body;
More information about the lldb-commits
mailing list