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

[Pavel Labath via Phabricator via lldb-commits]

labath marked an inline comment as done.
labath added a comment.

Thanks. This looks is starting to look good now. I didn't get a chance to review all of the changes, but here's a few more things that I noticed.



================
Comment at: lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.h:72-76
+  uint32_t SetRegisterInfoMode(uint32_t mode, uint32_t offset = 0);
+
+  uint32_t GetRegisterInfoMode() const;
+
+  bool RegisterInfoModeIsValid(uint32_t mode) {
----------------
Now that this is arm-specific, I am wondering if there isn't a better name for this than "register info 'mode'". Maybe it could be just "enable/disable SVE" ? Specifically, if the validity of the "mode" is checked by the caller, then we can avoid this `RegisterInfoModeIsValid` business...


================
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) {
----------------
omjavaid wrote:
> labath wrote:
> > no else after return
> Ack.
Well.. that's one way of handing that.. :) It's not necessarily bad, but I was imagining you would just delete all the "else"s...


================
Comment at: lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp:21
 RegisterContextCorePOSIX_arm64::RegisterContextCorePOSIX_arm64(
     Thread &thread, RegisterInfoInterface *register_info,
     const DataExtractor &gpregset, llvm::ArrayRef<CoreNote> notes)
----------------
It would be also nice if this constructor accepted a `RegisterInfoPOSIX_arm64` instead of plain `RegisterInfoInterface`. That would make it much harder to misuse this class. For that we'd need to slightly refactor the construction code in ThreadElfCore. For example, we could swap the order of the "os" and "machine" switches and then join the two "machine" switches that construct RegisterInfoInterfaces and RegisterContexts.

Maybe you could do that as a separate patch?


================
Comment at: lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.h:17-22
+#include <asm/ptrace.h>
+
+#ifndef SVE_PT_REGS_SVE
+#define INCLUDE_LINUX_PTRACE_DEFINITIONS_FOR_SVE_ARM64
+#include "Plugins/Process/Linux/LinuxPTraceDefines_arm64sve.h"
+#endif
----------------
This is including platform-specific headers, but elf cores should be readable from any host. I guess you should just re-define the relevant constants somewhere. (I'm not sure what they are.)


================
Comment at: lldb/source/Utility/ARM64_DWARF_Registers.h:54-146
+  // 34-45 reserved
+
+  // 64-bit SVE Vector granule pseudo register
+  vg = 46,
+
+  // VG ́8-bit SVE first fault register
+  ffr = 47,
----------------
All of these numbers are from the official spec, right? Maybe you could just commit this straight away so it does not clutter this review.


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

https://reviews.llvm.org/D77047