[lldb] [ABI/X86] Support combining xmm* and ymm*h regs into ymm*

[mgorny]

Summary

gdbserver does not expose combined ymm* registers but rather XSAVE-style
split xmm* and ymm*h portions. Extend value_regs to support combining
multiple registers and use it to create user-friendly ymm* registers
that are combined from split xmm* and ymm*h portions.

[mgorny]

Rebase to use the new ABIX86::AugmentRegisterInfo() API.

[labath]

I like the solution with multiple value_regs.

However, I am wondering if instead of the various piecemeal maps we couldn't just have one uber-map to rule them all. I'm imagining something like

enum (class?) RegKind { GPR, FPR, XMM, ...};
struct RegData {
  size_t index;
  StringRef name;
  SmallVector<StringRef> subregs;
};

map<pair<RegKind, unsigned>, RegData> reg_map = makeRegMap(); // whichever kind of a map
// or possibly, map<RegKind, map<unsigned, RegData>> = ...

for (const auto &kv : reg_map)
  subreg_name_set.insert(kv.second.subregs.begin(), kv.second.subregs.end());

for (const auto &x : llvm::enumerate(regs)) {
  llvm::StringRef reg_name = x.value().name.GetStringRef();
  if (llvm::is_contained(subreg_name_set, reg_name))
    return;
  if (Optional<pair<RegKind, size_t>> reg = get_reg(reg_name))
    reg_map[reg].index = x.index(); 
}

...

This would allow us to factor out two tedious jobs (constructing the register map and identifying registers) into separate functions, leaving only the only (simplified, I hope) core logic for the main function. The get_reg function could use whichever method is easiest for the translation of register names, or even a combination of methods (a StringSwitch for gprs and some consume_fronts for the regularly-named registers).

WDYT?

lldb/source/Plugins/ABI/X86/ABIX86.cpp

235

For a simple list like this, I'd probably remove the trailing comma to have clang-format format it more succinctly.

[mgorny]

I'm not sure what exactly you're proposing and whether your proposal would work but I think it's a step in the right direction.

Let's start by reiterating what we need:

1. A way to iterate over all subregisters of given RegKind, sorted by base register index, yielding base register index and subregister name (map<RegKind, vector<...>>.
2. A way to match register name against all *expected* subregisters in order to abort if any of them exists (set<StringRef>).
3. A way to match register name against all known base registers and store their indices (map<StringRef, ...>).
4. We should be able to construct all of the above from some readable input.

lldb/source/Plugins/ABI/X86/ABIX86.cpp

235

Heh, I was wondering why clang-format chooses one format sometimes, and the other sometimes but it wouldn't occur to me that it's due to the trailing comma ;-).

[mgorny]

WIP alternative approach.

[labath]

I think I like this.

lldb/source/Plugins/ABI/X86/ABIX86.cpp

300

llvm::Optional gives you this assert for free.

360

It would be nice to have some comment explaining the purpose of this variable (I guess its there to ensure matching register order).

[mgorny]

Major refactoring of the generator function.

[labath]

Looks good. Thanks for your patience.

lldb/source/Plugins/ABI/X86/ABIX86.cpp

36–64

anonymous namespace here

lldb/include/lldb/lldb-private-types.h

struct RegisterInfo {
   lldb::Encoding encoding;
   /// Default display format.
   lldb::Format format;
   /// Holds all of the various register numbers for all register kinds.
   uint32_t kinds[lldb::kNumRegisterKinds]; //
   /// List of registers (terminated with LLDB_INVALID_REGNUM). If this value is
   /// not null, all registers in this list will be read first, at which point
   /// the value for this register will be valid. For example, the value list
-  /// for ah would be eax (x86) or rax (x64).
-  uint32_t *value_regs; //
+  /// for ah would be eax (x86) or rax (x64). Register numbers are
+  /// of eRegisterKindLLDB. If multiple registers are listed, the final
+  /// value will be the concatenation of them.
+  uint32_t *value_regs;
   /// List of registers (terminated with LLDB_INVALID_REGNUM). If this value is
   /// not null, all registers in this list will be invalidated when the value of
   /// this register changes. For example, the invalidate list for eax would be
   /// rax ax, ah, and al.
   uint32_t *invalidate_regs;
 
