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

[Zachary Turner via lldb-commits]

New patch is fine. Lgtm
On Thu, Oct 19, 2017 at 4:56 AM Ana Julia Caetano via Phabricator <
reviews at reviews.llvm.org> wrote:

> 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
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20171019/ca5e26d2/attachment-0001.html>