[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;
+
+TEST(DynamicRegisterInfoTest, finalize_regs) {
+  DynamicRegisterInfo info;
+  ConstString group{"group"};
+
+  uint32_t regnums[3] = {LLDB_INVALID_REGNUM, LLDB_INVALID_REGNUM,
+                         LLDB_INVALID_REGNUM};
+
+  // Add regular registers
+  struct RegisterInfo new_reg {
+    "b1", nullptr, 8, LLDB_INVALID_INDEX32, lldb::eEncodingUint,
+        lldb::eFormatUnsigned,
+        {LLDB_INVALID_REGNUM, LLDB_INVALID_REGNUM, LLDB_INVALID_REGNUM, 0, 0},
+        nullptr, nullptr, nullptr, 0
+  };
+  info.AddRegister(new_reg, group);
+  new_reg.name = "b2";
+  new_reg.kinds[lldb::eRegisterKindProcessPlugin]++;
+  new_reg.kinds[lldb::eRegisterKindLLDB]++;
+  info.AddRegister(new_reg, group);
+
+  // Add a few sub-registers
+  new_reg.name = "s1";
+  new_reg.byte_size = 4;
+  new_reg.kinds[lldb::eRegisterKindProcessPlugin]++;
+  new_reg.kinds[lldb::eRegisterKindLLDB]++;
+  regnums[0] = 0;
+  new_reg.value_regs = regnums;
+  info.AddRegister(new_reg, group);
+
+  new_reg.name = "s2";
+  new_reg.kinds[lldb::eRegisterKindProcessPlugin]++;
+  new_reg.kinds[lldb::eRegisterKindLLDB]++;
+  regnums[0] = 1;
+  info.AddRegister(new_reg, group);
+
+  // Add a register with weird invalidation
+  new_reg.name = "x";
+  new_reg.kinds[lldb::eRegisterKindProcessPlugin]++;
+  new_reg.kinds[lldb::eRegisterKindLLDB]++;
+  new_reg.value_regs = nullptr;
+  regnums[0] = 1;
+  regnums[1] = 0;
+  new_reg.invalidate_regs = regnums;
+  info.AddRegister(new_reg, group);
+
+  info.Finalize(lldb_private::ArchSpec());
+
+  const RegisterInfo *added_reg = info.GetRegisterInfoAtIndex(0);
+  ASSERT_NE(added_reg, nullptr);
+  EXPECT_EQ(added_reg->byte_offset, 0U);
+
+  added_reg = info.GetRegisterInfoAtIndex(1);
+  ASSERT_NE(added_reg, nullptr);
+  EXPECT_EQ(added_reg->byte_offset, 8U);
+
+  added_reg = info.GetRegisterInfoAtIndex(2);
+  ASSERT_NE(added_reg, nullptr);
+  EXPECT_EQ(added_reg->byte_offset, 0U);
+  EXPECT_EQ(added_reg->value_regs[0], 0U);
+  EXPECT_EQ(added_reg->value_regs[1], LLDB_INVALID_REGNUM);
+
+  added_reg = info.GetRegisterInfoAtIndex(3);
+  ASSERT_NE(added_reg, nullptr);
+  EXPECT_EQ(added_reg->byte_offset, 8U);
+  EXPECT_EQ(added_reg->value_regs[0], 1U);
+  EXPECT_EQ(added_reg->value_regs[1], LLDB_INVALID_REGNUM);
+
+  added_reg = info.GetRegisterInfoAtIndex(4);
+  ASSERT_NE(added_reg, nullptr);
+  EXPECT_EQ(added_reg->byte_offset, 16U);
+  EXPECT_EQ(added_reg->invalidate_regs[0], 0U);
+  EXPECT_EQ(added_reg->invalidate_regs[1], 1U);
+  EXPECT_EQ(added_reg->invalidate_regs[2], LLDB_INVALID_REGNUM);
+}