[Lldb-commits] [PATCH] D110023: [lldb] [DynamicRegisterInfo] Add a convenience method to add suppl. registers

[Pavel Labath via Phabricator via lldb-commits]

labath accepted this revision.
labath added a comment.
This revision is now accepted and ready to land.

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.



================
Comment at: lldb/source/Plugins/Process/Utility/DynamicRegisterInfo.cpp:441
+
+  reg_to_regs_map to_add;
+  for (uint32_t value_reg : m_value_regs_map[reg_num]) {
----------------
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...


================
Comment at: lldb/source/Plugins/Process/Utility/DynamicRegisterInfo.cpp:443-444
+  for (uint32_t value_reg : m_value_regs_map[reg_num]) {
+    if (value_reg == LLDB_INVALID_REGNUM)
+      break;
+
----------------
Is this still needed?


================
Comment at: lldb/source/Plugins/Process/Utility/DynamicRegisterInfo.cpp:457
+
+  for (reg_to_regs_map::value_type &x : to_add)
+    llvm::append_range(m_invalidate_regs_map[x.first], x.second);
----------------
This would be a good use case for `(const) auto`, as `value_type` does not say much anyway.


================
Comment at: lldb/unittests/Process/Utility/DynamicRegisterInfoTest.cpp:184-190
+  struct RegisterInfo ah_reg {
+    "ah", nullptr, 1, LLDB_INVALID_INDEX32, lldb::eEncodingUint,
+        lldb::eFormatUnsigned,
+        {LLDB_INVALID_REGNUM, LLDB_INVALID_REGNUM, LLDB_INVALID_REGNUM, ah, ah},
+        value_regs, nullptr, nullptr, 0
+  };
+  info.AddSupplementaryRegister(ah_reg, group);
----------------
Could we remove ah from this test, as its offset is going to be wrong?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110023/new/

https://reviews.llvm.org/D110023