[Lldb-commits] [PATCH] D96458: [LLDB] Add support for Arm64/Linux dynamic register sets

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added inline comments.


================
Comment at: lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp:454
 bool NativeRegisterContextLinux_arm64::IsSVE(unsigned reg) const {
-  if (GetRegisterInfo().GetRegisterSetFromRegisterIndex(reg) ==
-      RegisterInfoPOSIX_arm64::SVERegSet)
+  if (GetRegisterInfo().IsSVEReg(reg))
     return true;
----------------
`return GetRegisterInfo().IsSVEReg(reg)` ?


================
Comment at: lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp:1120
+      GetRegisterInfo().ConfigureVectorLength(vq);
       m_sve_ptrace_payload.resize(SVE_PT_SIZE(vq, SVE_PT_REGS_SVE));
     }
----------------
I spent a good few minutes getting my head around this logic but tell me if I missed something. (a comment explaining it would be great)

* If we don't have a cached sve header and we don't know that SVE is disabled...
* Try to read the SVE header
* if it succeeded and we're doing this read for the first time, setup register infos
* otherwise it failed so SVE must be disabled
* if it succeeded (first time or otherwise) query the vector length (this is the bit that tripped me up trying to think how you'd refactor it)
* if we did have a cached sve header, we'd already know the config, no point reading it again
* if it's disabled, then it's disabled, same thing.

I think that means you're missing the corner case where we have SVEState::Full say, but then the header fails to read. Is that possible on a real system though? I guess not.

And vector length can change at runtime, correct?


================
Comment at: lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_arm64.cpp:43
 bool RegisterContextPOSIX_arm64::IsSVE(unsigned reg) const {
-  if (m_register_info_up->GetRegisterSetFromRegisterIndex(reg) ==
-      RegisterInfoPOSIX_arm64::SVERegSet)
+  if (m_register_info_up->IsSVEReg(reg))
     return true;
----------------
`return m_register_info_up->IsSVEReg(reg);` ?


================
Comment at: lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.cpp:243
+  for (const auto &regset_range : m_per_regset_regnum_range) {
+    if (reg_index >= regset_range.second.first &&
+        reg_index <= regset_range.second.second)
----------------
Should the last condition be <? Given:
```
m_per_regset_regnum_range[FPRegSet] = std::make_pair(fpu_v0, fpu_fpcr);
```
Where the last index is the first non FP register.


================
Comment at: lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.cpp:258
+void RegisterInfoPOSIX_arm64::ConfigureRegisterInfos(uint32_t opt_regsets) {
+  // TODO: Comment Here
+  if (opt_regsets > ARM64_REGS_CONFIG_SVE) {
----------------
Comments here would really help. 


================
Comment at: lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.h:93
+
+  uint32_t ConfigureVectorLength(uint32_t mode);
 
----------------
Just curious, is "mode" the term we use for the SVE length? I guess because it's going to be some multiples of 128 isn't it, not an arbitrary bit length.


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

https://reviews.llvm.org/D96458