[LLDB] Support custom expedited register set in gdb-remote

[omjavaid]

Summary

This patch adds capability to introduce a custom expedited register set in gdb remote. Currently we sent register set 0 as expedited register but for the case of AArch64 SVE we intend to send additional information about register set configuration stored in vg register. This will happen only when SVE mode is selected so we need a way to inform gdb-remote which register set to use when.

[omjavaid]

@labath ping!

[labath]

Looking at the dependent patch, I see that you need to conjure up a fake register set to make this work. That works, but doesn't seem entirely optimal. This expedition function doesn't really care about register sets, it just needs a way to get a list of registers. What if we just made the interface return that? (e.g. ArrayRef<uint32_t> GetExpeditedRegisters())

And instead of handing the nullptr specially in the caller, we could just make the default implementation of this function return the first (zeroth) register set...

[omjavaid]

This reworks previous implementation by returning a vector containing register numbers to be expedited. Default case is minimal set of generic registers or complete register set 0.

In the child rev D82855 for NativeRegisterContextLinux_arm64 we call the base function and push_back vg reg num into register number vector before return.

[labath]

I like this a lot. LGTM with some small fixes inline.

lldb/source/Host/common/NativeRegisterContext.cpp

428–434

Remove LLDB_INVALID_REGNUM from the list. Then iterate as for(uint32_t gen_reg: k_expedited_registers).

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

513–515

Let's change the result type to Optional<Object> and return None here -- now that we support customizing the expedited registers, I think it's reasonable to allow a register context to say it does not want to expedite any registers. (The current for of the code already kind of supports that, but the use of Expected makes it sound like that is an erroneous state.)

798

I don't think this if is needed now -- we could remove it and save one level of indentation.

[omjavaid]

Fixed review comments.

