[lldb] [Process/FreeBSDRemote] Modernize and simplify YMM logic

[mgorny]

Summary

Eliminate the remaining swith-case code for register getters,
and migrate YMM registers to regset-oriented model. Since these
registers are recombined from XMM and YMM_Hi128 XSAVE blocks, while LLDB
gdb-server protocol transmits YMM registers whole, the offset-based
model will not work here. Nevertheless, some improvement was possible.

Replace generic 'XSaveRegSet' along with sub-sets for XSAVE components
with 'YMMRegSet' (and more regsets in the future as further components
are implemented). Create a helper GetYMMSplitReg() method that obtains
pointers to the appropriate XMM and YMM_Hi128 blocks to reduce code
duplication.

[labath]

lldb/source/Plugins/Process/FreeBSDRemote/NativeRegisterContextFreeBSD_x86_64.cpp

633

When does this return false? When the ymm regs are not available?

lldb/source/Plugins/Process/FreeBSDRemote/NativeRegisterContextFreeBSD_x86_64.h

88

void *&

[mgorny]

lldb/source/Plugins/Process/FreeBSDRemote/NativeRegisterContextFreeBSD_x86_64.cpp

633

Yes. Note that this change brings us closer to being able to disable whole regsets — I just need to figure out how to do it ;-).

lldb/source/Plugins/Process/FreeBSDRemote/NativeRegisterContextFreeBSD_x86_64.h

88

