[lldb-vscode] Allow an empty 'breakpoints' field to clear breakpoints.

[rupprecht]

Summary

Per the DAP spec for SetBreakpoints [1], the way to clear breakpoints is: To clear all breakpoint for a source, specify an empty array.

However, leaving the breakpoints field unset is also a well formed request (note the breakpoints?: in the SetBreakpointsArguments definition). If it's unset, we have a couple choices:

1. Crash (current behavior)
2. Clear breakpoints
3. Return an error response that the breakpoints field is missing.

I propose we do (2) instead of (1), and treat an unset breakpoints field the same as an empty breakpoints field.

[1] https://microsoft.github.io/debug-adapter-protocol/specification#Requests_SetBreakpoints

[wallace]

Very good! Thank you

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

228

you might want to add one additional breakpoint here to make the test more robust

lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/vscode.py

def request_scopes(self, frameId):
             'type': 'request',
             'arguments': args_dict
         }
         return self.send_recv(command_dict)
 
     def request_setBreakpoints(self, file_path, line_array, condition=None,
                                hitCondition=None):
         (dir, base) = os.path.split(file_path)
-        breakpoints = []
-        for line in line_array:
-            bp = {'line': line}
-            if condition is not None:
-                bp['condition'] = condition
-            if hitCondition is not None:
-                bp['hitCondition'] = hitCondition
-            breakpoints.append(bp)
         source_dict = {
             'name': base,
             'path': file_path
         }
         args_dict = {
             'source': source_dict,
-            'breakpoints': breakpoints,
-            'lines': '%s' % (line_array),
             'sourceModified': False,
         }
+        if line_array is not None:
+            args_dict['lines'] = '%s' % line_array
+            breakpoints = []
+            for line in line_array:
+                bp = {'line': line}
+                if condition is not None:
+                    bp['condition'] = condition
+                if hitCondition is not None:
+                    bp['hitCondition'] = hitCondition
+                breakpoints.append(bp)
+            args_dict['breakpoints'] = breakpoints
+
         command_dict = {
             'command': 'setBreakpoints',
             'type': 'request',
             'arguments': args_dict
         }
         return self.send_recv(command_dict)
 
     def request_setExceptionBreakpoints(self, filters):

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

def test_set_and_clear(self):
             for breakpoint in breakpoints:
                 line = breakpoint['line']
                 self.assertTrue(line in lines, "line expected in lines array")
                 self.assertTrue(breakpoint['verified'],
                                 "expect breakpoint still verified")
 
     @skipIfWindows
     @skipIfRemote
+    def test_clear_breakpoints_unset_breakpoints(self):
+        '''Test clearing breakpoints like test_set_and_clear, but clear
+           breakpoints by omitting the breakpoints array instead of sending an
+           empty one.'''
+        lines = [line_number('main.cpp', 'break 12'),
+                 line_number('main.cpp', 'break 13')]
+
+        # 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 one breakpoint and verify that it got set correctly.
+        response = self.vscode.request_setBreakpoints(self.main_path, lines)
+        line_to_id = {}
+        breakpoints = response['body']['breakpoints']
+        self.assertEquals(len(breakpoints), len(lines),
+                        "expect %u source breakpoints" % (len(lines)))
+        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")
+
+        # Now clear all breakpoints for the source file by not setting the
+        # lines array.
+        lines = None
+        response = self.vscode.request_setBreakpoints(self.main_path, lines)
+        breakpoints = response['body']['breakpoints']
+        self.assertEquals(len(breakpoints), 0, "expect no source breakpoints")
+
+        # Verify with the target that all breakpoints have been cleared.
+        response = self.vscode.request_testGetTargetBreakpoints()
+        breakpoints = response['body']['breakpoints']
+        self.assertEquals(len(breakpoints), 0, "expect no source breakpoints")
+
+    @skipIfWindows
+    @skipIfRemote
     def test_functionality(self):
         '''Tests hitting breakpoints and the functionality of a single
            breakpoint, like 'conditions' and 'hitCondition' settings.'''
         loop_line = line_number('main.cpp', '// break loop')
 
         program = self.getBuildArtifact("a.out")
         self.build_and_launch(program)
         # Set a breakpoint at the loop line with no condition and no

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

void request_setBreakpoints(const llvm::json::Object &request) {
   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;
+  // "breakpoints" may be unset, in which case we treat it the same as being set
+  // to an empty array.
+  if (breakpoints) {
     for (const auto &bp : *breakpoints) {
       auto bp_obj = bp.getAsObject();
       if (bp_obj) {
         SourceBreakpoint src_bp(*bp_obj);
         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);
+          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, path,
                              src_bp.line);
             continue;
           }
         }
         // At this point the breakpoint is new
         src_bp.SetBreakpoint(path.data());
         AppendBreakpoint(src_bp.bp, response_breakpoints, path, src_bp.line);
         g_vsc.source_breakpoints[path][src_bp.line] = std::move(src_bp);
       }
     }
+  }
 
   // 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()) {
     for (auto &old_bp : old_src_bp_pos->second) {
       auto request_pos = request_bps.find(old_bp.first);