[lldb] [ABI] Apply AugmentRegisterInfo() to DynamicRegisterInfo::Registers

[mgorny]

Summary

Call ABI::AugmentRegisterInfo() once with a vector of all defined
registers rather than calling it for every individual register. Move
and rename RemoteRegisterInfo from gdb-remote to
DynamicRegisterInfo::Register, and use this class when augmenting
registers.

[mgorny]

Add one missing call to AugmentRegisterInfo() before Finalize().

[mgorny]

Pass a vector of RemoteRegisterInfos instead of DynamicRegisterInfo class.

[labath]

I like everything except the class name. :)

I don't think it makes sense in the new location, and ideally I'd like to also use this for other plugins in the future (so that e.g. core file plugins don't have to excruciatingly list everything). Maybe we could move it into the the DynamicRegisterInfo class (as the intended use is to convert it to that eventually), and give it some generic name like Register or Descriptor or RegisterDescriptor ?

lldb/include/lldb/Target/DynamicRegisterInfo.h

80–82

What's up with this?

[mgorny]

lldb/include/lldb/Target/DynamicRegisterInfo.h

80–82

Probably leftover from the previous version. I need to double-check if no other commit requires that.

[mgorny]

Move/rename RemoteRegisterInfo to DynamicRegisterInfo::Register. Remove leftover DRI changes.

lldb/include/lldb/Target/ABI.h

 //===-- ABI.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_TARGET_ABI_H
 #define LLDB_TARGET_ABI_H
 
 #include "lldb/Core/PluginInterface.h"
 #include "lldb/Symbol/UnwindPlan.h"
+#include "lldb/Target/RemoteRegisterInfo.h"
 #include "lldb/Utility/Status.h"
 #include "lldb/lldb-private.h"
 
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/MC/MCRegisterInfo.h"
 
 namespace llvm {
 class Type;
public:
   /// plug-ins would strip those bits.
   /// @{
   virtual lldb::addr_t FixCodeAddress(lldb::addr_t pc) { return pc; }
   virtual lldb::addr_t FixDataAddress(lldb::addr_t pc) { return pc; }
   /// @}
 
   llvm::MCRegisterInfo &GetMCRegisterInfo() { return *m_mc_register_info_up; }
 
-  virtual void AugmentRegisterInfo(RegisterInfo &info) = 0;
+  virtual void AugmentRegisterInfo(std::vector<RemoteRegisterInfo> &regs) = 0;
 
   virtual bool GetPointerReturnRegister(const char *&name) { return false; }
 
   static lldb::ABISP FindPlugin(lldb::ProcessSP process_sp, const ArchSpec &arch);
 
 protected:
   ABI(lldb::ProcessSP process_sp, std::unique_ptr<llvm::MCRegisterInfo> info_up)
       : m_process_wp(process_sp), m_mc_register_info_up(std::move(info_up)) {

 
 private:
   ABI(const ABI &) = delete;
   const ABI &operator=(const ABI &) = delete;
 };
 
 class RegInfoBasedABI : public ABI {
 public:
-  void AugmentRegisterInfo(RegisterInfo &info) override;
+  void AugmentRegisterInfo(std::vector<RemoteRegisterInfo> &regs) override;
 
 protected:
   using ABI::ABI;
 
   bool GetRegisterInfoByName(llvm::StringRef name, RegisterInfo &info);
 
   virtual const RegisterInfo *GetRegisterInfoArray(uint32_t &count) = 0;
 };
 
 class MCBasedABI : public ABI {
 public:
-  void AugmentRegisterInfo(RegisterInfo &info) override;
+  void AugmentRegisterInfo(std::vector<RemoteRegisterInfo> &regs) override;
 
   /// If the register name is of the form "<from_prefix>[<number>]" then change
   /// the name to "<to_prefix>[<number>]". Otherwise, leave the name unchanged.
   static void MapRegisterName(std::string &reg, llvm::StringRef from_prefix,
                llvm::StringRef to_prefix);
 protected:
   using ABI::ABI;
 

lldb/include/lldb/Target/DynamicRegisterInfo.h

public:
 
   void Clear();
 
   bool IsReconfigurable();
 
   const lldb_private::RegisterInfo *
   GetRegisterInfo(llvm::StringRef reg_name) const;
 
+  lldb_private::RegisterInfo *
+  GetRegisterInfo(llvm::StringRef reg_name);
+
   typedef std::vector<lldb_private::RegisterInfo> reg_collection;
   llvm::iterator_range<reg_collection::const_iterator> registers() const {
     return llvm::iterator_range<reg_collection::const_iterator>(m_regs);
   }
 
 protected:
   // Classes that inherit from DynamicRegisterInfo can see and modify these
   typedef std::vector<lldb_private::RegisterSet> set_collection;

lldb/include/lldb/Target/RemoteRegisterInfo.h

+//===-- RemoteRegisterInfo.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_TARGET_REMOTEREGISTERINFO_H
+#define LLDB_TARGET_REMOTEREGISTERINFO_H
+
+#include "lldb/Utility/ConstString.h"
+#include "lldb/lldb-private.h"
+
+#include <vector>
+
+namespace lldb_private {
+
+struct RemoteRegisterInfo {
+  ConstString name;
+  ConstString alt_name;
+  ConstString set_name;
+  uint32_t byte_size = LLDB_INVALID_INDEX32;
+  uint32_t byte_offset = LLDB_INVALID_INDEX32;
+  lldb::Encoding encoding = lldb::eEncodingUint;
+  lldb::Format format = lldb::eFormatHex;
+  uint32_t regnum_dwarf = LLDB_INVALID_REGNUM;
+  uint32_t regnum_ehframe = LLDB_INVALID_REGNUM;
+  uint32_t regnum_generic = LLDB_INVALID_REGNUM;
+  uint32_t regnum_remote = LLDB_INVALID_REGNUM;
+  std::vector<uint32_t> value_regs;
+  std::vector<uint32_t> invalidate_regs;
+};
+
+} // namespace lldb_private
+
+#endif // LLDB_TARGET_REMOTEREGISTERINFO_H

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 {
+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;
+  void AugmentRegisterInfo(
+      std::vector<lldb_private::RemoteRegisterInfo> &regs) override;
 
   using lldb_private::MCBasedABI::MCBasedABI;
 };
 #endif

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

return llvm::StringSwitch<uint32_t>(name)
       .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);
+void ABIAArch64::AugmentRegisterInfo(
+    std::vector<lldb_private::RemoteRegisterInfo> &regs) {
+  lldb_private::MCBasedABI::AugmentRegisterInfo(regs);
 
+  lldb_private::ConstString sp_string{"sp"};
+  for (lldb_private::RemoteRegisterInfo &info : regs) {
     // GDB sends x31 as "sp".  Add the "x31" alt_name for convenience.
-  if (!strcmp(info.name, "sp") && !info.alt_name)
-    info.alt_name = "x31";
+    if (info.name == sp_string && !info.alt_name)
+      info.alt_name.SetCString("x31");
+  }
 }

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

 #include <string>
 #include <vector>
 
 #include "lldb/Core/LoadedModuleInfoList.h"
 #include "lldb/Core/ModuleSpec.h"
 #include "lldb/Core/ThreadSafeValue.h"
 #include "lldb/Host/HostThread.h"
 #include "lldb/Target/Process.h"
+#include "lldb/Target/RemoteRegisterInfo.h"
 #include "lldb/Target/Thread.h"
 #include "lldb/Utility/ArchSpec.h"
 #include "lldb/Utility/Broadcaster.h"
 #include "lldb/Utility/ConstString.h"
 #include "lldb/Utility/GDBRemote.h"
 #include "lldb/Utility/Status.h"
 #include "lldb/Utility/StreamString.h"
 #include "lldb/Utility/StringExtractor.h"
 #include "lldb/Utility/StringList.h"
 #include "lldb/Utility/StructuredData.h"
 #include "lldb/lldb-private-forward.h"
 
 #include "GDBRemoteCommunicationClient.h"
 #include "GDBRemoteCommunicationReplayServer.h"
 #include "GDBRemoteRegisterContext.h"
 
 #include "llvm/ADT/DenseMap.h"
 
 namespace lldb_private {
 namespace repro {
 class Loader;
 }
 namespace process_gdb_remote {
 
-struct RemoteRegisterInfo {
-  ConstString name;
-  ConstString alt_name;
-  ConstString set_name;
-  uint32_t byte_size = LLDB_INVALID_INDEX32;
-  uint32_t byte_offset = LLDB_INVALID_INDEX32;
-  lldb::Encoding encoding = lldb::eEncodingUint;
-  lldb::Format format = lldb::eFormatHex;
-  uint32_t regnum_dwarf = LLDB_INVALID_REGNUM;
-  uint32_t regnum_ehframe = LLDB_INVALID_REGNUM;
-  uint32_t regnum_generic = LLDB_INVALID_REGNUM;
-  uint32_t regnum_remote = LLDB_INVALID_REGNUM;
-  std::vector<uint32_t> value_regs;
-  std::vector<uint32_t> invalidate_regs;
-};
-
 class ThreadGDBRemote;
 
 class ProcessGDBRemote : public Process,
                          private GDBRemoteClientBase::ContinueDelegate {
 public:
   ProcessGDBRemote(lldb::TargetSP target_sp, lldb::ListenerSP listener_sp);
 
   ~ProcessGDBRemote() override;

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

if (arch_to_use.IsValid() &&
          arch_to_use.GetMachine() == llvm::Triple::thumb) &&
         arch_to_use.GetTriple().getVendor() == llvm::Triple::Apple)
       m_register_info_sp->HardcodeARMRegisters(from_scratch);
   } else if (target_arch.GetMachine() == llvm::Triple::arm ||
              target_arch.GetMachine() == llvm::Triple::thumb) {
     m_register_info_sp->HardcodeARMRegisters(from_scratch);
   }
 
-  // At this point, we can finalize our register info.
   m_register_info_sp->Finalize(GetTarget().GetArchitecture());
 }
 
 Status ProcessGDBRemote::WillLaunch(lldb_private::Module *module) {
   return WillLaunchOrAttach();
 }
 
 Status ProcessGDBRemote::WillAttachToProcessWithID(lldb::pid_t pid) {
bool ProcessGDBRemote::GetGDBServerRegisterInfoXMLAndProcess(
   } else {
     return false;
   }
   return true;
 }
 
 void ProcessGDBRemote::AddRemoteRegisters(
     std::vector<RemoteRegisterInfo> &registers, const ArchSpec &arch_to_use) {
-  // Don't use Process::GetABI, this code gets called from DidAttach, and
-  // in that context we haven't set the Target's architecture yet, so the
-  // ABI is also potentially incorrect.
-  ABISP abi_sp = ABI::FindPlugin(shared_from_this(), arch_to_use);
-
   std::map<uint32_t, uint32_t> remote_to_local_map;
   uint32_t remote_regnum = 0;
   for (auto it : llvm::enumerate(registers)) {
     RemoteRegisterInfo &remote_reg_info = it.value();
 
     // Assign successive remote regnums if missing.
     if (remote_reg_info.regnum_remote == LLDB_INVALID_REGNUM)
       remote_reg_info.regnum_remote = remote_regnum;
 
     // Create a mapping from remote to local regnos.
     remote_to_local_map[remote_reg_info.regnum_remote] = it.index();
 
     remote_regnum = remote_reg_info.regnum_remote + 1;
   }
 
-  for (auto it : llvm::enumerate(registers)) {
-    uint32_t local_regnum = it.index();
-    RemoteRegisterInfo &remote_reg_info = it.value();
-
+  for (RemoteRegisterInfo &remote_reg_info : registers) {
     auto proc_to_lldb = [&remote_to_local_map](uint32_t process_regnum) {
       auto lldb_regit = remote_to_local_map.find(process_regnum);
       return lldb_regit != remote_to_local_map.end() ? lldb_regit->second
                                                      : LLDB_INVALID_REGNUM;
     };
 
     llvm::transform(remote_reg_info.value_regs,
                     remote_reg_info.value_regs.begin(), proc_to_lldb);
     llvm::transform(remote_reg_info.invalidate_regs,
                     remote_reg_info.invalidate_regs.begin(), proc_to_lldb);
+  }
+
+  // Don't use Process::GetABI, this code gets called from DidAttach, and
+  // in that context we haven't set the Target's architecture yet, so the
+  // ABI is also potentially incorrect.
+  if (ABISP abi_sp = ABI::FindPlugin(shared_from_this(), arch_to_use))
+    abi_sp->AugmentRegisterInfo(registers);
+
+  for (auto it : llvm::enumerate(registers)) {
+    uint32_t local_regnum = it.index();
+    RemoteRegisterInfo &remote_reg_info = it.value();
 
     auto regs_with_sentinel = [](std::vector<uint32_t> &vec) -> uint32_t * {
       if (!vec.empty()) {
         vec.push_back(LLDB_INVALID_REGNUM);
         return vec.data();
       }
       return nullptr;
     };
 
     struct RegisterInfo reg_info {
       remote_reg_info.name.AsCString(), remote_reg_info.alt_name.AsCString(),
           remote_reg_info.byte_size, remote_reg_info.byte_offset,
           remote_reg_info.encoding, remote_reg_info.format,
           {remote_reg_info.regnum_ehframe, remote_reg_info.regnum_dwarf,
            remote_reg_info.regnum_generic, remote_reg_info.regnum_remote,
            local_regnum},
           regs_with_sentinel(remote_reg_info.value_regs),
           regs_with_sentinel(remote_reg_info.invalidate_regs),
     };
-
-    if (abi_sp)
-      abi_sp->AugmentRegisterInfo(reg_info);
     m_register_info_sp->AddRegister(reg_info, remote_reg_info.set_name);
   };
 
   m_register_info_sp->Finalize(arch_to_use);
 }
 
 // query the target of gdb-remote for extended target information returns
 // true on success (got register definitions), false on failure (did not).

lldb/source/Target/ABI.cpp

if (!target) {
     return nullptr;
   }
   std::unique_ptr<llvm::MCRegisterInfo> info_up(
       target->createMCRegInfo(triple));
   assert(info_up);
   return info_up;
 }
 
-void RegInfoBasedABI::AugmentRegisterInfo(RegisterInfo &info) {
-  if (info.kinds[eRegisterKindEHFrame] != LLDB_INVALID_REGNUM &&
-      info.kinds[eRegisterKindDWARF] != LLDB_INVALID_REGNUM)
-    return;
+void RegInfoBasedABI::AugmentRegisterInfo(
+    std::vector<RemoteRegisterInfo> &regs) {
+  for (RemoteRegisterInfo &info : regs) {
+    if (info.regnum_ehframe != LLDB_INVALID_REGNUM &&
+        info.regnum_dwarf != LLDB_INVALID_REGNUM)
+      continue;
 
     RegisterInfo abi_info;
-  if (!GetRegisterInfoByName(info.name, abi_info))
-    return;
+    if (!GetRegisterInfoByName(info.name.GetStringRef(), abi_info))
+      continue;
 
-  if (info.kinds[eRegisterKindEHFrame] == LLDB_INVALID_REGNUM)
-    info.kinds[eRegisterKindEHFrame] = abi_info.kinds[eRegisterKindEHFrame];
-  if (info.kinds[eRegisterKindDWARF] == LLDB_INVALID_REGNUM)
-    info.kinds[eRegisterKindDWARF] = abi_info.kinds[eRegisterKindDWARF];
-  if (info.kinds[eRegisterKindGeneric] == LLDB_INVALID_REGNUM)
-    info.kinds[eRegisterKindGeneric] = abi_info.kinds[eRegisterKindGeneric];
+    if (info.regnum_ehframe == LLDB_INVALID_REGNUM)
+      info.regnum_ehframe = abi_info.kinds[eRegisterKindEHFrame];
+    if (info.regnum_dwarf == LLDB_INVALID_REGNUM)
+      info.regnum_dwarf = abi_info.kinds[eRegisterKindDWARF];
+    if (info.regnum_generic == LLDB_INVALID_REGNUM)
+      info.regnum_generic = abi_info.kinds[eRegisterKindGeneric];
+  }
 }
 
-void MCBasedABI::AugmentRegisterInfo(RegisterInfo &info) {
+void MCBasedABI::AugmentRegisterInfo(std::vector<RemoteRegisterInfo> &regs) {
+  for (RemoteRegisterInfo &info : regs) {
     uint32_t eh, dwarf;
-  std::tie(eh, dwarf) = GetEHAndDWARFNums(info.name);
+    std::tie(eh, dwarf) = GetEHAndDWARFNums(info.name.GetStringRef());
 
-  if (info.kinds[eRegisterKindEHFrame] == LLDB_INVALID_REGNUM)
-    info.kinds[eRegisterKindEHFrame] = eh;
-  if (info.kinds[eRegisterKindDWARF] == LLDB_INVALID_REGNUM)
-    info.kinds[eRegisterKindDWARF] = dwarf;
-  if (info.kinds[eRegisterKindGeneric] == LLDB_INVALID_REGNUM)
-    info.kinds[eRegisterKindGeneric] = GetGenericNum(info.name);
+    if (info.regnum_ehframe == LLDB_INVALID_REGNUM)
+      info.regnum_ehframe = eh;
+    if (info.regnum_dwarf == LLDB_INVALID_REGNUM)
+      info.regnum_dwarf = dwarf;
+    if (info.regnum_generic == LLDB_INVALID_REGNUM)
+      info.regnum_generic = GetGenericNum(info.name.GetStringRef());
+  }
 }
 
 std::pair<uint32_t, uint32_t>
 MCBasedABI::GetEHAndDWARFNums(llvm::StringRef name) {
   std::string mc_name = GetMCName(name.str());
   for (char &c : mc_name)
     c = std::toupper(c);
   int eh = -1;

lldb/source/Target/DynamicRegisterInfo.cpp

 
 const lldb_private::RegisterInfo *
 DynamicRegisterInfo::GetRegisterInfo(llvm::StringRef reg_name) const {
   for (auto &reg_info : m_regs)
     if (reg_info.name == reg_name)
       return &reg_info;
   return nullptr;
 }
+
+lldb_private::RegisterInfo *
+DynamicRegisterInfo::GetRegisterInfo(llvm::StringRef reg_name) {
+  for (auto &reg_info : m_regs)
+    if (reg_info.name == reg_name)
+      return &reg_info;
+  return nullptr;
+}