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