[lldb] [DynamicRegisterInfo] Support setting from vector<Register>

[mgorny]

Summary

Add an overload of DynamicRegisterInfo::SetRegisterInfo() that accepts
a std::vector<Register> as an argument. This moves the conversion
from DRI::Register to RegisterInfo directly into DynamicRegisterInfo,
and avoids the necessity of creating fully-compatible intermediate
RegisterInfo instances.

While the new method could technically reuse AddRegister(), the ultimate
goal is to replace AddRegister() with SetRegisterInfo() entirely.

[mgorny]

Add a test.

[labath]

lldb/source/Target/DynamicRegisterInfo.cpp

394–395

m_value_regs_map[local_regnum] = reg.value_regs ?

Since I think the callers have no used for the input vector after this call, you might even try to take it by rvalue reference and then move these subvectors in place

lldb/unittests/Target/DynamicRegisterInfoTest.cpp

176–178 ↗

(On Diff #378443)

ASSERT_NE(reg, nullptr);

[mgorny]

lldb/unittests/Target/DynamicRegisterInfoTest.cpp

176–178 ↗

(On Diff #378443)

Actually, the idea was to test all registers even if one failed; though I suppose since we're now starting with the lowest index, higher indices won't work either.

[labath]

lldb/unittests/Target/DynamicRegisterInfoTest.cpp

176–178 ↗

(On Diff #378443)

I'll let you in on a dirty secret. They only difference between EXPECT_xxx and ASSERT_xxx is that the ASSERT version does a return; in case of failure. So it does not actually terminate the test -- just the current function.

That's why the recommended practice is to use gtest macros at the top level (in the TEST function), but I've now given up on getting people to do that.

[mgorny]

Use fancy move semantics and ASSERT_NE.

lldb/include/lldb/Target/DynamicRegisterInfo.h

public:
   virtual ~DynamicRegisterInfo() = default;
 
   DynamicRegisterInfo(DynamicRegisterInfo &&info);
   DynamicRegisterInfo &operator=(DynamicRegisterInfo &&info);
 
   size_t SetRegisterInfo(const lldb_private::StructuredData::Dictionary &dict,
                          const lldb_private::ArchSpec &arch);
 
+  size_t SetRegisterInfo(const std::vector<Register> &regs,
+                         const lldb_private::ArchSpec &arch);
+
   void AddRegister(lldb_private::RegisterInfo reg_info,
                    lldb_private::ConstString &set_name);
 
   void Finalize(const lldb_private::ArchSpec &arch);
 
   size_t GetNumRegisters() const;
 
   size_t GetNumRegisterSets() const;
 
   size_t GetRegisterDataByteSize() const;
 
   const lldb_private::RegisterInfo *GetRegisterInfoAtIndex(uint32_t i) const;
 
   const lldb_private::RegisterSet *GetRegisterSet(uint32_t i) const;
 
-  uint32_t GetRegisterSetIndexByName(lldb_private::ConstString &set_name,
+  uint32_t GetRegisterSetIndexByName(const lldb_private::ConstString &set_name,
                                      bool can_create);
 
   uint32_t ConvertRegisterKindToRegisterNumber(uint32_t kind,
                                                uint32_t num) const;
 
   const lldb_private::RegisterInfo *GetRegisterInfo(uint32_t kind,
                                                     uint32_t num) const;
 

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

void ProcessGDBRemote::AddRemoteRegisters(
   }
 
   // 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();
-    DynamicRegisterInfo::Register &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),
-    };
-    m_register_info_sp->AddRegister(reg_info, remote_reg_info.set_name);
-  };
-
-  m_register_info_sp->Finalize(arch_to_use);
+  m_register_info_sp->SetRegisterInfo(registers, 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).
 bool ProcessGDBRemote::GetGDBServerRegisterInfo(ArchSpec &arch_to_use) {
   // Make sure LLDB has an XML parser it can use first
   if (!XMLDocument::XMLEnabled())
     return false;

lldb/source/Target/DynamicRegisterInfo.cpp

for (uint32_t i = 0; i < num_regs; ++i) {
 
     m_regs.push_back(reg_info);
     m_set_reg_nums[set].push_back(i);
   }
   Finalize(arch);
   return m_regs.size();
 }
 
+size_t DynamicRegisterInfo::SetRegisterInfo(
+    const std::vector<DynamicRegisterInfo::Register> &regs,
+    const ArchSpec &arch) {
+  assert(!m_finalized);
+
+  for (auto it : llvm::enumerate(regs)) {
+    uint32_t local_regnum = it.index();
+    const DynamicRegisterInfo::Register &reg = it.value();
+
+    assert(reg.name);
+    assert(reg.set_name);
+
+    for (uint32_t value_reg : reg.value_regs)
+      m_value_regs_map[local_regnum].push_back(value_reg);
+    for (uint32_t invalidate_reg : reg.invalidate_regs)
+      m_invalidate_regs_map[local_regnum].push_back(invalidate_reg);
+
+    struct RegisterInfo reg_info {
+      reg.name.AsCString(), reg.alt_name.AsCString(), reg.byte_size,
+          reg.byte_offset, reg.encoding, reg.format,
+          {reg.regnum_ehframe, reg.regnum_dwarf, reg.regnum_generic,
+           reg.regnum_remote, local_regnum},
+          // value_regs and invalidate_regs are filled by Finalize()
+          nullptr, nullptr
+    };
+
+    m_regs.push_back(reg_info);
+
+    uint32_t set = GetRegisterSetIndexByName(reg.set_name, true);
+    assert(set < m_sets.size());
+    assert(set < m_set_reg_nums.size());
+    assert(set < m_set_names.size());
+    m_set_reg_nums[set].push_back(local_regnum);
+  };
+
+  Finalize(arch);
+  return m_regs.size();
+}
+
 void DynamicRegisterInfo::AddRegister(RegisterInfo reg_info,
                                       ConstString &set_name) {
   assert(!m_finalized);
   const uint32_t reg_num = m_regs.size();
   assert(reg_info.name);
   uint32_t i;
   if (reg_info.value_regs) {
     for (i = 0; reg_info.value_regs[i] != LLDB_INVALID_REGNUM; ++i)

 }
 
 const RegisterSet *DynamicRegisterInfo::GetRegisterSet(uint32_t i) const {
   if (i < m_sets.size())
     return &m_sets[i];
   return nullptr;
 }
 
-uint32_t DynamicRegisterInfo::GetRegisterSetIndexByName(ConstString &set_name,
-                                                        bool can_create) {
+uint32_t
+DynamicRegisterInfo::GetRegisterSetIndexByName(const ConstString &set_name,
   name_collection::iterator pos, end = m_set_names.end();
   for (pos = m_set_names.begin(); pos != end; ++pos) {
     if (*pos == set_name)
       return std::distance(m_set_names.begin(), pos);
   }
 
   m_set_names.push_back(set_name);
   m_set_reg_nums.resize(m_set_reg_nums.size() + 1);