[lldb] [test] Add unittest for DynamicRegisterInfo::Finalize()

[mgorny]

Add a helper function. Test the weird invalidate_regs expansion logic.

[mgorny]

Eliminate unused vars.

[labath]

lldb/unittests/Process/Utility/DynamicRegisterInfoTest.cpp

24

It would be better to create a fixture class, and make this a member variable of that class. If this ends up used from two tests, it will begin returning different values depending on the order of tests, whether they are run standalone, etc.

[mgorny]

Made fixture class. Now you made me think of adding a helper method to test register properties too.

[mgorny]

Create a helper class and move adding regs there. Add a helper function to assert on registers.

[mgorny]

Test for register name too.

[labath]

In general, I'd prefer to avoid adding new methods to the class under test, and save subclassing for the cases where it's impossible to do things otherwise (abstract class, visibility restrictions (although that often means you're not testing what you should be)). These things could live inside an gtest fixture (subclass of testing::Test), which could have a DynamicRegisterInfo member if needed.

lldb/unittests/Process/Utility/DynamicRegisterInfoTest.cpp

52–53

Maybe use SCOPED_TRACE instead?

62–94

I'd probably write a helper function to convert the register lists to a vector, and then have gtest compare the vectors:

std::vector<uint32_t> to_vector(uint32_t *regs) {
  std::vector<uint32_t> result;
  if (regs) {
    while(*regs != LLDB_INVALID_REGNUM)
      result.push_back(*regs++);
  }
  return result;
}
...
ASSERT_THAT(to_vector(info->value_regs), value_regs);

[mgorny]

Implemented all the suggested improvements.

lldb/unittests/Process/Utility/CMakeLists.txt

elseif (CMAKE_SYSTEM_NAME MATCHES "NetBSD")
   list(APPEND PLATFORM_SOURCES ${NETBSD_SOURCES})
 endif()
 
 set(LLVM_OPTIONAL_SOURCES
   ${FREEBSD_SOURCES}
   ${NETBSD_SOURCES})
 
 add_lldb_unittest(ProcessUtilityTests
-  RegisterContextTest.cpp
+  DynamicRegisterInfoTest.cpp
   LinuxProcMapsTest.cpp
   MemoryTagManagerAArch64MTETest.cpp
+  RegisterContextTest.cpp
   ${PLATFORM_SOURCES}
 
   LINK_LIBS
     lldbPluginProcessUtility
     LLVMTestingSupport)

lldb/unittests/Process/Utility/DynamicRegisterInfoTest.cpp

+//===-- DynamicRegisterInfoTest.cpp ---------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+#include "Plugins/Process/Utility/DynamicRegisterInfo.h"
+
+#include "lldb/Utility/ArchSpec.h"
+
+using namespace lldb_private;
+
+static std::vector<uint32_t> regs_to_vector(uint32_t *regs) {
+  std::vector<uint32_t> ret;
+  if (regs) {
+    while (*regs != LLDB_INVALID_REGNUM)
+      ret.push_back(*regs++);
+  }
+  return ret;
+}
+
+class DynamicRegisterInfoTest : public ::testing::Test {
+protected:
+  DynamicRegisterInfo info;
+  uint32_t next_regnum = 0;
+  ConstString group{"group"};
+
+  uint32_t AddTestRegister(const char *name, uint32_t byte_size,
+                           std::vector<uint32_t> value_regs = {},
+                           std::vector<uint32_t> invalidate_regs = {}) {
+    struct RegisterInfo new_reg {
+      name, nullptr, byte_size, LLDB_INVALID_INDEX32, lldb::eEncodingUint,
+          lldb::eFormatUnsigned,
+          {LLDB_INVALID_REGNUM, LLDB_INVALID_REGNUM, LLDB_INVALID_REGNUM,
+           next_regnum, next_regnum},
+          nullptr, nullptr, nullptr, 0
+    };
+
+    if (!value_regs.empty()) {
+      value_regs.push_back(LLDB_INVALID_REGNUM);
+      new_reg.value_regs = value_regs.data();
+    }
+    if (!invalidate_regs.empty()) {
+      invalidate_regs.push_back(LLDB_INVALID_REGNUM);
+      new_reg.invalidate_regs = invalidate_regs.data();
+    }
+
+    info.AddRegister(new_reg, group);
+    return next_regnum++;
+  }
+
+  void AssertRegisterInfo(uint32_t reg_num, const char *reg_name,
+                          uint32_t byte_offset,
+                          std::vector<uint32_t> value_regs = {},
+                          std::vector<uint32_t> invalidate_regs = {}) {
+    const RegisterInfo *reg = info.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);
+  }
+};
+
+#define ASSERT_REG(reg, ...) { \
+  SCOPED_TRACE("at register " #reg); \
+  AssertRegisterInfo(reg, #reg, __VA_ARGS__); \
+  }
+
+TEST_F(DynamicRegisterInfoTest, finalize_regs) {
+  // Add regular registers
+  uint32_t b1 = AddTestRegister("b1", 8);
+  uint32_t b2 = AddTestRegister("b2", 8);
+
+  // Add a few sub-registers
+  uint32_t s1 = AddTestRegister("s1", 4, {b1});
+  uint32_t s2 = AddTestRegister("s2", 4, {b2});
+
+  // Add a register with invalidate_regs
+  uint32_t i1 = AddTestRegister("i1", 8, {}, {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", 4, {b2}, {i1});
+
+  info.Finalize(lldb_private::ArchSpec());
+
+  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});
+}