[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

174–176

ASSERT_NE(reg, nullptr);

[mgorny]

lldb/unittests/Target/DynamicRegisterInfoTest.cpp

174–176

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

174–176

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(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(std::move(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(
+    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);
+
+    if (!reg.value_regs.empty())
+      m_value_regs_map[local_regnum] = std::move(reg.value_regs);
+    if (!reg.invalidate_regs.empty())
+      m_invalidate_regs_map[local_regnum] = std::move(reg.invalidate_regs);
+
+    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);

lldb/unittests/Target/DynamicRegisterInfoTest.cpp

TEST_F(DynamicRegisterInfoTest, no_finalize_regs) {
   ASSERT_REG(s2, LLDB_INVALID_INDEX32);
   ASSERT_REG(i1, LLDB_INVALID_INDEX32);
   ASSERT_REG(i2, LLDB_INVALID_INDEX32);
 }
 
 class DynamicRegisterInfoRegisterTest : public ::testing::Test {
 protected:
   std::vector<DynamicRegisterInfo::Register> m_regs;
+  DynamicRegisterInfo m_dyninfo;
 
   uint32_t AddTestRegister(
       const char *name, const char *group, uint32_t byte_size,
       std::function<void(const DynamicRegisterInfo::Register &)> adder,
       std::vector<uint32_t> value_regs = {},
       std::vector<uint32_t> invalidate_regs = {}) {
-    DynamicRegisterInfo::Register new_reg{
-        ConstString(name),     ConstString(),
-        ConstString(group),    byte_size,
-        LLDB_INVALID_INDEX32,  lldb::eEncodingUint,
-        lldb::eFormatUnsigned, LLDB_INVALID_REGNUM,
-        LLDB_INVALID_REGNUM,   LLDB_INVALID_REGNUM,
-        LLDB_INVALID_REGNUM,   value_regs,
+    DynamicRegisterInfo::Register new_reg{ConstString(name),
+                                          ConstString(),
+                                          ConstString(group),
+                                          byte_size,
+                                          LLDB_INVALID_INDEX32,
+                                          lldb::eEncodingUint,
+                                          lldb::eFormatUnsigned,
+                                          LLDB_INVALID_REGNUM,
+                                          LLDB_INVALID_REGNUM,
+                                          LLDB_INVALID_REGNUM,
+                                          static_cast<uint32_t>(m_regs.size()),
+                                          value_regs,
                                           invalidate_regs};
     adder(new_reg);
     return m_regs.size() - 1;
   }
 
   void ExpectInRegs(uint32_t reg_num, const char *reg_name,
                     std::vector<uint32_t> value_regs,
                     std::vector<uint32_t> invalidate_regs) {
     ASSERT_GT(m_regs.size(), reg_num);
 
     const DynamicRegisterInfo::Register &reg = m_regs[reg_num];
     ConstString expected_reg_name{reg_name};
     EXPECT_EQ(reg.name, expected_reg_name);
     EXPECT_EQ(reg.value_regs, value_regs);
     EXPECT_EQ(reg.invalidate_regs, invalidate_regs);
   }
+
+  void ExpectInDynInfo(uint32_t reg_num, const char *reg_name,
+                       uint32_t byte_offset,
+                       std::vector<uint32_t> value_regs = {},
+                       std::vector<uint32_t> invalidate_regs = {}) {
+    const RegisterInfo *reg = m_dyninfo.GetRegisterInfoAtIndex(reg_num);
+    ASSERT_NE(reg, nullptr);
+    EXPECT_STREQ(reg->name, reg_name);
+    EXPECT_EQ(reg->byte_offset, byte_offset);
+    EXPECT_THAT(regs_to_vector(reg->value_regs), value_regs);
+    EXPECT_THAT(regs_to_vector(reg->invalidate_regs), invalidate_regs);
+  }
 };
 
 #define EXPECT_IN_REGS(reg, ...)                                               \
   {                                                                            \
     SCOPED_TRACE("at register " #reg);                                         \
     ExpectInRegs(reg, #reg, __VA_ARGS__);                                      \
   }
 
+#define EXPECT_IN_DYNINFO(reg, ...)                                            \
+  {                                                                            \
+    SCOPED_TRACE("at register " #reg);                                         \
+    ExpectInDynInfo(reg, #reg, __VA_ARGS__);                                   \
+  }
+
 TEST_F(DynamicRegisterInfoRegisterTest, addSupplementaryRegister) {
   // Add a base register
   uint32_t rax = AddTestRegister(
       "rax", "group", 8,
       [this](const DynamicRegisterInfo::Register &r) { m_regs.push_back(r); });
 
   // Add supplementary registers
   auto suppl_adder = [this](const DynamicRegisterInfo::Register &r) {
     addSupplementaryRegister(m_regs, r);
   };
   uint32_t eax = AddTestRegister("eax", "supplementary", 4, suppl_adder, {rax});
   uint32_t ax = AddTestRegister("ax", "supplementary", 2, suppl_adder, {rax});
   uint32_t al = AddTestRegister("al", "supplementary", 1, suppl_adder, {rax});
 
   EXPECT_IN_REGS(rax, {}, {eax, ax, al});
   EXPECT_IN_REGS(eax, {rax}, {rax, ax, al});
   EXPECT_IN_REGS(ax, {rax}, {rax, eax, al});
   EXPECT_IN_REGS(al, {rax}, {rax, eax, ax});
 }
+
+TEST_F(DynamicRegisterInfoRegisterTest, SetRegisterInfo) {
+  auto adder = [this](const DynamicRegisterInfo::Register &r) {
+    m_regs.push_back(r);
+  };
+  // Add regular registers
+  uint32_t b1 = AddTestRegister("b1", "base", 8, adder);
+  uint32_t b2 = AddTestRegister("b2", "other", 8, adder);
+
+  // Add a few sub-registers
+  uint32_t s1 = AddTestRegister("s1", "base", 4, adder, {b1});
+  uint32_t s2 = AddTestRegister("s2", "other", 4, adder, {b2});
+
+  // Add a register with invalidate_regs
+  uint32_t i1 = AddTestRegister("i1", "third", 8, adder, {}, {b1});
+
+  // Add a register with indirect invalidate regs to be expanded
+  // TODO: why is it done conditionally to value_regs?
+  uint32_t i2 = AddTestRegister("i2", "third", 4, adder, {b2}, {i1});
+
+  EXPECT_EQ(m_dyninfo.SetRegisterInfo(std::move(m_regs), ArchSpec()),
+            m_regs.size());
+  EXPECT_IN_DYNINFO(b1, 0, {}, {});
+  EXPECT_IN_DYNINFO(b2, 8, {}, {});
+  EXPECT_IN_DYNINFO(s1, 0, {b1}, {});
+  EXPECT_IN_DYNINFO(s2, 8, {b2}, {});
+  EXPECT_IN_DYNINFO(i1, 16, {}, {b1});
+  EXPECT_IN_DYNINFO(i2, 8, {b2}, {b1, i1});
+}