[lldb] DynamicRegisterInfo: fix wrong regnos in Dump()

[mgorny]

Summary

Fix the DynamicRegisterInfo::Dump() method to correctly assume that
the register numbers in value_regs and invalidate_regs arrays are
remote (eRegisterKindProcessPlugin) rather than local
(eRegisterKindLLDB). This fixes invalid memory references when the two
register numbers are not in sync.

[mgorny]

TestRemoteRegNums suggests that there's also code relying on remote regnums being used there. What a mess :-(.

[mgorny]

Ok, I think it might just be Dump() being broken. I've found another bug with my code, and things seem to work after fixing it, so I suppose the problem is not really worth my effort right now.

[mgorny]

So I've given it some thought and found a simple fix.

[mgorny]

Also updated the comments in struct RegisterInfo to indicate what kind of numbers go in these arrays.

[labath]

I'm not sure if this is correct, or if I want it to be correct. I think that a lot of the confusion comes from the fact that this structure is used both in lldb-server and lldb, and I'm not sure that the fields have the same meaning in both cases (specifically, the "process plugin" concept is not really a thing in lldb-server). I think it would be better to standardize on the "lldb" numbers, as those should be available everywhere, but I may be forgetting some important issues. Omair may remember this in more detail.

[omjavaid]

This is complicated mainly because we are trying to merge two different register numbering scheme. On LLDB (host) side we try to construct register number from parsing register xml assigning register numbers in the increasing order. But we also have target supplied register numbers in regnum field. However registers on LLDB server side have fixed numbering although it may look like assigned in increasing order but this may not always be the case. Presence of optional register sets also doesnt help so we have the mess on managing target vs host register numbers not always in sync.

For invalidate_regs and value_regs register numbers should always be the ones assigned by remote i-e eRegisterKindProcessPlugin. So I would say this patch is ok as it corrects that info.

Broadly speaking we should do something about integrating user typed registers which may be unions of various other registers but should not require numbering.

[mgorny]

Replaced by D110027.

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

 //===-- lldb-private-types.h ------------------------------------*- C++ -*-===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLDB_LLDB_PRIVATE_TYPES_H
 #define LLDB_LLDB_PRIVATE_TYPES_H
 
 #if defined(__cplusplus)
 
 #include "lldb/lldb-private.h"
 
 #include "llvm/ADT/ArrayRef.h"
 
 namespace llvm {
 namespace sys {
 class DynamicLibrary;
 }
 }
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 eRegisterKindProcessPlugin.
+  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.
+  /// rax ax, ah, and al. Register numbers are of eRegisterKindProcessPlugin.
   uint32_t *invalidate_regs;
   /// A DWARF expression that when evaluated gives the byte size of this
   /// register.
   const uint8_t *dynamic_size_dwarf_expr_bytes;
   /// The length of the DWARF expression in bytes in the
   /// dynamic_size_dwarf_expr_bytes member.
   size_t dynamic_size_dwarf_len;
 

lldb/source/Plugins/Process/Utility/DynamicRegisterInfo.cpp

if (m_regs[i].kinds[eRegisterKindEHFrame] != LLDB_INVALID_REGNUM)
       s.Printf(", ehframe = %3u", m_regs[i].kinds[eRegisterKindEHFrame]);
     if (m_regs[i].kinds[eRegisterKindGeneric] != LLDB_INVALID_REGNUM)
       s.Printf(", generic = %3u", m_regs[i].kinds[eRegisterKindGeneric]);
     if (m_regs[i].alt_name)
       s.Printf(", alt-name = %s", m_regs[i].alt_name);
     if (m_regs[i].value_regs) {
       s.Printf(", value_regs = [ ");
       for (size_t j = 0; m_regs[i].value_regs[j] != LLDB_INVALID_REGNUM; ++j) {
-        s.Printf("%s ", m_regs[m_regs[i].value_regs[j]].name);
+        const RegisterInfo *value_reg = GetRegisterInfo(
+            eRegisterKindProcessPlugin, m_regs[i].value_regs[j]);
+        if (value_reg)
+          s.Printf("%s ", value_reg->name);
+        else
+          s.Printf("(%d) ", m_regs[i].value_regs[j]);
       }
       s.Printf("]");
     }
     if (m_regs[i].invalidate_regs) {
       s.Printf(", invalidate_regs = [ ");
       for (size_t j = 0; m_regs[i].invalidate_regs[j] != LLDB_INVALID_REGNUM;
            ++j) {
-        s.Printf("%s ", m_regs[m_regs[i].invalidate_regs[j]].name);
+        const RegisterInfo *invalidate_reg = GetRegisterInfo(
+            eRegisterKindProcessPlugin, m_regs[i].invalidate_regs[j]);
+        if (invalidate_reg)
+          s.Printf("%s ", invalidate_reg->name);
+        else
+          s.Printf("(%d) ", m_regs[i].invalidate_regs[j]);
       }
       s.Printf("]");
     }
     s.EOL();
   }
 
   const size_t num_sets = m_sets.size();
   s.Printf("%p: DynamicRegisterInfo contains %" PRIu64 " register sets:\n",