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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Some more comments (and questions) from me. Sorry about the back-and-forth. It's taking me a while to wrap my head around this, and as I start to understand things, new questions arise...



================
Comment at: lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.cpp:282-290
+  if (!m_per_vq_reg_infos.count(sve_vq)) {
+
+    m_per_vq_reg_infos.insert(std::make_pair(
+        sve_vq, std::vector<lldb_private::RegisterInfo>(
+                    g_register_infos_arm64_sve_le,
+                    g_register_infos_arm64_sve_le + m_register_info_count)));
+
----------------
I'd probably try relying on the fact that `operator[]` constructs an empty vector, and that the empty vector is not a valid register info value. Something like:
```
std::vector<RegisterInfo> &new_reg_info = m_per_vq_reg_infos[sve_vq];
if (new_reg_info.empty()) {
  new_reg_info = llvm::makeArrayRef(g_register_infos_arm64_sve_le, m_register_info_count);
  ...
}
m_register_info_p = new_reg_info.data();
```

That would make this less of a mouthful and also avoid looking up the same key in the map three or four times.


================
Comment at: lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.h:17-22
+enum class SVE_STATE {
+  SVE_STATE_UNKNOWN,
+  SVE_STATE_DISABLED,
+  SVE_STATE_FPSIMD,
+  SVE_STATE_FULL
+};
----------------
If this is going to be a class enum (which I think is a good idea), then there's no need to repeat the type name in the actual enumerators. It also seems like this could/should follow the lldb naming conventions (SveState::Disabled, etc).


================
Comment at: lldb/source/Plugins/Process/Utility/lldb-arm64-register-enums.h:261
+  k_num_fpr_registers_arm64 = k_last_fpr_arm64 - k_first_fpr_arm64 + 1,
+
+  k_first_sve_arm64 = exc_far_arm64,
----------------
I guess these changes should be reverted now?


================
Comment at: lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp:82-106
+uint32_t
+RegisterContextCorePOSIX_arm64::CalculateSVEOffset(uint32_t reg_num) const {
+  uint32_t sve_offset = 0;
+  if (m_sve_state == SVE_STATE::SVE_STATE_FPSIMD) {
+    if (IsSVEZ(reg_num))
+      sve_offset = (reg_num - GetRegNumSVEZ0()) * 16;
+    else if (reg_num == GetRegNumFPSR())
----------------
I'm confused by this function. If I understant the SVE_PT macros and the logic in `RegisterInfoPOSIX_arm64::ConfigureVectorRegisterInfos` correctly, then they both seem to encode the same information. And it seems to me that this function should just be `reg_infos[reg_num].offset - some_constant`, which is the same thing that we're doing for the arm FP registers when SVE is disabled, and also for most other architectures too.

Why is that not the case? Am I missing something? If they are not encoding the same thing, could they be made to encode the same thing?


================
Comment at: lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.h:17
 
+#include "Plugins/Process/Utility/LinuxPTraceDefines_arm64sve.h"
+
----------------
Please group the header next to other `Plugins/Process/Utility` header


================
Comment at: lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py:326-327
 
+    #@skipIf(triple='^mips')
+    #@skipIfLLVMTargetMissing("AArch64")
+    def test_aarch64_sve_regs(self):
----------------
What's up with these?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77047/new/

https://reviews.llvm.org/D77047