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

[Pavel Labath via Phabricator via lldb-commits]

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);
----------------
http://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return


================
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?


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

https://reviews.llvm.org/D77047