[lldb] [Target] Make addSupplementaryRegister() work on Register vector

[mgorny]

Summary

Move DynamicRegisterInfo::AddSupplementaryRegister() into a standalone
function working on std::vector<DynamicRegisterInfo::Register>.

[labath]

Looks good. I'm not sure about the RemoteRegisterInfo name, but that's a topic for the patch that introduces it.

[mgorny]

Rebase after renaming RRI to DRI::Register.

[mgorny]

Remove duplicate ASSERT_REG.

[mgorny]

Add m_ prefix to vars in DynamicRegisterInfoRegisterTest class.

[mgorny]

Make AssertRegisterInfo() args non-optional.

lldb/include/lldb/Target/DynamicRegisterInfo.h

public:
   DynamicRegisterInfo &operator=(DynamicRegisterInfo &&info);
 
   size_t SetRegisterInfo(const lldb_private::StructuredData::Dictionary &dict,
                          const lldb_private::ArchSpec &arch);
 
   void AddRegister(lldb_private::RegisterInfo reg_info,
                    lldb_private::ConstString &set_name);
 
-  // Add a new register and cross-link it via invalidate_regs with other
-  // registers sharing its value_regs.
-  void AddSupplementaryRegister(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;
 
protected:
   reg_to_regs_map m_value_regs_map;
   reg_to_regs_map m_invalidate_regs_map;
   size_t m_reg_data_byte_size = 0u; // The number of bytes required to store
                                     // all registers
   bool m_finalized = false;
   bool m_is_reconfigurable = false;
 };
 
+void addSupplementaryRegister(std::vector<DynamicRegisterInfo::Register> &regs,
+                              DynamicRegisterInfo::Register new_reg_info);
+
 } // namespace lldb_private
 
 #endif // LLDB_TARGET_DYNAMICREGISTERINFO_H

lldb/source/Target/DynamicRegisterInfo.cpp

