[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

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

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

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

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 ↗

(On Diff #378533)

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

91 ↗

(On Diff #379653)

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

124–190 ↗

(On Diff #379653)

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.

211–213 ↗

(On Diff #379653)

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

51 ↗

(On Diff #379921)

const RegisterMap& -- it's expensive to copy

59 ↗

(On Diff #379921)

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

92–94 ↗

(On Diff #379921)

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

185

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/Process/gdb-remote/CMakeLists.txt

add_lldb_library(lldbPluginProcessGDBRemote PLUGIN
   GDBRemoteCommunicationClient.cpp
   GDBRemoteCommunicationHistory.cpp
   GDBRemoteCommunicationReplayServer.cpp
   GDBRemoteCommunicationServer.cpp
   GDBRemoteCommunicationServerCommon.cpp
   GDBRemoteCommunicationServerLLGS.cpp
   GDBRemoteCommunicationServerPlatform.cpp
   GDBRemoteRegisterContext.cpp
+  GDBRemoteRegisterContext_x86.cpp
   ProcessGDBRemote.cpp
   ProcessGDBRemoteLog.cpp
   ThreadGDBRemote.cpp
 
   LINK_LIBS
     lldbBreakpoint
     lldbCore
     lldbDataFormatters

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

 class GDBRemoteDynamicRegisterInfo final : public DynamicRegisterInfo {
 public:
   GDBRemoteDynamicRegisterInfo() : DynamicRegisterInfo() {}
 
   ~GDBRemoteDynamicRegisterInfo() override = default;
 
   void HardcodeARMRegisters(bool from_scratch);
   bool UpdateARM64SVERegistersInfos(uint64_t vg);
+  void Finalize(const lldb_private::ArchSpec &arch);
+
+  void PreFinalize_x86_64();
+  void PostFinalize_x86_64();
 };
 
 class GDBRemoteRegisterContext : public RegisterContext {
 public:
   GDBRemoteRegisterContext(ThreadGDBRemote &thread, uint32_t concrete_frame_idx,
                            GDBRemoteDynamicRegisterInfoSP reg_info_sp,
                            bool read_all_at_once, bool write_all_at_once);
 

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

if (match) {
                           vfp_reg_set);
             }
           }
         }
       }
     }
   }
 }
+
+void GDBRemoteDynamicRegisterInfo::Finalize(const ArchSpec &arch) {
+  if (m_finalized)
+    return;
+
+  switch (arch.GetMachine()) {
+  case llvm::Triple::x86_64:
+    PreFinalize_x86_64();
+    break;
+
+  default:
+    break;
+  }
+
+  DynamicRegisterInfo::Finalize(arch);
+
+  switch (arch.GetMachine()) {
+  case llvm::Triple::x86_64:
+    PostFinalize_x86_64();
+    break;
+
+  default:
+    break;
+  }
+}

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

+//===-- GDBRemoteRegisterContext_x86.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 "GDBRemoteRegisterContext.h"
+
+using namespace lldb;
+using namespace lldb_private;
+using namespace lldb_private::process_gdb_remote;
+
+enum PartialGPRRegKind {
+  eRegKind32,
+  eRegKind16,
+  eRegKind8h,
+  eRegKind8l,
+
+  eRegKindCount
+};
+
+struct PartialGPRReg {
+  const char *name;
+
+  // extra storage used for pointers
+  uint32_t value_regs[2];
+  uint32_t invalidate_regs[eRegKindCount];
+};
+
+struct PartialGPRRegSet {
+  const char *reg64;
+  PartialGPRReg r[eRegKindCount];
+};
+
+static std::array<PartialGPRRegSet, 16> partial_gpr_regs = {{
+    {"rax", {{"eax"}, {"ax"}, {"ah"}, {"al"}}},
+    {"rbx", {{"ebx"}, {"bx"}, {"bh"}, {"bl"}}},
+    {"rcx", {{"ecx"}, {"cx"}, {"ch"}, {"cl"}}},
+    {"rdx", {{"edx"}, {"dx"}, {"dh"}, {"dl"}}},
+    {"rdi", {{"edi"}, {"di"}, {nullptr}, {"dil"}}},
+    {"rsi", {{"esi"}, {"si"}, {nullptr}, {"sil"}}},
+    {"rbp", {{"ebp"}, {"bp"}, {nullptr}, {"bpl"}}},
+    {"rsp", {{"esp"}, {"sp"}, {nullptr}, {"spl"}}},
+    {"r8", {{"r8d"}, {"r8w"}, {nullptr}, {"r8l"}}},
+    {"r9", {{"r9d"}, {"r9w"}, {nullptr}, {"r9l"}}},
+    {"r10", {{"r10d"}, {"r10w"}, {nullptr}, {"r10l"}}},
+    {"r11", {{"r11d"}, {"r11w"}, {nullptr}, {"r11l"}}},
+    {"r12", {{"r12d"}, {"r12w"}, {nullptr}, {"r12l"}}},
+    {"r13", {{"r13d"}, {"r13w"}, {nullptr}, {"r13l"}}},
+    {"r14", {{"r14d"}, {"r14w"}, {nullptr}, {"r14l"}}},
+    {"r15", {{"r15d"}, {"r15w"}, {nullptr}, {"r15l"}}},
+}};
+
+static uint32_t partial_gpr_sizes[eRegKindCount] = {4, 2, 1, 1};
+
+struct MMReg {
+  const char *streg;
+  const char *name;
+
+  // extra storage used for pointers
+  uint32_t value_regs[2];
+};
+
+static std::array<MMReg, 16> mm_regs = {{
+    {"st0", "mm0"},
+    {"st1", "mm1"},
+    {"st2", "mm2"},
+    {"st3", "mm3"},
+    {"st4", "mm4"},
+    {"st5", "mm5"},
+    {"st6", "mm6"},
+    {"st7", "mm7"},
+}};
+
+void GDBRemoteDynamicRegisterInfo::PreFinalize_x86_64() {
+  uint32_t max_regnum = 0;
+  uint32_t next_regindex = m_regs.size();
+  for (const RegisterInfo &reg : m_regs)
+    max_regnum = std::max(max_regnum, reg.kinds[eRegisterKindProcessPlugin]);
+
+  ConstString group;
+  group.SetCString("partial registers");
+
+  for (PartialGPRRegSet &partial_regset : partial_gpr_regs) {
+    const RegisterInfo *reg64 = GetRegisterInfo(partial_regset.reg64);
+    if (!reg64 || reg64->byte_size != 8)
+      continue;
+
+    const RegisterInfo *reginfo[eRegKindCount];
+    uint32_t regno[eRegKindCount];
+
+    for (int i = 0; i < eRegKindCount; i++) {
+      if (!partial_regset.r[i].name)
+        continue;
+
+      reginfo[i] = GetRegisterInfo(partial_regset.r[i].name);
+      if (reginfo[i])
+        regno[i] = reginfo[i]->kinds[eRegisterKindProcessPlugin];
+      else
+        regno[i] = ++max_regnum;
+    }
+
+    for (int i = 0; i < eRegKindCount; i++) {
+      if (!partial_regset.r[i].name)
+        continue;
+
+      partial_regset.r[i].value_regs[0] =
+          reg64->kinds[eRegisterKindProcessPlugin];
+      partial_regset.r[i].value_regs[1] = LLDB_INVALID_REGNUM;
+
+      int k = 0;
+      for (int j = 0; j < eRegKindCount; j++) {
+        if (i != j && partial_regset.r[j].name)
+          partial_regset.r[i].invalidate_regs[k++] = regno[j];
+      }
+      partial_regset.r[i].invalidate_regs[k] = LLDB_INVALID_REGNUM;
+
+      struct RegisterInfo new_reg {
+        partial_regset.r[i].name, nullptr, partial_gpr_sizes[i],
+            LLDB_INVALID_INDEX32, reg64->encoding, reg64->format,
+            {
+                LLDB_INVALID_REGNUM, LLDB_INVALID_REGNUM,
+                LLDB_INVALID_REGNUM, regno[i],
+                next_regindex++,
+            },
+            partial_regset.r[i].value_regs, partial_regset.r[i].invalidate_regs,
+            nullptr, 0
+      };
+
+      ConstString name;
+      ConstString alt_name;
+      name.SetCString(new_reg.name);
+      AddRegister(new_reg, name, alt_name, group);
+    }
+  }
+
+  for (MMReg &mmreg : mm_regs) {
+    const RegisterInfo *streg = GetRegisterInfo(mmreg.streg);
+    if (!streg || streg->byte_size != 10)
+      continue;
+
+    if (GetRegisterInfo(mmreg.name))
+      continue;
+
+    mmreg.value_regs[0] = streg->kinds[eRegisterKindProcessPlugin];
+    mmreg.value_regs[1] = LLDB_INVALID_REGNUM;
+
+    struct RegisterInfo new_reg {
+      mmreg.name, nullptr, 8, LLDB_INVALID_INDEX32, eEncodingUint, eFormatHex,
+          {
+              LLDB_INVALID_REGNUM, LLDB_INVALID_REGNUM, LLDB_INVALID_REGNUM,
+              ++max_regnum,        next_regindex++,
+          },
+          mmreg.value_regs, nullptr, nullptr, 0
+    };
+
+    ConstString name;
+    ConstString alt_name;
+    name.SetCString(new_reg.name);
+    AddRegister(new_reg, name, alt_name, group);
+  }
+}
+
+void GDBRemoteDynamicRegisterInfo::PostFinalize_x86_64() {
+  for (PartialGPRRegSet &partial_regset : partial_gpr_regs) {
+    if (!partial_regset.r[eRegKind8h].name)
+      continue;
+
+    llvm::StringRef reg8h_name = partial_regset.r[eRegKind8h].name;
+    const RegisterInfo *reg8h = GetRegisterInfo(reg8h_name);
+    const RegisterInfo *reg8l =
+        GetRegisterInfo(partial_regset.r[eRegKind8l].name);
+    if (!reg8h || !reg8l || reg8h->byte_size != 1 || reg8l->byte_size != 1)
+      continue;
+
+    // we currently have no way of indicating offset via value_regs, so we
+    // instead increment it after Finalize() assigns byte offsets
+    if (reg8h->byte_offset == reg8l->byte_offset) {
+      for (auto &reg_info : m_regs) {
+        if (reg_info.name == reg8h_name) {
+          reg_info.byte_offset++;
+          break;
+        }
+      }
+    }
+  }
+}

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

def test_x86_64_regs(self):
                    ["r8 = 0x6867666564636261"])
         self.match("register read arg6",
                    ["r9 = 0x7877767574737271"])
         self.match("register read pc",
                    ["rip = 0x8887868584838281"])
         self.match("register read flags",
                    ["eflags = 0x94939291"])
 
+        # 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"])
+
         # 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 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}"])