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