jasonmolenda added a comment.
Hey all, got distracted while reading the patch and jotted a few notes, but I
know that's not as much what Pavel wanted to discuss. We already have some
target specific goop in these ProcessGDBRemote type classes for registers and
such, this doesn't cause my great pain to see added.
The question of what information the stub provides to lldb at startup (through
qRegisterInfo, or XML - to be honest, we should be pushing the XML approach
everywhere now, we added qRegisterInfo back before it was as commonly used) - I
agree with Pavel, the remote stub's register number, register name, register
size, register offset is a good set, we should have everything else available
from Target-specific knowledge in lldb. gdb started out with the requirement
that the remote stub and gdb were in complete agreement about what registers
were available, what offsets they were in the register context, all of it. We
started lldb with the idea that the remote stub was the source of truth for
everything about registers - their eh_frame and DWARF numberings, their
formatting, their type, their register sets. But nearly all of this is
Target/ABI specific details that lldb can *safely* know already, and that
reduces the burden on remote stubs to provide a ton of different information.
And the g/G packets just scare the willies out of me, they cause so many
problems if there is a mis-coordination between the remote stub and lldb, I
never want to stop the remote stub from providing those offsets.
Overall, this looks fine to me, fwiw. The SVE registers were always going to
be a tricky thing to support, and I'm not surprised we're looking at changes
like this.
================
Comment at:
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:512
+ if (reg_set_p == nullptr)
+ reg_set_p = reg_ctx_sp->GetRegisterSet(0);
+
----------------
I'm not thrilled with GetRegisterSet(0) meaning "get the GPRs" but I already
see that being done in GDBRemoteCommunicationServerLLGS.cpp and
CommandObjectRegister.cpp does the same, so no reason to do anything there, we
assume the first set of registers are the GPRs.
================
Comment at:
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:545
+ uint32_t reg_num =
+ reg_ctx.ConvertRegisterKindToRegisterNumber(eRegisterKindDWARF, 46);
+ if (reg_num != LLDB_INVALID_REGNUM)
----------------
Can we define this register in ARM64_DWARF_Registers.h and use the enum name
instead of the # here?
================
Comment at:
lldb/source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.cpp:355
+ const ArchSpec &arch = process->GetTarget().GetArchitecture();
+ if (arch.IsValid() && (arch.GetMachine() == llvm::Triple::aarch64)) {
+ if (reg_info->kinds[eRegisterKindDWARF] == 46)
----------------
You also matched llvm::Triple::aarch64_be earlier, do you mean to do that here
too?
================
Comment at:
lldb/source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.cpp:356
+ if (arch.IsValid() && (arch.GetMachine() == llvm::Triple::aarch64)) {
+ if (reg_info->kinds[eRegisterKindDWARF] == 46)
+ do_reconfigure_arm64_sve = true;
----------------
Same with the enum suggestion earlier, I'd like to avoid encoding the AArch64
DWARF ABI reg numbers here.
================
Comment at:
lldb/source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.cpp:752
+ uint32_t vg_reg_num =
+ ConvertRegisterKindToRegisterNumber(eRegisterKindDWARF, 46);
+ uint64_t vg_reg_value = ReadRegisterAsUnsigned(vg_reg_num, fail_value);
----------------
Same dwarf const.
================
Comment at:
lldb/source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.cpp:767
+ m_reg_data.Clear();
+ m_reg_data.SetData(reg_data_sp);
+ m_reg_data.SetByteOrder(GetByteOrder());
----------------
I'm probably not following this correctly, but isn't this going to shorten the
register buffer m_reg_data to the end of the SVE registers in the buffer,
right? If the goal here is to create a heap object, why not just copy the
entire m_data_reg? If someone ever adds a register past the SVE's, this would
truncate it away.
================
Comment at:
lldb/source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.cpp:773
+ uint32_t v0_reg_num =
+ ConvertRegisterKindToRegisterNumber(eRegisterKindDWARF, 64);
+ for (uint16_t i = v0_reg_num; i < vg_reg_num; i++)
----------------
DWARF constant.
================
Comment at:
lldb/source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.cpp:790
+ uint32_t z0_reg_num =
+ ConvertRegisterKindToRegisterNumber(eRegisterKindDWARF, 96);
+ RegisterInfo *reg_info = GetRegisterInfoAtIndex(z0_reg_num);
----------------
Another constant I'd like in the header file.
================
Comment at: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:1771
+ if (arch.IsValid() && (arch.GetMachine() == llvm::Triple::aarch64)) {
+ uint8_t arm64_sve_vg_dwarf_regnum = 46;
+ GDBRemoteRegisterContext *reg_ctx_sp =
----------------
DWARF constant, and do we need to handle aarch64_be?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D82863/new/
https://reviews.llvm.org/D82863
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
