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 ®set_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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
