[Lldb-commits] [PATCH] D91504: [lldb] Use translated full ftag values

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Looks good. Some minor questions inline.



================
Comment at: lldb/source/Plugins/Process/FreeBSDRemote/NativeRegisterContextFreeBSD_x86_64.cpp:523
+  case DBRegSet: {
+    uint8_t *data = GetOffsetRegSetData(set, reg_info->byte_offset);
+    FXSAVE *fpr = reinterpret_cast<FXSAVE*>(m_fpr.data());
----------------
if you make data a `void*`, then you can remove the `reinterpret_cast` two lines below.


================
Comment at: lldb/source/Plugins/Process/FreeBSDRemote/NativeRegisterContextFreeBSD_x86_64.cpp:528
+    else
+      ::memcpy(GetOffsetRegSetData(set, reg_info->byte_offset),
+               reg_value.GetBytes(), reg_value.GetByteSize());
----------------
reuse data from above


================
Comment at: lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.cpp:537
+    uint16_t sw = m_xstate->fxsave.fstat;
+    llvm::ArrayRef<MMSReg> st_regs{m_xstate->fxsave.stmm, 8};
+    reg_value.SetUInt16(AbridgedToFullTagWord(abridged_tw, sw, st_regs));
----------------
Is this really needed? I would have hoped that just passing `m_xstate->fxsave.stmm` would be enough and that the c array constructor of ArrayRef would do the right thing...


================
Comment at: lldb/unittests/Process/Utility/RegisterContextTest.cpp:55
+  for (const TagWordTestVector &x : tag_word_test_vectors) {
+    std::array<MMSReg, 8> test_regs;
+    for (int i = 0; i < x.st_reg_num; ++i)
----------------
add `SCOPED_TRACE` macro to make it clear which test case failed, when it fails. You can use llvm::enumerate to produce sequence numbers for the test cases...


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

https://reviews.llvm.org/D91504