[Lldb-commits] [lldb] [lldb-dap] Deduplicate watchpoints starting at the same address on SetDataBreakpointsRequest. (PR #83192)
Zequan Wu via lldb-commits
lldb-commits at lists.llvm.org
Tue Feb 27 13:48:47 PST 2024
https://github.com/ZequanWu created https://github.com/llvm/llvm-project/pull/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.
>From 563a4e5c9306fb5f06bdcc4a1b71dc92efbc86f8 Mon Sep 17 00:00:00 2001
From: Zequan Wu <zequanwu at google.com>
Date: Tue, 27 Feb 2024 16:40:38 -0500
Subject: [PATCH] [lldb-dap] Deduplicate watchpoints starting at the same
address on SetDataBreakpointsRequest.
---
.../TestDAP_setDataBreakpoints.py | 45 +++++++++++++++++++
lldb/tools/lldb-dap/Watchpoint.cpp | 23 +++++-----
lldb/tools/lldb-dap/Watchpoint.h | 5 +++
lldb/tools/lldb-dap/lldb-dap.cpp | 16 ++++++-
4 files changed, 78 insertions(+), 11 deletions(-)
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));
More information about the lldb-commits
mailing list