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

[Muhammad Omair Javaid via Phabricator via lldb-commits]

omjavaid marked 9 inline comments as done.
omjavaid added inline comments.


================
Comment at: lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.cpp:289
+
+    lldb_private::RegisterInfo *new_reg_info_p = reg_info_ref.data();
+
----------------
labath wrote:
> I think all uses of `new_reg_info_p` could just be replaced by `reg_info_ref`
Ack.


================
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:
> 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.


================
Comment at: lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp:84-86
+  const uint32_t reg = reg_info->kinds[lldb::eRegisterKindLLDB];
+  if (reg == LLDB_INVALID_REGNUM)
+    return false;
----------------
labath wrote:
> This is already checked by ReadRegister and the false -> uint32_t conversion is dodgy. An assertion would completely suffice here.
Ack.


================
Comment at: lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp:140
+      offset -= GetGPRSize();
+      if (IsFPR(reg) && offset < m_fpregset.GetByteSize()) {
+        value.SetFromMemoryData(reg_info, m_fpregset.GetDataStart() + offset,
----------------
labath wrote:
> This `IsFPR` check is now redundant.
Ack,


================
Comment at: lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp:152-153
+        sve_reg_num = reg_info->value_regs[0];
+      else if (reg == GetRegNumFPCR() || reg == GetRegNumFPSR())
+        sve_reg_num = reg;
+      if (sve_reg_num != LLDB_INVALID_REGNUM) {
----------------
labath wrote:
> These two registers are special-cased both here and in the `CalculateSVEOffset`. Given that both functions also do a switch over the sve "states", it makes following the code quite challenging. What if we moved the handling of these registers completely up-front, and removed their handling from `CalculateSVEOffset` completely?
> I'm thinking of something like:
> ```
> if (reg == GetRegNumFPCR() && m_sve_state != SVEState::Disabled) {
>   src = GetSVEBuffer() + GetFPCROffset(); // Or maybe just inline GetFPCROffset here
> } else if (reg == GetRegNumFPSR() && m_sve_state != SVEState::Disabled) {
>   src = ...;
> } if (IsFPR(reg)) {
>   // as usual, only you can assume that FP[CS]R are already handled if SVE is enabled
> }
> ```
Ok will try to update in next revision.


================
Comment at: lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp:161-162
+  } else if (IsSVEVG(reg)) {
+    sve_vg = GetSVERegVG();
+    src = (uint8_t *)&sve_vg;
+  } else if (IsSVE(reg)) {
----------------
labath wrote:
> This also looks endian-incorrect. Maybe `value = GetSVERegVG(); return true;` ?
Endian invariant as explained above.


================
Comment at: lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp:164
+  } else if (IsSVE(reg)) {
+    if (m_sve_state != SVEState::Disabled) {
+      if (m_sve_state == SVEState::FPSIMD) {
----------------
labath wrote:
> A switch over possible `m_sve_state` values would likely be cleaner.
Ack.


================
Comment at: lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp:174
+          ::memcpy(sve_reg_non_live.data(),
+                   (const uint8_t *)GetSVEBuffer() + offset, 16);
+        }
----------------
labath wrote:
> I guess it would be better to do the cast inside GetSVEBuffer. (and please make that a c++ reinterpret_cast).
Ack.


================
Comment at: lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp:180
+        assert(offset < m_sveregset.GetByteSize());
+        src = (uint8_t *)GetSVEBuffer() + offset;
+      }
----------------
labath wrote:
> it seems that `src` should be a const pointer.
Ack.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77047/new/

https://reviews.llvm.org/D77047