-
Notifications
You must be signed in to change notification settings - Fork 11.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[lldb-dap] Add support for data breakpoint. #81541
Conversation
@llvm/pr-subscribers-lldb Author: Zequan Wu (ZequanWu) ChangesThis implements functionality to handle It doesn't handle the case when This is based on top of #80753. Patch is 62.88 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/81541.diff 20 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 bb863bb8719176..2d48cfbd819366 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
@@ -501,6 +501,17 @@ def get_local_variable_value(self, name, frameIndex=0, threadId=None):
return variable["value"]
return None
+ def get_local_variable_child(self, name, child_name, frameIndex=0, threadId=None):
+ local = self.get_local_variable(name, frameIndex, threadId)
+ if local["variablesReference"] == 0:
+ return None
+ children = self.request_variables(local["variablesReference"])[
+ "body"]["variables"]
+ for child in children:
+ if child["name"] == child_name:
+ return child
+ return None
+
def replay_packets(self, replay_file_path):
f = open(replay_file_path, "r")
mode = "invalid"
@@ -895,6 +906,32 @@ def request_setFunctionBreakpoints(self, names, condition=None, hitCondition=Non
}
return self.send_recv(command_dict)
+ def request_dataBreakpointInfo(self, variablesReference, name):
+ args_dict = {"variablesReference": variablesReference, "name": name}
+ command_dict = {
+ "command": "dataBreakpointInfo",
+ "type": "request",
+ "arguments": args_dict,
+ }
+ return self.send_recv(command_dict)
+
+ def request_setDataBreakpoint(self, dataBreakpoints):
+ """dataBreakpoints is a list of dictionary with following fields:
+ {
+ dataId: (address in hex)/(size in bytes)
+ accessType: read/write/readWrite
+ [condition]: string
+ [hitCondition]: string
+ }
+ """
+ args_dict = {"breakpoints": dataBreakpoints}
+ command_dict = {
+ "command": "setDataBreakpoints",
+ "type": "request",
+ "arguments": args_dict,
+ }
+ return self.send_recv(command_dict)
+
def request_compileUnits(self, moduleId):
args_dict = {"moduleId": moduleId}
command_dict = {
diff --git a/lldb/test/API/tools/lldb-dap/databreakpoint/Makefile b/lldb/test/API/tools/lldb-dap/databreakpoint/Makefile
new file mode 100644
index 00000000000000..99998b20bcb050
--- /dev/null
+++ b/lldb/test/API/tools/lldb-dap/databreakpoint/Makefile
@@ -0,0 +1,3 @@
+CXX_SOURCES := main.cpp
+
+include Makefile.rules
diff --git a/lldb/test/API/tools/lldb-dap/databreakpoint/TestDAP_setDataBreakpoints.py b/lldb/test/API/tools/lldb-dap/databreakpoint/TestDAP_setDataBreakpoints.py
new file mode 100644
index 00000000000000..a13b4399a02f18
--- /dev/null
+++ b/lldb/test/API/tools/lldb-dap/databreakpoint/TestDAP_setDataBreakpoints.py
@@ -0,0 +1,100 @@
+"""
+Test lldb-dap dataBreakpointInfo and setDataBreakpoints requests
+"""
+
+
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+import lldbdap_testcase
+
+
+class TestDAP_setDataBreakpoints(lldbdap_testcase.DAPTestCaseBase):
+ def setUp(self):
+ lldbdap_testcase.DAPTestCaseBase.setUp(self)
+ self.accessTypes = ["read", "write", "readWrite"]
+
+ @skipIfWindows
+ @skipIfRemote
+ def test_functionality(self):
+ """Tests setting data breakpoints.
+ """
+ program = self.getBuildArtifact("a.out")
+ self.build_and_launch(program)
+ source = "main.cpp"
+ first_loop_break_line = line_number(
+ source, "// first loop breakpoint")
+ breakpoint_ids = self.set_source_breakpoints(
+ source, [first_loop_break_line])
+ self.continue_to_breakpoints(breakpoint_ids)
+ self.dap_server.get_local_variables()
+ # Test write watchpoints on x, arr[2]
+ response_x = self.dap_server.request_dataBreakpointInfo(1, "x")
+ arr = self.dap_server.get_local_variable("arr")
+ response_arr_2 = self.dap_server.request_dataBreakpointInfo(
+ arr["variablesReference"], "[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], "1")
+ self.assertEquals(response_arr_2["body"]
+ ["accessTypes"], self.accessTypes)
+ dataBreakpoints = [
+ {
+ "dataId": response_x["body"]["dataId"],
+ "accessType": "write"
+ },
+ {
+ "dataId": response_arr_2["body"]["dataId"],
+ "accessType": "write"
+ }
+ ]
+ self.dap_server.request_setDataBreakpoint(dataBreakpoints)
+
+ self.dap_server.request_continue()
+ self.dap_server.wait_for_stopped()
+ 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.dap_server.request_continue()
+ self.dap_server.wait_for_stopped()
+ 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"], "'z'")
+ self.assertEquals(i_val, "2")
+ self.dap_server.request_setDataBreakpoint([])
+
+ # Test hit condition
+ second_loop_break_line = line_number(
+ source, "// second loop breakpoint")
+ breakpoint_ids = self.set_source_breakpoints(
+ source, [second_loop_break_line])
+ self.continue_to_breakpoints(breakpoint_ids)
+ dataBreakpoints = [
+ {
+ "dataId": response_x["body"]["dataId"],
+ "accessType": "write",
+ "hitCondition": "2"
+ }
+ ]
+ self.dap_server.request_setDataBreakpoint(dataBreakpoints)
+ self.dap_server.request_continue()
+ self.dap_server.wait_for_stopped()
+ x_val = self.dap_server.get_local_variable_value("x")
+ self.assertEquals(x_val, "3")
+
+ # Test condition
+ dataBreakpoints = [
+ {
+ "dataId": response_x["body"]["dataId"],
+ "accessType": "write",
+ "condition": "x==10"
+ }
+ ]
+ self.dap_server.request_setDataBreakpoint(dataBreakpoints)
+ self.dap_server.request_continue()
+ self.dap_server.wait_for_stopped()
+ x_val = self.dap_server.get_local_variable_value("x")
+ self.assertEquals(x_val, "10")
diff --git a/lldb/test/API/tools/lldb-dap/databreakpoint/main.cpp b/lldb/test/API/tools/lldb-dap/databreakpoint/main.cpp
new file mode 100644
index 00000000000000..8082fe02f3e534
--- /dev/null
+++ b/lldb/test/API/tools/lldb-dap/databreakpoint/main.cpp
@@ -0,0 +1,17 @@
+int main(int argc, char const *argv[]) {
+ // Test for data breakpoint
+ int x = 0;
+ char arr[4] = {'a', 'b', 'c', 'd'};
+ for (int i = 0; i < 5; ++i) { // first loop breakpoint
+ if (i == 1) {
+ x = i + 1;
+ } else if (i == 2) {
+ arr[i] = 'z';
+ }
+ }
+
+ x = 1;
+ for (int i = 0; i < 10; ++i) { // second loop breakpoint
+ ++x;
+ }
+}
diff --git a/lldb/tools/lldb-dap/Breakpoint.cpp b/lldb/tools/lldb-dap/Breakpoint.cpp
new file mode 100644
index 00000000000000..0c33d4b114d760
--- /dev/null
+++ b/lldb/tools/lldb-dap/Breakpoint.cpp
@@ -0,0 +1,76 @@
+//===-- Breakpoint.cpp ----------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "Breakpoint.h"
+#include "DAP.h"
+#include "JSONUtils.h"
+#include "llvm/ADT/StringExtras.h"
+
+using namespace lldb_dap;
+
+void Breakpoint::SetCondition() { bp.SetCondition(condition.c_str()); }
+
+void Breakpoint::SetHitCondition() {
+ uint64_t hitCount = 0;
+ if (llvm::to_integer(hitCondition, hitCount))
+ bp.SetIgnoreCount(hitCount - 1);
+}
+
+void Breakpoint::CreateJsonObject(llvm::json::Object &object) {
+ // Each breakpoint location is treated as a separate breakpoint for VS code.
+ // They don't have the notion of a single breakpoint with multiple locations.
+ if (!bp.IsValid())
+ return;
+ object.try_emplace("verified", bp.GetNumResolvedLocations() > 0);
+ object.try_emplace("id", bp.GetID());
+ // VS Code DAP doesn't currently allow one breakpoint to have multiple
+ // locations so we just report the first one. If we report all locations
+ // then the IDE starts showing the wrong line numbers and locations for
+ // other source file and line breakpoints in the same file.
+
+ // Below we search for the first resolved location in a breakpoint and report
+ // this as the breakpoint location since it will have a complete location
+ // that is at least loaded in the current process.
+ lldb::SBBreakpointLocation bp_loc;
+ const auto num_locs = bp.GetNumLocations();
+ for (size_t i = 0; i < num_locs; ++i) {
+ bp_loc = bp.GetLocationAtIndex(i);
+ if (bp_loc.IsResolved())
+ break;
+ }
+ // If not locations are resolved, use the first location.
+ if (!bp_loc.IsResolved())
+ bp_loc = bp.GetLocationAtIndex(0);
+ auto bp_addr = bp_loc.GetAddress();
+
+ if (bp_addr.IsValid()) {
+ std::string formatted_addr =
+ "0x" + llvm::utohexstr(bp_addr.GetLoadAddress(g_dap.target));
+ object.try_emplace("instructionReference", formatted_addr);
+ auto line_entry = bp_addr.GetLineEntry();
+ const auto line = line_entry.GetLine();
+ if (line != UINT32_MAX)
+ object.try_emplace("line", line);
+ const auto column = line_entry.GetColumn();
+ if (column != 0)
+ object.try_emplace("column", column);
+ object.try_emplace("source", CreateSource(line_entry));
+ }
+}
+
+bool Breakpoint::MatchesName(const char *name) { return bp.MatchesName(name); }
+
+void Breakpoint::SetBreakpoint() {
+ // See comments in BreakpointBase::GetBreakpointLabel() for details of why
+ // we add a label to our breakpoints.
+ bp.AddName(GetBreakpointLabel());
+ if (!condition.empty())
+ SetCondition();
+ if (!hitCondition.empty())
+ SetHitCondition();
+}
diff --git a/lldb/tools/lldb-dap/Breakpoint.h b/lldb/tools/lldb-dap/Breakpoint.h
new file mode 100644
index 00000000000000..47a9d9c59ae2b7
--- /dev/null
+++ b/lldb/tools/lldb-dap/Breakpoint.h
@@ -0,0 +1,33 @@
+//===-- Breakpoint.h --------------------------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLDB_TOOLS_LLDB_DAP_BREAKPOINT_H
+#define LLDB_TOOLS_LLDB_DAP_BREAKPOINT_H
+
+#include "BreakpointBase.h"
+
+namespace lldb_dap {
+
+struct Breakpoint : public BreakpointBase {
+ // The LLDB breakpoint associated wit this source breakpoint
+ lldb::SBBreakpoint bp;
+
+ Breakpoint() = default;
+ Breakpoint(const llvm::json::Object &obj) : BreakpointBase(obj){};
+ Breakpoint(lldb::SBBreakpoint bp) : bp(bp) {}
+
+ void SetCondition() override;
+ void SetHitCondition() override;
+ void CreateJsonObject(llvm::json::Object &object) override;
+
+ bool MatchesName(const char *name);
+ void SetBreakpoint();
+};
+} // namespace lldb_dap
+
+#endif
diff --git a/lldb/tools/lldb-dap/BreakpointBase.cpp b/lldb/tools/lldb-dap/BreakpointBase.cpp
index fb4b27fbe315fc..519729f5519ffc 100644
--- a/lldb/tools/lldb-dap/BreakpointBase.cpp
+++ b/lldb/tools/lldb-dap/BreakpointBase.cpp
@@ -8,306 +8,13 @@
#include "BreakpointBase.h"
#include "DAP.h"
-#include "JSONUtils.h"
#include "llvm/ADT/StringExtras.h"
using namespace lldb_dap;
BreakpointBase::BreakpointBase(const llvm::json::Object &obj)
: condition(std::string(GetString(obj, "condition"))),
- hitCondition(std::string(GetString(obj, "hitCondition"))),
- logMessage(std::string(GetString(obj, "logMessage"))) {}
-
-void BreakpointBase::SetCondition() { bp.SetCondition(condition.c_str()); }
-
-void BreakpointBase::SetHitCondition() {
- uint64_t hitCount = 0;
- if (llvm::to_integer(hitCondition, hitCount))
- bp.SetIgnoreCount(hitCount - 1);
-}
-
-lldb::SBError BreakpointBase::AppendLogMessagePart(llvm::StringRef part,
- bool is_expr) {
- if (is_expr) {
- logMessageParts.emplace_back(part, is_expr);
- } else {
- std::string formatted;
- lldb::SBError error = FormatLogText(part, formatted);
- if (error.Fail())
- return error;
- logMessageParts.emplace_back(formatted, is_expr);
- }
- return lldb::SBError();
-}
-
-// TODO: consolidate this code with the implementation in
-// FormatEntity::ParseInternal().
-lldb::SBError BreakpointBase::FormatLogText(llvm::StringRef text,
- std::string &formatted) {
- lldb::SBError error;
- while (!text.empty()) {
- size_t backslash_pos = text.find_first_of('\\');
- if (backslash_pos == std::string::npos) {
- formatted += text.str();
- return error;
- }
-
- formatted += text.substr(0, backslash_pos).str();
- // Skip the characters before and including '\'.
- text = text.drop_front(backslash_pos + 1);
-
- if (text.empty()) {
- error.SetErrorString(
- "'\\' character was not followed by another character");
- return error;
- }
-
- const char desens_char = text[0];
- text = text.drop_front(); // Skip the desensitized char character
- switch (desens_char) {
- case 'a':
- formatted.push_back('\a');
- break;
- case 'b':
- formatted.push_back('\b');
- break;
- case 'f':
- formatted.push_back('\f');
- break;
- case 'n':
- formatted.push_back('\n');
- break;
- case 'r':
- formatted.push_back('\r');
- break;
- case 't':
- formatted.push_back('\t');
- break;
- case 'v':
- formatted.push_back('\v');
- break;
- case '\'':
- formatted.push_back('\'');
- break;
- case '\\':
- formatted.push_back('\\');
- break;
- case '0':
- // 1 to 3 octal chars
- {
- if (text.empty()) {
- error.SetErrorString("missing octal number following '\\0'");
- return error;
- }
-
- // Make a string that can hold onto the initial zero char, up to 3
- // octal digits, and a terminating NULL.
- char oct_str[5] = {0, 0, 0, 0, 0};
-
- size_t i;
- for (i = 0;
- i < text.size() && i < 4 && (text[i] >= '0' && text[i] <= '7');
- ++i) {
- oct_str[i] = text[i];
- }
-
- text = text.drop_front(i);
- unsigned long octal_value = ::strtoul(oct_str, nullptr, 8);
- if (octal_value <= UINT8_MAX) {
- formatted.push_back((char)octal_value);
- } else {
- error.SetErrorString("octal number is larger than a single byte");
- return error;
- }
- }
- break;
-
- case 'x': {
- if (text.empty()) {
- error.SetErrorString("missing hex number following '\\x'");
- return error;
- }
- // hex number in the text
- if (isxdigit(text[0])) {
- // Make a string that can hold onto two hex chars plus a
- // NULL terminator
- char hex_str[3] = {0, 0, 0};
- hex_str[0] = text[0];
-
- text = text.drop_front();
-
- if (!text.empty() && isxdigit(text[0])) {
- hex_str[1] = text[0];
- text = text.drop_front();
- }
-
- unsigned long hex_value = strtoul(hex_str, nullptr, 16);
- if (hex_value <= UINT8_MAX) {
- formatted.push_back((char)hex_value);
- } else {
- error.SetErrorString("hex number is larger than a single byte");
- return error;
- }
- } else {
- formatted.push_back(desens_char);
- }
- break;
- }
-
- default:
- // Just desensitize any other character by just printing what came
- // after the '\'
- formatted.push_back(desens_char);
- break;
- }
- }
- return error;
-}
-
-// logMessage will be divided into array of LogMessagePart as two kinds:
-// 1. raw print text message, and
-// 2. interpolated expression for evaluation which is inside matching curly
-// braces.
-//
-// The function tries to parse logMessage into a list of LogMessageParts
-// for easy later access in BreakpointHitCallback.
-void BreakpointBase::SetLogMessage() {
- logMessageParts.clear();
-
- // Contains unmatched open curly braces indices.
- std::vector<int> unmatched_curly_braces;
-
- // Contains all matched curly braces in logMessage.
- // Loop invariant: matched_curly_braces_ranges are sorted by start index in
- // ascending order without any overlap between them.
- std::vector<std::pair<int, int>> matched_curly_braces_ranges;
-
- lldb::SBError error;
- // Part1 - parse matched_curly_braces_ranges.
- // locating all curly braced expression ranges in logMessage.
- // The algorithm takes care of nested and imbalanced curly braces.
- for (size_t i = 0; i < logMessage.size(); ++i) {
- if (logMessage[i] == '{') {
- unmatched_curly_braces.push_back(i);
- } else if (logMessage[i] == '}') {
- if (unmatched_curly_braces.empty())
- // Nothing to match.
- continue;
-
- int last_unmatched_index = unmatched_curly_braces.back();
- unmatched_curly_braces.pop_back();
-
- // Erase any matched ranges included in the new match.
- while (!matched_curly_braces_ranges.empty()) {
- assert(matched_curly_braces_ranges.back().first !=
- last_unmatched_index &&
- "How can a curley brace be matched twice?");
- if (matched_curly_braces_ranges.back().first < last_unmatched_index)
- break;
-
- // This is a nested range let's earse it.
- assert((size_t)matched_curly_braces_ranges.back().second < i);
- matched_curly_braces_ranges.pop_back();
- }
-
- // Assert invariant.
- assert(matched_curly_braces_ranges.empty() ||
- matched_curly_braces_ranges.back().first < last_unmatched_index);
- matched_curly_braces_ranges.emplace_back(last_unmatched_index, i);
- }
- }
-
- // Part2 - parse raw text and expresions parts.
- // All expression ranges have been parsed in matched_curly_braces_ranges.
- // The code below uses matched_curly_braces_ranges to divide logMessage
- // into raw text parts and expression parts.
- int last_raw_text_start = 0;
- for (const std::pair<int, int> &curly_braces_range :
- matched_curly_braces_ranges) {
- // Raw text before open curly brace.
- assert(curly_braces_range.first >= last_raw_text_start);
- size_t raw_text_len = curly_braces_range.first - last_raw_text_start;
- if (raw_text_len > 0) {
- error = AppendLogMessagePart(
- llvm::StringRef(logMessage.c_str() + last_raw_text_start,
- raw_text_len),
- /*is_expr=*/false);
- if (error.Fail()) {
- NotifyLogMessageError(error.GetCString());
- return;
- }
- }
-
- // Expression between curly braces.
- assert(curly_braces_range.second > curly_braces_range.first);
- size_t expr_len = curly_braces_range.second - curly_braces_range.first - 1;
- error = AppendLogMessagePart(
- llvm::StringRef(logMessage.c_str() + curly_braces_range.first + 1,
- expr_len),
- /*is_expr=*/true);
- if (error.Fail()) {
- NotifyLogMessageError(error.GetCString());
- return;
- }
-
- last_raw_text_start = curly_braces_range.second + 1;
- }
- // Trailing raw text after close curly brace.
- assert(last_raw_text_start >= 0);
- if (logMessage.size() > (size_t)last_raw_text_start) {
- error = AppendLogMessagePart(
- llvm::StringRef(logMessage.c_str() + las...
[truncated]
|
✅ With the latest revision this PR passed the Python code formatter. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great overall!
Could you split this PR into two? One with the refactoring and another one with the new features? That would make reviewing easier
return self.send_recv(command_dict) | ||
|
||
def request_setDataBreakpoint(self, dataBreakpoints): | ||
"""dataBreakpoints is a list of dictionary with following fields: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
list of dictionary
sounds very weird. Did you mean something else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dataBreakpoints
is a list of the dataBreakpoint which has following format:
{
dataId: (address in hex)/(size in bytes)
accessType: read/write/readWrite
[condition]: string
[hitCondition]: string
}
Yes, I already split it out: #80753 is the refactor pr. I'll rebase this one once that is merged. |
just ping me when this PR is rebased |
This implements functionality to handle DataBreakpointInfo request and SetDataBreakpoints request. It doesn't handle the case when name is an expression, see Todo comment for details.
2743af5
to
a2d2869
Compare
Rebased. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing stuff! I've been wanting this for a while. I left some minor comments.
const auto variablesReference = | ||
GetUnsigned(arguments, "variablesReference", 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it'd be better to use a non-zero default value, which might collide with an actual variable ref id
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If variablesReference
is 0, it's treated as not provided as it's only meaningful if its value > 0. This is also how request_variables
and request_setVariable
handle it.
The spec https://microsoft.github.io/debug-adapter-protocol/specification#Types_Variable says: "If variablesReference
is > 0, the variable is structured ...".
- Add support for setting data breakpoint with expression. If `variablesReference` is 0 or not provided, interprete `name` as `${number of bytes}@${expression}` to set data breakpoint at the given expression.
I updated to add support for setting data breakpoint with expression. If |
I think using @ is fine, but we can revisit it later if we see any issues. |
std::string variable_name = CreateUniqueVariableNameForDisplay( | ||
curr_variable, is_duplicated_variable_name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to check the name that might include the "foo @ main.cpp:12" or just look for "foo"? Also, some members of a struct be anonymous unions, we probably want to make sure we can do the right thing with those
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This functions is shared between request_setVariable
and request_dataBreakpointInfo
, so it checks name that contains foo @ main.cpp:12
. I tested this manually.
For anonymous unions inside struct, I also tested manually. If we watch either c
or d
, changes on either value stops the program as expected.
struct A {
int a;
int b;
union {
char c;
int d;
};
};
} | ||
} | ||
if (variable.IsValid()) { | ||
addr = llvm::utohexstr(variable.GetLoadAddress()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need some error checking here. variable.GetLoadAddress()
will only return a valid address if the variable is actually in memory. So if variable.GetLoadAddress()
returns LLDB_INVALID_ADDRESS we need to return an appropriate error like " does not exist in memory, its location is " where is the result of const char *SBValue::GetLocation()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sent a fix at #81680
} | ||
if (variable.IsValid()) { | ||
addr = llvm::utohexstr(variable.GetLoadAddress()); | ||
size = llvm::utostr(variable.GetByteSize()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to check if the byte size is not zero and return an error if it is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sent a fix at #81680
// 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 '@': | ||
// "${size}@${expression}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this format VS Code supported, or just something we are making up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm making this up, since the spec doesn't say how to specify the number of bytes to watch. Do you have better idea?
? std::string(error_cstr) | ||
: "evaluation failed"); | ||
} else | ||
addr = llvm::utohexstr(value.GetValueAsUnsigned()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably want to check this address by making sure it is valid. What if this return zero?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sent a fix at #81680
@@ -52,5 +52,6 @@ executable("lldb-dap") { | |||
"RunInTerminal.cpp", | |||
"SourceBreakpoint.cpp", | |||
"lldb-dap.cpp", | |||
"Watchpoint.cpp" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't update gn build files if you don't use the gn build. There's a bot that is able to do it most of the time, and it doesn't make mistakes like forgetting the trailing comma here :)
We (Fuchsia clang toolchain team) are facing a LLDB test failure in our ARM64 Clang toolchain builders and I suspect this change is causing the failure.
I am building current ToT without this commit to verify if this failure goes away. |
This reverts commit 8c56e78. Reverting to address the LLDB test failure in ARM64.
From logs, looks like setting breakpoint with size being 1 byte failed on arm, and works with size being 4 bytes:
|
This implements functionality to handle
DataBreakpointInfo
request andSetDataBreakpoints
request.If variablesReference is 0 or not provided, interpret name as ${number of bytes}@${expression} to set data breakpoint at the given expression because the spec https://microsoft.github.io/debug-adapter-protocol/specification#Requests_DataBreakpointInfo doesn't say how the client could specify the number of bytes to watch.
This is based on top of #80753.