[lldb-vscode] fix breakpoint result ordering

[wallace]

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.

For example, after setting and unsetting breakpoints, some ended up being duplicated and the IDE stopped working properly

[clayborg]

LGTM as long as all test are passing!

lldb/test/API/tools/lldb-vscode/breakpoint/TestVSCode_setBreakpoints.py

def test_set_and_clear(self):
            and makes sure things happen correctly. It doesn't test hitting
            breakpoints and the functionality of each breakpoint, like
            'conditions' and 'hitCondition' settings.'''
         source_basename = 'main.cpp'
         source_path = os.path.join(os.getcwd(), source_basename)
         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
         # process in order to be able to set breakpoints.
         program = self.getBuildArtifact("a.out")
         self.build_and_launch(program)
 
         # Set 3 breakoints and verify that they got set correctly
         response = self.vscode.request_setBreakpoints(source_path, lines)
         line_to_id = {}
         if response:
             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")
                 self.assertTrue(breakpoint['verified'],
                                 "expect breakpoint verified")
 
         # There is no breakpoint delete packet, clients just send another
         # setBreakpoints packet with the same source file with fewer lines.
         # Below we remove the second line entry and call the setBreakpoints
         # function again. We want to verify that any breakpoints that were set
         # before still have the same "id". This means we didn't clear the
         # breakpoint and set it again at the same location. We also need to
         # verify that the second line location was actually removed.
         lines.remove(second_line)
         # Set 2 breakoints and verify that the previous breakoints that were
         # set above are still set.
         response = self.vscode.request_setBreakpoints(source_path, lines)
         if response:
             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'],
                                 "verify previous breakpoints stayed the same")
                 self.assertTrue(line in lines, "line expected in lines array")
                 self.assertTrue(breakpoint['verified'],
                                 "expect breakpoint still verified")
 

lldb/tools/lldb-vscode/lldb-vscode.cpp

void request_setBreakpoints(const llvm::json::Object &request) {
   llvm::json::Object response;
   lldb::SBError error;
   FillResponse(request, response);
   auto arguments = request.getObject("arguments");
   auto source = arguments->getObject("source");
   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
-  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);
+  auto old_src_bp_pos = g_vsc.source_breakpoints.find(path);
+  if (old_src_bp_pos != g_vsc.source_breakpoints.end()) {
+    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);
-      }
-    }
-    // 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);
+        g_vsc.target.BreakpointDelete(old_bp.second.bp.GetID());
+        old_src_bp_pos->second.erase(old_bp.first);
       }
-  } 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;
   body.try_emplace("breakpoints", std::move(response_breakpoints));
   response.try_emplace("body", std::move(body));
   g_vsc.SendJSON(llvm::json::Value(std::move(response)));
 }