New patch is fine. Lgtm
On Thu, Oct 19, 2017 at 4:56 AM Ana Julia Caetano via Phabricator <
revi...@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
>
>
>
>
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
