[lldb-dap] Deduplicate watchpoints starting at the same address on SetDataBreakpointsRequest.

[ZequanWu]

If a SetDataBreakpointsRequest contains a list data breakpoints which have duplicate starting addresses, the current behaviour is returning {verified: true} to both watchpoints with duplicated starting addresses. This confuses the client and what actually happens in lldb is the second one overwrite the first one.

This fixes it by letting the last watchpoint at given address have {verified: true} and all previous watchpoints at the same address should have {verfied: false} at response.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Zequan Wu (ZequanWu)

Changes

If a SetDataBreakpointsRequest contains a list data breakpoints which have duplicate starting addresses, the current behaviour is returning {verified: true} to both watchpoints with duplicated starting addresses. This confuses the client and what actually happens in lldb is the second one overwrite the first one.

This fixes it by letting the last watchpoint at given address have {verified: true} and all previous watchpoints at the same address should have {verfied: false} at response.

---

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

4 Files Affected:

• (modified) lldb/test/API/tools/lldb-dap/databreakpoint/TestDAP_setDataBreakpoints.py (+45)
• (modified) lldb/tools/lldb-dap/Watchpoint.cpp (+13-10)
• (modified) lldb/tools/lldb-dap/Watchpoint.h (+5)
• (modified) lldb/tools/lldb-dap/lldb-dap.cpp (+15-1)

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 17cdad89aa6d10..52c0bbfb33dad8 100644
--- a/lldb/test/API/tools/lldb-dap/databreakpoint/TestDAP_setDataBreakpoints.py
+++ b/lldb/test/API/tools/lldb-dap/databreakpoint/TestDAP_setDataBreakpoints.py
@@ -12,6 +12,51 @@ def setUp(self):
         lldbdap_testcase.DAPTestCaseBase.setUp(self)
         self.accessTypes = ["read", "write", "readWrite"]
 
+    @skipIfWindows
+    @skipIfRemote
+    def test_duplicate_start_addresses(self):
+        """Test setDataBreakpoints with multiple watchpoints starting at the same addresses."""
+        program = self.getBuildArtifact("a.out")
+        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])
+        self.continue_to_next_stop()
+        self.dap_server.get_stackFrame()
+        # Test setting write watchpoint using expressions: &x, arr+2
+        response_x = self.dap_server.request_dataBreakpointInfo(0, "&x")
+        response_arr_2 = self.dap_server.request_dataBreakpointInfo(0, "arr+2")
+        # Test response from dataBreakpointInfo request.
+        self.assertEquals(response_x["body"]["dataId"].split("/")[1], "4")
+        self.assertEquals(response_x["body"]["accessTypes"], self.accessTypes)
+        self.assertEquals(response_arr_2["body"]["dataId"].split("/")[1], "4")
+        self.assertEquals(response_arr_2["body"]["accessTypes"], self.accessTypes)
+        # The first one should be overwritten by the third one as they start at
+        # the same address. This is indicated by returning {verified: False} for
+        # the first one.
+        dataBreakpoints = [
+            {"dataId": response_x["body"]["dataId"], "accessType": "read"},
+            {"dataId": response_arr_2["body"]["dataId"], "accessType": "write"},
+            {"dataId": response_x["body"]["dataId"], "accessType": "write"},
+        ]
+        set_response = self.dap_server.request_setDataBreakpoint(dataBreakpoints)
+        self.assertEquals(
+            set_response["body"]["breakpoints"],
+            [{"verified": False}, {"verified": True}, {"verified": True}],
+        )
+
+        self.continue_to_next_stop()
+        x_val = self.dap_server.get_local_variable_value("x")
+        i_val = self.dap_server.get_local_variable_value("i")
+        self.assertEquals(x_val, "2")
+        self.assertEquals(i_val, "1")
+
+        self.continue_to_next_stop()
+        arr_2 = self.dap_server.get_local_variable_child("arr", "[2]")
+        i_val = self.dap_server.get_local_variable_value("i")
+        self.assertEquals(arr_2["value"], "42")
+        self.assertEquals(i_val, "2")
+
     @skipIfWindows
     @skipIfRemote
     def test_expression(self):
