omjavaid marked an inline comment as done.
omjavaid added inline comments.
================
Comment at:
lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp:82-106
+uint32_t
+RegisterContextCorePOSIX_arm64::CalculateSVEOffset(uint32_t reg_num) const {
+ uint32_t sve_offset = 0;
+ if (m_sve_state == SVE_STATE::SVE_STATE_FPSIMD) {
+ if (IsSVEZ(reg_num))
+ sve_offset = (reg_num - GetRegNumSVEZ0()) * 16;
+ else if (reg_num == GetRegNumFPSR())
----------------
labath wrote:
> omjavaid wrote:
> > labath wrote:
> > > omjavaid wrote:
> > > > labath wrote:
> > > > > I'm confused by this function. If I understant the SVE_PT macros and
> > > > > the logic in `RegisterInfoPOSIX_arm64::ConfigureVectorRegisterInfos`
> > > > > correctly, then they both seem to encode the same information. And it
> > > > > seems to me that this function should just be
> > > > > `reg_infos[reg_num].offset - some_constant`, which is the same thing
> > > > > that we're doing for the arm FP registers when SVE is disabled, and
> > > > > also for most other architectures too.
> > > > >
> > > > > Why is that not the case? Am I missing something? If they are not
> > > > > encoding the same thing, could they be made to encode the same thing?
> > > > This function calculates offset of a particular register in core note
> > > > data. SVE data in core dump is similar to what PTrace emits and offsets
> > > > into this data is not linear. SVE macros are used to access those
> > > > offsets based on register numbers and currently selected vector length.
> > > > Also for the purpose of ease we have linear offsets in SVE register
> > > > infos and it helps us simplify register data once it makes way to
> > > > GDBRemoteRegisterContext on the client side.
> > > Could you give an example of the non-linearity of the core dump data?
> > > (Some registers, and their respective core file and gdb-remote offsets)
> > In case of core file we create a buffer m_sveregset which stores SVE core
> > note information
> > m_sveregset =
> > getRegset(notes,
> > m_register_info_up->GetTargetArchitecture().GetTriple(),
> > AARCH64_SVE_Desc);
> >
> > At this point we do not know what was the vector length and at what offsets
> > in the data our registers are located. We read top bytes of size
> > use_sve_header and read vector length. Based on this information we
> > configure vector length in Register infos. While the SVE payload starts
> > with user_sve_header then there are some allignment bytes followed by
> > vector length based Z registers followed by P and FFR, then there are some
> > more allginment bytes followd by FPCR and FPSR. Macros provided by Linux
> > help us jump to the desired offset by providing register number and vq into
> > the core note or Ptrace payload.
> >
> > In case of client side storage we store GPRs at linear offset followed by
> > Vector Granule register. Then there are SVE registers Z, P, FFR, FPSR and
> > FPCR. Offsets of V, D and S registers in FPR regset overlap with
> > corresponding first bytes of Z registers and will be same as corresponding
> > Z register. While both FP/SVE FPSR share same register offset, size and
> > register number.
> >
> > Here is an excerpt from
> > https://github.com/torvalds/linux/blob/master/Documentation/arm64/sve.rst
> >
> > SVE_PT_REGS_FPSIMD
> > SVE registers are not live (GETREGSET) or are to be made non-live
> > (SETREGSET).
> > The payload is of type struct user_fpsimd_state, with the same meaning as
> > for NT_PRFPREG, starting at offset SVE_PT_FPSIMD_OFFSET from the start of
> > user_sve_header.
> > Extra data might be appended in the future: the size of the payload should
> > be obtained using SVE_PT_FPSIMD_SIZE(vq, flags).
> > vq should be obtained using sve_vq_from_vl(vl).
> >
> > or
> >
> > SVE_PT_REGS_SVE
> > SVE registers are live (GETREGSET) or are to be made live (SETREGSET).
> > The payload contains the SVE register data, starting at offset
> > SVE_PT_SVE_OFFSET from the start of user_sve_header, and with size
> > SVE_PT_SVE_SIZE(vq, flags);
> Given this
> > SVE payload starts with ... followed by vector length based Z registers
> > followed by P and FFR,
> and this
> > In case of client side storage we store GPRs ... Then there are SVE
> > registers Z, P, FFR, FPSR and FPCR
> I would expect that for each of the Z, P and FFR registers, the expression
> `offset_in_core(reg) - offset_in_gdb_remote(reg)` is always the same constant
> (and is basically equal to `SVE_PT_SVE_ZREG_OFFSET(vq, 0) -
> reg_info[Z0].byte_offset`). So we could just add/subtract that constant to
> the gdb-remote byte_offset field instead of painstakingly decomposing the
> register number only for the linux macros to reconstruct it back again. Is
> that not so?
The standard never talks about Z, P and FFR being contagious that is what I
learnt by reading macros. There standard states this:
If the registers are present, the remainder of the record has a vl-dependent
size and layout. Macros SVE_SIG_* are defined [1] to facilitate access to the
members.
Each scalable register (Zn, Pn, FFR) is stored in an endianness-invariant
layout, with bits [(8 * i + 7) : (8 * i)] stored at byte offset i from the
start of the register's representation in memory.
If the SVE context is too big to fit in sigcontext.__reserved[], then extra
space is allocated on the stack, an extra_context record is written in
__reserved[] referencing this space. sve_context is then written in the extra
space. Refer to [1] for further details about this mechanism.
I understand what you are talking about but given the macros were specifically
provided and above line about register record was vague and I thought best is
to follow the macros for offset calculation although other way around is
simpler but may be slightly unreliable.
I suggest to keep this as it is unless there is strong reason apart from slight
performance penalty. This resembles with GDB implementation which was done by
ARM and I am only following that as reference. May be we can revise this in
future when the feature becomes more mainstream.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D77047/new/
https://reviews.llvm.org/D77047
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
