[Lldb-commits] [lldb] [lldb-dap] Make Breakpoint ids unique. (PR #193526)

[via lldb-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-lldb

Author: Ebuka Ezike (da-viper)

<details>
<summary>Changes</summary>

In normal lldb you can have a breakpoint and watchpoint with the same id. This is not the case in DAP protocol as Breakpoint id is unique for each session.

So we end up waiting for the wrong breakpoint id that never gets hit.

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


4 Files Affected:

- (modified) lldb/test/API/tools/lldb-dap/databreakpoint/TestDAP_setDataBreakpoints.py (+10-1) 
- (modified) lldb/tools/lldb-dap/EventHelper.cpp (+2-1) 
- (modified) lldb/tools/lldb-dap/ProtocolUtils.h (+9) 
- (modified) lldb/tools/lldb-dap/Watchpoint.cpp (+2-1) 


``````````diff
diff --git a/lldb/test/API/tools/lldb-dap/databreakpoint/TestDAP_setDataBreakpoints.py b/lldb/test/API/tools/lldb-dap/databreakpoint/TestDAP_setDataBreakpoints.py
index 0c8ba4e5d07af..de7ace66633d7 100644
--- a/lldb/test/API/tools/lldb-dap/databreakpoint/TestDAP_setDataBreakpoints.py
+++ b/lldb/test/API/tools/lldb-dap/databreakpoint/TestDAP_setDataBreakpoints.py
@@ -108,7 +108,8 @@ def test_functionality(self):
         self.build_and_launch(program)
         source = "main.cpp"
         first_loop_break_line = line_number(source, "// first loop breakpoint")
-        self.set_source_breakpoints(source, [first_loop_break_line])
+        first_bp_ids = self.set_source_breakpoints(source, [first_loop_break_line])
+        self.assertEqual(len(first_bp_ids), 1)
         self.continue_to_next_stop()
         self.dap_server.get_local_variables()
         locals_ref = self.get_locals_scope_reference()
@@ -150,6 +151,14 @@ def test_functionality(self):
         self.assertEqual(i_val, "2")
         self.dap_server.request_setDataBreakpoint([])
 
+        # Verify breakpoints are unique.
+        all_breakpoints = set(
+            [first_bp_ids[0], breakpoints[0]["id"], breakpoints[1]["id"]]
+        )
+        self.assertEqual(
+            len(all_breakpoints), 3, f"found breakpoints {all_breakpoints}"
+        )
+
         # Test hit condition
         second_loop_break_line = line_number(source, "// second loop breakpoint")
         breakpoint_ids = self.set_source_breakpoints(source, [second_loop_break_line])
diff --git a/lldb/tools/lldb-dap/EventHelper.cpp b/lldb/tools/lldb-dap/EventHelper.cpp
index a41b202a998ba..3661cc36affd1 100644
--- a/lldb/tools/lldb-dap/EventHelper.cpp
+++ b/lldb/tools/lldb-dap/EventHelper.cpp
@@ -215,7 +215,8 @@ static void SendStoppedEvent(DAP &dap, lldb::SBThread &thread, bool on_entry,
     } break;
     case lldb::eStopReasonWatchpoint: {
       body.reason = protocol::eStoppedReasonDataBreakpoint;
-      lldb::break_id_t bp_id = thread.GetStopReasonDataAtIndex(0);
+      lldb::break_id_t bp_id =
+          ApplyWatchpointMask(thread.GetStopReasonDataAtIndex(0));
       body.hitBreakpointIds.push_back(bp_id);
       body.text = llvm::formatv("data breakpoint {0}", bp_id).str();
     } break;
diff --git a/lldb/tools/lldb-dap/ProtocolUtils.h b/lldb/tools/lldb-dap/ProtocolUtils.h
index f3ce2f22ac1c2..c7742c04c2c68 100644
--- a/lldb/tools/lldb-dap/ProtocolUtils.h
+++ b/lldb/tools/lldb-dap/ProtocolUtils.h
@@ -17,6 +17,7 @@
 #include "Protocol/ProtocolTypes.h"
 
 #include "lldb/API/SBAddress.h"
+#include "lldb/lldb-types.h"
 
 namespace lldb_dap {
 
@@ -107,6 +108,14 @@ CreateExceptionBreakpointFilter(const ExceptionBreakpoint &bp);
 ///     "2 MB").
 std::string ConvertDebugInfoSizeToString(uint64_t debug_size);
 
+/// Add a mask to the breakpoint's id, this is to avoid id collision
+/// as internally, lldb breakpoint's id and watchpoint's id starts from one.
+/// Similar to the variables_reference we start from 8'000'000.
+inline lldb::break_id_t ApplyWatchpointMask(lldb::break_id_t breakpoint_id) {
+  constexpr lldb::break_id_t watchpoint_mask = 8'000'000;
+  return watchpoint_mask + breakpoint_id;
+}
+
 } // namespace lldb_dap
 
 #endif
diff --git a/lldb/tools/lldb-dap/Watchpoint.cpp b/lldb/tools/lldb-dap/Watchpoint.cpp
index e730e71c0dc31..f9512104128d2 100644
--- a/lldb/tools/lldb-dap/Watchpoint.cpp
+++ b/lldb/tools/lldb-dap/Watchpoint.cpp
@@ -9,6 +9,7 @@
 #include "Watchpoint.h"
 #include "DAP.h"
 #include "Protocol/ProtocolTypes.h"
+#include "ProtocolUtils.h"
 #include "lldb/API/SBTarget.h"
 #include "lldb/lldb-enumerations.h"
 #include "llvm/ADT/StringExtras.h"
@@ -45,7 +46,7 @@ protocol::Breakpoint Watchpoint::ToProtocolBreakpoint() {
       breakpoint.message = m_error.GetCString();
   } else {
     breakpoint.verified = true;
-    breakpoint.id = m_wp.GetID();
+    breakpoint.id = ApplyWatchpointMask(m_wp.GetID());
   }
 
   return breakpoint;

``````````

</details>


https://github.com/llvm/llvm-project/pull/193526