From 9d1cc1bcc91290aa32253462ea3bc6df1a9d83c5 Mon Sep 17 00:00:00 2001 From: Gabe Black Date: Mon, 5 Dec 2022 05:03:53 -0800 Subject: [PATCH] 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 Reviewed-on: https://gem5-review.googlesource.com/c/public/gem5/+/66431 Reviewed-by: Yu-hsin Wang Maintainer: Gabe Black Tested-by: kokoro --- src/dev/reg_bank.hh | 70 +++++++++++++++++++++++++++++++++------- src/dev/reg_bank.test.cc | 39 ++++++++++++++++++++++ 2 files changed, 98 insertions(+), 11 deletions(-) diff --git a/src/dev/reg_bank.hh b/src/dev/reg_bank.hh index 42af7bce89..31c0ce5b66 100644 --- a/src/dev/reg_bank.hh +++ b/src/dev/reg_bank.hh @@ -37,6 +37,7 @@ #include #include #include +#include #include #include @@ -84,6 +85,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 +95,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 +820,52 @@ class RegisterBank : public RegisterBankBase virtual ~RegisterBank() {} - void - addRegisters( - std::initializer_list> 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: + std::optional offset; + std::optional reg; + + 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) : + offset(new_offset), reg(&new_reg) + {} + // No register, just check that the offset is what we expect. + RegisterAdder(Addr new_offset) : offset(new_offset) {} + + friend class RegisterBank; + }; + + void + addRegisters(std::initializer_list adders) + { + panic_if(std::empty(adders), + "Adding an empty list of registers to %s?", name()); + for (auto &adder: adders) { + const Addr offset = _base + _size; + + if (adder.reg) { + auto *reg = adder.reg.value(); + if (adder.offset && adder.offset.value() != offset) { + panic( + "Expected offset of register %s.%s to be %#x, is %#x.", + name(), reg->name(), adder.offset.value(), offset); + } + _offsetMap.emplace(offset, *reg); + _size += reg->size(); + } else if (adder.offset) { + if (adder.offset.value() != offset) { + panic("Expected current offset of %s to be %#x, is %#x.", + name(), adder.offset.value(), 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 534f86295b..b4bc969724 100644 --- a/src/dev/reg_bank.test.cc +++ b/src/dev/reg_bank.test.cc @@ -55,6 +55,7 @@ #include +#include "base/gtest/logging.hh" #include "dev/reg_bank.hh" using namespace gem5; @@ -64,6 +65,9 @@ using testing::ElementsAre; // 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 @@ TEST_F(RegisterBankTest, AddRegistersSize) 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)