diff --git a/lldb/tools/lldb-dap/Watchpoint.cpp b/lldb/tools/lldb-dap/Watchpoint.cpp
index 2f176e0da84f15..21765509449140 100644
--- a/lldb/tools/lldb-dap/Watchpoint.cpp
+++ b/lldb/tools/lldb-dap/Watchpoint.cpp
@@ -16,17 +16,11 @@ Watchpoint::Watchpoint(const llvm::json::Object &obj) : BreakpointBase(obj) {
   llvm::StringRef dataId = GetString(obj, "dataId");
   std::string accessType = GetString(obj, "accessType").str();
   auto [addr_str, size_str] = dataId.split('/');
-  lldb::addr_t addr;
-  size_t size;
   llvm::to_integer(addr_str, addr, 16);
   llvm::to_integer(size_str, size);
-  lldb::SBWatchpointOptions options;
   options.SetWatchpointTypeRead(accessType != "write");
   if (accessType != "read")
     options.SetWatchpointTypeWrite(lldb::eWatchpointWriteTypeOnModify);
-  wp = g_dap.target.WatchpointCreateByAddress(addr, size, options, error);
-  SetCondition();
-  SetHitCondition();
 }
 
 void Watchpoint::SetCondition() { wp.SetCondition(condition.c_str()); }
@@ -38,11 +32,20 @@ void Watchpoint::SetHitCondition() {
 }
 
 void Watchpoint::CreateJsonObject(llvm::json::Object &object) {
-  if (error.Success()) {
-    object.try_emplace("verified", true);
-  } else {
+  if (!error.IsValid() || error.Fail()) {
     object.try_emplace("verified", false);
-    EmplaceSafeString(object, "message", error.GetCString());
+    if (error.Fail())
+      EmplaceSafeString(object, "message", error.GetCString());
+  } else {
+    object.try_emplace("verified", true);
   }
 }
+
+void Watchpoint::SetWatchpoint() {
+  wp = g_dap.target.WatchpointCreateByAddress(addr, size, options, error);
+  if (!condition.empty())
+    SetCondition();
+  if (!hitCondition.empty())
+    SetHitCondition();
+}
 } // namespace lldb_dap
diff --git a/lldb/tools/lldb-dap/Watchpoint.h b/lldb/tools/lldb-dap/Watchpoint.h
index 026b07d67241ce..4d2e58ed753360 100644
--- a/lldb/tools/lldb-dap/Watchpoint.h
+++ b/lldb/tools/lldb-dap/Watchpoint.h
@@ -17,6 +17,9 @@
 namespace lldb_dap {
 
 struct Watchpoint : public BreakpointBase {
+  lldb::addr_t addr;
+  size_t size;
+  lldb::SBWatchpointOptions options;
   // The LLDB breakpoint associated wit this watchpoint.
   lldb::SBWatchpoint wp;
   lldb::SBError error;
@@ -28,6 +31,8 @@ struct Watchpoint : public BreakpointBase {
   void SetCondition() override;
   void SetHitCondition() override;
   void CreateJsonObject(llvm::json::Object &object) override;
+
+  void SetWatchpoint();
 };
 } // namespace lldb_dap
 
diff --git a/lldb/tools/lldb-dap/lldb-dap.cpp b/lldb/tools/lldb-dap/lldb-dap.cpp
index c6a275bcf8140c..55f8c920e60016 100644
--- a/lldb/tools/lldb-dap/lldb-dap.cpp
+++ b/lldb/tools/lldb-dap/lldb-dap.cpp
@@ -2880,15 +2880,29 @@ void request_setDataBreakpoints(const llvm::json::Object &request) {
   const auto *breakpoints = arguments->getArray("breakpoints");
   llvm::json::Array response_breakpoints;
   g_dap.target.DeleteAllWatchpoints();
+  std::vector<Watchpoint> watchpoints;
   if (breakpoints) {
     for (const auto &bp : *breakpoints) {
       const auto *bp_obj = bp.getAsObject();
       if (bp_obj) {
         Watchpoint wp(*bp_obj);
-        AppendBreakpoint(&wp, response_breakpoints);
+        watchpoints.push_back(wp);
       }
     }
   }
+  // If two watchpoints start at the same address, the latter overwrite the
+  // former. So, we only enable those at first-seen addresses when iterating
+  // backward.
+  std::set<lldb::addr_t> addresses;
+  for (auto iter = watchpoints.rbegin(); iter != watchpoints.rend(); ++iter) {
+    if (addresses.count(iter->addr) == 0) {
+      iter->SetWatchpoint();
+      addresses.insert(iter->addr);
+    }
+  }
+  for (auto wp : watchpoints)
+    AppendBreakpoint(&wp, response_breakpoints);
+
   llvm::json::Object body;
   body.try_emplace("breakpoints", std::move(response_breakpoints));
   response.try_emplace("body", std::move(body));