Are you sure about this? I feel like the whole & deal is quite confusing (read: shouldn't have happened). You don't know whether the method modifies the variables passed to it unless you look at the method prototype.

[labath]

lldb/source/Plugins/Process/FreeBSDRemote/NativeRegisterContextFreeBSD_x86_64.h

88

well.. there certainly are code styles which prohibit non-const reference arguments. However, llvm is not one of them. Also lldb has a issue with compulsive null checks, so I think it's important to communicate the fact the two arguments cannot be null, which reference arguments do.

Another possibility would be to just make this a real return value (pair<void *, void*> ?). The downside there is that it's not as self-documenting. Pick your poison...

[mgorny]

Changed prototype to use pointer referenec.

[mgorny]

Use a struct-based return type.

[labath]

lldb/source/Plugins/Process/FreeBSDRemote/NativeRegisterContextFreeBSD_x86_64.cpp

633

Ok, got it.

lldb/source/Plugins/Process/FreeBSDRemote/NativeRegisterContextFreeBSD_x86_64.h

35–38

Declare this inside the class (next to the function returning it), since it's really a private thing.

88

Ok, that feels a bit over-engineered, but works too...

[mgorny]

Split LLDB-visible regsets into separate AVX and MPX.

[mgorny]

Perform offsetting in GetOffsetRegSetData() to avoid UB.

[labath]

In D91293#2393764, @mgorny wrote:

Perform offsetting in GetOffsetRegSetData() to avoid UB.

Shouldn't have the last update gone to D91411 instead ?

[mgorny]

Upload the correct diff.

[mgorny]

In D91293#2393803, @labath wrote:

In D91293#2393764, @mgorny wrote:

Perform offsetting in GetOffsetRegSetData() to avoid UB.

Shouldn't have the last update gone to D91411 instead ?

Indeed.

[mgorny]

Restore old register set indices, to unbreak output.

(Note: I'm going to make that struct move soonish)

[mgorny]

Move YMMSplitPtr inside the class.

[mgorny]

Rename AVX/MPX regsets to avoid matching strings in commands/register/register/register_command/TestRegisters.py that cause the test to wrongly assume that I have MPX on this machine.

lldb/source/Plugins/Process/FreeBSDRemote/NativeRegisterContextFreeBSD_x86_64.h

 
 #define LLDB_INVALID_XSAVE_OFFSET UINT32_MAX
 
 namespace lldb_private {
 namespace process_freebsd {
 
 class NativeProcessFreeBSD;
 
+struct YMMSplitPtr {
+  void *xmm;
+  void *ymm_hi;
+};
+
 class NativeRegisterContextFreeBSD_x86_64
     : public NativeRegisterContextFreeBSD,
       public NativeRegisterContextWatchpoint_x86 {
 public:
   NativeRegisterContextFreeBSD_x86_64(const ArchSpec &target_arch,
                                       NativeThreadProtocol &native_thread);
   uint32_t GetRegisterSetCount() const override;
 
public:
   llvm::Error
   CopyHardwareWatchpointsFrom(NativeRegisterContextFreeBSD &source) override;
 
 private:
   // Private member types.
   enum RegSetKind {
     GPRegSet,
     FPRegSet,
-    XSaveRegSet,
     DBRegSet,
-  };
-  enum {
-    YMMXSaveSet,
-    MaxXSaveSet = YMMXSaveSet,
+    YMMRegSet,
+    MPXRegSet,
+    MaxRegSet = MPXRegSet,
   };
 
   // Private member variables.
   std::array<uint8_t, sizeof(struct reg)> m_gpr;
   std::array<uint8_t, 512> m_fpr; // FXSAVE
   std::array<uint8_t, sizeof(struct dbreg)> m_dbr;
   std::vector<uint8_t> m_xsave;
-  std::array<uint32_t, MaxXSaveSet + 1> m_xsave_offsets;
+  std::array<uint32_t, MaxRegSet + 1> m_xsave_offsets;
 
   llvm::Optional<enum RegSetKind> GetSetForNativeRegNum(int reg_num) const;
 
   Status ReadRegisterSet(uint32_t set);
   Status WriteRegisterSet(uint32_t set);
 
   size_t GetFPROffset() const;
   size_t GetDBROffset() const;
+  llvm::Optional<YMMSplitPtr> GetYMMSplitReg(uint32_t reg);
 };
 
 } // namespace process_freebsd
 } // namespace lldb_private
 
 #endif // #ifndef lldb_NativeRegisterContextFreeBSD_x86_64_h
 
 #endif // defined(__x86_64__)

lldb/source/Plugins/Process/FreeBSDRemote/NativeRegisterContextFreeBSD_x86_64.cpp

static const uint32_t g_fpu_regnums_x86_64[] = {
     lldb_xmm13_x86_64,     lldb_xmm14_x86_64, lldb_xmm15_x86_64,
     LLDB_INVALID_REGNUM // register sets need to end with this flag
 };
 static_assert((sizeof(g_fpu_regnums_x86_64) / sizeof(g_fpu_regnums_x86_64[0])) -
                       1 ==
                   k_num_fpr_registers_x86_64,
               "g_fpu_regnums_x86_64 has wrong number of register infos");
 
-// x86 64-bit registers available via XState.
-static const uint32_t g_xstate_regnums_x86_64[] = {
-    lldb_ymm0_x86_64, lldb_ymm1_x86_64, lldb_ymm2_x86_64, lldb_ymm3_x86_64,
-    lldb_ymm4_x86_64, lldb_ymm5_x86_64, lldb_ymm6_x86_64, lldb_ymm7_x86_64,
-    lldb_ymm8_x86_64, lldb_ymm9_x86_64, lldb_ymm10_x86_64, lldb_ymm11_x86_64,
-    lldb_ymm12_x86_64, lldb_ymm13_x86_64, lldb_ymm14_x86_64, lldb_ymm15_x86_64,
+static const uint32_t g_avx_regnums_x86_64[] = {
+    lldb_ymm0_x86_64,   lldb_ymm1_x86_64,  lldb_ymm2_x86_64,  lldb_ymm3_x86_64,
+    lldb_ymm4_x86_64,   lldb_ymm5_x86_64,  lldb_ymm6_x86_64,  lldb_ymm7_x86_64,
+    lldb_ymm8_x86_64,   lldb_ymm9_x86_64,  lldb_ymm10_x86_64, lldb_ymm11_x86_64,
+    lldb_ymm12_x86_64,  lldb_ymm13_x86_64, lldb_ymm14_x86_64, lldb_ymm15_x86_64,
+    LLDB_INVALID_REGNUM // register sets need to end with this flag
+};
+static_assert((sizeof(g_avx_regnums_x86_64) / sizeof(g_avx_regnums_x86_64[0])) -
+                      1 ==
+                  k_num_avx_registers_x86_64,
+              "g_avx_regnums_x86_64 has wrong number of register infos");
+
+static const uint32_t g_mpx_regnums_x86_64[] = {
     // Note: we currently do not provide them but this is needed to avoid
     // unnamed groups in SBFrame::GetRegisterContext().
-    lldb_bnd0_x86_64, lldb_bnd1_x86_64, lldb_bnd2_x86_64, lldb_bnd3_x86_64,
-    lldb_bndcfgu_x86_64, lldb_bndstatus_x86_64,
+    lldb_bnd0_x86_64,   lldb_bnd1_x86_64,    lldb_bnd2_x86_64,
+    lldb_bnd3_x86_64,   lldb_bndcfgu_x86_64, lldb_bndstatus_x86_64,
     LLDB_INVALID_REGNUM // register sets need to end with this flag
 };
-static_assert((sizeof(g_xstate_regnums_x86_64) /
-               sizeof(g_xstate_regnums_x86_64[0])) -
+static_assert((sizeof(g_mpx_regnums_x86_64) / sizeof(g_mpx_regnums_x86_64[0])) -
                       1 ==
-                  k_num_avx_registers_x86_64 + k_num_mpx_registers_x86_64,
-              "g_xstate_regnums_x86_64 has wrong number of register infos");
+                  k_num_mpx_registers_x86_64,
+              "g_mpx_regnums_x86_64 has wrong number of register infos");
 
 // x86 debug registers.
 static const uint32_t g_dbr_regnums_x86_64[] = {
     lldb_dr0_x86_64,    lldb_dr1_x86_64, lldb_dr2_x86_64, lldb_dr3_x86_64,
     lldb_dr4_x86_64,    lldb_dr5_x86_64, lldb_dr6_x86_64, lldb_dr7_x86_64,
     LLDB_INVALID_REGNUM // register sets need to end with this flag
 };
 static_assert((sizeof(g_dbr_regnums_x86_64) / sizeof(g_dbr_regnums_x86_64[0])) -
const uint32_t g_fpu_regnums_i386[] = {
     lldb_xmm6_i386,     lldb_xmm7_i386,
     LLDB_INVALID_REGNUM // register sets need to end with this flag
 };
 static_assert((sizeof(g_fpu_regnums_i386) / sizeof(g_fpu_regnums_i386[0])) -
                       1 ==
                   k_num_fpr_registers_i386,
               "g_fpu_regnums_i386 has wrong number of register infos");
 
-// x86 64-bit registers available via XState.
-static const uint32_t g_xstate_regnums_i386[] = {
-    lldb_ymm0_i386, lldb_ymm1_i386, lldb_ymm2_i386, lldb_ymm3_i386,
-    lldb_ymm4_i386, lldb_ymm5_i386, lldb_ymm6_i386, lldb_ymm7_i386,
+static const uint32_t g_avx_regnums_i386[] = {
+    lldb_ymm0_i386,     lldb_ymm1_i386, lldb_ymm2_i386, lldb_ymm3_i386,
+    lldb_ymm4_i386,     lldb_ymm5_i386, lldb_ymm6_i386, lldb_ymm7_i386,
+    LLDB_INVALID_REGNUM // register sets need to end with this flag
+};
+static_assert((sizeof(g_avx_regnums_i386) / sizeof(g_avx_regnums_i386[0])) -
+                      1 ==
+                  k_num_avx_registers_i386,
+              "g_avx_regnums_i386 has wrong number of register infos");
+
+static const uint32_t g_mpx_regnums_i386[] = {
     // Note: we currently do not provide them but this is needed to avoid
     // unnamed groups in SBFrame::GetRegisterContext().
-    lldb_bnd0_i386, lldb_bnd1_i386, lldb_bnd2_i386, lldb_bnd3_i386,
-    lldb_bndcfgu_i386, lldb_bndstatus_i386,
+    lldb_bnd0_i386,     lldb_bnd1_i386,    lldb_bnd2_i386,
+    lldb_bnd3_i386,     lldb_bndcfgu_i386, lldb_bndstatus_i386,
     LLDB_INVALID_REGNUM // register sets need to end with this flag
 };
-static_assert(
-    (sizeof(g_xstate_regnums_i386) / sizeof(g_xstate_regnums_i386[0])) - 1 ==
-        k_num_avx_registers_i386 + k_num_mpx_registers_i386,
-    "g_xstate_regnums_i386 has wrong number of register infos");
+static_assert((sizeof(g_mpx_regnums_i386) / sizeof(g_mpx_regnums_i386[0])) -
+                      1 ==
+                  k_num_mpx_registers_i386,
+              "g_mpx_regnums_i386 has wrong number of register infos");
 
 // x86 debug registers.
 static const uint32_t g_dbr_regnums_i386[] = {
     lldb_dr0_i386,      lldb_dr1_i386, lldb_dr2_i386, lldb_dr3_i386,
     lldb_dr4_i386,      lldb_dr5_i386, lldb_dr6_i386, lldb_dr7_i386,
     LLDB_INVALID_REGNUM // register sets need to end with this flag
 };
 static_assert((sizeof(g_dbr_regnums_i386) / sizeof(g_dbr_regnums_i386[0])) -
                       1 ==
                   k_num_dbr_registers_i386,
               "g_dbr_regnums_i386 has wrong number of register infos");
 
 // Number of register sets provided by this context.
-enum { k_num_register_sets = 4 };
+enum { k_num_register_sets = 5 };
 
 // Register sets for x86 32-bit.
 static const RegisterSet g_reg_sets_i386[k_num_register_sets] = {
     {"General Purpose Registers", "gpr", k_num_gpr_registers_i386,
      g_gpr_regnums_i386},
     {"Floating Point Registers", "fpu", k_num_fpr_registers_i386,
      g_fpu_regnums_i386},
-    {"Extended State Registers", "xstate",
-     k_num_avx_registers_i386 + k_num_mpx_registers_i386,
-     g_xstate_regnums_i386},
     {"Debug Registers", "dbr", k_num_dbr_registers_i386, g_dbr_regnums_i386},
+    {"Advanced Vector Extensions", "avx", k_num_avx_registers_i386,
+     g_avx_regnums_i386},
+    {"Memory Protection Extensions", "mpx", k_num_mpx_registers_i386,
+     g_mpx_regnums_i386},
 };
 
 // Register sets for x86 64-bit.
 static const RegisterSet g_reg_sets_x86_64[k_num_register_sets] = {
     {"General Purpose Registers", "gpr", k_num_gpr_registers_x86_64,
      g_gpr_regnums_x86_64},
     {"Floating Point Registers", "fpu", k_num_fpr_registers_x86_64,
      g_fpu_regnums_x86_64},
-    {"Extended State Registers", "xstate",
-     k_num_avx_registers_x86_64 + k_num_mpx_registers_x86_64,
-     g_xstate_regnums_x86_64},
     {"Debug Registers", "dbr", k_num_dbr_registers_x86_64,
      g_dbr_regnums_x86_64},
+    {"Advanced Vector Extensions", "avx", k_num_avx_registers_x86_64,
+     g_avx_regnums_x86_64},
+    {"Memory Protection Extensions", "mpx", k_num_mpx_registers_x86_64,
+     g_mpx_regnums_x86_64},
 };
 
 #define REG_CONTEXT_SIZE (GetRegisterInfoInterface().GetGPRSize())
 } // namespace
 
 NativeRegisterContextFreeBSD *
 NativeRegisterContextFreeBSD::CreateHostNativeRegisterContextFreeBSD(
     const ArchSpec &target_arch, NativeThreadProtocol &native_thread) {

 
 NativeRegisterContextFreeBSD_x86_64::NativeRegisterContextFreeBSD_x86_64(
     const ArchSpec &target_arch, NativeThreadProtocol &native_thread)
     : NativeRegisterContextRegisterInfo(
           native_thread, CreateRegisterInfoInterface(target_arch)) {
   assert(m_gpr.size() == GetRegisterInfoInterface().GetGPRSize());
 }
 
-// CONSIDER after local and llgs debugging are merged, register set support can
-// be moved into a base x86-64 class with IsRegisterSetAvailable made virtual.
 uint32_t NativeRegisterContextFreeBSD_x86_64::GetRegisterSetCount() const {
   uint32_t sets = 0;
   for (uint32_t set_index = 0; set_index < k_num_register_sets; ++set_index) {
     if (GetSetForNativeRegNum(set_index))
       ++sets;
   }
 
   return sets;
 }
 
 const RegisterSet *
 NativeRegisterContextFreeBSD_x86_64::GetRegisterSet(uint32_t set_index) const {
   switch (GetRegisterInfoInterface().GetTargetArchitecture().GetMachine()) {
   case llvm::Triple::x86:
     return &g_reg_sets_i386[set_index];
   case llvm::Triple::x86_64:
     return &g_reg_sets_x86_64[set_index];
   default:
     llvm_unreachable("Unhandled target architecture.");
   }
 }
 
-static constexpr int RegNumX86ToX86_64(int regnum) {
-  switch (regnum) {
-  case lldb_ymm0_i386:
-  case lldb_ymm1_i386:
-  case lldb_ymm2_i386:
-  case lldb_ymm3_i386:
-  case lldb_ymm4_i386:
-  case lldb_ymm5_i386:
-  case lldb_ymm6_i386:
-  case lldb_ymm7_i386:
-    return lldb_ymm0_x86_64 + regnum - lldb_ymm0_i386;
-  case lldb_bnd0_i386:
-  case lldb_bnd1_i386:
-  case lldb_bnd2_i386:
-  case lldb_bnd3_i386:
-    return lldb_bnd0_x86_64 + regnum - lldb_bnd0_i386;
-  case lldb_bndcfgu_i386:
-    return lldb_bndcfgu_x86_64;
-  case lldb_bndstatus_i386:
-    return lldb_bndstatus_x86_64;
-  default:
-    llvm_unreachable("Unhandled i386 register.");
-  }
-}
-
 llvm::Optional<enum NativeRegisterContextFreeBSD_x86_64::RegSetKind>
 NativeRegisterContextFreeBSD_x86_64::GetSetForNativeRegNum(int reg_num) const {
   switch (GetRegisterInfoInterface().GetTargetArchitecture().GetMachine()) {
   case llvm::Triple::x86:
     if (reg_num >= k_first_gpr_i386 && reg_num <= k_last_gpr_i386)
       return GPRegSet;
     if (reg_num >= k_first_fpr_i386 && reg_num <= k_last_fpr_i386)
       return FPRegSet;
     if (reg_num >= k_first_avx_i386 && reg_num <= k_last_avx_i386)
-      return XSaveRegSet; // AVX
+      return YMMRegSet;
     if (reg_num >= k_first_mpxr_i386 && reg_num <= k_last_mpxr_i386)
       return llvm::None; // MPXR
     if (reg_num >= k_first_mpxc_i386 && reg_num <= k_last_mpxc_i386)
       return llvm::None; // MPXC
     if (reg_num >= k_first_dbr_i386 && reg_num <= k_last_dbr_i386)
       return DBRegSet; // DBR
     break;
   case llvm::Triple::x86_64:
     if (reg_num >= k_first_gpr_x86_64 && reg_num <= k_last_gpr_x86_64)
       return GPRegSet;
     if (reg_num >= k_first_fpr_x86_64 && reg_num <= k_last_fpr_x86_64)
       return FPRegSet;
     if (reg_num >= k_first_avx_x86_64 && reg_num <= k_last_avx_x86_64)
-      return XSaveRegSet; // AVX
+      return YMMRegSet;
     if (reg_num >= k_first_mpxr_x86_64 && reg_num <= k_last_mpxr_x86_64)
       return llvm::None; // MPXR
     if (reg_num >= k_first_mpxc_x86_64 && reg_num <= k_last_mpxc_x86_64)
       return llvm::None; // MPXC
     if (reg_num >= k_first_dbr_x86_64 && reg_num <= k_last_dbr_x86_64)
       return DBRegSet; // DBR
     break;
   default:
return NativeProcessFreeBSD::PtraceWrapper(PT_GETFPREGS, m_thread.GetID(),
                                                m_fpr.data());
 #else
     return NativeProcessFreeBSD::PtraceWrapper(PT_GETXMMREGS, m_thread.GetID(),
                                                m_fpr.data());
 #endif
   case DBRegSet:
     return NativeProcessFreeBSD::PtraceWrapper(PT_GETDBREGS, m_thread.GetID(),
                                                m_dbr.data());
-  case XSaveRegSet: {
+  case YMMRegSet:
+  case MPXRegSet: {
     struct ptrace_xstate_info info;
     Status ret = NativeProcessFreeBSD::PtraceWrapper(
         PT_GETXSTATE_INFO, GetProcessPid(), &info, sizeof(info));
     if (!ret.Success())
       return ret;
 
     assert(info.xsave_mask & XFEATURE_ENABLED_X87);
     assert(info.xsave_mask & XFEATURE_ENABLED_SSE);
 
-    m_xsave_offsets[YMMXSaveSet] = LLDB_INVALID_XSAVE_OFFSET;
+    m_xsave_offsets[YMMRegSet] = LLDB_INVALID_XSAVE_OFFSET;
     if (info.xsave_mask & XFEATURE_ENABLED_YMM_HI128) {
       uint32_t eax, ecx, edx;
-      __get_cpuid_count(0x0D, 2, &eax, &m_xsave_offsets[YMMXSaveSet], &ecx,
-                        &edx);
+      __get_cpuid_count(0x0D, 2, &eax, &m_xsave_offsets[YMMRegSet], &ecx, &edx);
     }
 
     m_xsave.resize(info.xsave_len);
     return NativeProcessFreeBSD::PtraceWrapper(PT_GETXSTATE, GetProcessPid(),
                                                m_xsave.data(), m_xsave.size());
   }
   }
   llvm_unreachable("NativeRegisterContextFreeBSD_x86_64::ReadRegisterSet");
return NativeProcessFreeBSD::PtraceWrapper(PT_SETFPREGS, m_thread.GetID(),
                                                m_fpr.data());
 #else
     return NativeProcessFreeBSD::PtraceWrapper(PT_SETXMMREGS, m_thread.GetID(),
                                                m_fpr.data());
 #endif
   case DBRegSet:
     return NativeProcessFreeBSD::PtraceWrapper(PT_SETDBREGS, m_thread.GetID(),
                                                m_dbr.data());
-  case XSaveRegSet:
+  case YMMRegSet:
+  case MPXRegSet:
     // ReadRegisterSet() must always be called before WriteRegisterSet().
     assert(m_xsave.size() > 0);
     return NativeProcessFreeBSD::PtraceWrapper(PT_SETXSTATE, GetProcessPid(),
                                                m_xsave.data(), m_xsave.size());
   }
   llvm_unreachable("NativeRegisterContextFreeBSD_x86_64::WriteRegisterSet");
 }
 
NativeRegisterContextFreeBSD_x86_64::ReadRegister(const RegisterInfo *reg_info,
   error = ReadRegisterSet(set);
   if (error.Fail())
     return error;
 
   switch (set) {
   case GPRegSet:
     reg_value.SetBytes(m_gpr.data() + reg_info->byte_offset,
                        reg_info->byte_size, endian::InlHostByteOrder());
-    return error;
+    break;
   case FPRegSet:
     reg_value.SetBytes(m_fpr.data() + reg_info->byte_offset - GetFPROffset(),
                        reg_info->byte_size, endian::InlHostByteOrder());
-    return error;
+    break;
   case DBRegSet:
     reg_value.SetBytes(m_dbr.data() + reg_info->byte_offset - GetDBROffset(),
                        reg_info->byte_size, endian::InlHostByteOrder());
-    return error;
-  case XSaveRegSet:
-    // legacy logic
     break;
-  }
-
-  switch (GetRegisterInfoInterface().GetTargetArchitecture().GetMachine()) {
-  case llvm::Triple::x86_64:
-    break;
-  case llvm::Triple::x86:
-    reg = RegNumX86ToX86_64(reg);
-    break;
-  default:
-    llvm_unreachable("Unhandled target architecture.");
-  }
-
-  switch (reg) {
-  case lldb_ymm0_x86_64:
-  case lldb_ymm1_x86_64:
-  case lldb_ymm2_x86_64:
-  case lldb_ymm3_x86_64:
-  case lldb_ymm4_x86_64:
-  case lldb_ymm5_x86_64:
-  case lldb_ymm6_x86_64:
-  case lldb_ymm7_x86_64:
-  case lldb_ymm8_x86_64:
-  case lldb_ymm9_x86_64:
-  case lldb_ymm10_x86_64:
-  case lldb_ymm11_x86_64:
-  case lldb_ymm12_x86_64:
-  case lldb_ymm13_x86_64:
-  case lldb_ymm14_x86_64:
-  case lldb_ymm15_x86_64: {
-    uint32_t offset = m_xsave_offsets[YMMXSaveSet];
-    if (offset == LLDB_INVALID_XSAVE_OFFSET) {
+  case YMMRegSet: {
+    llvm::Optional<YMMSplitPtr> ymm_reg = GetYMMSplitReg(reg);
+    if (!ymm_reg) {
       error.SetErrorStringWithFormat(
           "register \"%s\" not supported by CPU/kernel", reg_info->name);
     } else {
-      uint32_t reg_index = reg - lldb_ymm0_x86_64;
-      auto *fpreg = reinterpret_cast<struct savexmm_ymm *>(m_xsave.data());
-      auto *ymmreg = reinterpret_cast<struct ymmacc *>(m_xsave.data() + offset);
-
-      YMMReg ymm =
-          XStateToYMM(reinterpret_cast<void *>(&fpreg->sv_xmm[reg_index]),
-                      reinterpret_cast<void *>(&ymmreg[reg_index]));
+      YMMReg ymm = XStateToYMM(ymm_reg->xmm, ymm_reg->ymm_hi);
       reg_value.SetBytes(ymm.bytes, reg_info->byte_size,
                          endian::InlHostByteOrder());
     }
     break;
   }
-  default:
-    llvm_unreachable("Reading unknown/unsupported register");
+  case MPXRegSet:
+    llvm_unreachable("MPX regset should have returned error");
   }
 
   return error;
 }
 
 Status NativeRegisterContextFreeBSD_x86_64::WriteRegister(
     const RegisterInfo *reg_info, const RegisterValue &reg_value) {
 
Status NativeRegisterContextFreeBSD_x86_64::WriteRegister(
   error = ReadRegisterSet(set);
   if (error.Fail())
     return error;
 
   switch (set) {
   case GPRegSet:
     ::memcpy(m_gpr.data() + reg_info->byte_offset, reg_value.GetBytes(),
              reg_value.GetByteSize());
-    return WriteRegisterSet(set);
+    break;
   case FPRegSet:
     ::memcpy(m_fpr.data() + reg_info->byte_offset - GetFPROffset(),
              reg_value.GetBytes(), reg_value.GetByteSize());
-    return WriteRegisterSet(set);
+    break;
   case DBRegSet:
     ::memcpy(m_dbr.data() + reg_info->byte_offset - GetDBROffset(),
              reg_value.GetBytes(), reg_value.GetByteSize());
-    return WriteRegisterSet(set);
-  case XSaveRegSet:
-    // legacy logic
-    break;
-  }
-
-  switch (GetRegisterInfoInterface().GetTargetArchitecture().GetMachine()) {
-  case llvm::Triple::x86_64:
     break;
-  case llvm::Triple::x86:
-    reg = RegNumX86ToX86_64(reg);
-    break;
-  default:
-    llvm_unreachable("Unhandled target architecture.");
-  }
-
-  switch (reg) {
-  case lldb_ymm0_x86_64:
-  case lldb_ymm1_x86_64:
-  case lldb_ymm2_x86_64:
-  case lldb_ymm3_x86_64:
-  case lldb_ymm4_x86_64:
-  case lldb_ymm5_x86_64:
-  case lldb_ymm6_x86_64:
-  case lldb_ymm7_x86_64:
-  case lldb_ymm8_x86_64:
-  case lldb_ymm9_x86_64:
-  case lldb_ymm10_x86_64:
-  case lldb_ymm11_x86_64:
-  case lldb_ymm12_x86_64:
-  case lldb_ymm13_x86_64:
-  case lldb_ymm14_x86_64:
-  case lldb_ymm15_x86_64: {
-    uint32_t offset = m_xsave_offsets[YMMXSaveSet];
-    if (offset == LLDB_INVALID_XSAVE_OFFSET) {
+  case YMMRegSet: {
+    llvm::Optional<YMMSplitPtr> ymm_reg = GetYMMSplitReg(reg);
+    if (!ymm_reg) {
       error.SetErrorStringWithFormat(
           "register \"%s\" not supported by CPU/kernel", reg_info->name);
     } else {
-      uint32_t reg_index = reg - lldb_ymm0_x86_64;
-      auto *fpreg = reinterpret_cast<struct savexmm_ymm *>(m_xsave.data());
-      auto *ymmreg = reinterpret_cast<struct ymmacc *>(m_xsave.data() + offset);
-
       YMMReg ymm;
       ::memcpy(ymm.bytes, reg_value.GetBytes(), reg_value.GetByteSize());
-      YMMToXState(ymm, reinterpret_cast<void *>(&fpreg->sv_xmm[reg_index]),
-                  reinterpret_cast<void *>(&ymmreg[reg_index]));
+      YMMToXState(ymm, ymm_reg->xmm, ymm_reg->ymm_hi);
     }
     break;
   }
-  default:
-    llvm_unreachable("Reading unknown/unsupported register");
+  case MPXRegSet:
+    llvm_unreachable("MPX regset should have returned error");
   }
 
   return WriteRegisterSet(set);
 }
 
 Status NativeRegisterContextFreeBSD_x86_64::ReadAllRegisterValues(
     lldb::DataBufferSP &data_sp) {
   Status error;
case llvm::Triple::x86_64:
     break;
   default:
     llvm_unreachable("Unhandled target architecture.");
   }
 
   return GetRegisterInfoInterface().GetRegisterInfo()[regno].byte_offset;
 }
 
+llvm::Optional<YMMSplitPtr>
+NativeRegisterContextFreeBSD_x86_64::GetYMMSplitReg(uint32_t reg) {
+  uint32_t offset = m_xsave_offsets[YMMRegSet];
+  if (offset == LLDB_INVALID_XSAVE_OFFSET)
+    return llvm::None;
+
+  uint32_t reg_index;
+  switch (GetRegisterInfoInterface().GetTargetArchitecture().GetMachine()) {
+  case llvm::Triple::x86:
+    reg_index = reg - lldb_ymm0_i386;
+    break;
+  case llvm::Triple::x86_64:
+    reg_index = reg - lldb_ymm0_x86_64;
+    break;
+  default:
+    llvm_unreachable("Unhandled target architecture.");
+  }
+
+  auto *fpreg = reinterpret_cast<struct savexmm_ymm *>(m_xsave.data());
+  auto *ymmreg = reinterpret_cast<struct ymmacc *>(m_xsave.data() + offset);
+
+  return YMMSplitPtr{&fpreg->sv_xmm[reg_index], &ymmreg[reg_index]};
+}
+
 #endif // defined(__x86_64__)