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

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

Sorry this fell off my radar. I was on medical leave for a surgery for 3 
months. Back now




Comment at: lldb/tools/lldb-vscode/Watchpoint.cpp:72
+  m_verified = true;
+  m_error = "";
+}





Comment at: lldb/tools/lldb-vscode/Watchpoint.cpp:90
+  m_verified = false;
+  m_error = "";
+}





Comment at: lldb/tools/lldb-vscode/Watchpoint.h:1
+//===-- Watchpoint.h --*- C++ -*-===//
+//

Fix width of this comment to match line 7



Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:2449
+  << " with size " << std::dec << v_size;
+  populate_data_breakpoint_info(response, data.str(), description.str(), 
error.Success() && region_info.IsReadable(), error.Success() && 
region_info.IsWritable());
+  g_vsc.SendJSON(llvm::json::Value(std::move(response)));

Any line over 80 chars should be formatted to not exceed. Best to try and. use 
clang-format


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140630/new/

https://reviews.llvm.org/D140630

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[walter erquinigo via Phabricator via lldb-commits]

wallace added a comment.

Greg hasn't been working for a while. You can go ahead and land this, and when 
he gets back he can share some feedback.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140630/new/

https://reviews.llvm.org/D140630

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Callum Macmillan via Phabricator via lldb-commits]

cimacmillan added a comment.

Ping

@clayborg would you mind having another look? Thanks


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140630/new/

https://reviews.llvm.org/D140630

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[walter erquinigo via Phabricator via lldb-commits]

wallace accepted this revision.
wallace added a comment.

This seems reasonable to me and I'm accepting it to unblock this feature. When 
Greg gets back he should be able to provide some additional comments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140630/new/

https://reviews.llvm.org/D140630

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Callum Macmillan via Phabricator via lldb-commits]

cimacmillan added a comment.

Ping.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140630/new/

https://reviews.llvm.org/D140630

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

In D140630#4276855 , @cimacmillan 
wrote:

> Ping.

Greg's been out since late March, and isn't expected back for a while still.  I 
am entirely unfamiliar with the lldb-vscode part of lldb, so I don't feel 
comfortable reviewing this.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140630/new/

https://reviews.llvm.org/D140630

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Callum Macmillan via Phabricator via lldb-commits]

cimacmillan added a comment.

Ping.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140630/new/

https://reviews.llvm.org/D140630

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Callum Macmillan via Phabricator via lldb-commits]

cimacmillan added a comment.

Ping.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140630/new/

https://reviews.llvm.org/D140630

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

Sorry for delay, I have been on vacation for the last two weeks. I will be back 
next Monday and get to this soon. Feel free to ping again next week if I don't 
get to it!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140630/new/

https://reviews.llvm.org/D140630

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Callum Macmillan via Phabricator via lldb-commits]

cimacmillan added a comment.

Ping.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140630/new/

https://reviews.llvm.org/D140630

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Callum Macmillan via Phabricator via lldb-commits]

cimacmillan marked 12 inline comments as done.
cimacmillan added a comment.

@clayborg Thanks for your feedback. I've refactored the Watchpoint class 
slightly so that it encapsulates the parsing of the setDataBreakpoints request 
and be used to get the Breakpoint info to attach to the response. This is more 
similar to how the other breakpoint classes are written, and encapsulates 
things a bit better.




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)
+

clayborg wrote:
> might be better as suggested?
There isn't an IsValid function in this case as it's not the SBValue type. I 
suppose is the variable was invalid, the rest of the test would fail.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140630/new/

https://reviews.llvm.org/D140630

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Callum Macmillan via Phabricator via lldb-commits]

cimacmillan updated this revision to Diff 493936.
cimacmillan added a comment.

Refactored Watchpoint class and addressed comments


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140630/new/

https://reviews.llvm.org/D140630

Files:
  lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/lldbvscode_testcase.py
  lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/vscode.py
  lldb/test/API/tools/lldb-vscode/breakpoint_data/Makefile
  
lldb/test/API/tools/lldb-vscode/breakpoint_data/TestVSCode_setDataBreakpoints.py
  lldb/test/API/tools/lldb-vscode/breakpoint_data/main.cpp
  lldb/test/API/tools/lldb-vscode/breakpoint_data_optimized/Makefile
  
