[Lldb-commits] [PATCH] D77047: AArch64 SVE register infos and core file support

[Pavel Labath via Phabricator via lldb-commits]

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