[Lldb-commits] [PATCH] D38897: Add specific ppc64le hardware watchpoint handler

[Zachary Turner via Phabricator via lldb-commits]

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