lldb/test/API/tools/lldb-vscode/breakpoint_data_optimized/TestVSCode_setDataBreakpoints.py
  lldb/test/API/tools/lldb-vscode/breakpoint_data_optimized/main.cpp
  lldb/tools/lldb-vscode/CMakeLists.txt
  lldb/tools/lldb-vscode/VSCode.h
  lldb/tools/lldb-vscode/Watchpoint.cpp
  lldb/tools/lldb-vscode/Watchpoint.h
  lldb/tools/lldb-vscode/lldb-vscode.cpp

Index: lldb/tools/lldb-vscode/lldb-vscode.cpp
===
--- lldb/tools/lldb-vscode/lldb-vscode.cpp
+++ lldb/tools/lldb-vscode/lldb-vscode.cpp
@@ -41,6 +41,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 
 #include "llvm/ADT/ArrayRef.h"
@@ -56,6 +57,8 @@
 #include "llvm/Support/PrettyStackTrace.h"
 #include "llvm/Support/raw_ostream.h"
 
+#include "lldb/API/SBMemoryRegionInfo.h"
+
 #include "JSONUtils.h"
 #include "LLDBUtils.h"
 #include "OutputRedirector.h"
@@ -1541,6 +1544,8 @@
   body.try_emplace("supportsProgressReporting", true);
   // The debug adapter supports 'logMessage' in breakpoint.
   body.try_emplace("supportsLogPoints", true);
+  // The debug adapter supports data breakpoints
+  body.try_emplace("supportsDataBreakpoints", true);
 
   response.try_emplace("body", std::move(body));
   g_vsc.SendJSON(llvm::json::Value(std::move(response)));
@@ -2117,6 +2122,334 @@
   g_vsc.SendJSON(llvm::json::Value(std::move(response)));
 }
 
+static lldb::SBValue get_variable(std::string variable_name,
+  uint32_t variables_reference) {
+  bool is_duplicated_variable_name =
+  variable_name.find(" @") != llvm::StringRef::npos;
+  lldb::SBValue variable;
+
+  if (lldb::SBValueList *top_scope = GetTopLevelScope(variables_reference)) {
+// variablesReference is one of our scopes, not an actual variable it is
+// asking for a variable in locals or globals or registers
+int64_t end_idx = top_scope->GetSize();
+// Searching backward so that we choose the variable in closest scope
+// among variables of the same name.
+for (int64_t i = end_idx - 1; i >= 0; --i) {
+  lldb::SBValue curr_variable = top_scope->GetValueAtIndex(i);
+  std::string local_variable = CreateUniqueVariableNameForDisplay(
+  curr_variable, is_duplicated_variable_name);
+  if (variable_name == local_variable) {
+variable = curr_variable;
+break;
+  }
+}
+  } else {
+// This is not under the globals or locals scope, so there are no duplicated
+// names.
+
+// We have a named item within an actual variable so we need to find it
+// withing the container variable by name.
+lldb::SBValue container = g_vsc.variables.GetVariable(variables_reference);
+if (!container.IsValid()) {
+  return variable;
+}
+
+variable = container.GetChildMemberWithName(variable_name.data());
+if (!variable.IsValid()) {
+  if (variable_name.size() > 0 && variable_name[0] == '[') {
+llvm::StringRef index_str(std::move(variable_name.substr(1)));
+uint64_t index = 0;
+if (!index_str.consumeInteger(0, index)) {
+  if (index_str == "]")
+variable = container.GetChildAtIndex(index);
+}
+  }
+}
+  }
+
+  return variable;
+}
+
+// "SetDataBreakpointsRequest": {
+//   "allOf": [ { "$ref": "#/definitions/Request" }, {
+// "type": "object",
+// "description": "Replaces all existing data breakpoints with new data
+// breakpoints.\nTo clear all data breakpoints, specify an empty
+// array.\nWhen a data breakpoint is hit, a `stopped` event (with reason
+// `data breakpoint`) is generated.\nClients should only call this request
+// if the corresponding capability `supportsDataBreakpoints` is true.",
+// "properties": {
+//   "command": {
+// "type": "string",
+// "enum": [ "setDataBreakpoints" ]
+//   },
+//   "arguments": {
+// "$ref": "#/definitions/SetDataBreakpointsArguments"
+//   }
+// },
+// "required": [ "command", "arguments"  ]
+//   }]
+// },
+// "SetDataBreakpointsArguments": {
+//   "type": "object",
+//   "description": "Arguments for `setDataBreakpoints` request.",
+//   "properties": {
+// "breakpoints": {
+//   "type": "array",
+//   "items": {
+// "$ref": 

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

[Callum Macmillan via Phabricator via lldb-commits]

