Page MenuHomePhabricator

[lldb] [DynamicRegisterInfo] Support setting from vector<Register>
ClosedPublic

Authored by mgorny on Oct 8 2021, 9:13 AM.

Details

Summary

Add an overload of DynamicRegisterInfo::SetRegisterInfo() that accepts
a std::vector<Register> as an argument. This moves the conversion
from DRI::Register to RegisterInfo directly into DynamicRegisterInfo,
and avoids the necessity of creating fully-compatible intermediate
RegisterInfo instances.

While the new method could technically reuse AddRegister(), the ultimate
goal is to replace AddRegister() with SetRegisterInfo() entirely.

Diff Detail

Event Timeline

mgorny requested review of this revision.Oct 8 2021, 9:13 AM
mgorny created this revision.
mgorny updated this revision to Diff 378443.Oct 9 2021, 5:58 AM

Add a test.

labath accepted this revision.Oct 11 2021, 3:41 AM
labath added inline comments.
lldb/source/Target/DynamicRegisterInfo.cpp
394–395

m_value_regs_map[local_regnum] = reg.value_regs ?

Since I think the callers have no used for the input vector after this call, you might even try to take it by rvalue reference and then move these subvectors in place

lldb/unittests/Target/DynamicRegisterInfoTest.cpp
174–176

ASSERT_NE(reg, nullptr);

This revision is now accepted and ready to land.Oct 11 2021, 3:41 AM
mgorny added inline comments.Oct 11 2021, 3:53 AM
lldb/unittests/Target/DynamicRegisterInfoTest.cpp
174–176

Actually, the idea was to test all registers even if one failed; though I suppose since we're now starting with the lowest index, higher indices won't work either.

labath added inline comments.Oct 11 2021, 3:59 AM
lldb/unittests/Target/DynamicRegisterInfoTest.cpp
174–176

I'll let you in on a dirty secret. They only difference between EXPECT_xxx and ASSERT_xxx is that the ASSERT version does a return; in case of failure. So it does not actually terminate the test -- just the current function.

That's why the recommended practice is to use gtest macros at the top level (in the TEST function), but I've now given up on getting people to do that.

mgorny updated this revision to Diff 378626.Oct 11 2021, 4:56 AM
mgorny marked 2 inline comments as done.

Use fancy move semantics and ASSERT_NE.

mgorny marked an inline comment as done.Oct 11 2021, 7:59 AM
This revision was landed with ongoing or failed builds.Oct 11 2021, 8:03 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptOct 11 2021, 8:03 AM