[Lldb-commits] [PATCH] D77047: AArch64 SVE register infos and core file support

[Pavel Labath via Phabricator via lldb-commits]

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