[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
Wed Jul 1 05:54:51 PDT 2020

labath added a comment.

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.

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:
> 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).

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);

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; }
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.

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) {
no else after return

Comment at: lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.h:53-55
+  void ConfigureRegisterContext();
+  uint32_t CalculateSVEOffset(uint32_t reg_num) const;
Do these need to be public? If so, why?

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;
Why is this mutable?



More information about the lldb-commits mailing list