[lldb-dap] Correctly report breakpoints as resolved on macOS

[JDevlieghere]

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

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Jonas Devlieghere (JDevlieghere)

Changes

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

---

Full diff: https://github.com/llvm/llvm-project/pull/129589.diff

2 Files Affected:

• (modified) lldb/test/API/tools/lldb-dap/breakpoint-events/TestDAP_breakpointEvents.py (+55-55)
• (modified) lldb/tools/lldb-dap/Handler/InitializeRequestHandler.cpp (+17-14)

diff --git a/lldb/test/API/tools/lldb-dap/breakpoint-events/TestDAP_breakpointEvents.py b/lldb/test/API/tools/lldb-dap/breakpoint-events/TestDAP_breakpointEvents.py
index 11573eba06907..e5590e1b332a0 100644
--- a/lldb/test/API/tools/lldb-dap/breakpoint-events/TestDAP_breakpointEvents.py
+++ b/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")
diff --git a/lldb/tools/lldb-dap/Handler/InitializeRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/InitializeRequestHandler.cpp
index e9041f3985523..a3981769782d9 100644
--- a/lldb/tools/lldb-dap/Handler/InitializeRequestHandler.cpp
+++ b/lldb/tools/lldb-dap/Handler/InitializeRequestHandler.cpp
@@ -137,27 +137,30 @@ 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
+          // If the breakpoint was originated from DAP, 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 ((event_type & lldb::eBreakpointEventTypeLocationsAdded ||
-               event_type & lldb::eBreakpointEventTypeLocationsRemoved) &&
-              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.
+          // of whether  locations were added, removed, or resolved, the
+          // breakpoint isn't going away and the reason is always "changed".
+          if (bp.MatchesName(BreakpointBase::GetBreakpointLabel()) &&
+              (event_type & lldb::eBreakpointEventTypeLocationsAdded ||
+               event_type & lldb::eBreakpointEventTypeLocationsRemoved ||
+               event_type & lldb::eBreakpointEventTypeLocationsResolved)) {
+            // 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
+            // prevent us from 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)));
           }
         }

[Jlalond]

Is the first sentence redundant because we're in LLDB-DAP?

[JDevlieghere]

No, it distinguishes from a breakpoint that was set through a command in the console or for example through your ~/.lldbinit.

[Jlalond]

LGTM