[Lldb-commits] [PATCH] D91216: [lldb] [Process/FreeBSDRemote] Access GPR via reginfo offsets

[Pavel Labath via Phabricator via lldb-commits]

labath accepted this revision.
labath added inline comments.
This revision is now accepted and ready to land.


================
Comment at: lldb/unittests/Process/FreeBSD/RegisterContextFreeBSDTests.cpp:26-31
+#define ASSERT_REG(regname, offset, size)                                      \
+  {                                                                            \
+    const RegisterInfo *reg_info = &reg_ctx.GetRegisterInfo()[lldb_##regname]; \
+    EXPECT_EQ(reg_info->byte_offset, offset);                                  \
+    EXPECT_EQ(reg_info->byte_size, size);                                      \
+  }
----------------
mgorny wrote:
> labath wrote:
> > ```
> > pair<size_t(?), size_t> GetRegParams(const RegisterContext &ctx, unsigned reg) {
> >   const RegisterInfo &info = reg_ctx.GetRegisterInfo()[reg];
> >   return {info.byte_offset, info.byte_size};
> > }
> > ```
> Fun fact: `RegisterContextFreeBSD_*` is not a `RegisterContext&` but a `RegisterInfoInterface&` ;-).
Yeah, tell me about it...


================
Comment at: lldb/unittests/Process/Utility/CMakeLists.txt:1-2
+if (CMAKE_SYSTEM_NAME MATCHES "FreeBSD")
+  add_lldb_unittest(RegisterContextFreeBSDTests
+    RegisterContextFreeBSDTests.cpp
----------------
Rename to `ProcessUtilityTests` -- all files in this folder should go into a single binary. We have more tests coming in D87442.


================
Comment at: lldb/unittests/Process/Utility/CMakeLists.txt:3
+  add_lldb_unittest(RegisterContextFreeBSDTests
+    RegisterContextFreeBSDTests.cpp
+
----------------
It seems we have some files ending in `Tests`, but the prevalent convention is just `Test`


================
Comment at: lldb/unittests/Process/Utility/RegisterContextFreeBSDTests.cpp:38
+TEST(RegisterContextFreeBSDTest, x86_64) {
+  ArchSpec arch{"x86_64-unknown-freebsd12.2"};
+  RegisterContextFreeBSD_x86_64 reg_ctx{arch};
----------------
I guess the version is not really relevant for this..


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91216/new/

https://reviews.llvm.org/D91216