[Lldb-commits] [PATCH] D111435: [lldb] [DynamicRegisterInfo] Support setting from vector
This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG660632778f30: [lldb] [DynamicRegisterInfo] Support setting from vectorRegister (authored by mgorny). Herald added a project: LLDB. Changed prior to commit: https://reviews.llvm.org/D111435?vs=378626=378672#toc Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D111435/new/ https://reviews.llvm.org/D111435 Files: lldb/include/lldb/Target/DynamicRegisterInfo.h lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp lldb/source/Target/DynamicRegisterInfo.cpp lldb/unittests/Target/DynamicRegisterInfoTest.cpp Index: lldb/unittests/Target/DynamicRegisterInfoTest.cpp === --- lldb/unittests/Target/DynamicRegisterInfoTest.cpp +++ lldb/unittests/Target/DynamicRegisterInfoTest.cpp @@ -130,20 +130,26 @@ class DynamicRegisterInfoRegisterTest : public ::testing::Test { protected: std::vector m_regs; + DynamicRegisterInfo m_dyninfo; uint32_t AddTestRegister( const char *name, const char *group, uint32_t byte_size, std::function adder, std::vector value_regs = {}, std::vector invalidate_regs = {}) { -DynamicRegisterInfo::Register new_reg{ -ConstString(name), ConstString(), -ConstString(group),byte_size, -LLDB_INVALID_INDEX32, lldb::eEncodingUint, -lldb::eFormatUnsigned, LLDB_INVALID_REGNUM, -LLDB_INVALID_REGNUM, LLDB_INVALID_REGNUM, -LLDB_INVALID_REGNUM, value_regs, -invalidate_regs}; +DynamicRegisterInfo::Register new_reg{ConstString(name), + ConstString(), + ConstString(group), + byte_size, + LLDB_INVALID_INDEX32, + lldb::eEncodingUint, + lldb::eFormatUnsigned, + LLDB_INVALID_REGNUM, + LLDB_INVALID_REGNUM, + LLDB_INVALID_REGNUM, + static_cast(m_regs.size()), + value_regs, + invalidate_regs}; adder(new_reg); return m_regs.size() - 1; } @@ -159,6 +165,18 @@ EXPECT_EQ(reg.value_regs, value_regs); EXPECT_EQ(reg.invalidate_regs, invalidate_regs); } + + void ExpectInDynInfo(uint32_t reg_num, const char *reg_name, + uint32_t byte_offset, + std::vector value_regs = {}, + std::vector invalidate_regs = {}) { +const RegisterInfo *reg = m_dyninfo.GetRegisterInfoAtIndex(reg_num); +ASSERT_NE(reg, nullptr); +EXPECT_STREQ(reg->name, reg_name); +EXPECT_EQ(reg->byte_offset, byte_offset); +EXPECT_THAT(regs_to_vector(reg->value_regs), value_regs); +EXPECT_THAT(regs_to_vector(reg->invalidate_regs), invalidate_regs); + } }; #define EXPECT_IN_REGS(reg, ...) \ @@ -167,6 +185,12 @@ ExpectInRegs(reg, #reg, __VA_ARGS__); \ } +#define EXPECT_IN_DYNINFO(reg, ...)\ + {\ +SCOPED_TRACE("at register " #reg); \ +ExpectInDynInfo(reg, #reg, __VA_ARGS__); \ + } + TEST_F(DynamicRegisterInfoRegisterTest, addSupplementaryRegister) { // Add a base register uint32_t rax = AddTestRegister( @@ -186,3 +210,32 @@ EXPECT_IN_REGS(ax, {rax}, {rax, eax, al}); EXPECT_IN_REGS(al, {rax}, {rax, eax, ax}); } + +TEST_F(DynamicRegisterInfoRegisterTest, SetRegisterInfo) { + auto adder = [this](const DynamicRegisterInfo::Register ) { +m_regs.push_back(r); + }; + // Add regular registers + uint32_t b1 = AddTestRegister("b1", "base", 8, adder); + uint32_t b2 = AddTestRegister("b2", "other", 8, adder); + + // Add a few sub-registers + uint32_t s1 = AddTestRegister("s1", "base", 4, adder, {b1}); + uint32_t s2 = AddTestRegister("s2", "other", 4, adder, {b2}); + + // Add a register with invalidate_regs + uint32_t i1 = AddTestRegister("i1", "third", 8, adder, {}, {b1}); + + // Add a register with indirect invalidate regs to be expanded + // TODO: why is it done conditionally to value_regs? + uint32_t i2 = AddTestRegister("i2", "third", 4, adder, {b2}, {i1}); + + EXPECT_EQ(m_dyninfo.SetRegisterInfo(std::move(m_regs), ArchSpec()), +m_regs.size()); + EXPECT_IN_DYNINFO(b1, 0, {}, {}); + EXPECT_IN_DYNINFO(b2, 8, {}, {}); + EXPECT_IN_DYNINFO(s1, 0, {b1}, {}); +
[Lldb-commits] [PATCH] D111435: [lldb] [DynamicRegisterInfo] Support setting from vector
mgorny updated this revision to Diff 378626. mgorny marked 2 inline comments as done. mgorny added a comment. Use fancy move semantics and `ASSERT_NE`. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D111435/new/ https://reviews.llvm.org/D111435 Files: lldb/include/lldb/Target/DynamicRegisterInfo.h lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp lldb/source/Target/DynamicRegisterInfo.cpp lldb/unittests/Target/DynamicRegisterInfoTest.cpp Index: lldb/unittests/Target/DynamicRegisterInfoTest.cpp === --- lldb/unittests/Target/DynamicRegisterInfoTest.cpp +++ lldb/unittests/Target/DynamicRegisterInfoTest.cpp @@ -130,20 +130,26 @@ class DynamicRegisterInfoRegisterTest : public ::testing::Test { protected: std::vector m_regs; + DynamicRegisterInfo m_dyninfo; uint32_t AddTestRegister( const char *name, const char *group, uint32_t byte_size, std::function adder, std::vector value_regs = {}, std::vector invalidate_regs = {}) { -DynamicRegisterInfo::Register new_reg{ -ConstString(name), ConstString(), -ConstString(group),byte_size, -LLDB_INVALID_INDEX32, lldb::eEncodingUint, -lldb::eFormatUnsigned, LLDB_INVALID_REGNUM, -LLDB_INVALID_REGNUM, LLDB_INVALID_REGNUM, -LLDB_INVALID_REGNUM, value_regs, -invalidate_regs}; +DynamicRegisterInfo::Register new_reg{ConstString(name), + ConstString(), + ConstString(group), + byte_size, + LLDB_INVALID_INDEX32, + lldb::eEncodingUint, + lldb::eFormatUnsigned, + LLDB_INVALID_REGNUM, + LLDB_INVALID_REGNUM, + LLDB_INVALID_REGNUM, + static_cast(m_regs.size()), + value_regs, + invalidate_regs}; adder(new_reg); return m_regs.size() - 1; } @@ -161,6 +167,18 @@ EXPECT_EQ(reg.value_regs, value_regs); EXPECT_EQ(reg.invalidate_regs, invalidate_regs); } + + void AssertRegisterInfo(uint32_t reg_num, const char *reg_name, + uint32_t byte_offset, + std::vector value_regs, + std::vector invalidate_regs) { +const RegisterInfo *reg = m_dyninfo.GetRegisterInfoAtIndex(reg_num); +ASSERT_NE(reg, nullptr); +EXPECT_STREQ(reg->name, reg_name); +EXPECT_EQ(reg->byte_offset, byte_offset); +EXPECT_THAT(regs_to_vector(reg->value_regs), value_regs); +EXPECT_THAT(regs_to_vector(reg->invalidate_regs), invalidate_regs); + } }; TEST_F(DynamicRegisterInfoRegisterTest, addSupplementaryRegister) { @@ -182,3 +200,32 @@ ASSERT_REG(ax, {rax}, {rax, eax, al}); ASSERT_REG(al, {rax}, {rax, eax, ax}); } + +TEST_F(DynamicRegisterInfoRegisterTest, SetRegisterInfo) { + auto adder = [this](const DynamicRegisterInfo::Register ) { +m_regs.push_back(r); + }; + // Add regular registers + uint32_t b1 = AddTestRegister("b1", "base", 8, adder); + uint32_t b2 = AddTestRegister("b2", "other", 8, adder); + + // Add a few sub-registers + uint32_t s1 = AddTestRegister("s1", "base", 4, adder, {b1}); + uint32_t s2 = AddTestRegister("s2", "other", 4, adder, {b2}); + + // Add a register with invalidate_regs + uint32_t i1 = AddTestRegister("i1", "third", 8, adder, {}, {b1}); + + // Add a register with indirect invalidate regs to be expanded + // TODO: why is it done conditionally to value_regs? + uint32_t i2 = AddTestRegister("i2", "third", 4, adder, {b2}, {i1}); + + EXPECT_EQ(m_dyninfo.SetRegisterInfo(std::move(m_regs), ArchSpec()), +m_regs.size()); + ASSERT_REG(b1, 0, {}, {}); + ASSERT_REG(b2, 8, {}, {}); + ASSERT_REG(s1, 0, {b1}, {}); + ASSERT_REG(s2, 8, {b2}, {}); + ASSERT_REG(i1, 16, {}, {b1}); + ASSERT_REG(i2, 8, {b2}, {b1, i1}); +} Index: lldb/source/Target/DynamicRegisterInfo.cpp === --- lldb/source/Target/DynamicRegisterInfo.cpp +++ lldb/source/Target/DynamicRegisterInfo.cpp @@ -379,6 +379,45 @@ return m_regs.size(); } +size_t DynamicRegisterInfo::SetRegisterInfo( +std::vector &, +const ArchSpec ) { + assert(!m_finalized); + + for (auto it : llvm::enumerate(regs)) { +uint32_t local_regnum = it.index(); +const DynamicRegisterInfo::Register = it.value(); + +assert(reg.name); +assert(reg.set_name); + +if (!reg.value_regs.empty()) + m_value_regs_map[local_regnum] = std::move(reg.value_regs); +if (!reg.invalidate_regs.empty()) + m_invalidate_regs_map[local_regnum] =
[Lldb-commits] [PATCH] D111435: [lldb] [DynamicRegisterInfo] Support setting from vector
labath added inline comments. Comment at: lldb/unittests/Target/DynamicRegisterInfoTest.cpp:176-178 +EXPECT_NE(reg, nullptr); +if (!reg) + return; mgorny wrote: > labath wrote: > > ASSERT_NE(reg, nullptr); > 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. 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. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D111435/new/ https://reviews.llvm.org/D111435 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D111435: [lldb] [DynamicRegisterInfo] Support setting from vector
mgorny added inline comments. Comment at: lldb/unittests/Target/DynamicRegisterInfoTest.cpp:176-178 +EXPECT_NE(reg, nullptr); +if (!reg) + return; labath wrote: > ASSERT_NE(reg, nullptr); 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. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D111435/new/ https://reviews.llvm.org/D111435 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D111435: [lldb] [DynamicRegisterInfo] Support setting from vector
labath accepted this revision. labath added inline comments. This revision is now accepted and ready to land. Comment at: lldb/source/Target/DynamicRegisterInfo.cpp:394-395 + +for (uint32_t value_reg : reg.value_regs) + m_value_regs_map[local_regnum].push_back(value_reg); +for (uint32_t invalidate_reg : reg.invalidate_regs) `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 Comment at: lldb/unittests/Target/DynamicRegisterInfoTest.cpp:176-178 +EXPECT_NE(reg, nullptr); +if (!reg) + return; ASSERT_NE(reg, nullptr); CHANGES SINCE LAST ACTION https://reviews.llvm.org/D111435/new/ https://reviews.llvm.org/D111435 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D111435: [lldb] [DynamicRegisterInfo] Support setting from vector
mgorny updated this revision to Diff 378443. mgorny added a comment. Add a test. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D111435/new/ https://reviews.llvm.org/D111435 Files: lldb/include/lldb/Target/DynamicRegisterInfo.h lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp lldb/source/Target/DynamicRegisterInfo.cpp lldb/unittests/Target/DynamicRegisterInfoTest.cpp Index: lldb/unittests/Target/DynamicRegisterInfoTest.cpp === --- lldb/unittests/Target/DynamicRegisterInfoTest.cpp +++ lldb/unittests/Target/DynamicRegisterInfoTest.cpp @@ -130,20 +130,26 @@ class DynamicRegisterInfoRegisterTest : public ::testing::Test { protected: std::vector m_regs; + DynamicRegisterInfo m_dyninfo; uint32_t AddTestRegister( const char *name, const char *group, uint32_t byte_size, std::function adder, std::vector value_regs = {}, std::vector invalidate_regs = {}) { -DynamicRegisterInfo::Register new_reg{ -ConstString(name), ConstString(), -ConstString(group),byte_size, -LLDB_INVALID_INDEX32, lldb::eEncodingUint, -lldb::eFormatUnsigned, LLDB_INVALID_REGNUM, -LLDB_INVALID_REGNUM, LLDB_INVALID_REGNUM, -LLDB_INVALID_REGNUM, value_regs, -invalidate_regs}; +DynamicRegisterInfo::Register new_reg{ConstString(name), + ConstString(), + ConstString(group), + byte_size, + LLDB_INVALID_INDEX32, + lldb::eEncodingUint, + lldb::eFormatUnsigned, + LLDB_INVALID_REGNUM, + LLDB_INVALID_REGNUM, + LLDB_INVALID_REGNUM, + static_cast(m_regs.size()), + value_regs, + invalidate_regs}; adder(new_reg); return m_regs.size() - 1; } @@ -161,6 +167,21 @@ EXPECT_EQ(reg.value_regs, value_regs); EXPECT_EQ(reg.invalidate_regs, invalidate_regs); } + + void AssertRegisterInfo(uint32_t reg_num, const char *reg_name, + uint32_t byte_offset, + std::vector value_regs, + std::vector invalidate_regs) { +const RegisterInfo *reg = m_dyninfo.GetRegisterInfoAtIndex(reg_num); +EXPECT_NE(reg, nullptr); +if (!reg) + return; + +EXPECT_STREQ(reg->name, reg_name); +EXPECT_EQ(reg->byte_offset, byte_offset); +EXPECT_THAT(regs_to_vector(reg->value_regs), value_regs); +EXPECT_THAT(regs_to_vector(reg->invalidate_regs), invalidate_regs); + } }; TEST_F(DynamicRegisterInfoRegisterTest, addSupplementaryRegister) { @@ -182,3 +203,31 @@ ASSERT_REG(ax, {rax}, {rax, eax, al}); ASSERT_REG(al, {rax}, {rax, eax, ax}); } + +TEST_F(DynamicRegisterInfoRegisterTest, SetRegisterInfo) { + auto adder = [this](const DynamicRegisterInfo::Register ) { +m_regs.push_back(r); + }; + // Add regular registers + uint32_t b1 = AddTestRegister("b1", "base", 8, adder); + uint32_t b2 = AddTestRegister("b2", "other", 8, adder); + + // Add a few sub-registers + uint32_t s1 = AddTestRegister("s1", "base", 4, adder, {b1}); + uint32_t s2 = AddTestRegister("s2", "other", 4, adder, {b2}); + + // Add a register with invalidate_regs + uint32_t i1 = AddTestRegister("i1", "third", 8, adder, {}, {b1}); + + // Add a register with indirect invalidate regs to be expanded + // TODO: why is it done conditionally to value_regs? + uint32_t i2 = AddTestRegister("i2", "third", 4, adder, {b2}, {i1}); + + EXPECT_EQ(m_dyninfo.SetRegisterInfo(m_regs, ArchSpec()), m_regs.size()); + ASSERT_REG(b1, 0, {}, {}); + ASSERT_REG(b2, 8, {}, {}); + ASSERT_REG(s1, 0, {b1}, {}); + ASSERT_REG(s2, 8, {b2}, {}); + ASSERT_REG(i1, 16, {}, {b1}); + ASSERT_REG(i2, 8, {b2}, {b1, i1}); +} Index: lldb/source/Target/DynamicRegisterInfo.cpp === --- lldb/source/Target/DynamicRegisterInfo.cpp +++ lldb/source/Target/DynamicRegisterInfo.cpp @@ -379,6 +379,45 @@ return m_regs.size(); } +size_t DynamicRegisterInfo::SetRegisterInfo( +const std::vector , +const ArchSpec ) { + assert(!m_finalized); + + for (auto it : llvm::enumerate(regs)) { +uint32_t local_regnum = it.index(); +const DynamicRegisterInfo::Register = it.value(); + +assert(reg.name); +assert(reg.set_name); + +for (uint32_t value_reg : reg.value_regs) + m_value_regs_map[local_regnum].push_back(value_reg); +for (uint32_t invalidate_reg : reg.invalidate_regs) + m_invalidate_regs_map[local_regnum].push_back(invalidate_reg); + +struct
[Lldb-commits] [PATCH] D111435: [lldb] [DynamicRegisterInfo] Support setting from vector
mgorny created this revision. mgorny added reviewers: labath, teemperor, krytarowski, emaste. mgorny requested review of this revision. Add an overload of DynamicRegisterInfo::SetRegisterInfo() that accepts a std::vector 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. https://reviews.llvm.org/D111435 Files: lldb/include/lldb/Target/DynamicRegisterInfo.h lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp lldb/source/Target/DynamicRegisterInfo.cpp Index: lldb/source/Target/DynamicRegisterInfo.cpp === --- lldb/source/Target/DynamicRegisterInfo.cpp +++ lldb/source/Target/DynamicRegisterInfo.cpp @@ -379,6 +379,45 @@ return m_regs.size(); } +size_t DynamicRegisterInfo::SetRegisterInfo( +const std::vector , +const ArchSpec ) { + assert(!m_finalized); + + for (auto it : llvm::enumerate(regs)) { +uint32_t local_regnum = it.index(); +const DynamicRegisterInfo::Register = it.value(); + +assert(reg.name); +assert(reg.set_name); + +for (uint32_t value_reg : reg.value_regs) + m_value_regs_map[local_regnum].push_back(value_reg); +for (uint32_t invalidate_reg : reg.invalidate_regs) + m_invalidate_regs_map[local_regnum].push_back(invalidate_reg); + +struct RegisterInfo reg_info { + reg.name.AsCString(), reg.alt_name.AsCString(), reg.byte_size, + reg.byte_offset, reg.encoding, reg.format, + {reg.regnum_ehframe, reg.regnum_dwarf, reg.regnum_generic, + reg.regnum_remote, local_regnum}, + // value_regs and invalidate_regs are filled by Finalize() + nullptr, nullptr +}; + +m_regs.push_back(reg_info); + +uint32_t set = GetRegisterSetIndexByName(reg.set_name, true); +assert(set < m_sets.size()); +assert(set < m_set_reg_nums.size()); +assert(set < m_set_names.size()); +m_set_reg_nums[set].push_back(local_regnum); + }; + + Finalize(arch); + return m_regs.size(); +} + void DynamicRegisterInfo::AddRegister(RegisterInfo reg_info, ConstString _name) { assert(!m_finalized); @@ -682,8 +721,9 @@ return nullptr; } -uint32_t DynamicRegisterInfo::GetRegisterSetIndexByName(ConstString _name, -bool can_create) { +uint32_t +DynamicRegisterInfo::GetRegisterSetIndexByName(const ConstString _name, + bool can_create) { name_collection::iterator pos, end = m_set_names.end(); for (pos = m_set_names.begin(); pos != end; ++pos) { if (*pos == set_name) Index: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp === --- lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp +++ lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp @@ -4502,32 +4502,7 @@ if (ABISP abi_sp = ABI::FindPlugin(shared_from_this(), arch_to_use)) abi_sp->AugmentRegisterInfo(registers); - for (auto it : llvm::enumerate(registers)) { -uint32_t local_regnum = it.index(); -DynamicRegisterInfo::Register _reg_info = it.value(); - -auto regs_with_sentinel = [](std::vector ) -> uint32_t * { - if (!vec.empty()) { -vec.push_back(LLDB_INVALID_REGNUM); -return vec.data(); - } - return nullptr; -}; - -struct RegisterInfo reg_info { - remote_reg_info.name.AsCString(), remote_reg_info.alt_name.AsCString(), - remote_reg_info.byte_size, remote_reg_info.byte_offset, - remote_reg_info.encoding, remote_reg_info.format, - {remote_reg_info.regnum_ehframe, remote_reg_info.regnum_dwarf, - remote_reg_info.regnum_generic, remote_reg_info.regnum_remote, - local_regnum}, - regs_with_sentinel(remote_reg_info.value_regs), - regs_with_sentinel(remote_reg_info.invalidate_regs), -}; -m_register_info_sp->AddRegister(reg_info, remote_reg_info.set_name); - }; - - m_register_info_sp->Finalize(arch_to_use); + m_register_info_sp->SetRegisterInfo(registers, arch_to_use); } // query the target of gdb-remote for extended target information returns Index: lldb/include/lldb/Target/DynamicRegisterInfo.h === --- lldb/include/lldb/Target/DynamicRegisterInfo.h +++ lldb/include/lldb/Target/DynamicRegisterInfo.h @@ -53,6 +53,9 @@ size_t SetRegisterInfo(const lldb_private::StructuredData::Dictionary , const lldb_private::ArchSpec ); + size_t SetRegisterInfo(const std::vector , + const lldb_private::ArchSpec ); + void