[lldb][lldb-dap] Support breakpoint info bytes #141122
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-lldb Author: Ebuka Ezike (da-viper) ChangesThis adds the support for Full diff: https://github.com/llvm/llvm-project/pull/141122.diff 7 Files Affected:
diff --git a/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py b/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py
index a028381a0a4f9..2468ca7e37f14 100644
--- a/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py
+++ b/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py
@@ -1042,16 +1042,31 @@ def request_setFunctionBreakpoints(self, names, condition=None, hitCondition=Non
return self.send_recv(command_dict)
def request_dataBreakpointInfo(
- self, variablesReference, name, frameIndex=0, threadId=None
+ self,
+ name: str,
+ variablesReference: int = None,
+ frameIndex: int = 0,
+ bytes_: int = None,
+ asAddress: bool = None,
):
- stackFrame = self.get_stackFrame(frameIndex=frameIndex, threadId=threadId)
- if stackFrame is None:
- return []
- args_dict = {
- "variablesReference": variablesReference,
- "name": name,
- "frameId": stackFrame["id"],
- }
+
+ args_dict = {}
+ if asAddress is not None:
+ args_dict = {
+ "name": name,
+ "asAddress": asAddress,
+ "bytes": bytes_,
+ }
+ else:
+ stackFrame = self.get_stackFrame(frameIndex=frameIndex, threadId=None)
+ if stackFrame is None:
+ return []
+ args_dict = {
+ "variablesReference": variablesReference,
+ "name": name,
+ "frameId": stackFrame["id"],
+ }
+
command_dict = {
"command": "dataBreakpointInfo",
"type": "request",
diff --git a/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/lldbdap_testcase.py b/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/lldbdap_testcase.py
index 91ae55977046b..996c85ea69622 100644
--- a/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/lldbdap_testcase.py
+++ b/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/lldbdap_testcase.py
@@ -104,7 +104,9 @@ def waitUntil(self, condition_callback):
time.sleep(0.5)
return False
- def verify_breakpoint_hit(self, breakpoint_ids, timeout=DEFAULT_TIMEOUT):
+ def verify_breakpoint_hit(
+ self, breakpoint_ids, timeout=DEFAULT_TIMEOUT, is_watchpoint=False
+ ):
"""Wait for the process we are debugging to stop, and verify we hit
any breakpoint location in the "breakpoint_ids" array.
"breakpoint_ids" should be a list of breakpoint ID strings
@@ -131,9 +133,10 @@ def verify_breakpoint_hit(self, breakpoint_ids, timeout=DEFAULT_TIMEOUT):
# So when looking at the description we just want to make sure
# the right breakpoint matches and not worry about the actual
# location.
+ type_name = "watchpoint" if is_watchpoint else "breakpoint"
description = body["description"]
for breakpoint_id in breakpoint_ids:
- match_desc = f"breakpoint {breakpoint_id}."
+ match_desc = f"{type_name} {breakpoint_id}"
if match_desc in description:
return
self.assertTrue(False, f"breakpoint not hit, stopped_events={stopped_events}")
@@ -329,12 +332,16 @@ def continue_to_next_stop(self, timeout=DEFAULT_TIMEOUT):
self.do_continue()
return self.dap_server.wait_for_stopped(timeout)
- def continue_to_breakpoint(self, breakpoint_id: str, timeout=DEFAULT_TIMEOUT):
- self.continue_to_breakpoints((breakpoint_id), timeout)
+ def continue_to_breakpoint(
+ self, breakpoint_id: str, timeout=DEFAULT_TIMEOUT, is_watchpoint=False
+ ):
+ self.continue_to_breakpoints([breakpoint_id], timeout, is_watchpoint)
- def continue_to_breakpoints(self, breakpoint_ids, timeout=DEFAULT_TIMEOUT):
+ def continue_to_breakpoints(
+ self, breakpoint_ids, timeout=DEFAULT_TIMEOUT, is_watchpoint=False
+ ):
self.do_continue()
- self.verify_breakpoint_hit(breakpoint_ids, timeout)
+ self.verify_breakpoint_hit(breakpoint_ids, timeout, is_watchpoint)
def continue_to_exception_breakpoint(self, filter_label, timeout=DEFAULT_TIMEOUT):
self.do_continue()
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 a542a318050dd..62392f2c49afd 100644
--- a/lldb/test/API/tools/lldb-dap/databreakpoint/TestDAP_setDataBreakpoints.py
+++ b/lldb/test/API/tools/lldb-dap/databreakpoint/TestDAP_setDataBreakpoints.py
@@ -23,8 +23,8 @@ def test_duplicate_start_addresses(self):
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")
+ response_x = self.dap_server.request_dataBreakpointInfo("&x", 0)
+ response_arr_2 = self.dap_server.request_dataBreakpointInfo("arr+2", 0)
# Test response from dataBreakpointInfo request.
self.assertEqual(response_x["body"]["dataId"].split("/")[1], "4")
self.assertEqual(response_x["body"]["accessTypes"], self.accessTypes)
@@ -56,6 +56,47 @@ def test_duplicate_start_addresses(self):
self.assertEqual(arr_2["value"], "42")
self.assertEqual(i_val, "2")
+ @skipIfWindows
+ def test_breakpoint_info_bytes(self):
+ """Test supportBreakpointInfoBytes
+ Set the watchpoint on `var` variable address + 6 characters.
+ """
+ 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()
+
+ # get the address of `var` variable
+ eval_response = self.dap_server.request_evaluate("&var", context="watch")
+ self.assertTrue(eval_response["success"])
+ var_address = eval_response["body"]["result"]
+
+ var_byte_watch_size = 5
+ bp_resp = self.dap_server.request_dataBreakpointInfo(
+ var_address, asAddress=True, bytes_=var_byte_watch_size
+ )
+ resp_data_id = bp_resp["body"]["dataId"]
+ self.assertTrue(
+ bp_resp["success"], f"dataBreakpointInfo request failed: {bp_resp}"
+ )
+ self.assertEqual(resp_data_id.split("/")[1], str(var_byte_watch_size))
+
+ data_breakpoints = [{"dataId": resp_data_id, "accessType": "write"}]
+ self.dap_server.request_setDataBreakpoint(data_breakpoints)
+
+ self.continue_to_breakpoint(breakpoint_id=1, is_watchpoint=True)
+ eval_response = self.dap_server.request_evaluate("var", context="watch")
+ self.assertTrue(eval_response["success"])
+ var_value = eval_response["body"]["result"]
+ self.assertEqual(var_value, '"HALLO"')
+
+ # Remove the watchpoint because once it leaves this function scope, the address can be
+ # be used by another variable or register.
+ self.dap_server.request_setDataBreakpoint([])
+ self.continue_to_exit()
+
@skipIfWindows
def test_expression(self):
"""Tests setting data breakpoints on expression."""
@@ -67,8 +108,8 @@ def test_expression(self):
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")
+ response_x = self.dap_server.request_dataBreakpointInfo("&x", 0)
+ response_arr_2 = self.dap_server.request_dataBreakpointInfo("arr+2", 0)
# Test response from dataBreakpointInfo request.
self.assertEqual(response_x["body"]["dataId"].split("/")[1], "4")
self.assertEqual(response_x["body"]["accessTypes"], self.accessTypes)
@@ -107,10 +148,10 @@ def test_functionality(self):
self.continue_to_next_stop()
self.dap_server.get_local_variables()
# Test write watchpoints on x, arr[2]
- response_x = self.dap_server.request_dataBreakpointInfo(1, "x")
+ response_x = self.dap_server.request_dataBreakpointInfo("x", 1)
arr = self.dap_server.get_local_variable("arr")
response_arr_2 = self.dap_server.request_dataBreakpointInfo(
- arr["variablesReference"], "[2]"
+ "[2]", arr["variablesReference"]
)
# Test response from dataBreakpointInfo request.
diff --git a/lldb/test/API/tools/lldb-dap/databreakpoint/main.cpp b/lldb/test/API/tools/lldb-dap/databreakpoint/main.cpp
index bef09c203845e..e4007980d27a8 100644
--- a/lldb/test/API/tools/lldb-dap/databreakpoint/main.cpp
+++ b/lldb/test/API/tools/lldb-dap/databreakpoint/main.cpp
@@ -1,5 +1,6 @@
int main(int argc, char const *argv[]) {
// Test for data breakpoint
+ char var[6] = "HELLO";
int x = 0;
int arr[4] = {1, 2, 3, 4};
for (int i = 0; i < 5; ++i) { // first loop breakpoint
@@ -10,6 +11,8 @@ int main(int argc, char const *argv[]) {
}
}
+ var[1] = 'A';
+
x = 1;
for (int i = 0; i < 10; ++i) { // second loop breakpoint
++x;
diff --git a/lldb/tools/lldb-dap/Handler/DataBreakpointInfoRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/DataBreakpointInfoRequestHandler.cpp
index 8cb25d0603449..2e429c045cdc0 100644
--- a/lldb/tools/lldb-dap/Handler/DataBreakpointInfoRequestHandler.cpp
+++ b/lldb/tools/lldb-dap/Handler/DataBreakpointInfoRequestHandler.cpp
@@ -7,7 +7,6 @@
//===----------------------------------------------------------------------===//
#include "DAP.h"
-#include "EventHelper.h"
#include "Protocol/ProtocolTypes.h"
#include "RequestHandler.h"
#include "lldb/API/SBMemoryRegionInfo.h"
@@ -16,12 +15,61 @@
namespace lldb_dap {
+static llvm::Expected<protocol::DataBreakpointInfoResponseBody>
+HandleDataBreakpointBytes(DAP &dap,
+ const protocol::DataBreakpointInfoArguments &args) {
+ llvm::StringRef address = args.name;
+
+ unsigned long long load_addr = LLDB_INVALID_ADDRESS;
+ if (llvm::getAsUnsignedInteger(address, 0, load_addr)) {
+ return llvm::make_error<DAPError>(llvm::formatv("invalid address"),
+ llvm::inconvertibleErrorCode(), false);
+ }
+
+ lldb::SBAddress sb_addr(load_addr, dap.target);
+ if (!sb_addr.IsValid()) {
+ return llvm::make_error<DAPError>(
+ llvm::formatv("address {:x} does not exist in the debuggee", load_addr),
+ llvm::inconvertibleErrorCode(), false);
+ }
+
+ const uint32_t byte_size =
+ args.bytes.value_or(dap.target.GetAddressByteSize());
+
+ protocol::DataBreakpointInfoResponseBody response;
+ response.dataId = llvm::formatv("{:x-}/{}", load_addr, byte_size);
+
+ lldb::SBMemoryRegionInfo region;
+ lldb::SBError err =
+ dap.target.GetProcess().GetMemoryRegionInfo(load_addr, region);
+ // Only lldb-server supports "qMemoryRegionInfo". So, don't fail this
+ // request if SBProcess::GetMemoryRegionInfo returns error.
+ if (err.Success() && !(region.IsReadable() || region.IsWritable())) {
+ response.description = llvm::formatv(
+ "memory region for address {} has no read or write permissions",
+ load_addr);
+
+ } else {
+ response.description =
+ llvm::formatv("{} bytes at {:x}", byte_size, load_addr);
+ response.accessTypes = {protocol::eDataBreakpointAccessTypeRead,
+ protocol::eDataBreakpointAccessTypeWrite,
+ protocol::eDataBreakpointAccessTypeReadWrite};
+ }
+
+ return response;
+}
+
/// Obtains information on a possible data breakpoint that could be set on an
/// expression or variable. Clients should only call this request if the
/// corresponding capability supportsDataBreakpoints is true.
llvm::Expected<protocol::DataBreakpointInfoResponseBody>
DataBreakpointInfoRequestHandler::Run(
const protocol::DataBreakpointInfoArguments &args) const {
+
+ if (args.asAddress.value_or(false))
+ return HandleDataBreakpointBytes(dap, args);
+
protocol::DataBreakpointInfoResponseBody response;
lldb::SBFrame frame = dap.GetLLDBFrame(args.frameId.value_or(UINT64_MAX));
lldb::SBValue variable = dap.variables.FindVariable(
@@ -40,7 +88,7 @@ DataBreakpointInfoRequestHandler::Run(
is_data_ok = false;
response.description = "variable size is 0";
} else {
- addr = llvm::utohexstr(load_addr);
+ addr = llvm::utohexstr(load_addr, /*lowerCase=*/true);
size = llvm::utostr(byte_size);
}
} else if (args.variablesReference.value_or(0) == 0 && frame.IsValid()) {
@@ -57,7 +105,7 @@ DataBreakpointInfoRequestHandler::Run(
lldb::SBData data = value.GetPointeeData();
if (data.IsValid()) {
size = llvm::utostr(data.GetByteSize());
- addr = llvm::utohexstr(load_addr);
+ addr = llvm::utohexstr(load_addr, /*lowerCase=*/true);
lldb::SBMemoryRegionInfo region;
lldb::SBError err =
dap.target.GetProcess().GetMemoryRegionInfo(load_addr, region);
@@ -86,7 +134,7 @@ DataBreakpointInfoRequestHandler::Run(
response.accessTypes = {protocol::eDataBreakpointAccessTypeRead,
protocol::eDataBreakpointAccessTypeWrite,
protocol::eDataBreakpointAccessTypeReadWrite};
- response.description = size + " bytes at " + addr + " " + args.name;
+ response.description = size + " bytes at 0x" + addr + " " + args.name;
}
return response;
diff --git a/lldb/tools/lldb-dap/Handler/RequestHandler.h b/lldb/tools/lldb-dap/Handler/RequestHandler.h
index 3a965bcc87a5e..dec68683fee65 100644
--- a/lldb/tools/lldb-dap/Handler/RequestHandler.h
+++ b/lldb/tools/lldb-dap/Handler/RequestHandler.h
@@ -420,6 +420,9 @@ class DataBreakpointInfoRequestHandler
public:
using RequestHandler::RequestHandler;
static llvm::StringLiteral GetCommand() { return "dataBreakpointInfo"; }
+ FeatureSet GetSupportedFeatures() const override {
+ return {protocol::eAdapterFeatureDataBreakpointBytes};
+ }
llvm::Expected<protocol::DataBreakpointInfoResponseBody>
Run(const protocol::DataBreakpointInfoArguments &args) const override;
};
diff --git a/lldb/tools/lldb-dap/Protocol/ProtocolRequests.h b/lldb/tools/lldb-dap/Protocol/ProtocolRequests.h
index 7c774e50d6e56..cde441351fc88 100644
--- a/lldb/tools/lldb-dap/Protocol/ProtocolRequests.h
+++ b/lldb/tools/lldb-dap/Protocol/ProtocolRequests.h
@@ -668,7 +668,7 @@ struct DataBreakpointInfoArguments {
/// pause on data access anywhere within that range.
/// Clients may set this property only if the `supportsDataBreakpointBytes`
/// capability is true.
- std::optional<int64_t> bytes;
+ std::optional<uint64_t> bytes;
/// If `true`, the `name` is a memory address and the debugger should
/// interpret it as a decimal value, or hex value if it is prefixed with `0x`.
|
|
✅ With the latest revision this PR passed the Python code formatter. |
ashgti
reviewed
May 23, 2025
lldb/tools/lldb-dap/Handler/DataBreakpointInfoRequestHandler.cpp
Outdated
Show resolved
Hide resolved
lldb/tools/lldb-dap/Handler/DataBreakpointInfoRequestHandler.cpp
Outdated
Show resolved
Hide resolved
lldb/tools/lldb-dap/Handler/DataBreakpointInfoRequestHandler.cpp
Outdated
Show resolved
Hide resolved
| lldb::SBMemoryRegionInfo region; | ||
| lldb::SBError err = | ||
| dap.target.GetProcess().GetMemoryRegionInfo(load_addr, region); | ||
| // Only lldb-server supports "qMemoryRegionInfo". So, don't fail this |
There was a problem hiding this comment.
I think debugserver also implements this
llvm-project/lldb/tools/debugserver/source/RNBRemote.cpp
Lines 4242 to 4329 in b878e0d
There was a problem hiding this comment.
I also wasn't 100% sure would check if the region is memory mapped suffice ?
There was a problem hiding this comment.
This API can return information about regions that are not mapped, you can check that with:
/// Check if this memory address is mapped into the process address
/// space.
///
/// \return
/// true if this memory address is in the process address space.
bool IsMapped();
Though, I would assume an unmapped region would be neither readable nor writeable, so it may not be relevant whether it's mapped or not.
In theory memory can be mapped and not readable, though I don't know how Linux presents it. Execute only memory you can fetch from but not load from. If you were using lldb-dap with an embedded target, it could be a concern.
I don't know if any of that is a concern for this code specifically though.
There was a problem hiding this comment.
The existing comment though is partially correct. Debugserver does implement it too, I've never seen anything GDB derived implement it.
There was a problem hiding this comment.
We could use it since GetMemoryRegionInfo falls back to qXfer-memory-map-read
llvm-project/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
Lines 1707 to 1729 in ba75feb
just have to update it to set readable and writeable.
lldb/tools/lldb-dap/Handler/DataBreakpointInfoRequestHandler.cpp
Outdated
Show resolved
Hide resolved
| } else { | ||
| response.description = | ||
| llvm::formatv("{} bytes at {:x}", byte_size, load_addr); | ||
| response.accessTypes = {protocol::eDataBreakpointAccessTypeRead, |
There was a problem hiding this comment.
Shouldn't we check the regionInfo for setting these access types?
There was a problem hiding this comment.
I did not check since I was not sure if the information from memoryRegion info is accurate.
There was a problem hiding this comment.
Internally we use a yes/no/don't know enum to represent the region properties. The SBI API converts yes to bool true:
bool SBMemoryRegionInfo::IsReadable() {
LLDB_INSTRUMENT_VA(this);
return m_opaque_up->GetReadable() == MemoryRegionInfo::eYes;
}
Maybe that helps?
I lldb couldn't read memory region info and returned you an incomplete object, it would return false for a lot of these properties because it doesn't know either way.
There was a problem hiding this comment.
Looking at the response in the spec this is optional. I think we should only set this if we know the values are correct, but here I think we're just assigning all the possible values. If we don't know if the address is read/write/rw then we should maybe just not set this value.
There was a problem hiding this comment.
Would this also apply to the default databreakpointinfo since since we know that we are able read and write to the address of the scope variables ?
There was a problem hiding this comment.
I think that depends on the variable, right? If its mutable, then it would be rw but some variables are const or if the compiler optimized a variable away, it may not be something we can mutate.
There was a problem hiding this comment.
I think in the SBWatchpoint we can check IsWatchingReads and IsWatchingWrites to check if the watchpoint is r/w/rw.
lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py
Outdated
Show resolved
Hide resolved
lldb/packages/Python/lldbsuite/test/tools/lldb-dap/lldbdap_testcase.py
Outdated
Show resolved
Hide resolved
lldb/packages/Python/lldbsuite/test/tools/lldb-dap/lldbdap_testcase.py
Outdated
Show resolved
Hide resolved
da-viper
force-pushed
the
support-breakpoint-info-bytes
branch
from
fbd5694 to
e9134d0
Compare
eronnen
approved these changes
May 25, 2025
DavidSpickett
changed the title
DavidSpickett
changed the title
[lldb][lldb-dap] add review changes
da-viper
force-pushed
the
support-breakpoint-info-bytes
branch
from
c88cbf2 to
a6f9c53
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This adds the support for
supportsBreakpointInfoBytesto set watchpoint on a address range.