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

[Ana Julia Caetano via Phabricator via lldb-commits]

anajuliapc added inline comments.


================
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.
+    }
+  }
----------------
zturner wrote:
> 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
I don't think it's worth the effort to do it like that. This function returns `wp_index` anyway, and it's easier to limit the number of watchpoints to PowerPC's actual limit, which is given by ptrace. 


================
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));
----------------
zturner wrote:
> Can this function accept a `const DREG&` instead of an index?
No because it's called by `NativeProcessLinux.`, I would have to change it too and I don't think I should since is a common file. 


================
Comment at: source/Plugins/Process/Linux/NativeRegisterContextLinux_ppc64le.h:70-72
+  uint32_t GetWatchpointSize(uint32_t wp_index);
+
+  bool WatchpointIsEnabled(uint32_t wp_index);
----------------
zturner wrote:
> 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.
I had some ideas while implementing your sugestions... I'm thinking about removing these functions and put these information in `m_hwp_regs` struct. Do you think I should do it in a new patch?


https://reviews.llvm.org/D38897