Fix DynamicRegisterInfo copying/moving issue

[tatyana-krasnukha]

Summary

Default copy/move constructors and assignment operators work incorrectly, leaving wrong m_sets[i].registers pointers.

Made the class movable and non-copyable (it's difficult to imagine when it needs to be copied).

[clayborg]

Only question is do we need the extra "struct Data"?

source/Plugins/Process/Utility/DynamicRegisterInfo.h

99 ↗

(On Diff #149801)

Is this struct necessary? Would be nice if we didn't have this extra layer?

[tatyana-krasnukha]

Hello Greg,

source/Plugins/Process/Utility/DynamicRegisterInfo.h

99 ↗

(On Diff #149801)

This struct helps to avoid two problems:

1. need to modify each constructor and operator= when add or remove some data from the class - easy to forget
2. duplication of constructor and operator code.

I hope this additional layer of abstraction is not too big price for safety.

[tatyana-krasnukha]

Get rid of additional structure.
Update pointers only if finalized info is passed to the function.

lldb/trunk/source/Plugins/Process/Utility/DynamicRegisterInfo.h

 // Other libraries and framework includes
 // Project includes
 #include "lldb/Utility/ConstString.h"
 #include "lldb/Utility/StructuredData.h"
 #include "lldb/lldb-private.h"
 
 class DynamicRegisterInfo {
 public:
-  DynamicRegisterInfo();
+  DynamicRegisterInfo() = default;
 
   DynamicRegisterInfo(const lldb_private::StructuredData::Dictionary &dict,
                       const lldb_private::ArchSpec &arch);
 
-  virtual ~DynamicRegisterInfo();
+  virtual ~DynamicRegisterInfo() = default;
+
+  DynamicRegisterInfo(DynamicRegisterInfo &) = delete;
+  void operator=(DynamicRegisterInfo &) = delete;
+
+  DynamicRegisterInfo(DynamicRegisterInfo &&info);
+  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 &reg_name,
                    lldb_private::ConstString &reg_alt_name,
                    lldb_private::ConstString &set_name);
protected:
   typedef std::vector<lldb_private::RegisterSet> set_collection;
   typedef std::vector<uint32_t> reg_num_collection;
   typedef std::vector<reg_num_collection> set_reg_num_collection;
   typedef std::vector<lldb_private::ConstString> name_collection;
   typedef std::map<uint32_t, reg_num_collection> reg_to_regs_map;
   typedef std::vector<uint8_t> dwarf_opcode;
   typedef std::map<uint32_t, dwarf_opcode> dynamic_reg_size_map;
 
-  lldb_private::RegisterInfo *
-  GetRegisterInfo(const lldb_private::ConstString &reg_name);
+  const lldb_private::RegisterInfo *
+  GetRegisterInfo(const lldb_private::ConstString &reg_name) const;
+
+  void MoveFrom(DynamicRegisterInfo &&info);
 
   reg_collection m_regs;
   set_collection m_sets;
   set_reg_num_collection m_set_reg_nums;
   name_collection m_set_names;
   reg_to_regs_map m_value_regs_map;
   reg_to_regs_map m_invalidate_regs_map;
   dynamic_reg_size_map m_dynamic_reg_size_map;
-  size_t m_reg_data_byte_size; // The number of bytes required to store all
-                               // registers
-  bool m_finalized;
+  size_t m_reg_data_byte_size = 0u; // The number of bytes required to store
+                                    // all registers
+  bool m_finalized = false;
 };
-
 #endif // lldb_DynamicRegisterInfo_h_

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

 #include "lldb/Utility/ArchSpec.h"
 #include "lldb/Utility/RegularExpression.h"
 #include "lldb/Utility/StringExtractor.h"
 #include "lldb/Utility/StructuredData.h"
 
 using namespace lldb;
 using namespace lldb_private;
 
-DynamicRegisterInfo::DynamicRegisterInfo()
-    : m_regs(), m_sets(), m_set_reg_nums(), m_set_names(), m_value_regs_map(),
-      m_invalidate_regs_map(), m_dynamic_reg_size_map(),
-      m_reg_data_byte_size(0), m_finalized(false) {}
-
 DynamicRegisterInfo::DynamicRegisterInfo(
     const lldb_private::StructuredData::Dictionary &dict,
-    const lldb_private::ArchSpec &arch)
-    : m_regs(), m_sets(), m_set_reg_nums(), m_set_names(), m_value_regs_map(),
-      m_invalidate_regs_map(), m_dynamic_reg_size_map(),
-      m_reg_data_byte_size(0), m_finalized(false) {
+    const lldb_private::ArchSpec &arch) {
   SetRegisterInfo(dict, arch);
 }
 
-DynamicRegisterInfo::~DynamicRegisterInfo() {}
+DynamicRegisterInfo::DynamicRegisterInfo(DynamicRegisterInfo &&info) {
+  MoveFrom(std::move(info));
+}
+
+DynamicRegisterInfo &
+DynamicRegisterInfo::operator=(DynamicRegisterInfo &&info) {
+  MoveFrom(std::move(info));
+  return *this;
+}
+
+void DynamicRegisterInfo::MoveFrom(DynamicRegisterInfo &&info) {
+  m_regs = std::move(info.m_regs);
+  m_sets = std::move(info.m_sets);
+  m_set_reg_nums = std::move(info.m_set_reg_nums);
+  m_set_names = std::move(info.m_set_names);
+  m_value_regs_map = std::move(info.m_value_regs_map);
+  m_invalidate_regs_map = std::move(info.m_invalidate_regs_map);
+  m_dynamic_reg_size_map = std::move(info.m_dynamic_reg_size_map);
+
+  m_reg_data_byte_size = info.m_reg_data_byte_size;
+  m_finalized = info.m_finalized;
+
+  if (m_finalized) {
+    const size_t num_sets = m_sets.size();
+    for (size_t set = 0; set < num_sets; ++set)
+      m_sets[set].registers = m_set_reg_nums[set].data();
+  }
+
+  info.Clear();
+}
 
 size_t
 DynamicRegisterInfo::SetRegisterInfo(const StructuredData::Dictionary &dict,
                                      const ArchSpec &arch) {
   assert(!m_finalized);
   StructuredData::Array *sets = nullptr;
   if (dict.GetValueForKeyAsArray("sets", sets)) {
     const uint32_t num_sets = sets->GetSize();
if (reg_info.byte_offset == UINT32_MAX) {
                 StringConvert::ToUInt32(lsbit_str.c_str(), UINT32_MAX);
             if (msbit != UINT32_MAX && lsbit != UINT32_MAX) {
               if (msbit > lsbit) {
                 const uint32_t msbyte = msbit / 8;
                 const uint32_t lsbyte = lsbit / 8;
 
                 ConstString containing_reg_name(reg_name_str);
 
-                RegisterInfo *containing_reg_info =
+                const RegisterInfo *containing_reg_info =
                     GetRegisterInfo(containing_reg_name);
                 if (containing_reg_info) {
                   const uint32_t max_bit = containing_reg_info->byte_size * 8;
                   if (msbit < max_bit && lsbit < max_bit) {
                     m_invalidate_regs_map[containing_reg_info
                                               ->kinds[eRegisterKindLLDB]]
                         .push_back(i);
                     m_value_regs_map[i].push_back(
if (reg_info.byte_offset == UINT32_MAX) {
           const size_t num_composite_regs = composite_reg_list->GetSize();
           if (num_composite_regs > 0) {
             uint32_t composite_offset = UINT32_MAX;
             for (uint32_t composite_idx = 0; composite_idx < num_composite_regs;
                  ++composite_idx) {
               ConstString composite_reg_name;
               if (composite_reg_list->GetItemAtIndexAsString(
                       composite_idx, composite_reg_name, nullptr)) {
-                RegisterInfo *composite_reg_info =
+                const RegisterInfo *composite_reg_info =
                     GetRegisterInfo(composite_reg_name);
                 if (composite_reg_info) {
                   composite_offset = std::min(composite_offset,
                                               composite_reg_info->byte_offset);
                   m_value_regs_map[i].push_back(
                       composite_reg_info->kinds[eRegisterKindLLDB]);
                   m_invalidate_regs_map[composite_reg_info
                                             ->kinds[eRegisterKindLLDB]]
if (reg_info_dict->GetValueForKeyAsArray("invalidate-regs",
                                              invalidate_reg_list)) {
       const size_t num_regs = invalidate_reg_list->GetSize();
       if (num_regs > 0) {
         for (uint32_t idx = 0; idx < num_regs; ++idx) {
           ConstString invalidate_reg_name;
           uint64_t invalidate_reg_num;
           if (invalidate_reg_list->GetItemAtIndexAsString(
                   idx, invalidate_reg_name)) {
-            RegisterInfo *invalidate_reg_info =
+            const RegisterInfo *invalidate_reg_info =
                 GetRegisterInfo(invalidate_reg_name);
             if (invalidate_reg_info) {
               m_invalidate_regs_map[i].push_back(
                   invalidate_reg_info->kinds[eRegisterKindLLDB]);
             } else {
               // TODO: print error invalid slice string that doesn't follow the
               // format
               printf("error: failed to find a 'invalidate-regs' register for "
s.Printf("set[%" PRIu64 "] name = %s, regs = [", (uint64_t)i,
              m_sets[i].name);
     for (size_t idx = 0; idx < m_sets[i].num_registers; ++idx) {
       s.Printf("%s ", m_regs[m_sets[i].registers[idx]].name);
     }
     s.Printf("]\n");
   }
 }
 
-lldb_private::RegisterInfo *DynamicRegisterInfo::GetRegisterInfo(
-    const lldb_private::ConstString &reg_name) {
+const lldb_private::RegisterInfo *DynamicRegisterInfo::GetRegisterInfo(
+    const lldb_private::ConstString &reg_name) const {
   for (auto &reg_info : m_regs) {
     // We can use pointer comparison since we used a ConstString to set the
     // "name" member in AddRegister()
     if (reg_info.name == reg_name.GetCString()) {
       return &reg_info;
     }
   }
   return NULL;
 }