[Lldb-commits] [PATCH] D77047: AArch64 SVE register infos and core file support
Pavel Labath via Phabricator via lldb-commits
lldb-commits at lists.llvm.org
Mon Jul 13 07:23:51 PDT 2020
labath added a comment.
I didn't notice this before, but I see now that there's more register number overlap in `lldb-arm64-register-enums.h`. Having one overlapping enum is bad enough, but two seems really too much? Can we avoid the second enum somehow, at least? Perhaps by switching `RegisterContextCorePOSIX_arm64` to use the other enum definitions for its work?
================
Comment at: lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.cpp:256-257
+ uint32_t offset) {
+ // Register info mode denotes SVE vector length in context of AArch64.
+ // Register info mode once set to zero permanently selects default static
+ // AArch64 register info and cannot be changed to SVE. Also if an invalid
----------------
This comment looks outdated.
================
Comment at: lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.cpp:280
+
+ m_sve_enabled = true;
+
----------------
IIUC, `m_sve_enabled` is only true if m_vector_reg_vq is not zero. Could we delete this variable and just make a function to make that comparison?
================
Comment at: lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.cpp:288-291
+ if (m_per_vq_reg_infos.count(sve_vq)) {
+ m_dynamic_register_infos = m_per_vq_reg_infos.at(sve_vq);
+ m_register_info_p = &m_dynamic_register_infos[0];
+ return m_vector_reg_vq;
----------------
It doesn't look like `m_dynamic_register_infos` is needed -- you could just make `m_register_info_p` point directly into the map.
================
Comment at: lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.h:91
+ uint32_t ConfigureVectorRegisterInfos(uint32_t mode, uint32_t offset = 0);
+
----------------
The `offset` argument is not set anywhere.
================
Comment at: lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.h:17
+#ifndef SVE_PT_REGS_SVE
+#define INCLUDE_LINUX_PTRACE_DEFINITIONS_FOR_SVE_ARM64
----------------
I guess this does not make sense now that the header is standalone
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D77047/new/
https://reviews.llvm.org/D77047
More information about the lldb-commits
mailing list