[Lldb-commits] [lldb] 2cacc7a - [lldb-dap] Deduplicate watchpoints starting at the same address on SetDataBreakpointsRequest. (#83192)
via lldb-commits
lldb-commits at lists.llvm.org
Wed Feb 28 11:58:42 PST 2024
Author: Zequan Wu
Date: 2024-02-28T14:56:55-05:00
New Revision: 2cacc7a61095577ff42177373d46c8cb8df0cb1f
URL: https://github.com/llvm/llvm-project/commit/2cacc7a61095577ff42177373d46c8cb8df0cb1f
DIFF: https://github.com/llvm/llvm-project/commit/2cacc7a61095577ff42177373d46c8cb8df0cb1f.diff
LOG: [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.
Added:
Modified:
lldb/test/API/tools/lldb-dap/databreakpoint/TestDAP_setDataBreakpoints.py
lldb/tools/lldb-dap/Watchpoint.cpp
lldb/tools/lldb-dap/Watchpoint.h
lldb/tools/lldb-dap/lldb-dap.cpp
Removed:
################################################################################
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