[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/ABI/AArch64/ABIAArch64.h

 //===-- AArch64.h -----------------------------------------------*- C++ -*-===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLDB_SOURCE_PLUGINS_ABI_AARCH64_ABIAARCH64_H
 #define LLDB_SOURCE_PLUGINS_ABI_AARCH64_ABIAARCH64_H
 
 #include "lldb/Target/ABI.h"
 
 class ABIAArch64: public lldb_private::MCBasedABI {
 public:
   static void Initialize();
   static void Terminate();
 
   virtual lldb::addr_t FixCodeAddress(lldb::addr_t pc) override;
   virtual lldb::addr_t FixDataAddress(lldb::addr_t pc) override;
 
 protected:
   virtual lldb::addr_t FixAddress(lldb::addr_t pc, lldb::addr_t mask) {
     return pc;
   }
 
   std::pair<uint32_t, uint32_t>
   GetEHAndDWARFNums(llvm::StringRef name) override;
 
   std::string GetMCName(std::string reg) override;
 
   uint32_t GetGenericNum(llvm::StringRef name) override;
 
+  void AugmentRegisterInfo(lldb_private::RegisterInfo &info) override;
+
   using lldb_private::MCBasedABI::MCBasedABI;
 };
 #endif

lldb/source/Plugins/ABI/AArch64/ABIAArch64.cpp

 }
 
 std::string ABIAArch64::GetMCName(std::string reg) {
   MapRegisterName(reg, "v", "q");
   MapRegisterName(reg, "x29", "fp");
   MapRegisterName(reg, "x30", "lr");
   return reg;
 }
+
 uint32_t ABIAArch64::GetGenericNum(llvm::StringRef name) {
   return llvm::StringSwitch<uint32_t>(name)
       .Case("pc", LLDB_REGNUM_GENERIC_PC)
       .Cases("lr", "x30", LLDB_REGNUM_GENERIC_RA)
       .Cases("sp", "x31", LLDB_REGNUM_GENERIC_SP)
       .Cases("fp", "x29", LLDB_REGNUM_GENERIC_FP)
       .Case("cpsr", LLDB_REGNUM_GENERIC_FLAGS)
       .Case("x0", LLDB_REGNUM_GENERIC_ARG1)
       .Case("x1", LLDB_REGNUM_GENERIC_ARG2)
       .Case("x2", LLDB_REGNUM_GENERIC_ARG3)
       .Case("x3", LLDB_REGNUM_GENERIC_ARG4)
       .Case("x4", LLDB_REGNUM_GENERIC_ARG5)
       .Case("x5", LLDB_REGNUM_GENERIC_ARG6)
       .Case("x6", LLDB_REGNUM_GENERIC_ARG7)
       .Case("x7", LLDB_REGNUM_GENERIC_ARG8)
       .Default(LLDB_INVALID_REGNUM);
 }
+
+void ABIAArch64::AugmentRegisterInfo(lldb_private::RegisterInfo &info) {
+  lldb_private::MCBasedABI::AugmentRegisterInfo(info);
+
+  // GDB sends x31 as "sp".  Add the "x31" alt_name for convenience.
+  if (!strcmp(info.name, "sp") && !info.alt_name)
+    info.alt_name = "x31";
+}

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

 //===-- ProcessGDBRemote.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 "lldb/Host/Config.h"
 
 #include <cerrno>
 #include <cstdlib>
 #if LLDB_ENABLE_POSIX
 #include <netinet/in.h>
 #include <sys/mman.h>
 #include <sys/socket.h>
 #include <unistd.h>
if (m_gdb_comm.SendPacketAndWaitForResponse(packet, response) ==
           invalidate_regs.push_back(LLDB_INVALID_REGNUM);
           reg_info.invalidate_regs = invalidate_regs.data();
         }
 
         reg_info.name = reg_name.AsCString();
         // We have to make a temporary ABI here, and not use the GetABI because
         // this code gets called in DidAttach, when the target architecture
         // (and consequently the ABI we'll get from the process) may be wrong.
-        if (ABISP abi_sp = ABI::FindPlugin(shared_from_this(), arch_to_use))
+        if (ABISP abi_sp = ABI::FindPlugin(shared_from_this(), arch_to_use)) {
           abi_sp->AugmentRegisterInfo(reg_info);
+          // AugmentRegisterInfo() can modify alt_name
+          if (reg_info.alt_name)
+            alt_name.SetCString(reg_info.alt_name);
+        }
 
         m_register_info_sp->AddRegister(reg_info, reg_name, alt_name, set_name);
       } else {
         break; // ensure exit before reg_num is incremented
       }
     } else {
       break;
     }
feature_node.ForEachChildElementWithName(
         if (!invalidate_regs.empty()) {
           invalidate_regs.push_back(LLDB_INVALID_REGNUM);
           reg_info.invalidate_regs = invalidate_regs.data();
         }
 
         reg_num_remote = reg_info.kinds[eRegisterKindProcessPlugin] + 1;
         ++reg_num_local;
         reg_info.name = reg_name.AsCString();
-        if (abi_sp)
+        if (abi_sp) {
           abi_sp->AugmentRegisterInfo(reg_info);
+          // AugmentRegisterInfo() can modify alt_name
+          if (reg_info.alt_name)
+            alt_name.SetCString(reg_info.alt_name);
+        }
         dyn_reg_info.AddRegister(reg_info, reg_name, alt_name, set_name);
 
         return true; // Keep iterating through all "reg" elements
       });
   return true;
 }
 
 } // namespace

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])
 
         # test GPRs
         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",
+                   ["sp = 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