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
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