lldb/include/lldb/Host/common/NativeRegisterContext.h

 
 #include "lldb/Host/common/NativeWatchpointList.h"
 #include "lldb/lldb-private.h"
 
 namespace lldb_private {
 
 class NativeThreadProtocol;
 
+enum class ExpeditedRegs { Minimal, Full };
+
 class NativeRegisterContext
     : public std::enable_shared_from_this<NativeRegisterContext> {
 public:
   // Constructors and Destructors
   NativeRegisterContext(NativeThreadProtocol &thread);
 
   virtual ~NativeRegisterContext();
 
WriteRegisterValueToMemory(const lldb_private::RegisterInfo *reg_info,
                              lldb::addr_t dst_addr, size_t dst_len,
                              const RegisterValue &reg_value);
 
   // Subclasses should not override these
   virtual lldb::tid_t GetThreadID() const;
 
   virtual NativeThreadProtocol &GetThread() { return m_thread; }
 
+  virtual std::vector<uint32_t>
+  GetExpeditedRegisters(ExpeditedRegs expType) const;
+
   const RegisterInfo *GetRegisterInfoByName(llvm::StringRef reg_name,
                                             uint32_t start_idx = 0);
 
   const RegisterInfo *GetRegisterInfo(uint32_t reg_kind, uint32_t reg_num);
 
   lldb::addr_t GetPC(lldb::addr_t fail_value = LLDB_INVALID_ADDRESS);
 
   virtual lldb::addr_t

lldb/source/Host/common/NativeRegisterContext.cpp

for (uint32_t reg_idx = 0; reg_idx < num_regs; ++reg_idx) {
     const RegisterInfo *reg_info = GetRegisterInfoAtIndex(reg_idx);
 
     if (reg_info->kinds[kind] == num)
       return reg_idx;
   }
 
   return LLDB_INVALID_REGNUM;
 }
+
+std::vector<uint32_t>
+NativeRegisterContext::GetExpeditedRegisters(ExpeditedRegs expType) const {
+  if (expType == ExpeditedRegs::Minimal) {
+    // Expedite only a minimum set of important generic registers.
+    static const uint32_t k_expedited_registers[] = {
+        LLDB_REGNUM_GENERIC_PC, LLDB_REGNUM_GENERIC_SP, LLDB_REGNUM_GENERIC_FP,
+        LLDB_REGNUM_GENERIC_RA, LLDB_INVALID_REGNUM};
+
+    std::vector<uint32_t> expedited_reg_nums;
+    for (const uint32_t *generic_reg_p = k_expedited_registers;
+         *generic_reg_p != LLDB_INVALID_REGNUM; ++generic_reg_p) {
+      uint32_t reg_num = ConvertRegisterKindToRegisterNumber(
+          eRegisterKindGeneric, *generic_reg_p);
+      if (reg_num == LLDB_INVALID_REGNUM)
+        continue; // Target does not support the given register.
+      else
+        expedited_reg_nums.push_back(reg_num);
+    }
+
+    return expedited_reg_nums;
+  }
+
+  if (GetRegisterSetCount() > 0 && expType == ExpeditedRegs::Full)
+    return std::vector<uint32_t>(GetRegisterSet(0)->registers,
+                                 GetRegisterSet(0)->registers +
+                                     GetRegisterSet(0)->num_registers);
+
+  return std::vector<uint32_t>();
+}

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

 GetRegistersAsJSON(NativeThreadProtocol &thread) {
   Log *log(GetLogIfAnyCategoriesSet(LIBLLDB_LOG_THREAD));
 
   NativeRegisterContext& reg_ctx = thread.GetRegisterContext();
 
   json::Object register_object;
 
 #ifdef LLDB_JTHREADSINFO_FULL_REGISTER_SET
-  // Expedite all registers in the first register set (i.e. should be GPRs)
-  // that are not contained in other registers.
-  const RegisterSet *reg_set_p = reg_ctx_sp->GetRegisterSet(0);
-  if (!reg_set_p)
-    return llvm::make_error<llvm::StringError>("failed to get registers",
-                                               llvm::inconvertibleErrorCode());
-  for (const uint32_t *reg_num_p = reg_set_p->registers;
-       *reg_num_p != LLDB_INVALID_REGNUM; ++reg_num_p) {
-    uint32_t reg_num = *reg_num_p;
+  const auto expedited_regs =
+      reg_ctx.GetExpeditedRegisters(ExpeditedRegs::Full);
 #else
-  // Expedite only a couple of registers until we figure out why sending
-  // registers is expensive.
-  static const uint32_t k_expedited_registers[] = {
-      LLDB_REGNUM_GENERIC_PC, LLDB_REGNUM_GENERIC_SP, LLDB_REGNUM_GENERIC_FP,
-      LLDB_REGNUM_GENERIC_RA, LLDB_INVALID_REGNUM};
-
-  for (const uint32_t *generic_reg_p = k_expedited_registers;
-       *generic_reg_p != LLDB_INVALID_REGNUM; ++generic_reg_p) {
-    uint32_t reg_num = reg_ctx.ConvertRegisterKindToRegisterNumber(
-        eRegisterKindGeneric, *generic_reg_p);
-    if (reg_num == LLDB_INVALID_REGNUM)
-      continue; // Target does not support the given register.
+  const auto expedited_regs =
+      reg_ctx.GetExpeditedRegisters(ExpeditedRegs::Minimal);
 #endif
-
+  if (expedited_regs.empty())
+    return llvm::make_error<llvm::StringError>("failed to get registers",
+                                               llvm::inconvertibleErrorCode());
+  for (auto &reg_num : expedited_regs) {
     const RegisterInfo *const reg_info_p =
         reg_ctx.GetRegisterInfoAtIndex(reg_num);
     if (reg_info_p == nullptr) {
       LLDB_LOGF(log,
                 "%s failed to get register info for register index %" PRIu32,
                 __FUNCTION__, reg_num);
       continue;
     }
GDBRemoteCommunicationServerLLGS::SendStopReplyPacketForThread(
   }
 
   //
   // Expedite registers.
   //
 
   // Grab the register context.
   NativeRegisterContext& reg_ctx = thread->GetRegisterContext();
-  // Expedite all registers in the first register set (i.e. should be GPRs)
-  // that are not contained in other registers.
-  const RegisterSet *reg_set_p;
-  if (reg_ctx.GetRegisterSetCount() > 0 &&
-      ((reg_set_p = reg_ctx.GetRegisterSet(0)) != nullptr)) {
-    LLDB_LOGF(log,
-              "GDBRemoteCommunicationServerLLGS::%s expediting registers "
-              "from set '%s' (registers set count: %zu)",
-              __FUNCTION__, reg_set_p->name ? reg_set_p->name : "<unnamed-set>",
-              reg_set_p->num_registers);
+  const auto expedited_regs =
+      reg_ctx.GetExpeditedRegisters(ExpeditedRegs::Full);
 
-    for (const uint32_t *reg_num_p = reg_set_p->registers;
-         *reg_num_p != LLDB_INVALID_REGNUM; ++reg_num_p) {
+  if (!expedited_regs.empty()) {
+    for (auto &reg_num : expedited_regs) {
       const RegisterInfo *const reg_info_p =
-          reg_ctx.GetRegisterInfoAtIndex(*reg_num_p);
-      if (reg_info_p == nullptr) {
-        LLDB_LOGF(log,
-                  "GDBRemoteCommunicationServerLLGS::%s failed to get "
-                  "register info for register set '%s', register index "
-                  "%" PRIu32,
-                  __FUNCTION__,
-                  reg_set_p->name ? reg_set_p->name : "<unnamed-set>",
-                  *reg_num_p);
-      } else if (reg_info_p->value_regs == nullptr) {
-        // Only expediate registers that are not contained in other registers.
+          reg_ctx.GetRegisterInfoAtIndex(reg_num);
+      if (reg_info_p != nullptr && reg_info_p->value_regs == nullptr) {
         RegisterValue reg_value;
         Status error = reg_ctx.ReadRegister(reg_info_p, reg_value);
         if (error.Success()) {
-          response.Printf("%.02x:", *reg_num_p);
+          response.Printf("%.02x:", reg_num);
           WriteRegisterValueInHexFixedWidth(response, reg_ctx, *reg_info_p,
                                             &reg_value, lldb::eByteOrderBig);
           response.PutChar(';');
         } else {
           LLDB_LOGF(log,
                     "GDBRemoteCommunicationServerLLGS::%s failed to read "
                     "register '%s' index %" PRIu32 ": %s",
                     __FUNCTION__,
                     reg_info_p->name ? reg_info_p->name : "<unnamed-register>",
-                    *reg_num_p, error.AsCString());
+                    reg_num, error.AsCString());
         }
       }
     }
   }
 
   const char *reason_str = GetStopReasonString(tid_stop_info.reason);
   if (reason_str != nullptr) {
     response.Printf("reason:%s;", reason_str);