void DynamicRegisterInfo::AddRegister(RegisterInfo reg_info,
   m_regs.push_back(reg_info);
   uint32_t set = GetRegisterSetIndexByName(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(reg_num);
 }
 
-void DynamicRegisterInfo::AddSupplementaryRegister(RegisterInfo new_reg_info,
-                                                   ConstString &set_name) {
-  assert(new_reg_info.value_regs != nullptr);
-  const uint32_t reg_num = m_regs.size();
-  AddRegister(new_reg_info, set_name);
-
-  reg_to_regs_map new_invalidates;
-  for (uint32_t value_reg : m_value_regs_map[reg_num]) {
-    // copy value_regs to invalidate_regs
-    new_invalidates[reg_num].push_back(value_reg);
-
-    // copy invalidate_regs from the parent register
-    llvm::append_range(new_invalidates[reg_num], m_invalidate_regs_map[value_reg]);
-
-    // add reverse invalidate entries
-    for (uint32_t x : new_invalidates[reg_num])
-      new_invalidates[x].push_back(new_reg_info.kinds[eRegisterKindLLDB]);
-  }
-
-  for (const auto &x : new_invalidates)
-    llvm::append_range(m_invalidate_regs_map[x.first], x.second);
-}
-
 void DynamicRegisterInfo::Finalize(const ArchSpec &arch) {
   if (m_finalized)
     return;
 
   m_finalized = true;
   const size_t num_sets = m_sets.size();
   for (size_t set = 0; set < num_sets; ++set) {
     assert(m_sets.size() == m_set_reg_nums.size());

 
 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;
 }
+
+void lldb_private::addSupplementaryRegister(
+    std::vector<DynamicRegisterInfo::Register> &regs,
+    DynamicRegisterInfo::Register new_reg_info) {
+  assert(!new_reg_info.value_regs.empty());
+  const uint32_t reg_num = regs.size();
+  regs.push_back(new_reg_info);
+
+  std::map<uint32_t, std::vector<uint32_t>> new_invalidates;
+  for (uint32_t value_reg : new_reg_info.value_regs) {
+    // copy value_regs to invalidate_regs
+    new_invalidates[reg_num].push_back(value_reg);
+
+    // copy invalidate_regs from the parent register
+    llvm::append_range(new_invalidates[reg_num],
+                       regs[value_reg].invalidate_regs);
+
+    // add reverse invalidate entries
+    for (uint32_t x : new_invalidates[reg_num])
+      new_invalidates[x].push_back(reg_num);
+  }
+
+  for (const auto &x : new_invalidates)
+    llvm::append_range(regs[x.first].invalidate_regs, x.second);
+}

lldb/unittests/Target/DynamicRegisterInfoTest.cpp

 //===-- DynamicRegisterInfoTest.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 "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 #include "lldb/Target/DynamicRegisterInfo.h"
 #include "lldb/Utility/ArchSpec.h"
 
+#include <functional>
+
 using namespace lldb_private;
 
 static std::vector<uint32_t> regs_to_vector(uint32_t *regs) {
   std::vector<uint32_t> ret;
   if (regs) {
     while (*regs != LLDB_INVALID_REGNUM)
       ret.push_back(*regs++);
   }
void AssertRegisterInfo(uint32_t reg_num, const char *reg_name,
 
     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 ASSERT_REG(reg, ...) { \
-  SCOPED_TRACE("at register " #reg); \
-  AssertRegisterInfo(reg, #reg, __VA_ARGS__); \
+#define ASSERT_REG(reg, ...)                                                   \
+  {                                                                            \
+    SCOPED_TRACE("at register " #reg);                                         \
+    AssertRegisterInfo(reg, #reg, __VA_ARGS__);                                \
   }
 
 TEST_F(DynamicRegisterInfoTest, finalize_regs) {
   // Add regular registers
   uint32_t b1 = AddTestRegister("b1", 8);
   uint32_t b2 = AddTestRegister("b2", 8);
 
   // Add a few sub-registers
TEST_F(DynamicRegisterInfoTest, no_finalize_regs) {
   ASSERT_REG(b1, LLDB_INVALID_INDEX32);
   ASSERT_REG(b2, LLDB_INVALID_INDEX32);
   ASSERT_REG(s1, LLDB_INVALID_INDEX32);
   ASSERT_REG(s2, LLDB_INVALID_INDEX32);
   ASSERT_REG(i1, LLDB_INVALID_INDEX32);
   ASSERT_REG(i2, LLDB_INVALID_INDEX32);
 }
 
-TEST_F(DynamicRegisterInfoTest, add_supplementary_register) {
-  // Add a base register
-  uint32_t rax = AddTestRegister("rax", 8);
+class DynamicRegisterInfoRegisterTest : public ::testing::Test {
+protected:
+  std::vector<DynamicRegisterInfo::Register> m_regs;
 
-  // Register numbers
-  uint32_t eax = 1;
-  uint32_t ax = 2;
-  uint32_t al = 3;
-
-  ConstString group{"supplementary registers"};
-  uint32_t value_regs[2] = {rax, LLDB_INVALID_REGNUM};
-  struct RegisterInfo eax_reg {
-    "eax", nullptr, 4, LLDB_INVALID_INDEX32, lldb::eEncodingUint,
-        lldb::eFormatUnsigned,
-        {LLDB_INVALID_REGNUM, LLDB_INVALID_REGNUM, LLDB_INVALID_REGNUM, eax,
-         eax},
-        value_regs, nullptr
-  };
-  info.AddSupplementaryRegister(eax_reg, group);
+  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,
+        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);
+  }
 
-  struct RegisterInfo ax_reg {
-    "ax", nullptr, 2, LLDB_INVALID_INDEX32, lldb::eEncodingUint,
-        lldb::eFormatUnsigned,
-        {LLDB_INVALID_REGNUM, LLDB_INVALID_REGNUM, LLDB_INVALID_REGNUM, ax, ax},
-        value_regs, nullptr
-  };
-  info.AddSupplementaryRegister(ax_reg, group);
+#define EXPECT_IN_REGS(reg, ...)                                               \
+  {                                                                            \
+    SCOPED_TRACE("at register " #reg);                                         \
+    ExpectInRegs(reg, #reg, __VA_ARGS__);                                      \
+  }
 
-  struct RegisterInfo al_reg {
-    "al", nullptr, 1, LLDB_INVALID_INDEX32, lldb::eEncodingUint,
-        lldb::eFormatUnsigned,
-        {LLDB_INVALID_REGNUM, LLDB_INVALID_REGNUM, LLDB_INVALID_REGNUM, al, al},
-        value_regs, nullptr
+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);
   };
-  info.AddSupplementaryRegister(al_reg, group);
-
-  info.Finalize(lldb_private::ArchSpec());
-
-  ASSERT_REG(rax, 0, {}, {eax, ax, al});
-  ASSERT_REG(eax, 0, {rax}, {rax, ax, al});
-  ASSERT_REG(ax, 0, {rax}, {rax, eax, al});
-  ASSERT_REG(al, 0, {rax}, {rax, eax, ax});
+  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});
 }