[lldb] [DynamicRegisterInfo] Add a convenience method to add suppl. registers

[mgorny]

Summary

Add a convenience method to add supplementary registers that takes care
of adding invalidate_regs to all (potentially) overlapping registers.

[mgorny]

Currently it's still using process plugin regnums in value_regs/invalidate_regs; I'm going to try changing that next.

[mgorny]

clang-format

[mgorny]

Update for using local (LLDB) regnos.

[mgorny]

lldb/source/Plugins/Process/Utility/DynamicRegisterInfo.cpp

448

@labath, any suggestion how to do this nicely while not copying the terminating LLDB_INVALID_REGNUM?

[labath]

lldb/source/Plugins/Process/Utility/DynamicRegisterInfo.cpp

448

How about by making sure that LLDB_INVALID_REGNUM is not present in the firstplace (by adding it only during finalization)?

[mgorny]

Rebased. Updated the code to assume dedupe and cleanup happens in Finalize(), and to use new test assertions.

[labath]

I think this is fine. The tricky thing will be deciding what to do with the x86 and arm registers which start at non-zero offsets (ah et al.)

One thing I was considering was doing this addition while were still in the original "vector of strings" form instead of this C thingy. The tricky part there is that (since this in would be the ABI classes which do this manipulation) we would need to expose the gdb-remote struct to the outside world. I don't think that would be necessarily bad (we just wouldn't call it "RemoteRegisterInfo, but something else), but it's not clear to me whether its worth the churn. Still, I think it's worth keeping this in the back of your mind as you work on this.

lldb/source/Plugins/Process/Utility/DynamicRegisterInfo.cpp

439

Maybe call this new_invalidates? I've found it hard to track what to_add means, with all the mixing of value_regs and invalidates...

441–442

Is this still needed?

455

This would be a good use case for (const) auto, as value_type does not say much anyway.

lldb/unittests/Process/Utility/DynamicRegisterInfoTest.cpp

159–165

Could we remove ah from this test, as its offset is going to be wrong?

[mgorny]

lldb/source/Plugins/Process/Utility/DynamicRegisterInfo.cpp

439

Good idea.

441–442

Probably not indeed.

lldb/unittests/Process/Utility/DynamicRegisterInfoTest.cpp

159–165

Sure.

lldb/source/Plugins/Process/Utility/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;
 

lldb/source/Plugins/Process/Utility/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());

lldb/unittests/Process/Utility/DynamicRegisterInfoTest.cpp

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);
+
+  // 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, nullptr, 0
+  };
+  info.AddSupplementaryRegister(eax_reg, group);
+
+  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, nullptr, 0
+  };
+  info.AddSupplementaryRegister(ax_reg, group);
+
+  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, nullptr, 0
+  };
+  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});
+}