[lldb-dap] Correctly report breakpoints as resolved on macOS (#129589)

On macOS, breakpoints are briefly unresolved between process launch and
when the dynamic loader has informed us about the loaded libraries. This
information was being forwarded by lldb-dap, but only partially. In the
event handler, we were listening for the `LocationsAdded` and
`LocationsRemoved` breakpoint events. For the scenario described above,
the latter would trigger and we'd send an event reporting the breakpoint
as unresolved. The problem is that when the breakpoint location is
resolved again, you receive a `LocationsResolved` event, not a
`LocationsAdded` event. As a result, the breakpoint would continue to
show up as unresolved in the DAP client.

I found a test that tried to test part of this behavior, but the test
was broken and disabled. I revived the test and added coverage for the
situation described above.

Fixes #112629
rdar://137968318

Author: JDevlieghere

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

@@ -2,7 +2,6 @@
 Test lldb-dap setBreakpoints request
 """
 
-
 import dap_server
 from lldbsuite.test.decorators import *
 from lldbsuite.test.lldbtest import *
@@ -12,9 +11,7 @@
 
 
 class TestDAP_breakpointEvents(lldbdap_testcase.DAPTestCaseBase):
-    @skipIfWindows
     @skipUnlessDarwin
-    @expectedFailureAll(macos_version=[">=", "13.0"])
     def test_breakpoint_events(self):
         """
         This test sets a breakpoint in a shared library and runs and stops
@@ -63,70 +60,73 @@ def test_breakpoint_events(self):
         response = self.dap_server.request_setBreakpoints(
             main_source_path, [main_bp_line]
         )
-        if response:
-            breakpoints = response["body"]["breakpoints"]
-            for breakpoint in breakpoints:
-                main_bp_id = breakpoint["id"]
-                dap_breakpoint_ids.append("%i" % (main_bp_id))
-                # line = breakpoint['line']
-                self.assertTrue(
-                    breakpoint["verified"], "expect main breakpoint to be verified"
-                )
+        self.assertTrue(response)
+        breakpoints = response["body"]["breakpoints"]
+        for breakpoint in breakpoints:
+            main_bp_id = breakpoint["id"]
+            dap_breakpoint_ids.append("%i" % (main_bp_id))
+            self.assertTrue(
+                breakpoint["verified"], "expect main breakpoint to be verified"
+            )
 
         response = self.dap_server.request_setBreakpoints(
             foo_source_path, [foo_bp1_line]
         )
-        if response:
-            breakpoints = response["body"]["breakpoints"]
-            for breakpoint in breakpoints:
-                foo_bp_id = breakpoint["id"]
-                dap_breakpoint_ids.append("%i" % (foo_bp_id))
-                self.assertFalse(
-                    breakpoint["verified"], "expect foo breakpoint to not be verified"
-                )
+        self.assertTrue(response)
+        breakpoints = response["body"]["breakpoints"]
+        for breakpoint in breakpoints:
+            foo_bp_id = breakpoint["id"]
+            dap_breakpoint_ids.append("%i" % (foo_bp_id))
+            self.assertFalse(
+                breakpoint["verified"], "expect foo breakpoint to not be verified"
+            )
 
         # Get the stop at the entry point
         self.continue_to_next_stop()
 
         # We are now stopped at the entry point to the program. Shared
-        # libraries are not loaded yet (at least on macOS they aren't) and any
-        # breakpoints set in foo.cpp should not be resolved.
+        # libraries are not loaded yet (at least on macOS they aren't) and only
+        # the breakpoint in the main executable should be resolved.
+        self.assertEqual(len(self.dap_server.breakpoint_events), 1)
+        event = self.dap_server.breakpoint_events[0]
+        body = event["body"]
         self.assertEqual(
-            len(self.dap_server.breakpoint_events),
-            0,
-            "no breakpoint events when stopped at entry point",
+            body["reason"], "changed", "breakpoint event should say changed"
         )
+        breakpoint = body["breakpoint"]
+        self.assertEqual(breakpoint["id"], main_bp_id)
+        self.assertTrue(breakpoint["verified"], "main breakpoint should be resolved")
+
+        # Clear the list of breakpoint events so we don't see this one again.
+        self.dap_server.breakpoint_events.clear()
 
         # Continue to the breakpoint
         self.continue_to_breakpoints(dap_breakpoint_ids)
 
-        # Make sure we only get an event for the breakpoint we set via a call
-        # to self.dap_server.request_setBreakpoints(...), not the breakpoint
-        # we set with with a LLDB command in preRunCommands.
-        self.assertEqual(
-            len(self.dap_server.breakpoint_events),
-            1,
-            "make sure we got a breakpoint event",
-        )
-        event = self.dap_server.breakpoint_events[0]
-        # Verify the details of the breakpoint changed notification.
-        body = event["body"]
-        self.assertEqual(
-            body["reason"], "changed", "breakpoint event is says breakpoint is changed"
-        )
-        breakpoint = body["breakpoint"]
-        self.assertTrue(
-            breakpoint["verified"], "breakpoint event is says it is verified"
-        )
-        self.assertEqual(
-            breakpoint["id"],
-            foo_bp_id,
-            "breakpoint event is for breakpoint %i" % (foo_bp_id),
-        )
-        self.assertTrue(
-            "line" in breakpoint and breakpoint["line"] > 0,
-            "breakpoint event is has a line number",
-        )
-        self.assertNotIn(
-            "source", breakpoint, "breakpoint event should not return a source object"
-        )
+        # When the process launches, we first expect to see both the main and
+        # foo breakpoint as unresolved.
+        for event in self.dap_server.breakpoint_events[:2]:
+            body = event["body"]
+            self.assertEqual(
+                body["reason"], "changed", "breakpoint event should say changed"
+            )
+            breakpoint = body["breakpoint"]
+            self.assertIn(str(breakpoint["id"]), dap_breakpoint_ids)
+            self.assertFalse(breakpoint["verified"], "breakpoint should be unresolved")
+
+        # Then, once the dynamic loader has given us a load address, they
+        # should show up as resolved again.
+        for event in self.dap_server.breakpoint_events[3:]:
+            body = event["body"]
+            self.assertEqual(
+                body["reason"], "changed", "breakpoint event should say changed"
+            )
+            breakpoint = body["breakpoint"]
+            self.assertIn(str(breakpoint["id"]), dap_breakpoint_ids)
+            self.assertTrue(breakpoint["verified"], "breakpoint should be resolved")
+            self.assertNotIn(
+                "source",
+                breakpoint,
+                "breakpoint event should not return a source object",
+            )
+            self.assertIn("line", breakpoint, "breakpoint event should have line")

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

@@ -137,27 +137,29 @@ static void EventThreadFunction(DAP &dap) {
               lldb::SBBreakpoint::GetBreakpointEventTypeFromEvent(event);
           auto bp = Breakpoint(
               dap, lldb::SBBreakpoint::GetBreakpointFromEvent(event));
-          // If the breakpoint was originated from the IDE, it will have the
-          // BreakpointBase::GetBreakpointLabel() label attached. Regardless
-          // of wether the locations were added or removed, the breakpoint
-          // ins't going away, so we the reason is always "changed".
+          // If the breakpoint was set through DAP, it will have the
+          // BreakpointBase::GetBreakpointLabel() label. Regardless
+          // of whether  locations were added, removed, or resolved, the
+          // breakpoint isn't going away and the reason is always "changed".
           if ((event_type & lldb::eBreakpointEventTypeLocationsAdded ||
-               event_type & lldb::eBreakpointEventTypeLocationsRemoved) &&
+               event_type & lldb::eBreakpointEventTypeLocationsRemoved ||
+               event_type & lldb::eBreakpointEventTypeLocationsResolved) &&
               bp.MatchesName(BreakpointBase::GetBreakpointLabel())) {
-            auto bp_event = CreateEventObject("breakpoint");
-            llvm::json::Object body;
-            // As VSCode already knows the path of this breakpoint, we don't
-            // need to send it back as part of a "changed" event. This
-            // prevent us from sending to VSCode paths that should be source
-            // mapped. Note that CreateBreakpoint doesn't apply source mapping.
-            // Besides, the current implementation of VSCode ignores the
-            // "source" element of breakpoint events.
+            // As the DAP client already knows the path of this breakpoint, we
+            // don't need to send it back as part of the "changed" event. This
+            // avoids sending paths that should be source mapped. Note that
+            // CreateBreakpoint doesn't apply source mapping and certain
+            // implementation ignore the source part of this event anyway.
             llvm::json::Value source_bp = CreateBreakpoint(&bp);
             source_bp.getAsObject()->erase("source");
 
+            llvm::json::Object body;
             body.try_emplace("breakpoint", source_bp);
             body.try_emplace("reason", "changed");
+
+            llvm::json::Object bp_event = CreateEventObject("breakpoint");
             bp_event.try_emplace("body", std::move(body));
+
             dap.SendJSON(llvm::json::Value(std::move(bp_event)));
           }
         }