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