omjavaid marked 6 inline comments as done. omjavaid added a comment. In D77047#2125220 <https://reviews.llvm.org/D77047#2125220>, @labath wrote:
> There's always a chance for ODR violations, particularly with header files > defining static objects. This looks better though what I really wanted was to > keep the non-sve register info array (`g_register_infos_arm64_le`) completely > out of the path of the sve header. Like, by using three headers, one defining > `g_register_infos_arm64_le` (and stuff specific to that like the exception > registers), one defining the sve array, and the third one which would contain > the things common to both. Nonetheless, I think we can now move on to aspects > of this patch. Please see my inline comments. I am working to fix this with D80105 <https://reviews.llvm.org/D80105> by defining dynamic register infos array based on register sets which will be requirement for Pointer Authentication and MTE. But I wanted to first get done with SVE and come back to register infos refactoring later. ================ Comment at: lldb/source/Plugins/Process/Utility/NativeRegisterContextRegisterInfo.h:38-40 + uint32_t SetRegisterInfoMode(uint32_t mode) { + return m_register_info_interface_up->SetRegisterInfoMode(mode); + } ---------------- labath wrote: > labath wrote: > > I can't find any callers of this function. Who is calling it? > It still seems to me that this function is not used (in this patch at least). Ack. This function is used by chiild revisions which adds ptrace support. I ll move it there. ================ Comment at: lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_arm64.cpp:261 + return set_index < k_num_register_sets; + else + return set_index < (k_num_register_sets - 1); ---------------- labath wrote: > http://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return Ack. ================ Comment at: lldb/source/Plugins/Process/Utility/RegisterInfoInterface.h:64-68 + virtual uint32_t SetRegisterInfoMode(uint32_t mode, uint32_t offset = 0) { + return 0; + } + + virtual uint32_t GetRegisterInfoMode() const { return 0; } ---------------- labath wrote: > I don't believe these functions should be present on the generic interface. > The only caller and the only implementation of them is already > arm64-specific, so it should be possible to arrange it such that they don't > need to go through the base class interface. For instance the > `RegisterContextCorePOSIX_arm64` constructor could accept an > `RegisterInfoPOSIX_arm64` instance to indicate what it needs to work with, > and then downcast `m_register_info_up` (via a helper function?) when it needs > to call these. Solutions without downcasting are possible too, though may > require a bit more refactoring. Agreed. I ll make an arrangement for this in updated rev. ================ Comment at: lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp:72-89 + if (IsSVEZReg(reg_num)) + return (reg_num - sve_z0_arm64) * 16; + else if (reg_num == fpu_fpsr_arm64) + return 32 * 16; + else if (reg_num == fpu_fpcr_arm64) + return (32 * 16) + 4; + } else if (m_sve_state == SVE_STATE::SVE_STATE_FULL) { ---------------- labath wrote: > no else after return Ack. ================ Comment at: lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.h:53-55 + void ConfigureRegisterContext(); + + uint32_t CalculateSVEOffset(uint32_t reg_num) const; ---------------- labath wrote: > Do these need to be public? If so, why? These should be private. I ll update this in updated rev. ================ Comment at: lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.h:76 + + mutable SVE_STATE m_sve_state; + struct user_sve_header m_sve_header; ---------------- labath wrote: > Why is this mutable? It was pulled in from NativeRegisterContextLinux_arm64 and got set mutable by mistake I ll correct in updated rev. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77047/new/ https://reviews.llvm.org/D77047 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
