[lldb-dap] Deduplicate watchpoints starting at the same address on SetDataBreakpointsRequest. (#83192)

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.

Author: ZequanWu

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):

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

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
 

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));