[lldb] [Process/gdb-remote] Alias sp to x31 on AArch64 for gdbserver

[mgorny]

Summary

Alias the "sp" register to "x31" on AArch64 if one is present and does
not have the alt_name. This is the case when connecting to gdbserver.

[labath]

Let's take a step back. First, I'd like to understand why are you adding a whole new register, instead of just an alias (alt_name) for an existing register. And second, have you considered putting this into the ABI plugin, as that's where the rest of the "augmentation" code lives (or most of it, anyway).

[mgorny]

In D109695#2999171, @labath wrote:

Let's take a step back. First, I'd like to understand why are you adding a whole new register, instead of just an alias (alt_name) for an existing register.

Well, I was thinking 'what if the register has already another alt_name?' While it's unlikely that such a thing would happen with gdbserver, it's valid input in the end, and adding a new register seemed a more reliable solution for arbitrary input.

And second, have you considered putting this into the ABI plugin, as that's where the rest of the "augmentation" code lives (or most of it, anyway).

I initially did, yes, and I've figured out that it doesn't belong there since 1) these assignments aren't really specific to a single ABI but are general architecture characteristics, and 2) the logic is really specific to the gdb-remote plugin.

I mean, putting things like generic regno assignments into ABI makes sense because different ABIs could use different registers for given purpose. However, things like ESP being derived from RSP or aliasing sp to x31 seem ABI-independent.

That said, I don't feel strongly about it.

[labath]

In D109695#2999221, @mgorny wrote:

In D109695#2999171, @labath wrote:

Let's take a step back. First, I'd like to understand why are you adding a whole new register, instead of just an alias (alt_name) for an existing register.

Well, I was thinking 'what if the register has already another alt_name?' While it's unlikely that such a thing would happen with gdbserver, it's valid input in the end, and adding a new register seemed a more reliable solution for arbitrary input.

While we shouldn't crash on such an input, I am not particularly worried about everything still functioning perfectly. And adding a new register has user-visible consequences (two entries in register read).

Overall, I think I'd be fine both with overwriting the alt-name in this case (this is an lldb construct anyway) and with just doing nothing.

And second, have you considered putting this into the ABI plugin, as that's where the rest of the "augmentation" code lives (or most of it, anyway).

I initially did, yes, and I've figured out that it doesn't belong there since 1) these assignments aren't really specific to a single ABI but are general architecture characteristics, and 2) the logic is really specific to the gdb-remote plugin.

I mean, putting things like generic regno assignments into ABI makes sense because different ABIs could use different registers for given purpose. However, things like ESP being derived from RSP or aliasing sp to x31 seem ABI-independent.

That is true, and it may actually make more sense to put this into the Architecture plugin. However, I am not sure that this architectural purity is worth the hassle of having RegisterInfo knowledge in two places, especially since the ABI plugins are nowadays structured in a way that the all ABIs for a given architecture share a common base class, and this common logic could easily go there. (For that reason, I am also not sure we really need a separate Architecture hierarchy, but anyway...)

[mgorny]

Rebased — we no longer need the alt_name dance.

lldb/source/Plugins/Process/gdb-remote/CMakeLists.txt

