[lldb] Support querying registers via generic names without alt_names

[mgorny]

Summary

Update GetRegisterInfoByName() methods to support getting registers
by a generic name independently of alt_name entries in the register
context. This makes it possible to use generic names when interacting
with gdbserver (that does not supply alt_names). It also makes it
possible to remove some of the duplicated information from register
context declarations and/or use alt_names for another purpose.

[mgorny]

Preserve the prior (probably accidental) behavior of generic names taking precedence over later registers with the same name (e.g. generic sp alias taking precedence over 16-bit sp pseudo-register).

[mgorny]

The test requires building with X86 target.

[labath]

I'm not sure we should be preserving this behavior for the native register context. User-provided strings should not make it there, and code should really be retrieving the registers in some other way (like via the LLDB_REGNUM_GENERIC_*** constants).

It would also be nice to follow this up by actually removing the generic altnames from register definitions, for cases where they don't match architecturally defined names.

[mgorny]

In D108554#2993766, @labath wrote:

I'm not sure we should be preserving this behavior for the native register context. User-provided strings should not make it there, and code should really be retrieving the registers in some other way (like via the LLDB_REGNUM_GENERIC_*** constants).

Could you rephrase this comment to indicate the requested change, if any? ;-)

It would also be nice to follow this up by actually removing the generic altnames from register definitions, for cases where they don't match architecturally defined names.

That was roughly my plan, presuming you like the idea. Should I include this in the same commit or separately for each platform?

[labath]

In D108554#2993830, @mgorny wrote:

In D108554#2993766, @labath wrote:

I'm not sure we should be preserving this behavior for the native register context. User-provided strings should not make it there, and code should really be retrieving the registers in some other way (like via the LLDB_REGNUM_GENERIC_*** constants).

Could you rephrase this comment to indicate the requested change, if any? ;-)

Well.. the comment was open ended, because my thoughts on this are quite open ended as well.. :) But if I wanted to be more precise, I suppose it boils down to this:

• Is the NativeRC change there for a particular reason (like, does anything break if you leave it out)?
• Do _you_ agree with my assessment of its necessity?

If there's no reason for its presence, and you don't see it as necessary, then I'd like to remove it. If not, let's talk.

It would also be nice to follow this up by actually removing the generic altnames from register definitions, for cases where they don't match architecturally defined names.

That was roughly my plan, presuming you like the idea. Should I include this in the same commit or separately for each platform?

Sounds good. I think we should do it as a separate patch. Whether it's one or more of them depends on how large they get...

[mgorny]

In D108554#2993993, @labath wrote:

In D108554#2993830, @mgorny wrote:

In D108554#2993766, @labath wrote:

I'm not sure we should be preserving this behavior for the native register context. User-provided strings should not make it there, and code should really be retrieving the registers in some other way (like via the LLDB_REGNUM_GENERIC_*** constants).

Could you rephrase this comment to indicate the requested change, if any? ;-)

Well.. the comment was open ended, because my thoughts on this are quite open ended as well.. :) But if I wanted to be more precise, I suppose it boils down to this:

• Is the NativeRC change there for a particular reason (like, does anything break if you leave it out)?
• Do _you_ agree with my assessment of its necessity?

If there's no reason for its presence, and you don't see it as necessary, then I'd like to remove it. If not, let's talk.

Ah, now I get what you mean. To be honest, I've done that because I wasn't sure whether there isn't any code relying on that. I guess I'll try removing it and see if any tests fail.

[mgorny]

Ok, so I've done some testing and indeed Native* part doesn't seem necessary.

However, after removing the aliases from x86_64 target def, there are two test regressions. The two following snippets no longer seem to work:

eip = frame.FindRegister("pc")
sp_value = frame0.FindValue("sp", lldb.eValueTypeRegister)

This doesn't seem to work anymore. Should I attempt to fix the API to make grabbing these registers by name work again, or should I try to modify the tests to grab these registers via generic reg nums?

[labath]

These are pretty much the SB equivalents of register read, and they should keep working

[mgorny]

Fixed SBFrame stuffs. While at it, changed ValueObjectRegister to take const RegisterInfo * instead of the index, since it's almost always what it really wants.

[labath]

lldb/source/API/SBFrame.cpp

