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 <yuhsingw@google.com>
Maintainer: Gabe Black <gabe.black@gmail.com>
Tested-by: kokoro <noreply+kokoro@google.com>

src/dev/reg_bank.hh

@@ -37,6 +37,7 @@
 #include <initializer_list>
 #include <iostream>
 #include <map>
+#include <optional>
 #include <sstream>
 #include <utility>
 
@@ -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<std::reference_wrapper<RegisterBase>> regs)
+    class RegisterAdder
     {
-        panic_if(regs.size() == 0, "Adding an empty list of registers to %s?",
-                 name());
-        for (auto &reg: regs) {
-            _offsetMap.emplace(_base + _size, reg);
-            _size += reg.get().size();
+      private:
+        std::optional<Addr> offset;
+        std::optional<RegisterBase *> 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<RegisterAdder> 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 &reg) { addRegisters({reg}); }
+    void addRegister(RegisterAdder reg) { addRegisters({reg}); }
 
     Addr base() const { return _base; }
     Addr size() const { return _size; }

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 @@ 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)