cimacmillan marked 6 inline comments as done.
cimacmillan added inline comments.



Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:2188
+
+static std::optional set_data_breakpoint(const llvm::json::Object 
*breakpoint) {
+  std::string breakpoint_id = GetString(breakpoint, "id").str();

clayborg wrote:
> 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?
Thanks for your feedback, working through comments now. About this and 

> Is there a way to return an error for a specific data breakpoint instead of 
> us printing to the console?

We can return whether the breakpoint is verified, which displays a greyed dot 
similar to when a line breakpoint can't be set. There doesn't seem to be a way 
to display the error in VS code with the message. I think the logging would be 
useful in that case, but I can also include it in the message response in case 
this is added to VS code.



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140630/new/

https://reviews.llvm.org/D140630

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[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 , bool , bool 
);
```
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 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 

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

[Callum Macmillan via Phabricator via lldb-commits]

cimacmillan updated this revision to Diff 490077.
cimacmillan edited the summary of this revision.
cimacmillan added a comment.

Add handling for const and register cases. Setting watchpoint on SBValue rather 
than the 
address.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140630/new/

https://reviews.llvm.org/D140630

Files:
  lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/lldbvscode_testcase.py
  lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/vscode.py
  lldb/test/API/tools/lldb-vscode/breakpoint_data/Makefile
  
lldb/test/API/tools/lldb-vscode/breakpoint_data/TestVSCode_setDataBreakpoints.py
  lldb/test/API/tools/lldb-vscode/breakpoint_data/main.cpp
  lldb/test/API/tools/lldb-vscode/breakpoint_data_optimized/Makefile
  
lldb/test/API/tools/lldb-vscode/breakpoint_data_optimized/TestVSCode_setDataBreakpoints.py
  lldb/test/API/tools/lldb-vscode/breakpoint_data_optimized/main.cpp
  lldb/tools/lldb-vscode/CMakeLists.txt
  lldb/tools/lldb-vscode/VSCode.h
  lldb/tools/lldb-vscode/Watchpoint.cpp
  lldb/tools/lldb-vscode/Watchpoint.h
  lldb/tools/lldb-vscode/lldb-vscode.cpp

Index: lldb/tools/lldb-vscode/lldb-vscode.cpp
===
--- lldb/tools/lldb-vscode/lldb-vscode.cpp
+++ lldb/tools/lldb-vscode/lldb-vscode.cpp
@@ -41,6 +41,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 
 #include "llvm/ADT/ArrayRef.h"
@@ -56,6 +57,8 @@
 #include "llvm/Support/PrettyStackTrace.h"
 #include "llvm/Support/raw_ostream.h"
 
+#include "lldb/API/SBMemoryRegionInfo.h"
+
 #include "JSONUtils.h"
 #include "LLDBUtils.h"
 #include "OutputRedirector.h"
@@ -1541,6 +1544,8 @@
   body.try_emplace("supportsProgressReporting", true);
   // The debug adapter supports 'logMessage' in breakpoint.
   body.try_emplace("supportsLogPoints", true);
+  // The debug adapter supports data breakpoints
+  body.try_emplace("supportsDataBreakpoints", true);
 
   response.try_emplace("body", std::move(body));
   g_vsc.SendJSON(llvm::json::Value(std::move(response)));
@@ -2117,6 +2122,365 @@
   g_vsc.SendJSON(llvm::json::Value(std::move(response)));
 }
 
+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;
+  }
+  std::string unknown_type_error = "Unknown watchpoint type: ";
+  unknown_type_error.append(type);
+  unknown_type_error.append(". Defaulting to ReadWrite.");
+  g_vsc.SendOutput(OutputType::Console, unknown_type_error);
+  return WatchpointType::ReadWrite;
+}
+
+static lldb::SBValue get_variable(std::string variable_name,
+  uint32_t variables_reference) {
+  bool is_duplicated_variable_name =
+  variable_name.find(" @") != llvm::StringRef::npos;
+  lldb::SBValue variable;
+
+  if (lldb::SBValueList *top_scope = GetTopLevelScope(variables_reference)) {
+// variablesReference is one of our scopes, not an actual variable it is
+// asking for a variable in locals or globals or registers
+int64_t end_idx = top_scope->GetSize();
+// Searching backward so that we choose the variable in closest scope
+// among variables of the same name.
+for (int64_t i = end_idx - 1; i >= 0; --i) {
+  lldb::SBValue curr_variable = top_scope->GetValueAtIndex(i);
+  std::string local_variable = CreateUniqueVariableNameForDisplay(
+  curr_variable, is_duplicated_variable_name);
+  if (variable_name == local_variable) {
+variable = curr_variable;
+break;
+  }
+}
+  } else {
+// This is not under the globals or locals scope, so there are no duplicated
+// names.
+
+// We have a named item within an actual variable so we need to find it
+// withing the container variable by name.
+lldb::SBValue container = g_vsc.variables.GetVariable(variables_reference);
+if (!container.IsValid()) {
+  return variable;
+}
+
+variable = container.GetChildMemberWithName(variable_name.data());
+if (!variable.IsValid()) {
+  if (variable_name.size() > 0 && variable_name[0] == '[') {
+llvm::StringRef index_str(std::move(variable_name.substr(1)));
+uint64_t index = 0;
+if (!index_str.consumeInteger(0, index)) {
+  if (index_str == "]")
+variable = container.GetChildAtIndex(index);
+}
+  }
+}
+  }
+
+  return variable;
+}
+
+static std::optional set_data_breakpoint(const llvm::json::Object *breakpoint) {
+  std::string breakpoint_id = GetString(breakpoint, "id").str();
+  std::string data_id = GetString(breakpoint, "dataId").str();
+  bool enabled = GetBoolean(breakpoint, "enabled", false);
+  WatchpointType watchpoint_type = get_watchpoint_type_from_request(
+  GetString(breakpoint, "accessType").str());
+
+  if 

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

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

In D140630#4056641 , @cimacmillan 
wrote:

> @clayborg Thanks for your feedback. I'm part the way through implementing 
> your changes. Specifically about this point:
>
>> I seem to remember that it will disable this watchpoint as soon as a local 
>> variable goes out of scope, though I might not be remember things correctly
>
> This is a behaviour I'd like to address, where for instance watchpoints are 
> triggered in different functions because the stack frame addresses align. I 
> have this example, I can add a test for:
>
>   int test_a() {
>   int x = 10; <- watchpoint set here
>   x = 20; <- watchpoint triggered here
>   }
>   
>   int test_b() {
>   int b = 10; <- watchpoint triggered here also
>   b = 20;
>   }
>   
>   int main() {
>   test_a();
>   test_b();
>
> I've refactored the Watchpoint class to use "SBValue::Watch", and I can still 
> reproduce this behaviour. I also can't find where this watchpoint disable on 
> scope change might be implemented. Do you have any suggestions for this? 
> Thanks

Upon further examination it doesn't seem like this is happening, though it 
should be possible to add some things to the watchpoint classes to make this 
happen. This is beyond the scope of this fix though, so no need to do it unless 
you have time. What we would need to do is store a StackID (see 
"lldb/Target/StackID.h") and thread ID with the watchpoint. When the watchpoint 
triggers, we would need to check if the StackID + thread ID is valid (we should 
store this info with the watchpoint when setting it from a SBValue::Watch(...), 
or not if we just watch the address) still exists in the thread's stack 
somewhere. If it does still exist then stop and report, else clear the 
watchpoint and don't report anything. If the stack ID isn't valid then we 
always stop.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140630/new/

https://reviews.llvm.org/D140630

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Callum Macmillan via Phabricator via lldb-commits]

cimacmillan added a comment.

@clayborg Thanks for your feedback. I'm part the way through implementing your 
changes. Specifically about this point:

> I seem to remember that it will disable this watchpoint as soon as a local 
> variable goes out of scope, though I might not be remember things correctly

This is a behaviour I'd like to address, where for instance watchpoints are 
triggered in different functions because the stack frame addresses align. I 
have this example, I can add a test for:

  int test_a() {
  int x = 10; <- watchpoint set here
  x = 20; <- watchpoint triggered here
  }
  
  int test_b() {
  int b = 10; <- watchpoint triggered here also
  b = 20;
  }
  
  int main() {
  test_a();
  test_b();

I've refactored the Watchpoint class to use "SBValue::Watch", and I can still 
reproduce this behaviour. I also can't find where this watchpoint disable on 
scope change might be implemented. Do you have any suggestions for this? Thanks


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140630/new/

https://reviews.llvm.org/D140630

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[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:74-81
+num_a = array_find(locals, lambda x: x['name'] == 'num_a')
+self.assertIsNotNone(num_a)
+
+# Get and verify the data breakpoint info
+data_breakpoint_info = self.vscode.request_dataBreakpointInfo(
+num_a['name'], 1
+)

This might fail on some systems if they put the value for the local variables 
"num_a" or "num_b" in a register. So you might need to modify this test to make 
sure it will run on any systems. The other thing you can do is to take the 
address of any local variable to ensure it is kept in memory, so adding "int 
*num_a_ptr = _a;" and "int *num_b_ptr = _b;" might help ensure this 
test always works.



Comment at: 
lldb/test/API/tools/lldb-vscode/breakpoint_data/TestVSCode_setDataBreakpoints.py:95
+expected_line = line_number('main.cpp', '// num_a second')
+self.verify_watchpoint_hit(expected_line)

I would suggest adding a test for a constant global value that appears in a 
section (like .rodata) that has no write permissions and verify that we 
correctly figure out that we can only do "read" access on such a variable (see 
code changes I suggest where we check the memory region).

We also need to test error conditions out when a variable is not in memory. If 
you compile some C code with optimizations, you can try to create a variable 
that is in a register with "int num_a = 1;" and in the test grab the SBValue 
for this local variable and test if the value has an invalid load address



Comment at: lldb/tools/lldb-vscode/Watchpoint.cpp:34-36
+  if (!this->enabled) {
+return;
+  }

no braces on single line if statements per llvm coding guidelines.



Comment at: lldb/tools/lldb-vscode/Watchpoint.cpp:38-44
+  this->watchpoint =
+  g_vsc.target.WatchAddress(this->addr, this->size,
+this->type == WatchpointType::Read ||
+this->type == WatchpointType::ReadWrite,
+this->type == WatchpointType::Write ||
+this->type == WatchpointType::ReadWrite,
+this->error);

We should be using the "SBValue::Watch(...)" instead of 
SBTarget.WatchAddress(...). If you create a watchpoint for a local variable 
that is on the stack, this will cause all sorts of problems if the variable 
goes out of scope. SBValue::Watch(...) has the smarts to be able to figure out 
if a variable is local, and I seem to remember that it will disable this 
watchpoint as soon as a local variable goes out of scope, though I might not be 
remember things correctly. It can also figure out if a variable is currently in 
memory or not and it won't set a watchpoint if possible. 

I might recommend that this Watchpoint class gets created with a SBValue, and 
then we use that for setting the watchpoint, the we set the watchpoint using:
```
lldb::SBWatchpoint lldb::SBValue::Watch(bool resolve_location, bool read, bool 
write, SBError );
```




Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:2429
+
+  auto v_address = variable.GetLoadAddress();
+  auto v_size = variable.GetByteSize();

Some variables don't have load addresses. This can happen when a variable is in 
a register. If a variable is in a register, then this function will return 
LLDB_INVALID_ADDRESS, and if that happens you will need to return something 
appropriate, like maybe returning "null" for the "dataId" in the response and 
then filling in an appropriate message in the "description" as it is documented 
above as "UI string that describes on what data the breakpoint is set on or why 
a data breakpoint is not available". 



Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:2439-2442
+  llvm::json::Array access_types;
+  access_types.emplace_back("read");
+  access_types.emplace_back("write");
+  access_types.emplace_back("readWrite");

if "v_address != LLDB_INVALID_ADDRESS", then you can ask for the memory region 
for this address using:
```
lldb::SBError lldb::SBProcess::GetMemoryRegionInfo(lldb::addr_t load_addr, 
lldb::SBMemoryRegionInfo _info);
```
If you get an error back, then this memory address is unreadable and you should 
return an error. If no error is returned, then you should check if the memory 
region has read or write permissions with:
```
llvm::json::Array access_types;
lldb::SBMemoryRegionInfo region_info;
lldb::SBError error = g_vsc.target.GetProcess().GetMemoryRegionInfo(v_address, 
region_info);
if (error.Success()) {
  if (region_info.IsReadable()) {
access_types.emplace_back("read");
if 

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

[Callum Macmillan via Phabricator via lldb-commits]

cimacmillan added a comment.

Ping.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140630/new/

https://reviews.llvm.org/D140630

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Callum Macmillan via Phabricator via lldb-commits]

cimacmillan added a comment.

Ping! :)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140630/new/

https://reviews.llvm.org/D140630

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Callum Macmillan via Phabricator via lldb-commits]

