Add hardware watchpoint funcionality for ppc64le
Details
Diff Detail
- Build Status
Buildable 11252 Build 11252: arc lint + arc unit
Event Timeline
source/Plugins/Process/Linux/NativeRegisterContextLinux_ppc64le.cpp | ||
---|---|---|
90–93 | Can you set these inline in the class body? e.g. when you declare them in the header file: uint32_t m_max_hwp_supported = 16; uint32_t m_max_hbp_supported = 16; bool m_refresh_hwdebug_info = true; | |
278–290 | Can you make an enum for these magic values 1, 2, and 3? | |
297–313 | How about: addr_t begin = llvm::alignDown(addr, 8); addr_t end = llvm::alignUp(addr+size, 8); size = llvm::NextPowerOf2(end-begin); | |
322–328 | 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 | |
344 | How about: m_hwp_regs[wp_index].control &= llvm::maskTrailingZeros<uint32_t>(1); | |
369 | reinterpret_cast from long* to long? Is this correct? | |
384 | Same as before. Is this correct? | |
397–408 | unsigned control = (m_hwp_regs[wp_index].control >> 5) & 0xFF; assert(llvm::isPowerOf2_32(control + 1)); return llvm::countPopulation(control); | |
411–412 | Can this function accept a const DREG& instead of an index? | |
416–419 | return !!(m_hwp_regs[wp_index].control & 0x1); | |
469 | No need for else here. | |
source/Plugins/Process/Linux/NativeRegisterContextLinux_ppc64le.h | ||
70–72 | 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. | |
102 | How about std::array<DREG, 4> m_hwp_regs; |
I agree with all of Zach's suggestions.
source/Plugins/Process/Linux/NativeRegisterContextLinux_ppc64le.h | ||
---|---|---|
98–99 | As long as this code is only ever compiled native, using "long" and "int" are ok. Just beware of code that isn't always compiled native. |
Thanks for supporting this change. I guess @anajuliapc will add you both as reviewer as soon as she updates this patch.
BTW, I agree that patches should be improving code quality but I wanted to highlight that these changes were actually based on the current ARM64 implementation. E.g: this snip and this requested change. But thanks for pointing it out.
source/Plugins/Process/Linux/NativeRegisterContextLinux_ppc64le.h | ||
---|---|---|
98–99 | It's a native only compiled file (see line 13. It defines a #if defined(__powerpc64__) that ends at the end of file) |
I'd be equally fine with extracting that logic into a separate function, it looks mostly the same? I don't think we should be duplicating code (or even algorithms written slightly differently)
source/Plugins/Process/Linux/NativeRegisterContextLinux_ppc64le.cpp | ||
---|---|---|
369 | Yes, It's strange but is the only way it works without changing NativeProcessLinux::PtraceWrapper. PPC_PTRACE_DELHWDEBUG works different from other PPC ptrace requests, because it must pass the return of PPC_PTRACE_SETHWDEBUG to ptrace instead of a data structure. If you pass it as long, PtraceWrapper fails because it must receive a void*. I'll check the other points, thanks for the comments. |
- Add values to header file
- Use std::array to declare m_hwp_regs
- Use llvm's mask
- Remove switch and use assert instead
- Remove unnecessary 'else'
- Change return
I applied some of the suggestions. I'm still working on the others, since I may need to change some logic
Thanks for the comments
- Change numeric values to enum
- Use llvm's functions to align addresses
source/Plugins/Process/Linux/NativeRegisterContextLinux_ppc64le.cpp | ||
---|---|---|
322–328 | 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. | |
411–412 | 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. | |
source/Plugins/Process/Linux/NativeRegisterContextLinux_ppc64le.h | ||
70–72 | 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? |
source/Plugins/Process/Linux/NativeRegisterContextLinux_ppc64le.cpp | ||
---|---|---|
278 | We should use the lldb::WatchpointKind from lldb-enumerations.h: //---------------------------------------------------------------------- // Watchpoint Kind // Indicates what types of events cause the watchpoint to fire. // Used by Native*Protocol-related classes. //---------------------------------------------------------------------- FLAGS_ENUM(WatchpointKind){eWatchpointKindRead = (1u << 0), eWatchpointKindWrite = (1u << 1)}; The you look at the bits and do the right thing. "access_mode" used below will just be if read and write are set. |
source/Plugins/Process/Linux/NativeRegisterContextLinux_ppc64le.cpp | ||
---|---|---|
278 | Variable watch_flags is set in GDBRemoteCommunicationServerLLGS with read = 2 and write = 1. |
source/Plugins/Process/Linux/NativeRegisterContextLinux_ppc64le.cpp | ||
---|---|---|
278 | I would switch them over to use eWatchpointKindRead and eWatchpointKindWrite and modify all code that falls out from that to use these definitions as well. |
source/Plugins/Process/Linux/NativeRegisterContextLinux_ppc64le.cpp | ||
---|---|---|
286 | As these enums are bitmasks, the traditional way of checking if more than one is set is to use the bitwise OR operator (|). Mostly because of the semantics, as you're not doing arithmetic there. |
source/Plugins/Process/Linux/NativeRegisterContextLinux_ppc64le.cpp | ||
---|---|---|
59 | I just realized I forgot to remove the enum I've created before. I know you already accepted this patch, but may I update it anyway? |
Hi, it's been already 4 days since this patch was accepted but not merged. Did something happen internally or we just need to wait a little longer?
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.