636–640

I like this.

lldb/source/Target/RegisterContext.cpp

57–59

This isn't just "consistent with" generic aliases taking precedence. This is actually the implementation of that precedence (at least, after we remove the aliases in the other patch. This should just state as a fact that we want the aliases to take priority. The historic trivia can go into the commit message.

lldb/include/lldb/Core/ValueObjectRegister.h

 };
 
 class ValueObjectRegister : public ValueObject {
 public:
   ~ValueObjectRegister() override;
 
   static lldb::ValueObjectSP Create(ExecutionContextScope *exe_scope,
                                     lldb::RegisterContextSP &reg_ctx_sp,
-                                    uint32_t reg_num);
+                                    const RegisterInfo *reg_info);
 
   llvm::Optional<uint64_t> GetByteSize() override;
 
   lldb::ValueType GetValueType() const override {
     return lldb::eValueTypeRegister;
   }
 
   ConstString GetTypeName() override;
protected:
 
   lldb::RegisterContextSP m_reg_ctx_sp;
   RegisterInfo m_reg_info;
   RegisterValue m_reg_value;
   ConstString m_type_name;
   CompilerType m_compiler_type;
 
 private:
-  void ConstructObject(uint32_t reg_num);
+  void ConstructObject(const RegisterInfo *reg_info);
 
   friend class ValueObjectRegisterSet;
 
   ValueObjectRegister(ValueObject &parent, lldb::RegisterContextSP &reg_ctx_sp,
-                      uint32_t reg_num);
+                      const RegisterInfo *reg_info);
   ValueObjectRegister(ExecutionContextScope *exe_scope,
                       ValueObjectManager &manager,
-                      lldb::RegisterContextSP &reg_ctx_sp, uint32_t reg_num);
+                      lldb::RegisterContextSP &reg_ctx_sp,
+                      const RegisterInfo *reg_info);
 
   // For ValueObject only
   ValueObjectRegister(const ValueObjectRegister &) = delete;
   const ValueObjectRegister &operator=(const ValueObjectRegister &) = delete;
 };
 
 } // namespace lldb_private
 
 #endif // LLDB_CORE_VALUEOBJECTREGISTER_H

lldb/source/API/SBFrame.cpp