cimacmillan created this revision.
Herald added a project: All.
cimacmillan requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D140630

Files:
  lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/lldbvscode_testcase.py
  lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/vscode.py
  lldb/test/API/tools/lldb-vscode/breakpoint_data/Makefile
  
lldb/test/API/tools/lldb-vscode/breakpoint_data/TestVSCode_setDataBreakpoints.py
  lldb/test/API/tools/lldb-vscode/breakpoint_data/main.cpp
  lldb/tools/lldb-vscode/CMakeLists.txt
  lldb/tools/lldb-vscode/VSCode.h
  lldb/tools/lldb-vscode/Watchpoint.cpp
  lldb/tools/lldb-vscode/Watchpoint.h
  lldb/tools/lldb-vscode/lldb-vscode.cpp

Index: lldb/tools/lldb-vscode/lldb-vscode.cpp
===
--- lldb/tools/lldb-vscode/lldb-vscode.cpp
+++ lldb/tools/lldb-vscode/lldb-vscode.cpp
@@ -41,6 +41,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 
 #include "llvm/ADT/ArrayRef.h"
@@ -1541,6 +1542,8 @@
   body.try_emplace("supportsProgressReporting", true);
   // The debug adapter supports 'logMessage' in breakpoint.
   body.try_emplace("supportsLogPoints", true);
