omjavaid marked an inline comment as done. omjavaid 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)); ---------------- labath wrote: > 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). @labath Thanks for being great help in this review. I think you were right about the user_sve_header being susceptible to endianity problems. I have plugged the issues now. 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
