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
