[lldb-dap] Followup fixs for data breakpoints #81680
Conversation
|
@llvm/pr-subscribers-lldb Author: Zequan Wu (ZequanWu) ChangesFollowup fixes to resolve comments in #81541 Full diff: https://github.com/llvm/llvm-project/pull/81680.diff 1 Files Affected:
diff --git a/lldb/tools/lldb-dap/lldb-dap.cpp b/lldb/tools/lldb-dap/lldb-dap.cpp
index 6bf2ec28432cd3..a3c6109370e685 100644
--- a/lldb/tools/lldb-dap/lldb-dap.cpp
+++ b/lldb/tools/lldb-dap/lldb-dap.cpp
@@ -2741,8 +2741,20 @@ void request_dataBreakpointInfo(const llvm::json::Object &request) {
std::string addr, size;
if (variable.IsValid()) {
- addr = llvm::utohexstr(variable.GetLoadAddress());
- size = llvm::utostr(variable.GetByteSize());
+ lldb::addr_t load_addr = variable.GetLoadAddress();
+ size_t byte_size = variable.GetByteSize();
+ if (load_addr == LLDB_INVALID_ADDRESS) {
+ body.try_emplace("dataId", nullptr);
+ body.try_emplace("description",
+ "does not exist in memory, its location is " +
+ std::string(variable.GetLocation()));
+ } else if (byte_size == 0) {
+ body.try_emplace("dataId", nullptr);
+ body.try_emplace("description", "variable size is 0");
+ } else {
+ addr = llvm::utohexstr(load_addr);
+ size = llvm::utostr(byte_size);
+ }
} else if (variablesReference == 0 && frame.IsValid()) {
// Name might be an expression. In this case we assume that name is composed
// of the number of bytes to watch and expression, separated by '@':
@@ -2757,13 +2769,18 @@ void request_dataBreakpointInfo(const llvm::json::Object &request) {
body.try_emplace("description", error_cstr && error_cstr[0]
? std::string(error_cstr)
: "evaluation failed");
- } else
- addr = llvm::utohexstr(value.GetValueAsUnsigned());
+ } else {
+ uint64_t value_as_unsigned = value.GetValueAsUnsigned();
+ if (value_as_unsigned == 0) {
+ body.try_emplace("dataId", nullptr);
+ body.try_emplace("description",
+ "unable to evaluate expression to an address.");
+ }
+ addr = llvm::utohexstr(value_as_unsigned);
+ }
} else {
- auto state = g_dap.target.GetProcess().GetState();
body.try_emplace("dataId", nullptr);
- body.try_emplace("description",
- "variable not found: " + llvm::utostr(state));
+ body.try_emplace("description", "variable not found");
}
if (!body.getObject("dataId")) {
|
| body.try_emplace("dataId", nullptr); | ||
| body.try_emplace("description", | ||
| "variable not found: " + llvm::utostr(state)); | ||
| body.try_emplace("description", "variable not found"); |
There was a problem hiding this comment.
Do we want more info shown here like the variable name that we tried to lookup?
There was a problem hiding this comment.
Do we have a way to get the complete name?
The name passed by the request is a child member name and variableReference only tells us its parent. For watching a.b.c, we only gets the name c and variableReference value for c's parent which is b.
There was a problem hiding this comment.
In terms of interpreting name as ${number of bytes}@${expression}, it's made-up. And I just noticed that someone already filed an issue about this: microsoft/debug-adapter-protocol#455. Should we revert the change inside the condition: variablesReference == 0 && frame.IsValid()?
| body.try_emplace("dataId", nullptr); | ||
| body.try_emplace("description", | ||
| "variable not found: " + llvm::utostr(state)); | ||
| body.try_emplace("description", "variable not found"); |
There was a problem hiding this comment.
Do we have a way to get the complete name?
The name passed by the request is a child member name and variableReference only tells us its parent. For watching a.b.c, we only gets the name c and variableReference value for c's parent which is b.
This implements functionality to handle DataBreakpointInfo request and SetDataBreakpoints request. Previous commit 8c56e78 was reverted because setting 1 byte watchpoint failed in the new test on ARM64. So, I changed the test to setting 4 byte watchpoint instead, and hope this won't break it again. It also adds the fixes from #81680.
This implements functionality to handle DataBreakpointInfo request and SetDataBreakpoints request. Previous commit llvm@8c56e78 was reverted because setting 1 byte watchpoint failed in the new test on ARM64. So, I changed the test to setting 4 byte watchpoint instead, and hope this won't break it again. It also adds the fixes from llvm#81680.
Followup fixes to resolve comments in #81541