[lldb] [ABI/X86] Add pseudo-registers if missing

[mgorny]

Remove unnecessary include.

[mgorny]

Add writing tests.

[mgorny]

Add tests for reading back (should verify invalidation).

[mgorny]

Fix filename in heading.

[mgorny]

Restore the sp test.

[mgorny]

Rename the file to _x86.cpp to future-proof it. Replace`assert()s` with explicit skipping, we don't really want to crash on unexpected data in target.xml.

[labath]

This is an interesting problem. I don't yet know how, but as I alluded to in the other patch, my instinct would be to find a way to put this into the ABI class, in order to keep the "fill in bits not provided by debug server" code in a single place.

[mgorny]

Another question: should we aim to remove the 'redundant afterwards' bits from lldb-server, and fill them in from client side entirely? I don't think our process plugins really need knowledge of EAX etc., and it would reduce the duplication a fair bit.

Also I'm not sure if I like my code here, two questions inline.

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

37–54 ↗

(On Diff #372261)

Also here I'm wondering if it wouldn't be better to just have an array of {"ax", "bx", ...} and construct all other names from that base, plus r8..r15 via numeric for loop.

66–75 ↗

(On Diff #372261)

I've been somewhat basing this thing on how our process plugins do registers but I'm wondering if it wouldn't be better to just do a for (int x = 0; i < 8; i++) loop and dynamically generate st${x} and mm${x} names.

[labath]

In D108831#2999227, @mgorny wrote:

Another question: should we aim to remove the 'redundant afterwards' bits from lldb-server, and fill them in from client side entirely? I don't think our process plugins really need knowledge of EAX etc., and it would reduce the duplication a fair bit.

I think that would be great. Besides the deduplication aspect, this will also decrease the chances of some gdb compatibility bug creeping in. I might consider waiting for one release, just in case someone wants to connect a newer lldb-server to an old lldb.

/And/, I would actually like to go further than that, and remove these register definitions from core file (&minidump, etc) plugins too. Since eax/ax/ah/al can be extracted from rax in a generic way, I don't see a reason for each plugin to have to redefine those (or forget doing like, like is the case with the windows plugin). This is the point where this logic stops being gdb-remote-specific. There are no compatibility issues here, though the process itself might be trickier, depending on how much the plugins are assuming a particular register order, etc.

[mgorny]

@labath, could you look at my inline comments before I start moving stuff? ;-)

[labath]

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

37–54 ↗

(On Diff #372261)

I don't have a strong opinion on this. The names are sufficiently irregular that the other approach will (probably) be messy as well.

I'm more bothered by the fact that you're writing into these global variables. IIRC, the DynamicRegisterInfo class has some sort of a buffer to store the array data. It would be good to reuse that (or do something similar).

66–75 ↗

(On Diff #372261)

The loop idea seems reasonable here.

[mgorny]

Rewritten for the new augmentation API based on ABI plugins.

[labath]

We don't really want to support adding an arbitrary subset of sub-registers, do we? I am thinking if this could be made simpler if it was all-or-nothing. Like, during the initial pass you could check whether the list contains _any_ subregister, and abort if it does. Then the subsequent pass could assume that all subregisters need to be added...

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

23

I think you should be able to get this from the ArchSpec of the process.

[mgorny]

In D108831#3063282, @labath wrote:

We don't really want to support adding an arbitrary subset of sub-registers, do we? I am thinking if this could be made simpler if it was all-or-nothing. Like, during the initial pass you could check whether the list contains _any_ subregister, and abort if it does. Then the subsequent pass could assume that all subregisters need to be added...

Sure, that makes sense. Should I update AArch64 too?

[labath]

I don't think this is that critical over there, but I think you could do that.

[mgorny]

Use ArchSpec instead of passing hardcoded 64bit. Abort the whole process if at least one subreg is present, eliminating all the have* bools.

[mgorny]

Move LLDB_INVALID_REGNUM check inside addPartialRegister().

[mgorny]

Halfway through to a new approach for getting regnames.

[labath]

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

92

I'd just do is64bit = arch.GetAddressByteSize() == 8

125–191

What if we introduced helper functions like:

// returns 0 for [re]ax, 1 for [re]bx, ...
Optional<unsigned> GetGPRNumber(llvm::StringRef name) { 
  // either a lookup table or the fancy name matching
}
GPRReg MakeGPRReg(size_t base_index, unsigned gpr_number) {
  // I'd probably implement this via lookup table(s)
}

.. or something similar?

The general idea is to split this function into smaller chunks and also try to abstract away the funky register names as much as possible.

212–214

llvm::is_contained(haystack, needle)

[mgorny]

Next iteration. I will try to simplify even more.

[mgorny]

Final round of refactoring. The code should be simpler and more readable now.

I don't have a strong opinion wrt generating names in a loop vs hardcoding the list (possibly using a macro). Please decide how I should proceed ;-).

[mgorny]

Use inline lists instead of for loops for registers. Use llvm::SmallDense* types with StringRefs.

[mgorny]

Use ConstString::GetStringRef() directly instead of converting through const char *. Not that it has any practical difference right now…

[labath]

I think we've managed to come to something that looks mostly reasonable.

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

54

const RegisterMap& -- it's expensive to copy

62

drop const & -- it's cheap to copy :)

95–97

how about you reverse this, so you set gpr_base_size first, and then use that to initialize is64bit?

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

343

Maybe also do a register read --all and check that all (sub)registers are present and have the expected values (you can try using self.filecheck for that).

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

 #include "lldb/Target/ABI.h"
 #include "lldb/lldb-private.h"
 
 class ABIX86 : public lldb_private::MCBasedABI {
 public:
   static void Initialize();
   static void Terminate();
 
+protected:
+  void AugmentRegisterInfo(
+      std::vector<lldb_private::DynamicRegisterInfo::Register> &regs) override;
+
 private:
   using lldb_private::MCBasedABI::MCBasedABI;
 };
 
 #endif

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

 //===-- ABIX86.cpp --------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
-#include "ABIX86.h"
 #include "ABIMacOSX_i386.h"
 #include "ABISysV_i386.h"
 #include "ABISysV_x86_64.h"
 #include "ABIWindows_x86_64.h"
+#include "ABIX86.h"
 #include "lldb/Core/PluginManager.h"
+#include "lldb/Target/Process.h"
+
+#include <unordered_map>
+#include <unordered_set>
+
+using namespace lldb;
+using namespace lldb_private;
 
 LLDB_PLUGIN_DEFINE(ABIX86)
 
 void ABIX86::Initialize() {
   ABIMacOSX_i386::Initialize();
   ABISysV_i386::Initialize();
   ABISysV_x86_64::Initialize();
   ABIWindows_x86_64::Initialize();
 }
 
 void ABIX86::Terminate() {
   ABIMacOSX_i386::Terminate();
   ABISysV_i386::Terminate();
   ABISysV_x86_64::Terminate();
   ABIWindows_x86_64::Terminate();
 }
+
+static void addPartialRegister(std::vector<DynamicRegisterInfo::Register> &regs,
+                               uint32_t full_reg_index, uint32_t full_reg_size,
+                               const std::string &partial_reg_name,
+                               uint32_t partial_reg_size, uint32_t offset = 0,
+                               lldb::Encoding encoding = eEncodingInvalid,
+                               lldb::Format format = eFormatInvalid) {
+  if (full_reg_index == LLDB_INVALID_REGNUM)
+    return;
+  DynamicRegisterInfo::Register &full_reg = regs[full_reg_index];
+  if (full_reg.byte_size != full_reg_size)
+    return;
+
+  lldb_private::DynamicRegisterInfo::Register partial_reg{
+      lldb_private::ConstString(partial_reg_name),
+      lldb_private::ConstString(),
+      lldb_private::ConstString("supplementary registers"),
+      partial_reg_size,
+      LLDB_INVALID_INDEX32,
+      encoding != eEncodingInvalid ? encoding : full_reg.encoding,
+      format != eFormatInvalid ? format : full_reg.format,
+      LLDB_INVALID_REGNUM,
+      LLDB_INVALID_REGNUM,
+      LLDB_INVALID_REGNUM,
+      LLDB_INVALID_REGNUM,
+      {full_reg_index},
+      {},
+      offset};
+
+  addSupplementaryRegister(regs, partial_reg);
+}
+
+struct GPRReg {
+  std::string name32;
+  std::string name16;
+  std::string name8h;
+  std::string name8;
+  std::array<const char *, 4> names;
+  uint32_t base_index = LLDB_INVALID_REGNUM;
+};
+
+enum {
+  GPR32 = 0,
+  GPR16,
+  GPR8h,
+  GPR8,
+};
+
+static void
+addGPRs(std::vector<DynamicRegisterInfo::Register> &regs,
+        llvm::ArrayRef<uint32_t> base_reg_indices,
+        std::unordered_map<std::string, std::array<const char *, 4>> reg_names,
+        uint32_t base_size, int name_index, 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());
+    DynamicRegisterInfo::Register &full_reg = regs[base_index];
+    const char *subreg_name =
+        reg_names[full_reg.name.AsCString() + 1][name_index];
+    if (!*subreg_name)
+      continue;
+    addPartialRegister(regs, base_index, base_size, subreg_name, subreg_size,
+                       subreg_offset);
+  }
+}
+
+struct STReg {
+  std::string name_mm;
+  uint32_t base_index = LLDB_INVALID_REGNUM;
+};
+
+void ABIX86::AugmentRegisterInfo(
+    std::vector<DynamicRegisterInfo::Register> &regs) {
+  MCBasedABI::AugmentRegisterInfo(regs);
+
+  ProcessSP process_sp = GetProcessSP();
+  if (!process_sp)
+    return;
+
+  ArchSpec arch = process_sp->GetTarget().GetArchitecture();
+  bool is64bit;
+  switch (arch.GetMachine()) {
+  case llvm::Triple::x86:
+    is64bit = false;
+    break;
+  case llvm::Triple::x86_64:
+    is64bit = true;
+    break;
+  default:
+    llvm_unreachable("unexpected arch");
+  }
+
+  std::unordered_set<std::string> gpr_basenames{
+      {"ax", "bx", "cx", "dx", "si", "di", "bp", "sp"}};
+  typedef std::pair<std::string, std::array<const char *, 4>> GPRPair;
+  std::unordered_map<std::string, std::array<const char *, 4>> gpr_regs{{
+      GPRPair("ax", {"eax", "ax", "ah", "al"}),
+      GPRPair("bx", {"ebx", "bx", "bh", "bl"}),
+      GPRPair("cx", {"ecx", "cx", "ch", "cl"}),
+      GPRPair("dx", {"edx", "dx", "dh", "dl"}),
+      GPRPair("si", {"esi", "si", "", "sil"}),
+      GPRPair("di", {"edi", "di", "", "dil"}),
+      GPRPair("bp", {"ebp", "bp", "", "bpl"}),
+      GPRPair("sp", {"esp", "sp", "", "spl"}),
+  }};
+
+  char gpr_base_reg_prefix = is64bit ? 'r' : 'e';
+  uint32_t gpr_base_size = is64bit ? 8 : 4;
+  // regs from gpr_basenames, in list order
+  std::array<uint32_t, 8> gpr_base_reg_indices;
+  assert(gpr_base_reg_indices.size() == gpr_basenames.size());
+  // r8..r15, sorted
+  std::array<GPRReg, 8> gpr_extra_reg_indices;
+  // st0..st7, sorted
+  std::array<STReg, 8> st_reg_indices;
+  // map used for fast register lookups
+  std::unordered_set<std::string> subreg_name_set;
+
+  // put all subreg names into the lookup set
+  for (const auto &kv : gpr_regs)
+    subreg_name_set.insert(kv.second.begin(), kv.second.end());
+
+  // find base registers and store their indices
+  // (note that with incorrect input we could get less than 8 regs)
+  size_t new_index = 0;
+  for (const auto &x : llvm::enumerate(regs)) {
+    const char *reg_name = x.value().name.AsCString();
+    if (reg_name[0] == gpr_base_reg_prefix) {
+      // handle regs from gpr_basenames
+      auto in_basenames = gpr_basenames.find(reg_name + 1);
+      if (in_basenames != gpr_basenames.end() &&
+          new_index < gpr_base_reg_indices.size())
+        gpr_base_reg_indices[new_index++] = x.index();
+
+      // handle r8..r15 regs
+      unsigned int suffix;
+      if (is64bit && llvm::to_integer(reg_name + 1, suffix, 10) &&
+          suffix >= 8 && suffix < 16) {
+        GPRReg &reg_rec = gpr_extra_reg_indices[suffix - 8];
+        reg_rec.base_index = x.index();
+
+        // construct derived register names
+        std::string new_regname{reg_name};
+        // 32-bit r*d registers
+        new_regname += 'd';
+        reg_rec.name32 = new_regname;
+        subreg_name_set.insert(new_regname);
+
+        // 16-bit r*w registers
+        new_regname.back() = 'w';
+        reg_rec.name16 = new_regname;
+        subreg_name_set.insert(new_regname);
+
+        // 8-bit r*l registers
+        new_regname.back() = 'l';
+        reg_rec.name8 = new_regname;
+        subreg_name_set.insert(new_regname);
+
+        continue;
+      }
+    }
+
+    // st0..st7 regs
+    if (llvm::StringRef(reg_name).startswith("st")) {
+      unsigned int suffix;
+      if (llvm::to_integer(reg_name + 2, suffix, 10) && suffix < 8) {
+        STReg &reg_rec = st_reg_indices[suffix];
+        reg_rec.base_index = x.index();
+
+        std::string new_regname{"mm"};
+        new_regname += llvm::utostr(suffix);
+        reg_rec.name_mm = new_regname;
+        subreg_name_set.insert(new_regname);
+
+        continue;
+      }
+    }
+  }
+
+  // abort if we have at least one existing subregister
+  for (const DynamicRegisterInfo::Register &x : regs) {
+    const char *reg_name = x.name.AsCString();
+    auto found = subreg_name_set.find(reg_name);
+    if (found != subreg_name_set.end())
+      return;
+  }
+
+  if (is64bit) {
+    // on amd64, add 32-bit e* registers
+    addGPRs(regs, gpr_base_reg_indices, gpr_regs, gpr_base_size, GPR32, 4);
+
+    // add 32-bit r*d registers
+    for (const GPRReg &reg : gpr_extra_reg_indices) {
+      addPartialRegister(regs, reg.base_index, gpr_base_size, reg.name32, 4);
+    }
+  }
+
+  // add 16-bit * registers
+  addGPRs(regs, gpr_base_reg_indices, gpr_regs, gpr_base_size, GPR16, 2);
+
+  if (is64bit) {
+    // add 16-bit r*w registers
+    for (const GPRReg &reg : gpr_extra_reg_indices) {
+      addPartialRegister(regs, reg.base_index, gpr_base_size, reg.name16, 2);
+    }
+  }
+
+  // add 8-bit *h registers
+  addGPRs(regs, gpr_base_reg_indices, gpr_regs, gpr_base_size, GPR8h, 1, 1);
+  // add 8-bit *l registers
+  addGPRs(regs, gpr_base_reg_indices, gpr_regs, gpr_base_size, GPR8, 1);
+
+  if (is64bit) {
+    // add 8-bit r*l registers
+    for (const GPRReg &reg : gpr_extra_reg_indices) {
+      addPartialRegister(regs, reg.base_index, gpr_base_size, reg.name8, 1);
+    }
+  }
+
+  // add mm registers
+  for (const STReg &reg : st_reg_indices) {
+    addPartialRegister(regs, reg.base_index, 10, reg.name_mm, 8, 0,
+                       eEncodingUint, eFormatHex);
+  }
+}

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

def test_x86_64_regs(self):
                    ["st1 = {0x11 0x12 0x13 0x14 0x15 0x16 0x17 0x18 0x19 0x1a}"])
         self.match("register read xmm0",
                    ["xmm0 = {0x81 0x82 0x83 0x84 0x85 0x86 0x87 0x88 "
                     "0x89 0x8a 0x8b 0x8c 0x8d 0x8e 0x8f 0x90}"])
         self.match("register read xmm1",
                    ["xmm1 = {0x91 0x92 0x93 0x94 0x95 0x96 0x97 0x98 "
                     "0x99 0x9a 0x9b 0x9c 0x9d 0x9e 0x9f 0xa0}"])
 
