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

[Muhammad Omair Javaid via Phabricator via lldb-commits]

omjavaid added a comment.

thanks @DavidSpickett for the review. I have updated diff incorporating you suggestions.



================
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;
----------------
DavidSpickett wrote:
> `return GetRegisterInfo().IsSVEReg(reg)` ?
Ack.


================
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));
     }
----------------
DavidSpickett wrote:
> 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?
I have updated the comment to reflect what this piece of code tries to do. 

Register Infos are maintained by debuggers per process however for SVE we may have per thread register infos but what I understood from linux documentation is for a process to support SVE ptrace call to NT_ARM_SVE should succeed on first stop. This is what we are using to enabled or disabled SVE at startup.

if SVEState::Full is full and we fail to read NT_ARM_SVE at some stage for any reason. we can do try to read again on subsequent stop.




================
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;
----------------
DavidSpickett wrote:
> `return m_register_info_up->IsSVEReg(reg);` ?
Ack.


================
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)
----------------
DavidSpickett wrote:
> 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.
m_per_regset_regnum_range includes start and end register number of a register set with start and end inlucded.

fpu_fpcr is included in FPU regset. On non SVE targets Vx, Dx. Sx register plus fpcr and fpsr are part of FPU regset.




================
Comment at: lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.h:93
+
+  uint32_t ConfigureVectorLength(uint32_t mode);
 
----------------
DavidSpickett wrote:
> 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.
We had chosen mode when we started but then moves away from this terminology. I ve fixed it in next update.


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

https://reviews.llvm.org/D96458