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 ®ion_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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
