[Lldb-commits] [PATCH] D140630: [lldb-vscode] Add data breakpoint support

[Greg Clayton via Phabricator via lldb-commits]

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