New patch is fine.  Lgtm<br><div class="gmail_quote"><div dir="ltr">On Thu, Oct 19, 2017 at 4:56 AM Ana Julia Caetano via Phabricator <<a href="mailto:reviews@reviews.llvm.org">reviews@reviews.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">anajuliapc added inline comments.<br>
<br>
<br>
================<br>
Comment at: source/Plugins/Process/Linux/NativeRegisterContextLinux_ppc64le.cpp:327-333<br>
+  for (uint32_t i = 0; i < m_max_hwp_supported; i++) {<br>
+    if ((m_hwp_regs[i].control & 1) == 0) {<br>
+      wp_index = i; // Mark last free slot<br>
+    } else if (m_hwp_regs[i].address == addr) {<br>
+      return LLDB_INVALID_INDEX32; // We do not support duplicate watchpoints.<br>
+    }<br>
+  }<br>
----------------<br>
zturner wrote:<br>
> Can we use a range-based for here?<br>
><br>
> ```<br>
> DREG *reg = nullptr;<br>
> for (auto &dr : m_hwp_regs) {<br>
>   if (dr.control & 1) == 0)<br>
>     reg = &dr;<br>
>   else if (dr.address == addr)<br>
>     return LLDB_INVALID_INDEX32;<br>
> }<br>
><br>
> if (!reg)<br>
>   return LLDB_INVALID_INDEX32;<br>
><br>
> reg->real_addr = real_addr;<br>
> reg->address = addr;<br>
> reg->control = control_value;<br>
> reg->mode = rw_mode;<br>
> ```<br>
><br>
> etc<br>
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.<br>
<br>
<br>
================<br>
Comment at: source/Plugins/Process/Linux/NativeRegisterContextLinux_ppc64le.cpp:416-417<br>
+<br>
+bool NativeRegisterContextLinux_ppc64le::WatchpointIsEnabled(<br>
+    uint32_t wp_index) {<br>
+  Log *log(ProcessPOSIXLog::GetLogIfAllCategoriesSet(POSIX_LOG_WATCHPOINTS));<br>
----------------<br>
zturner wrote:<br>
> Can this function accept a `const DREG&` instead of an index?<br>
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.<br>
<br>
<br>
================<br>
Comment at: source/Plugins/Process/Linux/NativeRegisterContextLinux_ppc64le.h:70-72<br>
+  uint32_t GetWatchpointSize(uint32_t wp_index);<br>
+<br>
+  bool WatchpointIsEnabled(uint32_t wp_index);<br>
----------------<br>
zturner wrote:<br>
> 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.<br>
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?<br>
<br>
<br>
<a href="https://reviews.llvm.org/D38897" rel="noreferrer" target="_blank">https://reviews.llvm.org/D38897</a><br>
<br>
<br>
<br>
</blockquote></div>