return errors for invalid assembly breakpoints, fix request_setBreakpoints in python tests to support both paths and source_reference

Author: eronnen

lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py

@@ -551,6 +551,14 @@ def get_local_variable_child(
                 return child
         return None
 
+    def get_source_for_source_reference(self, source_reference):
+        return {"sourceReference": source_reference}
+
+    def get_source_for_path(self, file_path):
+        (dir, base) = os.path.split(file_path)
+        source_dict = {"name": base, "path": file_path}
+        return source_dict
+
     def replay_packets(self, replay_file_path):
         f = open(replay_file_path, "r")
         mode = "invalid"
@@ -948,20 +956,11 @@ def request_scopes(self, frameId):
         command_dict = {"command": "scopes", "type": "request", "arguments": args_dict}
         return self.send_recv(command_dict)
 
-    def request_setBreakpoints(self, file_path, line_array, data=None):
+    def request_setBreakpoints(self, source_dict, line_array, data=None):
         """data is array of parameters for breakpoints in line_array.
         Each parameter object is 1:1 mapping with entries in line_entry.
         It contains optional location/hitCondition/logMessage parameters.
         """
-        (dir, base) = os.path.split(file_path)
-        source_dict = {"name": base, "path": file_path}
-        return self.request_setBreakpoints_with_source(source_dict, line_array, data)
-
-    def request_setBreakpointsAssembly(self, sourceReference, line_array, data=None):
-        source_dict = {"sourceReference": sourceReference}
-        return self.request_setBreakpoints_with_source(source_dict, line_array, data)
-
-    def request_setBreakpoints_with_source(self, source_dict, line_array, data=None):
         args_dict = {
             "source": source_dict,
             "sourceModified": False,
@@ -1388,7 +1387,9 @@ def run_vscode(dbg, args, options):
                 else:
                     source_to_lines[path] = [int(line)]
         for source in source_to_lines:
-            dbg.request_setBreakpoints(source, source_to_lines[source])
+            dbg.request_setBreakpoints(
+                dbg.get_source_for_path(source), source_to_lines[source]
+            )
     if options.funcBreakpoints:
         dbg.request_setFunctionBreakpoints(options.funcBreakpoints)
 

lldb/packages/Python/lldbsuite/test/tools/lldb-dap/lldbdap_testcase.py

@@ -55,7 +55,9 @@ def set_source_breakpoints(self, source_path, lines, data=None):
         Each object in data is 1:1 mapping with the entry in lines.
         It contains optional location/hitCondition/logMessage parameters.
         """
-        response = self.dap_server.request_setBreakpoints(source_path, lines, data)
+        response = self.dap_server.request_setBreakpoints(
+            self.dap_server.get_source_for_path(source_path), lines, data
+        )
         if response is None or not response["success"]:
             return []
         breakpoints = response["body"]["breakpoints"]
@@ -65,8 +67,10 @@ def set_source_breakpoints(self, source_path, lines, data=None):
         return breakpoint_ids
 
     def set_source_breakpoints_assembly(self, source_reference, lines, data=None):
-        response = self.dap_server.request_setBreakpointsAssembly(
-            source_reference, lines, data
+        response = self.dap_server.request_setBreakpoints(
+            self.dap_server.get_source_for_source_reference(source_reference),
+            lines,
+            data,
         )
         if response is None:
             return []

lldb/test/API/tools/lldb-dap/breakpoint-assembly/TestDAP_breakpointAssembly.py

@@ -1,31 +1,19 @@
 """
-Test lldb-dap setBreakpoints request
+Test lldb-dap setBreakpoints request in assembly source references.
 """
 
 
-import dap_server
-import shutil
 from lldbsuite.test.decorators import *
-from lldbsuite.test.lldbtest import line_number
-from lldbsuite.test import lldbutil
 import lldbdap_testcase
-import os
 
 
+# @skipIfWindows
 class TestDAP_setBreakpointsAssembly(lldbdap_testcase.DAPTestCaseBase):
-    # @skipIfWindows
-    def test_functionality(self):
+    def test_can_break_in_source_references(self):
         """Tests hitting assembly source breakpoints"""
         program = self.getBuildArtifact("a.out")
         self.build_and_launch(program)
 
-        self.dap_server.request_evaluate(
-            "`settings set stop-disassembly-display no-debuginfo", context="repl"
-        )
-
-        finish_line = line_number("main.c", "// Break here")
-        finish_breakpoints = self.set_source_breakpoints("main.c", [finish_line])
-
         assmebly_func_breakpoints = self.set_function_breakpoints(["assembly_func"])
         self.continue_to_breakpoints(assmebly_func_breakpoints)
 
@@ -52,4 +40,43 @@ def test_functionality(self):
         # Clear the breakpoint and then check that the assembly breakpoint does not hit next time
         self.set_source_breakpoints_assembly(source_reference, [])
         self.continue_to_breakpoints(assmebly_func_breakpoints)
-        self.continue_to_breakpoints(finish_breakpoints)
+        self.continue_to_exit()
+
+    def test_break_on_invalid_source_reference(self):
+        """Tests hitting assembly source breakpoints"""
+        program = self.getBuildArtifact("a.out")
+        self.build_and_launch(program)
+
+        # Verify that setting a breakpoint on an invalid source reference fails
+        response = self.dap_server.request_setBreakpoints(
+            self.dap_server.get_source_for_source_reference(-1), [1]
+        )
+        self.assertIsNotNone(response)
+        breakpoints = response["body"]["breakpoints"]
+        self.assertEqual(len(breakpoints), 1)
+        breakpoint = breakpoints[0]
+        self.assertFalse(
+            breakpoint["verified"], "Expected breakpoint to not be verified"
+        )
+        self.assertIn("message", breakpoint, "Expected message to be present")
+        self.assertEqual(
+            breakpoint["message"],
+            "Invalid sourceReference.",
+        )
+
+        # Verify that setting a breakpoint on a source reference without a symbol also fails
+        response = self.dap_server.request_setBreakpoints(
+            self.dap_server.get_source_for_source_reference(0), [1]
+        )
+        self.assertIsNotNone(response)
+        breakpoints = response["body"]["breakpoints"]
+        self.assertEqual(len(breakpoints), 1)
+        breakpoint = breakpoints[0]
+        self.assertFalse(
+            breakpoint["verified"], "Expected breakpoint to not be verified"
+        )
+        self.assertIn("message", breakpoint, "Expected message to be present")
+        self.assertEqual(
+            breakpoint["message"],
+            "Breakpoints in assembly without a valid symbol are not supported yet.",
+        )

lldb/test/API/tools/lldb-dap/breakpoint-assembly/main.c

@@ -1,5 +1,3 @@
-#include <stddef.h>
-
 __attribute__((nodebug)) int assembly_func(int n) {
   n += 1;
   n += 2;
@@ -12,5 +10,5 @@ int main(int argc, char const *argv[]) {
   assembly_func(10);
   assembly_func(20);
   assembly_func(30);
-  return 0; // Break here
+  return 0;
 }

lldb/test/API/tools/lldb-dap/breakpoint-events/TestDAP_breakpointEvents.py

@@ -58,7 +58,7 @@ def test_breakpoint_events(self):
         # Set breakpoints and verify that they got set correctly
         dap_breakpoint_ids = []
         response = self.dap_server.request_setBreakpoints(
-            main_source_path, [main_bp_line]
+            self.dap_server.get_source_for_path(main_source_path), [main_bp_line]
         )
         self.assertTrue(response["success"])
         breakpoints = response["body"]["breakpoints"]
@@ -70,7 +70,7 @@ def test_breakpoint_events(self):
             )
 
         response = self.dap_server.request_setBreakpoints(
-            foo_source_path, [foo_bp1_line]
+            self.dap_server.get_source_for_path(foo_source_path), [foo_bp1_line]
         )
         self.assertTrue(response["success"])
         breakpoints = response["body"]["breakpoints"]

lldb/test/API/tools/lldb-dap/breakpoint/TestDAP_setBreakpoints.py

@@ -58,7 +58,9 @@ def test_source_map(self):
         self.launch(program, sourceMap=source_map)
 
         # breakpoint in main.cpp
-        response = self.dap_server.request_setBreakpoints(new_main_path, [main_line])
+        response = self.dap_server.request_setBreakpoints(
+            self.dap_server.get_source_for_path(new_main_path), [main_line]
+        )
         breakpoints = response["body"]["breakpoints"]
         self.assertEqual(len(breakpoints), 1)
         breakpoint = breakpoints[0]
@@ -68,7 +70,9 @@ def test_source_map(self):
         self.assertEqual(new_main_path, breakpoint["source"]["path"])
 
         # 2nd breakpoint, which is from a dynamically loaded library
-        response = self.dap_server.request_setBreakpoints(new_other_path, [other_line])
+        response = self.dap_server.request_setBreakpoints(
+            self.dap_server.get_source_for_path(new_other_path), [other_line]
+        )
         breakpoints = response["body"]["breakpoints"]
         breakpoint = breakpoints[0]
         self.assertEqual(breakpoint["line"], other_line)
@@ -81,7 +85,9 @@ def test_source_map(self):
         self.verify_breakpoint_hit([other_breakpoint_id])
 
         # 2nd breakpoint again, which should be valid at this point
-        response = self.dap_server.request_setBreakpoints(new_other_path, [other_line])
+        response = self.dap_server.request_setBreakpoints(
+            self.dap_server.get_source_for_path(new_other_path), [other_line]
+        )
         breakpoints = response["body"]["breakpoints"]
         breakpoint = breakpoints[0]
         self.assertEqual(breakpoint["line"], other_line)
@@ -124,7 +130,9 @@ def test_set_and_clear(self):
         self.build_and_launch(program)
 
         # Set 3 breakpoints and verify that they got set correctly
-        response = self.dap_server.request_setBreakpoints(self.main_path, lines)
+        response = self.dap_server.request_setBreakpoints(
+            self.dap_server.get_source_for_path(self.main_path), lines
+        )
         line_to_id = {}
         breakpoints = response["body"]["breakpoints"]
         self.assertEqual(
@@ -149,7 +157,9 @@ def test_set_and_clear(self):
         lines.remove(second_line)
         # Set 2 breakpoints and verify that the previous breakpoints that were
         # set above are still set.
-        response = self.dap_server.request_setBreakpoints(self.main_path, lines)
+        response = self.dap_server.request_setBreakpoints(
+            self.dap_server.get_source_for_path(self.main_path), lines
+        )
         breakpoints = response["body"]["breakpoints"]
         self.assertEqual(
             len(breakpoints),
@@ -194,7 +204,9 @@ def test_set_and_clear(self):
         # Now clear all breakpoints for the source file by passing down an
         # empty lines array
         lines = []
-        response = self.dap_server.request_setBreakpoints(self.main_path, lines)
+        response = self.dap_server.request_setBreakpoints(
+            self.dap_server.get_source_for_path(self.main_path), lines
+        )
         breakpoints = response["body"]["breakpoints"]
         self.assertEqual(
             len(breakpoints),
@@ -214,7 +226,9 @@ def test_set_and_clear(self):
         # Now set a breakpoint again in the same source file and verify it
         # was added.
         lines = [second_line]
-        response = self.dap_server.request_setBreakpoints(self.main_path, lines)
+        response = self.dap_server.request_setBreakpoints(
+            self.dap_server.get_source_for_path(self.main_path), lines
+        )
         if response:
             breakpoints = response["body"]["breakpoints"]
             self.assertEqual(
@@ -265,7 +279,9 @@ def test_clear_breakpoints_unset_breakpoints(self):
         self.build_and_launch(program)
 
         # Set one breakpoint and verify that it got set correctly.
-        response = self.dap_server.request_setBreakpoints(self.main_path, lines)
+        response = self.dap_server.request_setBreakpoints(
+            self.dap_server.get_source_for_path(self.main_path), lines
+        )
         line_to_id = {}
         breakpoints = response["body"]["breakpoints"]
         self.assertEqual(
@@ -281,7 +297,9 @@ def test_clear_breakpoints_unset_breakpoints(self):
         # Now clear all breakpoints for the source file by not setting the
         # lines array.
         lines = None
-        response = self.dap_server.request_setBreakpoints(self.main_path, lines)
+        response = self.dap_server.request_setBreakpoints(
+            self.dap_server.get_source_for_path(self.main_path), lines
+        )
         breakpoints = response["body"]["breakpoints"]
         self.assertEqual(len(breakpoints), 0, "expect no source breakpoints")
 
@@ -357,7 +375,9 @@ def test_column_breakpoints(self):
         # Set two breakpoints on the loop line at different columns.
         columns = [13, 39]
         response = self.dap_server.request_setBreakpoints(
-            self.main_path, [loop_line, loop_line], list({"column": c} for c in columns)
+            self.dap_server.get_source_for_path(self.main_path),
+            [loop_line, loop_line],
+            list({"column": c} for c in columns),
         )
 
         # Verify the breakpoints were set correctly

lldb/test/API/tools/lldb-dap/instruction-breakpoint/TestDAP_instruction_breakpoint.py

@@ -33,7 +33,9 @@ def instruction_breakpoint_test(self):
         self.build_and_launch(program)
 
         # Set source breakpoint 1
-        response = self.dap_server.request_setBreakpoints(self.main_path, [main_line])
+        response = self.dap_server.request_setBreakpoints(
+            self.dap_server.get_source_for_path(self.main_path), [main_line]
+        )
         breakpoints = response["body"]["breakpoints"]
         self.assertEqual(len(breakpoints), 1)
         breakpoint = breakpoints[0]

lldb/tools/lldb-dap/DAP.cpp

@@ -1658,9 +1658,16 @@ std::vector<protocol::Breakpoint> DAP::SetSourceBreakpoints(
       const auto [iv, inserted] =
           existing_breakpoints.try_emplace(bp_pos, src_bp);
       // We check if this breakpoint already exists to update it.
-      if (inserted)
-        iv->second.SetBreakpoint(source);
-      else
+      if (inserted) {
+        if (auto error = iv->second.SetBreakpoint(source)) {
+          protocol::Breakpoint invalid_breakpoint;
+          invalid_breakpoint.message = llvm::toString(std::move(error));
+          invalid_breakpoint.verified = false;
+          response_breakpoints.push_back(std::move(invalid_breakpoint));
+          existing_breakpoints.erase(iv);
+          continue;
+        }
+      } else
         iv->second.UpdateBreakpoint(src_bp);
 
       protocol::Breakpoint response_breakpoint =
@@ -1673,7 +1680,7 @@ std::vector<protocol::Breakpoint> DAP::SetSourceBreakpoints(
       if (!response_breakpoint.column &&
           src_bp.GetColumn() != LLDB_INVALID_COLUMN_NUMBER)
         response_breakpoint.column = src_bp.GetColumn();
-      response_breakpoints.push_back(response_breakpoint);
+      response_breakpoints.push_back(std::move(response_breakpoint));
     }
   }
 

lldb/tools/lldb-dap/Handler/BreakpointLocationsHandler.cpp

@@ -104,9 +104,9 @@ BreakpointLocationsRequestHandler::GetSourceBreakpointLocations(
 
 std::vector<std::pair<uint32_t, uint32_t>>
 BreakpointLocationsRequestHandler::GetAssemblyBreakpointLocations(
-    int64_t sourceReference, uint32_t start_line, uint32_t end_line) const {
+    int64_t source_reference, uint32_t start_line, uint32_t end_line) const {
   std::vector<std::pair<uint32_t, uint32_t>> locations;
-  lldb::SBAddress address(sourceReference, dap.target);
+  lldb::SBAddress address(source_reference, dap.target);
   if (!address.IsValid())
     return locations;
 

lldb/tools/lldb-dap/Handler/RequestHandler.h

@@ -239,7 +239,7 @@ class BreakpointLocationsRequestHandler
                                uint32_t start_column, uint32_t end_line,
                                uint32_t end_column) const;
   std::vector<std::pair<uint32_t, uint32_t>>
-  GetAssemblyBreakpointLocations(int64_t sourceReference, uint32_t start_line,
+  GetAssemblyBreakpointLocations(int64_t source_reference, uint32_t start_line,
                                  uint32_t end_line) const;
 };