Gabe Black has uploaded this change for review. (
https://gem5-review.googlesource.com/c/public/gem5/+/66431?usp=email )
Change subject: dev: Add an offset checking mechanism to RegisterBank.
......................................................................
dev: Add an offset checking mechanism to RegisterBank.
When adding a long list of registers, it can be easy to miss one which
will offset all the registers after it. It can be hard to find those
sorts of problems, and tedious and error prone to fix them.
This change adds a mechanism to simply annotate what offset a register
should have. That should also make the register list more self
documenting, since you'll be able to easily see what offset a register
has from the source without having to count up everything in front of it.
Change-Id: Ia7e419ffb062a64a10106305f875cec6f9fe9a80
---
M src/dev/reg_bank.hh
M src/dev/reg_bank.test.cc
2 files changed, 117 insertions(+), 11 deletions(-)
diff --git a/src/dev/reg_bank.hh b/src/dev/reg_bank.hh
index 42af7bc..6348597 100644
--- a/src/dev/reg_bank.hh
+++ b/src/dev/reg_bank.hh
@@ -84,6 +84,9 @@
* entire device, with the address from accesses passed into read or write
* unmodified.
*
+ * The base(), size() and name() methods can be used to access each of
those
+ * read only properties of the RegisterBank instance.
+ *
* To add actual registers to the RegisterBank (discussed below), you can
use
* either the addRegister method which adds a single register, or
addRegisters
* which adds an initializer list of them all at once. The register will be
@@ -91,8 +94,19 @@
* existing registers. The size of the bank is automatically accumulated as
* registers are added.
*
- * The base(), size() and name() methods can be used to access each of
those
- * read only properties of the RegisterBank instance.
+ * When adding a lot of registers, you might accidentally add an extra,
+ * or accidentally skip one in a long list. Because the offset is handled
+ * automatically, some of your registers might end up shifted higher or
lower
+ * than you expect. To help mitigate this, you can set what offset you
expect
+ * a register to have by specifying it as an offset, register pair.
+ *
+ * addRegisters({{0x1000, reg0}, reg1, reg2});
+ *
+ * If the register would end up at a different offset, gem5 will panic. You
+ * can also leave off the register if you want to just check the offset,
for
+ * instance between groups of registers.
+ *
+ * addRegisters({reg0, reg1, reg2, 0x100c})
*
* While the RegisterBank itself doesn't have any data in it directly and
so
* has no endianness, it's very likely all the registers within it will
have
@@ -805,19 +819,54 @@
virtual ~RegisterBank() {}
- void
- addRegisters(
- std::initializer_list<std::reference_wrapper<RegisterBase>>
regs)
+ class RegisterAdder
{
- panic_if(regs.size() == 0, "Adding an empty list of registers
to %s?",
- name());
- for (auto ®: regs) {
- _offsetMap.emplace(_base + _size, reg);
- _size += reg.get().size();
+ private:
+ bool checkOffset = false;
+ Addr offset = 0;
+ RegisterBase *reg = nullptr;
+
+ public:
+ // Nothing special to do for this register.
+ RegisterAdder(RegisterBase &new_reg) : reg(&new_reg) {}
+ // Ensure that this register is added at a particular offset.
+ RegisterAdder(Addr new_offset, RegisterBase &new_reg) :
+ checkOffset(true), offset(new_offset), reg(&new_reg)
+ {}
+ // No register, just check that the offset is what we expect.
+ RegisterAdder(Addr new_offset) : checkOffset(true),
offset(new_offset)
+ {}
+
+ friend class RegisterBank;
+ };
+
+ void
+ addRegisters(std::initializer_list<RegisterAdder> adders)
+ {
+ panic_if(adders.size() == 0,
+ "Adding an empty list of registers to %s?", name());
+ for (auto &adder: adders) {
+ const Addr offset = _base + _size;
+ auto *reg = adder.reg;
+
+ if (reg) {
+ if (adder.checkOffset && adder.offset != offset) {
+ panic(
+ "Expected offset of register %s.%s to be %#x,
is %#x.",
+ name(), reg->name(), adder.offset, offset);
+ }
+ _offsetMap.emplace(offset, *reg);
+ _size += reg->size();
+ } else if (adder.checkOffset) {
+ if (adder.offset != offset) {
+ panic("Expected current offset of %s to be %#x,
is %#x.",
+ name(), adder.offset, offset);
+ }
+ }
}
}
- void addRegister(RegisterBase ®) { addRegisters({reg}); }
+ void addRegister(RegisterAdder reg) { addRegisters({reg}); }
Addr base() const { return _base; }
Addr size() const { return _size; }
diff --git a/src/dev/reg_bank.test.cc b/src/dev/reg_bank.test.cc
index 534f862..b4bc969 100644
--- a/src/dev/reg_bank.test.cc
+++ b/src/dev/reg_bank.test.cc
@@ -55,6 +55,7 @@
#include <vector>
+#include "base/gtest/logging.hh"
#include "dev/reg_bank.hh"
using namespace gem5;
@@ -64,6 +65,9 @@
// This version is needed with enough elements, empirically more than 10.
using testing::ElementsAreArray;
+using testing::AllOf;
+using testing::HasSubstr;
+
/*
* The RegisterRaz (read as zero) type.
@@ -1011,6 +1015,41 @@
EXPECT_EQ(emptyBank.size(), 12);
}
+TEST_F(RegisterBankTest, AddRegistersWithOffsetChecks)
+{
+ emptyBank.addRegister({0x12345});
+ EXPECT_EQ(emptyBank.size(), 0);
+ emptyBank.addRegister({0x12345, reg0});
+ EXPECT_EQ(emptyBank.size(), 4);
+ emptyBank.addRegister({0x12349});
+ EXPECT_EQ(emptyBank.size(), 4);
+
+ emptyBank.addRegisters({{0x12349, reg1}, {0x1234d}, {0x1234d, reg2}});
+ EXPECT_EQ(emptyBank.size(), 12);
+}
+
+TEST_F(RegisterBankTest, BadRegisterOffsetDeath)
+{
+ gtestLogOutput.str("");
+ EXPECT_ANY_THROW(emptyBank.addRegisters({{0xabcd, reg0}, reg1}));
+
+ std::string actual = gtestLogOutput.str();
+ EXPECT_THAT(actual, HasSubstr("empty.reg0"));
+ EXPECT_THAT(actual, HasSubstr("to be 0xabcd"));
+ EXPECT_THAT(actual, HasSubstr("is 0x12345"));
+}
+
+TEST_F(RegisterBankTest, BadBankOffsetDeath)
+{
+ gtestLogOutput.str("");
+ EXPECT_ANY_THROW(emptyBank.addRegisters({{0xabcd}, reg0}));
+
+ std::string actual = gtestLogOutput.str();
+ EXPECT_THAT(actual, HasSubstr("empty "));
+ EXPECT_THAT(actual, HasSubstr("to be 0xabcd"));
+ EXPECT_THAT(actual, HasSubstr("is 0x12345"));
+}
+
// Reads.
TEST_F(RegisterBankTest, ReadOneAlignedFirst)
--
To view, visit
https://gem5-review.googlesource.com/c/public/gem5/+/66431?usp=email
To unsubscribe, or for help writing mail filters, visit
https://gem5-review.googlesource.com/settings
Gerrit-Project: public/gem5
Gerrit-Branch: develop
Gerrit-Change-Id: Ia7e419ffb062a64a10106305f875cec6f9fe9a80
Gerrit-Change-Number: 66431
Gerrit-PatchSet: 1
Gerrit-Owner: Gabe Black <gabe.bl...@gmail.com>
Gerrit-MessageType: newchange
_______________________________________________
gem5-dev mailing list -- gem5-dev@gem5.org
To unsubscribe send an email to gem5-dev-le...@gem5.org
