labath added inline comments.
================ Comment at: lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp:63-66 + m_sve_note_payload.resize(m_sveregset.GetByteSize()); + ::memcpy(GetSVEBuffer(), m_sveregset.GetDataStart(), + m_sveregset.GetByteSize()); + ::memcpy(&m_sve_header, m_sveregset.GetDataStart(), sizeof(m_sve_header)); ---------------- omjavaid wrote: > labath wrote: > > What's up with this copying? We already have the data in the `m_sveregset` > > DataExtractor. What's the reason for copying it into the > > `m_sve_note_payload` vector? Also, `m_sve_header` seems like it could just > > be a `reinterpret_cast`ed pointer into that buffer instead of a copy. > > (Maybe not even a pointer, just a utility function which performs the cast > > when called). > > > > Actually, when I think about casts and data extractors, I am reminded of > > endianness. This will access those fields using host endianness, which is > > most likely not what we want to do. So, probably the best/simplest solution > > would be to indeed declare a `user_sve_header` struct, but don't populate > > it via memcpy, but rather via the appropriate DataExtractor extraction > > methods. Since the only field of the user_sve_header used outside this > > function is the `vl` field, perhaps the struct could be a local variable > > and only the vector length would be persisted. That would be consistent > > with how the `flags` field is decoded and stored into the `m_sve_state` > > field. > > > > (If the struct fields are always little-endian (if that's true, I thought > > arm has BE variants) then you could also stick to the `reinterpret_cast` > > idea, but change the types of the struct fields to > > `llvm::support::ulittleXX_t` to read them as LE independently of the host.) > Agreed m_sve_note_payload is redundant. Most of the implementation was > mirrored from ptrace implementation and I was lazy enough and did not remove > the redundancies which are not needed in case of core dump. > > About endianess of SVE data I dont think it will create any problem as the > data is transferred in endian invariant format. According to the standard > here: > > * Whenever SVE scalable register values (Zn, Pn, FFR) are exchanged in memory > between userspace and the kernel, the register value is encoded in memory in > an endianness-invariant layout, with bits [(8 * i + 7) : (8 * i)] encoded at > byte offset i from the start of the memory representation. This affects for > example the signal frame (struct sve_context) and ptrace interface > (struct user_sve_header) and associated data. Ok, so if I parse that correctly, this is basically saying that the register values are always little-endian. Fair enough. However, lldb will be reading these things using the host endianness, so the values will be incorrect on a big-endian host. Also, the are you sure that this includes the metadata in the `user_sve_header` field? The documentation does mention the struct explicitly, but then it also very explicitly speaks about **register** values. The data in the struct is not a register value, and I can find no evidence of byte-swapping in the linux kernel (https://github.com/torvalds/linux/blob/master/arch/arm64/kernel/ptrace.c#L769). 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
