[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 = &num_a;" and "int *num_b_ptr = &num_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 &error);
```



================
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 &region_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 (region_info.IsWritable())
      access_types.emplace_back("readWrite");
  }
  if (region_info. IsWritable())
    access_types.emplace_back("write");
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140630