clayborg requested changes to this revision.
clayborg added inline comments.
This revision now requires changes to proceed.
================
Comment at:
lldb/test/API/tools/lldb-vscode/breakpoint_data/TestVSCode_setDataBreakpoints.py:73
+ num_a = array_find(locals, lambda x: x['name'] == 'num_a')
+ self.assertIsNotNone(num_a)
+
----------------
might be better as suggested?
================
Comment at: lldb/tools/lldb-vscode/Watchpoint.cpp:15
+Watchpoint::Watchpoint(lldb::SBValue value, bool enabled, WatchpointType type)
+ : value(value), enabled(enabled), type(type), watchpoint_active(false) {
+ this->sync();
----------------
We should either rename the argument variables so they don't match the member
variable names or if we add the "m_" prefix this all just will work.
================
Comment at: lldb/tools/lldb-vscode/Watchpoint.cpp:16
+ : value(value), enabled(enabled), type(type), watchpoint_active(false) {
+ this->sync();
+}
----------------
remove "this->"
================
Comment at: lldb/tools/lldb-vscode/Watchpoint.cpp:20-21
+void Watchpoint::set_enabled(bool enabled) {
+ this->enabled = enabled;
+ this->sync();
+}
----------------
================
Comment at: lldb/tools/lldb-vscode/Watchpoint.cpp:27
+void Watchpoint::sync() {
+ if (this->watchpoint_active) {
+ g_vsc.target.DeleteWatchpoint(this->watchpoint.GetID());
----------------
Remove all "this->" from this and all changes in this patch.
================
Comment at: lldb/tools/lldb-vscode/Watchpoint.cpp:36-41
+ this->value.Watch(true,
+ this->type == WatchpointType::Read ||
+ this->type == WatchpointType::ReadWrite,
+ this->type == WatchpointType::Write ||
+ this->type == WatchpointType::ReadWrite,
+ this->error);
----------------
If we use two bools instead of using WatchPointType enum, this is much easier
to code.
================
Comment at: lldb/tools/lldb-vscode/Watchpoint.cpp:46-48
+ std::string message = "Failed setting watchpoint: ";
+ message.append(this->error.GetCString());
+ g_vsc.SendOutput(OutputType::Console, message);
----------------
We should probably just leave the error inside of this object and then test if
the watchpoint was set correctly and report it back in the response to
"setDataBreakpoints" for each breakpoint instead of printing to console?
================
Comment at: lldb/tools/lldb-vscode/Watchpoint.h:20
+
+enum class WatchpointType { Read, Write, ReadWrite };
+
----------------
We could get rid of this enum and just store two bools and avoid the ReadWrite
case. See comment below where WatchpointType is used for member variable
================
Comment at: lldb/tools/lldb-vscode/Watchpoint.h:29-34
+ lldb::SBValue value;
+ bool enabled;
+ WatchpointType type;
+ bool watchpoint_active;
+ lldb::SBWatchpoint watchpoint;
+ lldb::SBError error;
----------------
For classes we prefer things to have a "m_" prefix for all member variables. I
know other parts of this code for breakpoints use "struct" instead. It helps
keep code readable and avoids having variables and member variables with the
same name. See next inline comment.
================
Comment at: lldb/tools/lldb-vscode/Watchpoint.h:31
+ bool enabled;
+ WatchpointType type;
+ bool watchpoint_active;
----------------
Maybe just use two bools and get rid of WatchPointType enum since we really
want a bitfield for read and write?
================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:2125-2138
+static WatchpointType get_watchpoint_type_from_request(std::string type) {
+ if (type == "write") {
+ return WatchpointType::Write;
+ } else if (type == "read") {
+ return WatchpointType::Read;
+ } else if (type == "readWrite") {
+ return WatchpointType::ReadWrite;
----------------
We shouldn't be logging to the debug console if the setDataBreakpoint packet is
not valid, but we should return an error for that packet. Might be better to
have this return a bool to indicate success or failure and have the prototype
be:
```
static bool get_watchpoint_type(const std::string &type, bool &read, bool
&write);
```
If this caller gets false returned from this, then we return an error to the
packet with an appropriate error message.
================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:2188
+
+static std::optional<std::string> set_data_breakpoint(const llvm::json::Object
*breakpoint) {
+ std::string breakpoint_id = GetString(breakpoint, "id").str();
----------------
This function should probably be able to return an error for each watchpoint if
anything fails so this can be reported back as a description or error response
in the "setDataBreakpoints" response? We shouldn't be printing to the console
for errors in the "breakpoint" object arguments or if the watchpoint actually
fails to resolve (was in a register). An error message should be returned
somehow if this is possible?
================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:2192-2193
+ bool enabled = GetBoolean(breakpoint, "enabled", false);
+ WatchpointType watchpoint_type = get_watchpoint_type_from_request(
+ GetString(breakpoint, "accessType").str());
+
----------------
We should return an error for this breakpoint if the watchpoint type is not
supported. Is there a way to return an error for a specific data breakpoint
instead of us printing to the console?
================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:2317-2318
+ auto id = set_data_breakpoint(breakpoint.getAsObject());
+ if (id.has_value())
+ breakpoint_ids.emplace(id.value());
+ }
----------------
You have to return a valid "Breakpoint" object for each item in "breakpoints".
If we fail, it looks like you can put any error into the "Breakpoint.message"
field and set "Breakpoint.verified = false".
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D140630/new/
https://reviews.llvm.org/D140630
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