+  // The debug adapter supports data breakpoints
+  body.try_emplace("supportsDataBreakpoints", true);
 
   response.try_emplace("body", std::move(body));
   g_vsc.SendJSON(llvm::json::Value(std::move(response)));
@@ -2117,6 +2120,337 @@
   g_vsc.SendJSON(llvm::json::Value(std::move(response)));
 }
 
+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;
+  }
+  std::string unknown_type_error = "Unknown watchpoint type: ";
+  unknown_type_error.append(type);
+  unknown_type_error.append(". Defaulting to ReadWrite.");
+  g_vsc.SendOutput(OutputType::Console, unknown_type_error);
+  return WatchpointType::ReadWrite;
+}
+
+static std::string set_data_breakpoint(const llvm::json::Object *breakpoint) {
+  std::string id = GetString(breakpoint, "id").str();
+  const char *data_c_str = GetString(breakpoint, "dataId").data();
+  bool enabled = GetBoolean(breakpoint, "enabled", false);
+  WatchpointType watchpoint_type = get_watchpoint_type_from_request(
+  GetString(breakpoint, "accessType").str());
+  uint64_t v_address;
+  size_t v_size;
+  sscanf(data_c_str, "%llu/%zu", _address, _size);
+
+  if (g_vsc.watchpoints.find(id) != g_vsc.watchpoints.end()) {
+auto existing_watchpoint = g_vsc.watchpoints.at(id);
+if (existing_watchpoint.is_enabled() != enabled)
+  existing_watchpoint.set_enabled(enabled);
+return id;
+  }
+
+  g_vsc.watchpoints.emplace(
+  id, Watchpoint(v_address, v_size, enabled, watchpoint_type));
+  return id;
+}
+
+// "SetDataBreakpointsRequest": {
+//   "allOf": [ { "$ref": "#/definitions/Request" }, {
+// "type": "object",
+// "description": "Replaces all existing data breakpoints with new data
+// breakpoints.\nTo clear all data breakpoints, specify an empty
+// array.\nWhen a data breakpoint is hit, a `stopped` event (with reason
+// `data breakpoint`) is generated.\nClients should only call this request
+// if the corresponding capability `supportsDataBreakpoints` is true.",
+// "properties": {
+//   "command": {
+// "type": "string",
+// "enum": [ "setDataBreakpoints" ]
+//   },
+//   "arguments": {
+// "$ref": "#/definitions/SetDataBreakpointsArguments"
+//   }
+// },
+// "required": [ "command", "arguments"  ]
+//   }]
+// },
+// "SetDataBreakpointsArguments": {
+//   "type": "object",
+//   "description": "Arguments for `setDataBreakpoints` request.",
+//   "properties": {
+// "breakpoints": {
+//   "type": "array",
+//   "items": {
+// "$ref": "#/definitions/DataBreakpoint"
+//   },
+//   "description": "The contents of this array replaces all existing data
+//   breakpoints. An empty array clears all data breakpoints."
+// }
+//   },
+//   "required": [ "breakpoints" ]
+// },
+// "SetDataBreakpointsResponse": {
+//   "allOf": [ { "$ref": "#/definitions/Response" }, {
+// "type": "object",
+// "description": "Response to `setDataBreakpoints` request.\nReturned is
+// information about each breakpoint created by this request.",
+// "properties": {
+//   "body": {
+// "type": "object",
+// "properties": {
+//   "breakpoints": {
+// "type": "array",
+// "items": {
+//   "$ref": "#/definitions/Breakpoint"
+// },
+// "description": "Information about the data breakpoints. The array
+// elements correspond to the elements of the input argument
+//