DavidSpickett added inline comments.
================ Comment at: lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp:454 bool NativeRegisterContextLinux_arm64::IsSVE(unsigned reg) const { - if (GetRegisterInfo().GetRegisterSetFromRegisterIndex(reg) == - RegisterInfoPOSIX_arm64::SVERegSet) + if (GetRegisterInfo().IsSVEReg(reg)) return true; ---------------- `return GetRegisterInfo().IsSVEReg(reg)` ? ================ Comment at: lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp:1120 + GetRegisterInfo().ConfigureVectorLength(vq); m_sve_ptrace_payload.resize(SVE_PT_SIZE(vq, SVE_PT_REGS_SVE)); } ---------------- I spent a good few minutes getting my head around this logic but tell me if I missed something. (a comment explaining it would be great) * If we don't have a cached sve header and we don't know that SVE is disabled... * Try to read the SVE header * if it succeeded and we're doing this read for the first time, setup register infos * otherwise it failed so SVE must be disabled * if it succeeded (first time or otherwise) query the vector length (this is the bit that tripped me up trying to think how you'd refactor it) * if we did have a cached sve header, we'd already know the config, no point reading it again * if it's disabled, then it's disabled, same thing. I think that means you're missing the corner case where we have SVEState::Full say, but then the header fails to read. Is that possible on a real system though? I guess not. And vector length can change at runtime, correct? ================ Comment at: lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_arm64.cpp:43 bool RegisterContextPOSIX_arm64::IsSVE(unsigned reg) const { - if (m_register_info_up->GetRegisterSetFromRegisterIndex(reg) == - RegisterInfoPOSIX_arm64::SVERegSet) + if (m_register_info_up->IsSVEReg(reg)) return true; ---------------- `return m_register_info_up->IsSVEReg(reg);` ? ================ Comment at: lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.cpp:243 + for (const auto ®set_range : m_per_regset_regnum_range) { + if (reg_index >= regset_range.second.first && + reg_index <= regset_range.second.second) ---------------- Should the last condition be <? Given: ``` m_per_regset_regnum_range[FPRegSet] = std::make_pair(fpu_v0, fpu_fpcr); ``` Where the last index is the first non FP register. ================ Comment at: lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.cpp:258 +void RegisterInfoPOSIX_arm64::ConfigureRegisterInfos(uint32_t opt_regsets) { + // TODO: Comment Here + if (opt_regsets > ARM64_REGS_CONFIG_SVE) { ---------------- Comments here would really help. ================ Comment at: lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.h:93 + + uint32_t ConfigureVectorLength(uint32_t mode); ---------------- Just curious, is "mode" the term we use for the SVE length? I guess because it's going to be some multiples of 128 isn't it, not an arbitrary bit length. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96458/new/ https://reviews.llvm.org/D96458 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits