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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I like this. Some comments in the tests -- trying to reduce the amount of preprocessor magic..



================
Comment at: lldb/unittests/Process/FreeBSD/CMakeLists.txt:1
+add_lldb_unittest(RegisterContextFreeBSDTests
+  RegisterContextFreeBSDTests.cpp
----------------
I'd put this in `unittests/Process/Utility`, to match the location of the code being tested.


================
Comment at: lldb/unittests/Process/FreeBSD/CMakeLists.txt:8-9
+
+target_include_directories(RegisterContextFreeBSDTests PRIVATE
+  ${LLDB_SOURCE_DIR}/source/Plugins/Process/Utility)
----------------
Drop this, and include the file as `"Plugins/Process/Utility/..."`


================
Comment at: lldb/unittests/Process/FreeBSD/RegisterContextFreeBSDTests.cpp:16
+
+#if defined(__x86_64__) || defined(__i386__)
+#include "lldb-x86-register-enums.h"
----------------
These don't really have to be ifdefed, do they?


================
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);                                      \
+  }
----------------
```
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};
}
```


================
Comment at: lldb/unittests/Process/FreeBSD/RegisterContextFreeBSDTests.cpp:35
+
+#define ASSERT_GPR_X86_64(regname)                                             \
+  ASSERT_REG(regname##_x86_64, offsetof(reg, r_##regname),                     \
----------------
```
#define ASSERT_GPR_X86_64(regname)
  ASSERT_THAT(GetRegParams(reg_ctx, regname##_x86_64, testing::Pair(offsetof(reg, r_##regname), sizeof(reg::r_##regname))
```


================
Comment at: lldb/unittests/Process/FreeBSD/RegisterContextFreeBSDTests.cpp:72-74
+#if defined(__i386__)
+#define reg32 reg
+#endif
----------------
Inside the test function:
```
#ifdef __i386__
using native_i386_regs = ::reg32;
#else
using native_i386_regs = ::reg;
#endif
```


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

https://reviews.llvm.org/D91216