if (stop_locker.TryLock(&process->GetRunLock())) {
             sb_value.SetSP(value_sp, use_dynamic);
           }
         } break;
 
         case eValueTypeRegister: // stack frame register value
         {
           RegisterContextSP reg_ctx(frame->GetRegisterContext());
           if (reg_ctx) {
-            const uint32_t num_regs = reg_ctx->GetRegisterCount();
-            for (uint32_t reg_idx = 0; reg_idx < num_regs; ++reg_idx) {
-              const RegisterInfo *reg_info =
-                  reg_ctx->GetRegisterInfoAtIndex(reg_idx);
-              if (reg_info &&
-                  ((reg_info->name && strcasecmp(reg_info->name, name) == 0) ||
-                   (reg_info->alt_name &&
-                    strcasecmp(reg_info->alt_name, name) == 0))) {
-                value_sp = ValueObjectRegister::Create(frame, reg_ctx, reg_idx);
-                sb_value.SetSP(value_sp);
-                break;
-              }
+            if (const RegisterInfo *reg_info =
+                    reg_ctx->GetRegisterInfoByName(name)) {
+              value_sp = ValueObjectRegister::Create(frame, reg_ctx, reg_info);
             }
           }
         } break;
 
         case eValueTypeRegisterSet: // A collection of stack frame register
                                     // values
         {
           RegisterContextSP reg_ctx(frame->GetRegisterContext());
           if (reg_ctx) {
SBValue SBFrame::FindRegister(const char *name) {
   Process *process = exe_ctx.GetProcessPtr();
   if (target && process) {
     Process::StopLocker stop_locker;
     if (stop_locker.TryLock(&process->GetRunLock())) {
       frame = exe_ctx.GetFramePtr();
       if (frame) {
         RegisterContextSP reg_ctx(frame->GetRegisterContext());
         if (reg_ctx) {
-          const uint32_t num_regs = reg_ctx->GetRegisterCount();
-          for (uint32_t reg_idx = 0; reg_idx < num_regs; ++reg_idx) {
-            const RegisterInfo *reg_info =
-                reg_ctx->GetRegisterInfoAtIndex(reg_idx);
-            if (reg_info &&
-                ((reg_info->name && strcasecmp(reg_info->name, name) == 0) ||
-                 (reg_info->alt_name &&
-                  strcasecmp(reg_info->alt_name, name) == 0))) {
-              value_sp = ValueObjectRegister::Create(frame, reg_ctx, reg_idx);
-              result.SetSP(value_sp);
-              break;
-            }
+          if (const RegisterInfo *reg_info =
+                  reg_ctx->GetRegisterInfoByName(name)) {
+            value_sp = ValueObjectRegister::Create(frame, reg_ctx, reg_info);
           }
         }
       }
     }
   }
 
   return LLDB_RECORD_RESULT(result);
 }

lldb/source/Core/ValueObjectRegister.cpp

 }
 
 ValueObject *ValueObjectRegisterSet::CreateChildAtIndex(
     size_t idx, bool synthetic_array_member, int32_t synthetic_index) {
   ValueObject *valobj = nullptr;
   if (m_reg_ctx_sp && m_reg_set) {
     const size_t num_children = GetNumChildren();
     if (idx < num_children)
-      valobj = new ValueObjectRegister(*this, m_reg_ctx_sp,
-                                       m_reg_set->registers[idx]);
+      valobj = new ValueObjectRegister(
+          *this, m_reg_ctx_sp,
+          m_reg_ctx_sp->GetRegisterInfoAtIndex(m_reg_set->registers[idx]));
   }
   return valobj;
 }
 
 lldb::ValueObjectSP
 ValueObjectRegisterSet::GetChildMemberWithName(ConstString name,
                                                bool can_create) {
   ValueObject *valobj = nullptr;
   if (m_reg_ctx_sp && m_reg_set) {
     const RegisterInfo *reg_info =
         m_reg_ctx_sp->GetRegisterInfoByName(name.GetStringRef());
     if (reg_info != nullptr)
-      valobj = new ValueObjectRegister(*this, m_reg_ctx_sp,
-                                       reg_info->kinds[eRegisterKindLLDB]);
+      valobj = new ValueObjectRegister(*this, m_reg_ctx_sp, reg_info);
   }
   if (valobj)
     return valobj->GetSP();
   else
     return ValueObjectSP();
 }
 
 size_t
 ValueObjectRegisterSet::GetIndexOfChildWithName(ConstString name) {
   if (m_reg_ctx_sp && m_reg_set) {
     const RegisterInfo *reg_info =
         m_reg_ctx_sp->GetRegisterInfoByName(name.GetStringRef());
     if (reg_info != nullptr)
       return reg_info->kinds[eRegisterKindLLDB];
   }
   return UINT32_MAX;
 }
 
 #pragma mark -
 #pragma mark ValueObjectRegister
 
-void ValueObjectRegister::ConstructObject(uint32_t reg_num) {
-  const RegisterInfo *reg_info = m_reg_ctx_sp->GetRegisterInfoAtIndex(reg_num);
+void ValueObjectRegister::ConstructObject(const RegisterInfo *reg_info) {
   if (reg_info) {
     m_reg_info = *reg_info;
     if (reg_info->name)
       m_name.SetCString(reg_info->name);
     else if (reg_info->alt_name)
       m_name.SetCString(reg_info->alt_name);
   }
 }
 
 ValueObjectRegister::ValueObjectRegister(ValueObject &parent,
                                          lldb::RegisterContextSP &reg_ctx_sp,
-                                         uint32_t reg_num)
+                                         const RegisterInfo *reg_info)
     : ValueObject(parent), m_reg_ctx_sp(reg_ctx_sp), m_reg_info(),
       m_reg_value(), m_type_name(), m_compiler_type() {
   assert(reg_ctx_sp.get());
-  ConstructObject(reg_num);
+  ConstructObject(reg_info);
 }
 
 ValueObjectSP ValueObjectRegister::Create(ExecutionContextScope *exe_scope,
                                           lldb::RegisterContextSP &reg_ctx_sp,
-                                          uint32_t reg_num) {
+                                          const RegisterInfo *reg_info) {
   auto manager_sp = ValueObjectManager::Create();
-  return (new ValueObjectRegister(exe_scope, *manager_sp, reg_ctx_sp, reg_num))
+  return (new ValueObjectRegister(exe_scope, *manager_sp, reg_ctx_sp, reg_info))
       ->GetSP();
 }
 
 ValueObjectRegister::ValueObjectRegister(ExecutionContextScope *exe_scope,
                                          ValueObjectManager &manager,
                                          lldb::RegisterContextSP &reg_ctx,
-                                         uint32_t reg_num)
+                                         const RegisterInfo *reg_info)
     : ValueObject(exe_scope, manager), m_reg_ctx_sp(reg_ctx), m_reg_info(),
       m_reg_value(), m_type_name(), m_compiler_type() {
   assert(reg_ctx);
-  ConstructObject(reg_num);
+  ConstructObject(reg_info);
 }
 
 ValueObjectRegister::~ValueObjectRegister() = default;
 
 CompilerType ValueObjectRegister::GetCompilerTypeImpl() {
   if (!m_compiler_type.IsValid()) {
     ExecutionContext exe_ctx(GetExecutionContextRef());
     if (auto *target = exe_ctx.GetTargetPtr()) {

lldb/source/Host/common/NativeRegisterContext.cpp

 // }
 
 const RegisterInfo *
 NativeRegisterContext::GetRegisterInfoByName(llvm::StringRef reg_name,
                                              uint32_t start_idx) {
   if (reg_name.empty())
     return nullptr;
 
+  // Generic register names take precedence over specific register names.
+  // For example, on x86 we want "sp" to refer to the complete RSP/ESP register
+  // rather than the 16-bit SP pseudo-register.
+  uint32_t generic_reg = Args::StringToGenericRegister(reg_name);
+  if (generic_reg != LLDB_INVALID_REGNUM) {
+    const RegisterInfo *reg_info =
+        GetRegisterInfo(eRegisterKindGeneric, generic_reg);
+    if (reg_info)
+      return reg_info;
+  }
+
   const uint32_t num_registers = GetRegisterCount();
   for (uint32_t reg = start_idx; reg < num_registers; ++reg) {
     const RegisterInfo *reg_info = GetRegisterInfoAtIndex(reg);
 
     if (reg_name.equals_insensitive(reg_info->name) ||
         reg_name.equals_insensitive(reg_info->alt_name))
       return reg_info;
   }
+
   return nullptr;
 }
 
 const RegisterInfo *NativeRegisterContext::GetRegisterInfo(uint32_t kind,
                                                            uint32_t num) {
   const uint32_t reg_num = ConvertRegisterKindToRegisterNumber(kind, num);
   if (reg_num == LLDB_INVALID_REGNUM)
     return nullptr;

lldb/source/Target/RegisterContext.cpp

 }
 
 const RegisterInfo *
 RegisterContext::GetRegisterInfoByName(llvm::StringRef reg_name,
                                        uint32_t start_idx) {
   if (reg_name.empty())
     return nullptr;
 
+  // Generic register names take precedence over specific register names.
+  // For example, on x86 we want "sp" to refer to the complete RSP/ESP register
+  // rather than the 16-bit SP pseudo-register.
+  uint32_t generic_reg = Args::StringToGenericRegister(reg_name);
+  if (generic_reg != LLDB_INVALID_REGNUM) {
+    const RegisterInfo *reg_info =
+        GetRegisterInfo(eRegisterKindGeneric, generic_reg);
+    if (reg_info)
+      return reg_info;
+  }
+
   const uint32_t num_registers = GetRegisterCount();
   for (uint32_t reg = start_idx; reg < num_registers; ++reg) {
     const RegisterInfo *reg_info = GetRegisterInfoAtIndex(reg);
 
     if (reg_name.equals_insensitive(reg_info->name) ||
         reg_name.equals_insensitive(reg_info->alt_name))
       return reg_info;
   }
+
   return nullptr;
 }
 
 uint32_t
 RegisterContext::UpdateDynamicRegisterSize(const lldb_private::ArchSpec &arch,
                                            RegisterInfo *reg_info) {
   ExecutionContext exe_ctx(CalculateThread());
 

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

+from __future__ import print_function
+import lldb
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test.decorators import *
+from gdbclientutils import *
+
+
+class TestGDBServerTargetXML(GDBRemoteTestBase):
+
+    @skipIfXmlSupportMissing
+    @skipIfRemote
+    @skipIfLLVMTargetMissing("X86")
+    def test_x86_64_regs(self):
+        """Test grabbing various x86_64 registers from gdbserver."""
+        reg_data = [
+            "0102030405060708",  # rcx
+            "1112131415161718",  # rdx
+            "2122232425262728",  # rsi
+            "3132333435363738",  # rdi
+            "4142434445464748",  # rbp
+            "5152535455565758",  # rsp
+            "6162636465666768",  # r8
+            "7172737475767778",  # r9
+            "8182838485868788",  # rip
+            "91929394",  # eflags
+            "0102030405060708090a",  # st0
+            "1112131415161718191a",  # st1
+        ] + 6 * [
+            "2122232425262728292a"  # st2..st7
+        ] + [
+            "8182838485868788898a8b8c8d8e8f90",  # xmm0
+            "9192939495969798999a9b9c9d9e9fa0",  # xmm1
+        ] + 14 * [
+            "a1a2a3a4a5a6a7a8a9aaabacadaeafb0",  # xmm2..xmm15
+        ] + [
+            "00000000",  # mxcsr
+        ] + [
+            "b1b2b3b4b5b6b7b8b9babbbcbdbebfc0",  # ymm0h
+            "c1c2c3c4c5c6c7c8c9cacbcccdcecfd0",  # ymm1h
+        ] + 14 * [
+            "d1d2d3d4d5d6d7d8d9dadbdcdddedfe0",  # ymm2h..ymm15h
+        ]
+
+        class MyResponder(MockGDBServerResponder):
+            def qXferRead(self, obj, annex, offset, length):
+                if annex == "target.xml":
+                    return """<?xml version="1.0"?>
+                        <!DOCTYPE feature SYSTEM "gdb-target.dtd">
+                        <target>
+                          <architecture>i386:x86-64</architecture>
+                          <osabi>GNU/Linux</osabi>
+                          <feature name="org.gnu.gdb.i386.core">
+                            <reg name="rcx" bitsize="64" type="int64" regnum="2"/>
+                            <reg name="rdx" bitsize="64" type="int64" regnum="3"/>
+                            <reg name="rsi" bitsize="64" type="int64" regnum="4"/>
+                            <reg name="rdi" bitsize="64" type="int64" regnum="5"/>
+                            <reg name="rbp" bitsize="64" type="data_ptr" regnum="6"/>
+                            <reg name="rsp" bitsize="64" type="data_ptr" regnum="7"/>
+                            <reg name="r8" bitsize="64" type="int64" regnum="8"/>
+                            <reg name="r9" bitsize="64" type="int64" regnum="9"/>
+                            <reg name="rip" bitsize="64" type="code_ptr" regnum="16"/>
+                            <reg name="eflags" bitsize="32" type="i386_eflags" regnum="17"/>
+                            <reg name="st0" bitsize="80" type="i387_ext" regnum="24"/>
+                            <reg name="st1" bitsize="80" type="i387_ext" regnum="25"/>
+                            <reg name="st2" bitsize="80" type="i387_ext" regnum="26"/>
+                            <reg name="st3" bitsize="80" type="i387_ext" regnum="27"/>
+                            <reg name="st4" bitsize="80" type="i387_ext" regnum="28"/>
+                            <reg name="st5" bitsize="80" type="i387_ext" regnum="29"/>
+                            <reg name="st6" bitsize="80" type="i387_ext" regnum="30"/>
+                            <reg name="st7" bitsize="80" type="i387_ext" regnum="31"/>
+                          </feature>
+                          <feature name="org.gnu.gdb.i386.sse">
+                            <reg name="xmm0" bitsize="128" type="vec128" regnum="40"/>
+                            <reg name="xmm1" bitsize="128" type="vec128" regnum="41"/>
+                            <reg name="xmm2" bitsize="128" type="vec128" regnum="42"/>
+                            <reg name="xmm3" bitsize="128" type="vec128" regnum="43"/>
+                            <reg name="xmm4" bitsize="128" type="vec128" regnum="44"/>
+                            <reg name="xmm5" bitsize="128" type="vec128" regnum="45"/>
+                            <reg name="xmm6" bitsize="128" type="vec128" regnum="46"/>
+                            <reg name="xmm7" bitsize="128" type="vec128" regnum="47"/>
+                            <reg name="xmm8" bitsize="128" type="vec128" regnum="48"/>
+                            <reg name="xmm9" bitsize="128" type="vec128" regnum="49"/>
+                            <reg name="xmm10" bitsize="128" type="vec128" regnum="50"/>
+                            <reg name="xmm11" bitsize="128" type="vec128" regnum="51"/>
+                            <reg name="xmm12" bitsize="128" type="vec128" regnum="52"/>
+                            <reg name="xmm13" bitsize="128" type="vec128" regnum="53"/>
+                            <reg name="xmm14" bitsize="128" type="vec128" regnum="54"/>
+                            <reg name="xmm15" bitsize="128" type="vec128" regnum="55"/>
+                            <reg name="mxcsr" bitsize="32" type="i386_mxcsr" regnum="56" group="vector"/>
+                          </feature>
+                          <feature name="org.gnu.gdb.i386.avx">
+                            <reg name="ymm0h" bitsize="128" type="uint128" regnum="60"/>
+                            <reg name="ymm1h" bitsize="128" type="uint128" regnum="61"/>
+                            <reg name="ymm2h" bitsize="128" type="uint128" regnum="62"/>
+                            <reg name="ymm3h" bitsize="128" type="uint128" regnum="63"/>
+                            <reg name="ymm4h" bitsize="128" type="uint128" regnum="64"/>
+                            <reg name="ymm5h" bitsize="128" type="uint128" regnum="65"/>
+                            <reg name="ymm6h" bitsize="128" type="uint128" regnum="66"/>
+                            <reg name="ymm7h" bitsize="128" type="uint128" regnum="67"/>
+                            <reg name="ymm8h" bitsize="128" type="uint128" regnum="68"/>
+                            <reg name="ymm9h" bitsize="128" type="uint128" regnum="69"/>
+                            <reg name="ymm10h" bitsize="128" type="uint128" regnum="70"/>
+                            <reg name="ymm11h" bitsize="128" type="uint128" regnum="71"/>
+                            <reg name="ymm12h" bitsize="128" type="uint128" regnum="72"/>
+                            <reg name="ymm13h" bitsize="128" type="uint128" regnum="73"/>
+                            <reg name="ymm14h" bitsize="128" type="uint128" regnum="74"/>
+                            <reg name="ymm15h" bitsize="128" type="uint128" regnum="75"/>
+                          </feature>
+                        </target>""", False
+                else:
+                    return None, False
+
+            def readRegister(self, regnum):
+                return ""
+
+            def readRegisters(self):
+                return "".join(reg_data)
+
+            def writeRegisters(self, reg_hex):
+                return "OK"
+
+            def haltReason(self):
+                return "T02thread:1ff0d;threads:1ff0d;thread-pcs:000000010001bc00;07:0102030405060708;10:1112131415161718;"
+
+        self.server.responder = MyResponder()
+
+        target = self.createTarget("basic_eh_frame.yaml")
+        process = self.connect(target)
+        lldbutil.expect_state_changes(self, self.dbg.GetListener(), process,
+                                      [lldb.eStateStopped])
+
+        # test generic aliases
+        self.match("register read arg4",
+                   ["rcx = 0x0807060504030201"])
+        self.match("register read arg3",
+                   ["rdx = 0x1817161514131211"])
+        self.match("register read arg2",
+                   ["rsi = 0x2827262524232221"])
+        self.match("register read arg1",
+                   ["rdi = 0x3837363534333231"])
+        self.match("register read fp",
+                   ["rbp = 0x4847464544434241"])
+        self.match("register read sp",
+                   ["rsp = 0x5857565554535251"])
+        self.match("register read arg5",
+                   ["r8 = 0x6867666564636261"])
+        self.match("register read arg6",
+                   ["r9 = 0x7877767574737271"])
+        self.match("register read pc",
+                   ["rip = 0x8887868584838281"])
+        self.match("register read flags",
+                   ["eflags = 0x94939291"])