[lldb] [DynamicRegisterInfo] Support iterating over registers()

[mgorny]

Summary

Add DynamicRegisterInfo::registers() method that returns
llvm::iterator_range<> over RegisterInfos. This is a convenient
replacement for GetNumRegisters() + GetRegisterInfoAtIndex().

[labath]

lldb/include/lldb/Target/DynamicRegisterInfo.h

81

llvm (and, surprisingly, lldb -- I guess because its a new feature) follows c++ naming conventions for methods like these.

82

Could this return const iterators? It seems we already have some non-const accessors, but I'd rather not propagate that..

[mgorny]

lldb/include/lldb/Target/DynamicRegisterInfo.h

81

/me not understand.

82

It can't — we're using these iterators to augment register infos ;-).

[ted]

lldb/include/lldb/Target/DynamicRegisterInfo.h

82

Maybe make a non-const protected, and a const public, so random plugin #37 can't modify register info?

[mgorny]

lldb/include/lldb/Target/DynamicRegisterInfo.h

82

That would work for me; however, we'd have to make ABI and DynamicRegisterInfo friends then. @labath, what do you thik?

[labath]

lldb/include/lldb/Target/DynamicRegisterInfo.h

81

iterator_range-returning functions use the C++ snake_case naming convention for the most part: implicit_operands, info_section_units, external_symbols, ...

82

Yeah, I forgot what motivated this change...

I don't think friending would work the way you imagined it (one would need some extra tricks) as friendship is not inherited by subclasses. That wouldn't be the end of the world, but I am hoping we can come with a better solution.

So, currently we have this RemoteRegisterInfo struct as a kind of a DynamicRegisterInfo precursor. What if the ABI classes operated on that instead ? Besides solving the visibility issues (this data isn't live yet, so we're free to modify it as we see fit), I'm hoping it would also make things simpler as the data hasn't been converted to the C format yet. We could move RemoteRegisterInfo to the DynamicRegisterInfo class (and rename it to something else, obviously) so that it's visible to everyone involved.

Currently Finalization happens very close to the point where we convert the remote format, so I'm hoping this wouldn't be too hard to implement. The presence of the HardcodeARMRegisters calls complicates things a bit. The main difference would be that these hardcoded registers wouldn't go through the augmentation process (without additional refactoring), but from the looks of it, they don't actually need any augmentation, so we could just let them float.

WDYT?

[mgorny]

lldb/include/lldb/Target/DynamicRegisterInfo.h

82

Sure, I'll see if this can be done cleanly.

[mgorny]

Switch to a const_iterator.

[mgorny]

Switch to lowercase name.

[mgorny]

lldb/include/lldb/Target/DynamicRegisterInfo.h

82

So I've tried, and… well, it's technically doable but it feels like I'm reinventing the wheel every step of the way. I have to manually iterate over all registers to find the base registers. I have to manually iterate over the registers to check if supplementary registers exist or not. And in the end, I have to somehow figure out which registers to pass via AddRegister() and which via AddSupplementaryRegister().

At the same time, it really feels like DynamicRegisterInfo exists precisely to handle this kind of work, so avoiding it ends up feeling backwards.

Maybe we should instead look into making DynamicRegisterInfo immutable after finalization — either via making sure callers get only const DynamicRegisterInfo&s or even converting it into an ImmutableDynamicRegisterInfo of some kind.

lldb/include/lldb/Target/DynamicRegisterInfo.h

public:
 
   void Clear();
 
   bool IsReconfigurable();
 
   const lldb_private::RegisterInfo *
   GetRegisterInfo(llvm::StringRef reg_name) const;
 
+  typedef std::vector<lldb_private::RegisterInfo> reg_collection;
+  llvm::iterator_range<reg_collection::const_iterator> registers() const {
+    return llvm::iterator_range<reg_collection::const_iterator>(m_regs);
+  }
+
 protected:
   // Classes that inherit from DynamicRegisterInfo can see and modify these
-  typedef std::vector<lldb_private::RegisterInfo> reg_collection;
   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/source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.cpp

if (m_read_all_at_once && !m_gpacket_cached) {
               gdb_comm.ReadAllRegisters(m_thread.GetProtocolID())) {
         memcpy(const_cast<uint8_t *>(m_reg_data.GetDataStart()),
                buffer_sp->GetBytes(),
                std::min(buffer_sp->GetByteSize(), m_reg_data.GetByteSize()));
         if (buffer_sp->GetByteSize() >= m_reg_data.GetByteSize()) {
           SetAllRegisterValid(true);
           return true;
         } else if (buffer_sp->GetByteSize() > 0) {
-          const int regcount = m_reg_info_sp->GetNumRegisters();
-          for (int i = 0; i < regcount; i++) {
-            struct RegisterInfo *reginfo =
-                m_reg_info_sp->GetRegisterInfoAtIndex(i);
-            if (reginfo->byte_offset + reginfo->byte_size <=
-                buffer_sp->GetByteSize()) {
-              m_reg_valid[i] = true;
-            } else {
-              m_reg_valid[i] = false;
-            }
+          for (auto x : llvm::enumerate(m_reg_info_sp->registers())) {
+            const struct RegisterInfo &reginfo = x.value();
+            m_reg_valid[x.index()] =
+                (reginfo.byte_offset + reginfo.byte_size <=
+                 buffer_sp->GetByteSize());
           }
 
           m_gpacket_cached = true;
           if (GetRegisterIsValid(reg))
             return true;
         } else {
           Log *log(ProcessGDBRemoteLog::GetLogIfAnyCategoryIsSet(GDBR_LOG_THREAD |
                                                                 GDBR_LOG_PACKETS));