+        # test pseudo-registers
+        self.match("register read ecx",
+                   ["ecx = 0x04030201"])
+        self.match("register read cx",
+                   ["cx = 0x0201"])
+        self.match("register read ch",
+                   ["ch = 0x02"])
+        self.match("register read cl",
+                   ["cl = 0x01"])
+        self.match("register read r8d",
+                   ["r8d = 0x64636261"])
+        self.match("register read r8w",
+                   ["r8w = 0x6261"])
+        self.match("register read r8l",
+                   ["r8l = 0x61"])
+        self.match("register read mm0",
+                   ["mm0 = 0x0807060504030201"])
+        self.match("register read mm1",
+                   ["mm1 = 0x1817161514131211"])
+
+        # test writing into pseudo-registers
+        self.runCmd("register write ecx 0xfffefdfc")
+        reg_data[0] = "fcfdfeff05060708"
+        self.assertPacketLogContains(["G" + "".join(reg_data)])
+        self.match("register read rcx",
+                   ["rcx = 0x08070605fffefdfc"])
+
+        self.runCmd("register write cx 0xfbfa")
+        reg_data[0] = "fafbfeff05060708"
+        self.assertPacketLogContains(["G" + "".join(reg_data)])
+        self.match("register read ecx",
+                   ["ecx = 0xfffefbfa"])
+        self.match("register read rcx",
+                   ["rcx = 0x08070605fffefbfa"])
+
+        self.runCmd("register write ch 0xf9")
+        reg_data[0] = "faf9feff05060708"
+        self.assertPacketLogContains(["G" + "".join(reg_data)])
+        self.match("register read cx",
+                   ["cx = 0xf9fa"])
+        self.match("register read ecx",
+                   ["ecx = 0xfffef9fa"])
+        self.match("register read rcx",
+                   ["rcx = 0x08070605fffef9fa"])
+
+        self.runCmd("register write cl 0xf8")
+        reg_data[0] = "f8f9feff05060708"
+        self.assertPacketLogContains(["G" + "".join(reg_data)])
+        self.match("register read cx",
+                   ["cx = 0xf9f8"])
+        self.match("register read ecx",
+                   ["ecx = 0xfffef9f8"])
+        self.match("register read rcx",
+                   ["rcx = 0x08070605fffef9f8"])
+
+        self.runCmd("register write mm0 0xfffefdfcfbfaf9f8")
+        reg_data[10] = "f8f9fafbfcfdfeff090a"
+        self.assertPacketLogContains(["G" + "".join(reg_data)])
+        self.match("register read st0",
+                   ["st0 = {0xf8 0xf9 0xfa 0xfb 0xfc 0xfd 0xfe 0xff 0x09 0x0a}"])
+
     @skipIfXmlSupportMissing
     @skipIfRemote
     @skipIfLLVMTargetMissing("X86")
     def test_i386_regs(self):
         """Test grabbing various i386 registers from gdbserver."""
         reg_data = [
             "01020304",  # eax
             "11121314",  # ecx
def test_i386_regs(self):
         target = self.createTarget("basic_eh_frame-i386.yaml")
         process = self.connect(target)
         lldbutil.expect_state_changes(self, self.dbg.GetListener(), process,
                                       [lldb.eStateStopped])
 
         # test generic aliases
         self.match("register read fp",
                    ["ebp = 0x54535251"])
+        self.match("register read sp",
+                   ["esp = 0x44434241"])
         self.match("register read pc",
                    ["eip = 0x84838281"])
         self.match("register read flags",
                    ["eflags = 0x94939291"])
 
+        # test pseudo-registers
+        self.match("register read cx",
+                   ["cx = 0x1211"])
+        self.match("register read ch",
+                   ["ch = 0x12"])
+        self.match("register read cl",
+                   ["cl = 0x11"])
+        self.match("register read mm0",
+                   ["mm0 = 0x0807060504030201"])
+        self.match("register read mm1",
+                   ["mm1 = 0x1817161514131211"])
+
         # both stX and xmmX should be displayed as vectors
         self.match("register read st0",
                    ["st0 = {0x01 0x02 0x03 0x04 0x05 0x06 0x07 0x08 0x09 0x0a}"])
         self.match("register read st1",
                    ["st1 = {0x11 0x12 0x13 0x14 0x15 0x16 0x17 0x18 0x19 0x1a}"])
         self.match("register read xmm0",
                    ["xmm0 = {0x81 0x82 0x83 0x84 0x85 0x86 0x87 0x88 "
                     "0x89 0x8a 0x8b 0x8c 0x8d 0x8e 0x8f 0x90}"])
         self.match("register read xmm1",
                    ["xmm1 = {0x91 0x92 0x93 0x94 0x95 0x96 0x97 0x98 "
                     "0x99 0x9a 0x9b 0x9c 0x9d 0x9e 0x9f 0xa0}"])
 
+        # test writing into pseudo-registers
+        self.runCmd("register write cx 0xfbfa")
+        reg_data[1] = "fafb1314"
+        self.assertPacketLogContains(["G" + "".join(reg_data)])
+        self.match("register read ecx",
+                   ["ecx = 0x1413fbfa"])
+
+        self.runCmd("register write ch 0xf9")
+        reg_data[1] = "faf91314"
+        self.assertPacketLogContains(["G" + "".join(reg_data)])
+        self.match("register read cx",
+                   ["cx = 0xf9fa"])
+        self.match("register read ecx",
+                   ["ecx = 0x1413f9fa"])
+
+        self.runCmd("register write cl 0xf8")
+        reg_data[1] = "f8f91314"
+        self.assertPacketLogContains(["G" + "".join(reg_data)])
+        self.match("register read cx",
+                   ["cx = 0xf9f8"])
+        self.match("register read ecx",
+                   ["ecx = 0x1413f9f8"])
+
+        self.runCmd("register write mm0 0xfffefdfcfbfaf9f8")
+        reg_data[10] = "f8f9fafbfcfdfeff090a"
+        self.assertPacketLogContains(["G" + "".join(reg_data)])
+        self.match("register read st0",
+                   ["st0 = {0xf8 0xf9 0xfa 0xfb 0xfc 0xfd 0xfe 0xff 0x09 0x0a}"])
+
     @skipIfXmlSupportMissing
     @skipIfRemote
     @skipIfLLVMTargetMissing("AArch64")
     def test_aarch64_regs(self):
         """Test grabbing various aarch64 registers from gdbserver."""
         class MyResponder(MockGDBServerResponder):
             reg_data = (
                 "0102030405060708"  # x0