zturner added inline comments.
================
Comment at:
source/Plugins/Process/Linux/NativeRegisterContextLinux_ppc64le.cpp:90-93
+ // 16 is just a maximum value, query hardware for actual watchpoint count
+ m_max_hwp_supported = 16;
+ m_max_hbp_supported = 16;
+ m_refresh_hwdebug_info = true;
----------------
Can you set these inline in the class body? e.g. when you declare them in the
header file:
```
uint32_t m_max_hwp_supported = 16;
uint32_t m_max_hbp_supported = 16;
bool m_refresh_hwdebug_info = true;
```
================
Comment at:
source/Plugins/Process/Linux/NativeRegisterContextLinux_ppc64le.cpp:283-295
+ case 1:
+ rw_mode = PPC_BREAKPOINT_TRIGGER_WRITE;
+ watch_flags = 2;
+ break;
+ case 2:
+ rw_mode = PPC_BREAKPOINT_TRIGGER_READ;
+ watch_flags = 1;
----------------
Can you make an enum for these magic values `1`, `2`, and `3`?
================
Comment at:
source/Plugins/Process/Linux/NativeRegisterContextLinux_ppc64le.cpp:302-318
+ // Check 8-byte alignment for hardware watchpoint target address.
+ // Below is a hack to recalculate address and size in order to
+ // make sure we can watch non 8-byte alligned addresses as well.
+ if (addr & 0x07) {
+ uint8_t watch_mask = (addr & 0x07) + size;
+
+ if (watch_mask > 0x08)
----------------
How about:
```
addr_t begin = llvm::alignDown(addr, 8);
addr_t end = llvm::alignUp(addr+size, 8);
size = llvm::NextPowerOf2(end-begin);
```
================
Comment at:
source/Plugins/Process/Linux/NativeRegisterContextLinux_ppc64le.cpp:327-333
+ for (uint32_t i = 0; i < m_max_hwp_supported; i++) {
+ if ((m_hwp_regs[i].control & 1) == 0) {
+ wp_index = i; // Mark last free slot
+ } else if (m_hwp_regs[i].address == addr) {
+ return LLDB_INVALID_INDEX32; // We do not support duplicate watchpoints.
+ }
+ }
----------------
Can we use a range-based for here?
```
DREG *reg = nullptr;
for (auto &dr : m_hwp_regs) {
if (dr.control & 1) == 0)
reg = &dr;
else if (dr.address == addr)
return LLDB_INVALID_INDEX32;
}
if (!reg)
return LLDB_INVALID_INDEX32;
reg->real_addr = real_addr;
reg->address = addr;
reg->control = control_value;
reg->mode = rw_mode;
```
etc
================
Comment at:
source/Plugins/Process/Linux/NativeRegisterContextLinux_ppc64le.cpp:349
+ m_hwp_regs[wp_index].address = 0;
+ m_hwp_regs[wp_index].control &= ~1;
+
----------------
How about:
```
m_hwp_regs[wp_index].control &= llvm::maskTrailingZeros<uint32_t>(1);
```
================
Comment at:
source/Plugins/Process/Linux/NativeRegisterContextLinux_ppc64le.cpp:374
+ uint32_t tempControl = m_hwp_regs[wp_index].control;
+ long * tempSlot = reinterpret_cast<long *>(m_hwp_regs[wp_index].slot);
+
----------------
`reinterpret_cast` from `long*` to `long`? Is this correct?
================
Comment at:
source/Plugins/Process/Linux/NativeRegisterContextLinux_ppc64le.cpp:389
+ m_hwp_regs[wp_index].address = tempAddr;
+ m_hwp_regs[wp_index].slot = reinterpret_cast<long>(tempSlot);
+
----------------
Same as before. Is this correct?
================
Comment at:
source/Plugins/Process/Linux/NativeRegisterContextLinux_ppc64le.cpp:402-413
+ switch ((m_hwp_regs[wp_index].control >> 5) & 0xff) {
+ case 0x01:
+ return 1;
+ case 0x03:
+ return 2;
+ case 0x0f:
+ return 4;
----------------
```
unsigned control = (m_hwp_regs[wp_index].control >> 5) & 0xFF;
assert(llvm::isPowerOf2_32(control + 1));
return llvm::countPopulation(control);
```
================
Comment at:
source/Plugins/Process/Linux/NativeRegisterContextLinux_ppc64le.cpp:416-417
+
+bool NativeRegisterContextLinux_ppc64le::WatchpointIsEnabled(
+ uint32_t wp_index) {
+ Log *log(ProcessPOSIXLog::GetLogIfAllCategoriesSet(POSIX_LOG_WATCHPOINTS));
----------------
Can this function accept a `const DREG&` instead of an index?
================
Comment at:
source/Plugins/Process/Linux/NativeRegisterContextLinux_ppc64le.cpp:421-424
+ if ((m_hwp_regs[wp_index].control & 0x1) == 0x1)
+ return true;
+ else
+ return false;
----------------
`return !!(m_hwp_regs[wp_index].control & 0x1);`
================
Comment at:
source/Plugins/Process/Linux/NativeRegisterContextLinux_ppc64le.cpp:474
+ return m_hwp_regs[wp_index].hit_addr;
+ else
+ return LLDB_INVALID_ADDRESS;
----------------
No need for `else` here.
================
Comment at:
source/Plugins/Process/Linux/NativeRegisterContextLinux_ppc64le.h:70-72
+ uint32_t GetWatchpointSize(uint32_t wp_index);
+
+ bool WatchpointIsEnabled(uint32_t wp_index);
----------------
Can these two functions take `const DREG&` instead of indices? Also, it seems
like they could be raised out of the class and into global static functions in
the cpp file.
================
Comment at:
source/Plugins/Process/Linux/NativeRegisterContextLinux_ppc64le.h:102
+
+ struct DREG m_hwp_regs[4];
+
----------------
How about `std::array<DREG, 4> m_hwp_regs;`
https://reviews.llvm.org/D38897
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