add_lldb_library(lldbPluginProcessGDBRemote PLUGIN
   GDBRemoteCommunicationClient.cpp
   GDBRemoteCommunicationHistory.cpp
   GDBRemoteCommunicationReplayServer.cpp
   GDBRemoteCommunicationServer.cpp
   GDBRemoteCommunicationServerCommon.cpp
   GDBRemoteCommunicationServerLLGS.cpp
   GDBRemoteCommunicationServerPlatform.cpp
   GDBRemoteRegisterContext.cpp
+  GDBRemoteRegisterContext_aarch64.cpp
   GDBRemoteRegisterContext_x86.cpp
   ProcessGDBRemote.cpp
   ProcessGDBRemoteLog.cpp
   ThreadGDBRemote.cpp
 
   LINK_LIBS
     lldbBreakpoint
     lldbCore

lldb/source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.h

public:
   GDBRemoteDynamicRegisterInfo() : DynamicRegisterInfo() {}
 
   ~GDBRemoteDynamicRegisterInfo() override = default;
 
   void HardcodeARMRegisters(bool from_scratch);
   bool UpdateARM64SVERegistersInfos(uint64_t vg);
   void Finalize(const lldb_private::ArchSpec &arch);
 
+  void PreFinalize_aarch64();
   void PreFinalize_x86(const lldb_private::ArchSpec &arch);
   void PostFinalize_x86();
 };
 
 class GDBRemoteRegisterContext : public RegisterContext {
 public:
   GDBRemoteRegisterContext(ThreadGDBRemote &thread, uint32_t concrete_frame_idx,
                            GDBRemoteDynamicRegisterInfoSP reg_info_sp,

lldb/source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.cpp

//   ======   ======  ===  ===  =============     ==========      ===================  ===================  ======================  =============   ====    ==========    =============== ========= ========
   }
 }
 
 void GDBRemoteDynamicRegisterInfo::Finalize(const ArchSpec &arch) {
   if (m_finalized)
     return;
 
   switch (arch.GetMachine()) {
+  case llvm::Triple::aarch64:
+    PreFinalize_aarch64();
+    break;
   case llvm::Triple::x86:
   case llvm::Triple::x86_64:
     PreFinalize_x86(arch);
     break;
 
   default:
     break;
   }

lldb/source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext_aarch64.cpp

+//===-- GDBRemoteRegisterContext_aarch64.cpp ------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "GDBRemoteRegisterContext.h"
+
+using namespace lldb;
+using namespace lldb_private;
+using namespace lldb_private::process_gdb_remote;
+
+static uint32_t x31_value_regs[2];
+
+void GDBRemoteDynamicRegisterInfo::PreFinalize_aarch64() {
+  uint32_t max_regnum = 0;
+  uint32_t next_regindex = m_regs.size();
+  for (const RegisterInfo &reg : m_regs)
+    max_regnum = std::max(max_regnum, reg.kinds[eRegisterKindProcessPlugin]);
+
+  ConstString group;
+  group.SetCString("aliases");
+
+  // Alias "sp" into "x31" if necessary.
+  const RegisterInfo *sp_reg = GetRegisterInfo("sp");
+  const RegisterInfo *x31_reg = GetRegisterInfo("x31");
+
+  if (sp_reg && !x31_reg) {
+    x31_value_regs[0] = sp_reg->kinds[eRegisterKindProcessPlugin];
+    x31_value_regs[1] = LLDB_INVALID_REGNUM;
+
+    struct RegisterInfo new_reg {
+      "x31", nullptr, sp_reg->byte_size, LLDB_INVALID_INDEX32, sp_reg->encoding,
+          sp_reg->format,
+          {
+              LLDB_INVALID_REGNUM, LLDB_INVALID_REGNUM, LLDB_INVALID_REGNUM,
+              ++max_regnum,        next_regindex++,
+          },
+          x31_value_regs, nullptr, nullptr, 0
+    };
+
+    ConstString name;
+    ConstString alt_name;
+    name.SetCString(new_reg.name);
+    AddRegister(new_reg, name, alt_name, group);
+  }
+}

lldb/test/API/functionalities/gdb_remote_client/TestGDBServerTargetXML.py

def test_aarch64_regs(self):
         lldbutil.expect_state_changes(self, self.dbg.GetListener(), process,
                                       [lldb.eStateStopped])
 
         # GPRs should be displayed as uints
         self.match("register read x0",
                    ["x0 = 0x0807060504030201"])
         self.match("register read x1",
                    ["x1 = 0x1817161514131211"])
+        self.match("register read x29",
+                   ["x29 = 0x3837363534333231"])
+        self.match("register read x30",
+                   ["x30 = 0x4847464544434241"])
+        self.match("register read x31",
+                   ["x31 = 0x5857565554535251"])
         self.match("register read sp",
                    ["sp = 0x5857565554535251"])
         self.match("register read pc",
                    ["pc = 0x6867666564636261"])
         self.match("register read cpsr",
                    ["cpsr = 0x74737271"])
 
         # test generic aliases