   llvm::ArrayRef<uint8_t> data(const uint8_t *context_base) const {
     return llvm::ArrayRef<uint8_t>(context_base + byte_offset, byte_size);

lldb/source/Plugins/ABI/X86/ABIX86.cpp

 
 void ABIX86::Terminate() {
   ABIMacOSX_i386::Terminate();
   ABISysV_i386::Terminate();
   ABISysV_x86_64::Terminate();
   ABIWindows_x86_64::Terminate();
 }
 
-enum class RegKind {
-  GPR32 = 0,
+namespace {
+enum RegKind {
+  GPR32,
   GPR16,
   GPR8h,
   GPR8,
+  MM,
+  YMM_YMMh,
+  YMM_XMM,
 
-  MM = 0,
+  RegKindCount
+};
+};
+
+struct RegData {
+  RegKind subreg_kind;
+  llvm::StringRef subreg_name;
+  llvm::Optional<uint32_t> base_index;
 };
 
-typedef llvm::SmallDenseMap<llvm::StringRef,
-                            llvm::SmallVector<llvm::StringRef, 4>, 16>
-    RegisterMap;
-
-static void addPartialRegisters(
-    std::vector<DynamicRegisterInfo::Register> &regs,
-    llvm::ArrayRef<uint32_t> base_reg_indices, const RegisterMap &reg_names,
-    uint32_t base_size, RegKind name_index, lldb::Encoding encoding,
-    lldb::Format format, uint32_t subreg_size, uint32_t subreg_offset = 0) {
-  for (uint32_t base_index : base_reg_indices) {
-    if (base_index == LLDB_INVALID_REGNUM)
-      break;
-    assert(base_index < regs.size());
+static void
+addPartialRegisters(std::vector<DynamicRegisterInfo::Register> &regs,
+                    llvm::ArrayRef<RegData *> subregs, uint32_t base_size,
+                    lldb::Encoding encoding, lldb::Format format,
+                    uint32_t subreg_size, uint32_t subreg_offset = 0) {
+  for (const RegData *subreg : subregs) {
+    assert(subreg);
+    uint32_t base_index = subreg->base_index.getValue();
     DynamicRegisterInfo::Register &full_reg = regs[base_index];
-    llvm::StringRef subreg_name = reg_names.lookup(
-        full_reg.name.GetStringRef())[static_cast<int>(name_index)];
-    if (subreg_name.empty() || full_reg.byte_size != base_size)
+    if (full_reg.byte_size != base_size)
       continue;
 
-    lldb_private::DynamicRegisterInfo::Register subreg{
-        lldb_private::ConstString(subreg_name),
+    lldb_private::DynamicRegisterInfo::Register new_reg{
+        lldb_private::ConstString(subreg->subreg_name),
         lldb_private::ConstString(),
         lldb_private::ConstString("supplementary registers"),
         subreg_size,
         LLDB_INVALID_INDEX32,
         encoding,
         format,
         LLDB_INVALID_REGNUM,
         LLDB_INVALID_REGNUM,
         LLDB_INVALID_REGNUM,
         LLDB_INVALID_REGNUM,
         {base_index},
         {},
         subreg_offset};
 
-    addSupplementaryRegister(regs, subreg);
+    addSupplementaryRegister(regs, new_reg);
+  }
+}
+
+static void
+addCombinedRegisters(std::vector<DynamicRegisterInfo::Register> &regs,
+                     llvm::ArrayRef<RegData *> subregs1,
+                     llvm::ArrayRef<RegData *> subregs2, uint32_t base_size,
+                     lldb::Encoding encoding, lldb::Format format) {
+  for (auto it : llvm::zip(subregs1, subregs2)) {
+    RegData *regdata1, *regdata2;
+    std::tie(regdata1, regdata2) = it;
+    assert(regdata1);
+    assert(regdata2);
+
+    // verify that we've got matching target registers
+    if (regdata1->subreg_name != regdata2->subreg_name)
+      continue;
+
+    uint32_t base_index1 = regdata1->base_index.getValue();
+    uint32_t base_index2 = regdata2->base_index.getValue();
+    if (regs[base_index1].byte_size != base_size ||
+        regs[base_index2].byte_size != base_size)
+      continue;
+
+    lldb_private::DynamicRegisterInfo::Register new_reg{
+        lldb_private::ConstString(regdata1->subreg_name),
+        lldb_private::ConstString(),
+        lldb_private::ConstString("supplementary registers"),
+        base_size * 2,
+        LLDB_INVALID_INDEX32,
+        encoding,
+        format,
+        LLDB_INVALID_REGNUM,
+        LLDB_INVALID_REGNUM,
+        LLDB_INVALID_REGNUM,
+        LLDB_INVALID_REGNUM,
+        {base_index1, base_index2},
+        {}};
+
+    addSupplementaryRegister(regs, new_reg);
+  }
+}
+
+typedef llvm::SmallDenseMap<llvm::StringRef, llvm::SmallVector<RegData, 4>, 64>
+    BaseRegToRegsMap;
+
+#define GPRh(l)                                                                \
+  {                                                                            \
+    is64bit ? BaseRegToRegsMap::value_type("r" l "x", {{GPR32, "e" l "x"},     \
+                                                       {GPR16, l "x"},         \
+                                                       {GPR8h, l "h"},         \
+                                                       {GPR8, l "l"}})         \
+            : BaseRegToRegsMap::value_type(                                    \
+                  "e" l "x", {{GPR16, l "x"}, {GPR8h, l "h"}, {GPR8, l "l"}})  \
+  }
+
+#define GPR(r16)                                                               \
+  {                                                                            \
+    is64bit ? BaseRegToRegsMap::value_type(                                    \
+                  "r" r16, {{GPR32, "e" r16}, {GPR16, r16}, {GPR8, r16 "l"}})  \
+            : BaseRegToRegsMap::value_type("e" r16,                            \
+                                           {{GPR16, r16}, {GPR8, r16 "l"}})    \
+  }
+
+#define GPR64(n)                                                               \
+  {                                                                            \
+    BaseRegToRegsMap::value_type(                                              \
+        "r" #n,                                                                \
+        {{GPR32, "r" #n "d"}, {GPR16, "r" #n "w"}, {GPR8, "r" #n "l"}})        \
   }
+
+#define STMM(n)                                                                \
+  { BaseRegToRegsMap::value_type("st" #n, {{MM, "mm" #n}}) }
+
+#define YMM(n)                                                                 \
+  {BaseRegToRegsMap::value_type("ymm" #n "h", {{YMM_YMMh, "ymm" #n}})}, {      \
+    BaseRegToRegsMap::value_type("xmm" #n, {{YMM_XMM, "ymm" #n}})              \
+  }
+
+BaseRegToRegsMap makeBaseRegMap(bool is64bit) {
+  BaseRegToRegsMap out{
+      {// GPRs common to amd64 & i386
+       GPRh("a"), GPRh("b"), GPRh("c"), GPRh("d"), GPR("si"), GPR("di"),
+       GPR("bp"), GPR("sp"),
+
+       // ST/MM registers
+       STMM(0), STMM(1), STMM(2), STMM(3), STMM(4), STMM(5), STMM(6), STMM(7),
+
+       // lower YMM registers (common to amd64 & i386)
+       YMM(0), YMM(1), YMM(2), YMM(3), YMM(4), YMM(5), YMM(6), YMM(7)}};
+
+  if (is64bit) {
+    BaseRegToRegsMap amd64_regs{{// GPRs specific to amd64
+                                 GPR64(8), GPR64(9), GPR64(10), GPR64(11),
+                                 GPR64(12), GPR64(13), GPR64(14), GPR64(15),
+
+                                 // higher YMM registers (specific to amd64)
+                                 YMM(8), YMM(9), YMM(10), YMM(11), YMM(12),
+                                 YMM(13), YMM(14), YMM(15)}};
+    out.insert(amd64_regs.begin(), amd64_regs.end());
+  }
+
+  return out;
 }
 
 void ABIX86::AugmentRegisterInfo(
     std::vector<DynamicRegisterInfo::Register> &regs) {
   MCBasedABI::AugmentRegisterInfo(regs);
 
   ProcessSP process_sp = GetProcessSP();
   if (!process_sp)
     return;
 
   uint32_t gpr_base_size =
       process_sp->GetTarget().GetArchitecture().GetAddressByteSize();
-  bool is64bit = gpr_base_size == 8;
 
-  typedef RegisterMap::value_type RegPair;
-#define GPR_BASE(basename) (is64bit ? "r" basename : "e" basename)
-  RegisterMap gpr_regs{{
-      RegPair(GPR_BASE("ax"), {"eax", "ax", "ah", "al"}),
-      RegPair(GPR_BASE("bx"), {"ebx", "bx", "bh", "bl"}),
-      RegPair(GPR_BASE("cx"), {"ecx", "cx", "ch", "cl"}),
-      RegPair(GPR_BASE("dx"), {"edx", "dx", "dh", "dl"}),
-      RegPair(GPR_BASE("si"), {"esi", "si", "", "sil"}),
-      RegPair(GPR_BASE("di"), {"edi", "di", "", "dil"}),
-      RegPair(GPR_BASE("bp"), {"ebp", "bp", "", "bpl"}),
-      RegPair(GPR_BASE("sp"), {"esp", "sp", "", "spl"}),
-  }};
-#undef GPR_BASE
-  if (is64bit) {
-#define R(base) RegPair(base, {base "d", base "w", "", base "l"})
-    RegisterMap amd64_regs{{
-        R("r8"),
-        R("r9"),
-        R("r10"),
-        R("r11"),
-        R("r12"),
-        R("r13"),
-        R("r14"),
-        R("r15"),
-    }};
-#undef R
-    gpr_regs.insert(amd64_regs.begin(), amd64_regs.end());
-  }
-
-  RegisterMap st_regs{{
-      RegPair("st0", {"mm0"}),
-      RegPair("st1", {"mm1"}),
-      RegPair("st2", {"mm2"}),
-      RegPair("st3", {"mm3"}),
-      RegPair("st4", {"mm4"}),
-      RegPair("st5", {"mm5"}),
-      RegPair("st6", {"mm6"}),
-      RegPair("st7", {"mm7"}),
-  }};
-
-  // regs from gpr_basenames, in list order
-  std::vector<uint32_t> gpr_base_reg_indices;
-  // st0..st7, in list order
-  std::vector<uint32_t> st_reg_indices;
-  // map used for fast register lookups
+  // primary map from a base register to its subregisters
+  BaseRegToRegsMap base_reg_map = makeBaseRegMap(gpr_base_size == 8);
+  // set used for fast matching of register names to subregisters
   llvm::SmallDenseSet<llvm::StringRef, 64> subreg_name_set;
+  // convenience array providing access to all subregisters of given kind,
+  // sorted by base register index
+  std::array<llvm::SmallVector<RegData *, 16>, RegKindCount> subreg_by_kind;
 
-  // put all subreg names into the lookup set
-  for (const RegisterMap &regset : {gpr_regs, st_regs}) {
-    for (const RegPair &kv : regset)
-      subreg_name_set.insert(kv.second.begin(), kv.second.end());
+  // prepare the set of all known subregisters
+  for (const auto &x : base_reg_map) {
+    for (const auto &subreg : x.second)
+      subreg_name_set.insert(subreg.subreg_name);
   }
 
+  // iterate over all registers
   for (const auto &x : llvm::enumerate(regs)) {
     llvm::StringRef reg_name = x.value().name.GetStringRef();
-    // find expected base registers
-    if (gpr_regs.find(reg_name) != gpr_regs.end())
-      gpr_base_reg_indices.push_back(x.index());
-    else if (st_regs.find(reg_name) != st_regs.end())
-      st_reg_indices.push_back(x.index());
     // abort if at least one sub-register is already present
-    else if (llvm::is_contained(subreg_name_set, reg_name))
+    if (llvm::is_contained(subreg_name_set, reg_name))
       return;
+
+    auto found = base_reg_map.find(reg_name);
+    if (found == base_reg_map.end())
+      continue;
+
+    for (auto &subreg : found->second) {
+      // fill in base register indices
+      subreg.base_index = x.index();
+      // fill subreg_by_kind map-array
+      subreg_by_kind[static_cast<size_t>(subreg.subreg_kind)].push_back(
+          &subreg);
     }
+  }
+
+  // now add registers by kind
+  addPartialRegisters(regs, subreg_by_kind[GPR32], gpr_base_size, eEncodingUint,
+                      eFormatHex, 4);
+  addPartialRegisters(regs, subreg_by_kind[GPR16], gpr_base_size, eEncodingUint,
+                      eFormatHex, 2);
+  addPartialRegisters(regs, subreg_by_kind[GPR8h], gpr_base_size, eEncodingUint,
+                      eFormatHex, 1, 1);
+  addPartialRegisters(regs, subreg_by_kind[GPR8], gpr_base_size, eEncodingUint,
+                      eFormatHex, 1);
 
-  if (is64bit)
-    addPartialRegisters(regs, gpr_base_reg_indices, gpr_regs, gpr_base_size,
-                        RegKind::GPR32, eEncodingUint, eFormatHex, 4);
-  addPartialRegisters(regs, gpr_base_reg_indices, gpr_regs, gpr_base_size,
-                      RegKind::GPR16, eEncodingUint, eFormatHex, 2);
-  addPartialRegisters(regs, gpr_base_reg_indices, gpr_regs, gpr_base_size,
-                      RegKind::GPR8h, eEncodingUint, eFormatHex, 1, 1);
-  addPartialRegisters(regs, gpr_base_reg_indices, gpr_regs, gpr_base_size,
-                      RegKind::GPR8, eEncodingUint, eFormatHex, 1);
+  addPartialRegisters(regs, subreg_by_kind[MM], 10, eEncodingUint, eFormatHex,
+                      8);
 
-  addPartialRegisters(regs, st_reg_indices, st_regs, 10, RegKind::MM,
-                      eEncodingUint, eFormatHex, 8);
+  addCombinedRegisters(regs, subreg_by_kind[YMM_XMM], subreg_by_kind[YMM_YMMh],
+                       16, eEncodingVector, eFormatVectorOfUInt8);
 }

lldb/source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.cpp

 
 bool GDBRemoteRegisterContext::ReadRegister(const RegisterInfo *reg_info,
                                             RegisterValue &value) {
   // Read the register
   if (ReadRegisterBytes(reg_info)) {
     const uint32_t reg = reg_info->kinds[eRegisterKindLLDB];
     if (m_reg_valid[reg] == false)
       return false;
+    if (reg_info->value_regs &&
+        reg_info->value_regs[0] != LLDB_INVALID_REGNUM &&
+        reg_info->value_regs[1] != LLDB_INVALID_REGNUM) {
+      std::vector<char> combined_data;
+      uint32_t offset = 0;
+      for (int i = 0; reg_info->value_regs[i] != LLDB_INVALID_REGNUM; i++) {
+        const RegisterInfo *parent_reg = GetRegisterInfo(
+            eRegisterKindLLDB, reg_info->value_regs[i]);
+        if (!parent_reg)
+          return false;
+        combined_data.resize(offset + parent_reg->byte_size);
+        if (m_reg_data.CopyData(parent_reg->byte_offset, parent_reg->byte_size,
+                                combined_data.data() + offset) !=
+            parent_reg->byte_size)
+          return false;
+        offset += parent_reg->byte_size;
+      }
+
+      Status error;
+      return value.SetFromMemoryData(
+                 reg_info, combined_data.data(), combined_data.size(),
+                 m_reg_data.GetByteOrder(), error) == combined_data.size();
+    } else {
       const bool partial_data_ok = false;
       Status error(value.SetValueFromData(
           reg_info, m_reg_data, reg_info->byte_offset, partial_data_ok));
       return error.Success();
     }
+  }
   return false;
 }
 
 bool GDBRemoteRegisterContext::PrivateSetRegisterValue(
     uint32_t reg, llvm::ArrayRef<uint8_t> data) {
   const RegisterInfo *reg_info = GetRegisterInfoAtIndex(reg);
   if (reg_info == nullptr)
     return false;
bool GDBRemoteRegisterContext::ReadRegisterBytes(const RegisterInfo *reg_info) {
   }
 
   return true;
 }
 
 bool GDBRemoteRegisterContext::WriteRegister(const RegisterInfo *reg_info,
                                              const RegisterValue &value) {
   DataExtractor data;
-  if (value.GetData(data))
+  if (value.GetData(data)) {
+    if (reg_info->value_regs &&
+        reg_info->value_regs[0] != LLDB_INVALID_REGNUM &&
+        reg_info->value_regs[1] != LLDB_INVALID_REGNUM) {
+      uint32_t combined_size = 0;
+      for (int i = 0; reg_info->value_regs[i] != LLDB_INVALID_REGNUM; i++) {
+        const RegisterInfo *parent_reg = GetRegisterInfo(
+            eRegisterKindLLDB, reg_info->value_regs[i]);
+        if (!parent_reg)
+          return false;
+        combined_size += parent_reg->byte_size;
+      }
+
+      if (data.GetByteSize() < combined_size)
+        return false;
+
+      uint32_t offset = 0;
+      for (int i = 0; reg_info->value_regs[i] != LLDB_INVALID_REGNUM; i++) {
+        const RegisterInfo *parent_reg = GetRegisterInfo(
+            eRegisterKindLLDB, reg_info->value_regs[i]);
+        assert(parent_reg);
+
+        DataExtractor parent_data{data, offset, parent_reg->byte_size};
+        if (!WriteRegisterBytes(parent_reg, parent_data, 0))
+          return false;
+        offset += parent_reg->byte_size;
+      }
+      assert(offset == combined_size);
+      return true;
+    } else
       return WriteRegisterBytes(reg_info, data, 0);
+  }
   return false;
 }
 
 // Helper function for GDBRemoteRegisterContext::WriteRegisterBytes().
 bool GDBRemoteRegisterContext::SetPrimordialRegister(
     const RegisterInfo *reg_info, GDBRemoteCommunicationClient &gdb_comm) {
   StreamString packet;
   StringExtractorGDBRemote response;

lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

feature_node.ForEachChildElementWithName(
           } else if (gdb_type == "data_ptr" || gdb_type == "code_ptr") {
             reg_info.format = eFormatAddressInfo;
             reg_info.encoding = eEncodingUint;
           } else if (gdb_type == "float") {
             reg_info.format = eFormatFloat;
             reg_info.encoding = eEncodingIEEE754;
           } else if (gdb_type == "aarch64v" ||
                      llvm::StringRef(gdb_type).startswith("vec") ||
-                     gdb_type == "i387_ext") {
+                     gdb_type == "i387_ext" || gdb_type == "uint128") {
+            // lldb doesn't handle 128-bit uints correctly (for ymm*h), so treat
+            // them as vector (similarly to xmm/ymm)
             reg_info.format = eFormatVectorOfUInt8;
             reg_info.encoding = eEncodingVector;
           }
         }
 
         // Only update the register set name if we didn't get a "reg_set"
         // attribute. "set_name" will be empty if we didn't have a "reg_set"
         // attribute.

lldb/source/Target/DynamicRegisterInfo.cpp

void DynamicRegisterInfo::Finalize(const ArchSpec &arch) {
   m_finalized = true;
   const size_t num_sets = m_sets.size();
   for (size_t set = 0; set < num_sets; ++set) {
     assert(m_sets.size() == m_set_reg_nums.size());
     m_sets[set].num_registers = m_set_reg_nums[set].size();
     m_sets[set].registers = m_set_reg_nums[set].data();
   }
 
-  // sort and unique all value registers and make sure each is terminated with
-  // LLDB_INVALID_REGNUM
+  // make sure value_regs are terminated with LLDB_INVALID_REGNUM
 
   for (reg_to_regs_map::iterator pos = m_value_regs_map.begin(),
                                  end = m_value_regs_map.end();
        pos != end; ++pos) {
-    if (pos->second.size() > 1) {
-      llvm::sort(pos->second.begin(), pos->second.end());
-      reg_num_collection::iterator unique_end =
-          std::unique(pos->second.begin(), pos->second.end());
-      if (unique_end != pos->second.end())
-        pos->second.erase(unique_end, pos->second.end());
-    }
-    assert(!pos->second.empty());
     if (pos->second.back() != LLDB_INVALID_REGNUM)
       pos->second.push_back(LLDB_INVALID_REGNUM);
   }
 
   // Now update all value_regs with each register info as needed
   const size_t num_regs = m_regs.size();
   for (size_t i = 0; i < num_regs; ++i) {
     if (m_value_regs_map.find(i) != m_value_regs_map.end())
if (m_regs[regnum_pair.second].byte_offset == LLDB_INVALID_INDEX32 &&
       reg_offset = m_regs[regnum_pair.second].byte_offset +
                    m_regs[regnum_pair.second].byte_size;
     }
   }
 
   // Now update all value_regs with each register info as needed
   for (auto &reg : m_regs) {
     if (reg.value_regs != nullptr) {
-      // Assign a valid offset to all pseudo registers if not assigned by stub.
-      // Pseudo registers with value_regs list populated will share same offset
-      // as that of their corresponding primary register in value_regs list.
+      // Assign a valid offset to all pseudo registers that have only a single
+      // parent register in value_regs list, if not assigned by stub.  Pseudo
+      // registers with value_regs list populated will share same offset as
+      // that of their corresponding parent register.
       if (reg.byte_offset == LLDB_INVALID_INDEX32) {
         uint32_t value_regnum = reg.value_regs[0];
-        if (value_regnum != LLDB_INVALID_INDEX32) {
-          reg.byte_offset = GetRegisterInfoAtIndex(value_regnum)->byte_offset;
+        if (value_regnum != LLDB_INVALID_INDEX32 &&
+            reg.value_regs[1] == LLDB_INVALID_INDEX32) {
+          reg.byte_offset =
+              GetRegisterInfoAtIndex(value_regnum)->byte_offset;
           auto it = m_value_reg_offset_map.find(reg.kinds[eRegisterKindLLDB]);
           if (it != m_value_reg_offset_map.end())
             reg.byte_offset += it->second;
         }
       }
     }
 
     reg_offset = reg.byte_offset + reg.byte_size;

lldb/test/API/functionalities/gdb_remote_client/TestGDBServerTargetXML.py

def test_x86_64_regs(self):
                    ["ecx = 0xfffef9f8"])
         self.match("register read rcx",
                    ["rcx = 0x08070605fffef9f8"])
 
         self.runCmd("register write mm0 0xfffefdfcfbfaf9f8")
         self.match("register read st0",
                    ["st0 = {0xf8 0xf9 0xfa 0xfb 0xfc 0xfd 0xfe 0xff 0x09 0x0a}"])
 
+        self.runCmd("register write xmm0 \"{0xff 0xfe 0xfd 0xfc 0xfb 0xfa 0xf9 "
+                    "0xf8 0xf7 0xf6 0xf5 0xf4 0xf3 0xf2 0xf1 0xf0}\"")
+        self.match("register read ymm0",
+                   ["ymm0 = {0xff 0xfe 0xfd 0xfc 0xfb 0xfa 0xf9 0xf8 0xf7 0xf6 "
+                    "0xf5 0xf4 0xf3 0xf2 0xf1 0xf0 0xb1 0xb2 0xb3 0xb4 0xb5 "
+                    "0xb6 0xb7 0xb8 0xb9 0xba 0xbb 0xbc 0xbd 0xbe 0xbf 0xc0}"])
+
+        self.runCmd("register write ymm0h \"{0xef 0xee 0xed 0xec 0xeb 0xea 0xe9 "
+                    "0xe8 0xe7 0xe6 0xe5 0xe4 0xe3 0xe2 0xe1 0xe0}\"")
+        self.match("register read ymm0",
+                   ["ymm0 = {0xff 0xfe 0xfd 0xfc 0xfb 0xfa 0xf9 0xf8 0xf7 0xf6 "
+                    "0xf5 0xf4 0xf3 0xf2 0xf1 0xf0 0xef 0xee 0xed 0xec 0xeb "
+                    "0xea 0xe9 0xe8 0xe7 0xe6 0xe5 0xe4 0xe3 0xe2 0xe1 0xe0}"])
+
+        self.runCmd("register write ymm0 \"{0xd0 0xd1 0xd2 0xd3 0xd4 0xd5 0xd6 "
+                    "0xd7 0xd8 0xd9 0xda 0xdb 0xdc 0xdd 0xde 0xdf 0xe0 0xe1 "
+                    "0xe2 0xe3 0xe4 0xe5 0xe6 0xe7 0xe8 0xe9 0xea 0xeb 0xec "
+                    "0xed 0xee 0xef}\"")
+        self.match("register read ymm0",
+                   ["ymm0 = {0xd0 0xd1 0xd2 0xd3 0xd4 0xd5 0xd6 0xd7 0xd8 0xd9 "
+                    "0xda 0xdb 0xdc 0xdd 0xde 0xdf 0xe0 0xe1 0xe2 0xe3 0xe4 "
+                    "0xe5 0xe6 0xe7 0xe8 0xe9 0xea 0xeb 0xec 0xed 0xee 0xef}"])
+
     @skipIfXmlSupportMissing
     @skipIfRemote
     @skipIfLLVMTargetMissing("X86")
     def test_i386_regs(self):
         """Test grabbing various i386 registers from gdbserver."""
         class MyResponder(MockGDBServerResponder):
             reg_data = (
                 "01020304"  # eax
def test_i386_regs(self):
                    ["cx = 0xf9f8"])
         self.match("register read ecx",
                    ["ecx = 0x1413f9f8"])
 
         self.runCmd("register write mm0 0xfffefdfcfbfaf9f8")
         self.match("register read st0",
                    ["st0 = {0xf8 0xf9 0xfa 0xfb 0xfc 0xfd 0xfe 0xff 0x09 0x0a}"])
 
+        self.runCmd("register write xmm0 \"{0xff 0xfe 0xfd 0xfc 0xfb 0xfa 0xf9 "
+                    "0xf8 0xf7 0xf6 0xf5 0xf4 0xf3 0xf2 0xf1 0xf0}\"")
+        self.match("register read ymm0",
+                   ["ymm0 = {0xff 0xfe 0xfd 0xfc 0xfb 0xfa 0xf9 0xf8 0xf7 0xf6 "
+                    "0xf5 0xf4 0xf3 0xf2 0xf1 0xf0 0xb1 0xb2 0xb3 0xb4 0xb5 "
+                    "0xb6 0xb7 0xb8 0xb9 0xba 0xbb 0xbc 0xbd 0xbe 0xbf 0xc0}"])
+
+        self.runCmd("register write ymm0h \"{0xef 0xee 0xed 0xec 0xeb 0xea 0xe9 "
+                    "0xe8 0xe7 0xe6 0xe5 0xe4 0xe3 0xe2 0xe1 0xe0}\"")
+        self.match("register read ymm0",
+                   ["ymm0 = {0xff 0xfe 0xfd 0xfc 0xfb 0xfa 0xf9 0xf8 0xf7 0xf6 "
+                    "0xf5 0xf4 0xf3 0xf2 0xf1 0xf0 0xef 0xee 0xed 0xec 0xeb "
+                    "0xea 0xe9 0xe8 0xe7 0xe6 0xe5 0xe4 0xe3 0xe2 0xe1 0xe0}"])
+
+        self.runCmd("register write ymm0 \"{0xd0 0xd1 0xd2 0xd3 0xd4 0xd5 0xd6 "
+                    "0xd7 0xd8 0xd9 0xda 0xdb 0xdc 0xdd 0xde 0xdf 0xe0 0xe1 "
+                    "0xe2 0xe3 0xe4 0xe5 0xe6 0xe7 0xe8 0xe9 0xea 0xeb 0xec "
+                    "0xed 0xee 0xef}\"")
+        self.match("register read ymm0",
+                   ["ymm0 = {0xd0 0xd1 0xd2 0xd3 0xd4 0xd5 0xd6 0xd7 0xd8 0xd9 "
+                    "0xda 0xdb 0xdc 0xdd 0xde 0xdf 0xe0 0xe1 0xe2 0xe3 0xe4 "
+                    "0xe5 0xe6 0xe7 0xe8 0xe9 0xea 0xeb 0xec 0xed 0xee 0xef}"])
+
     @skipIfXmlSupportMissing
     @skipIfRemote
     @skipIfLLVMTargetMissing("AArch64")
     def test_aarch64_regs(self):
         """Test grabbing various aarch64 registers from gdbserver."""
         class MyResponder(MockGDBServerResponder):
             reg_data = (
                 "0102030405060708"  # x0