[Lldb-commits] [PATCH] D82863: [LLDB] Add support to resize SVE registers at run-time

[Jason Molenda via Phabricator via lldb-commits]

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