[lldb] Generic support for caching register set reads/writes

[mgorny]

Summary

Introduce a new class for caching register set reads and writes
in the generic NativeRegisterContextRegisterInfo class. Use it
in the NetBSD plugin for initial testing.

The implementation provides a generic caching version of ReadRegister()
and WriteRegister() that call virtual methods implemented in actual
NativeRegisterContext* classes. It supports both caching reads,
and delayed writes. The cache is flushed whenever the thread resumes
execution or a new read follows writes.

The hook API consists of five methods that need to be implemented
in NativeRegisterContext* classes:

• GetRegisterSetForRegNum() to map register number to a register set number (the latter being opaque to NativeRegisterContextRegisterInfo).

• ReadRegisterSet() and WriteRegisterSet() to respectively read regset into cache and write it back. The actual cache storage is provided by final classes (m_gpr, m_fpr...).

• GetRegisterFromCache() and SetRegisterInCache() to respectively get and update individual register values inside the cache. The implementation of both methods can assume that ReadRegisterSet() has been called to fill the cache, and must not call either ReadRegisterSet() or WriteRegisterSet().

The NativeRegisterContextRegisterInfo class maintains state map of cache
for individual regsets, and issues appropriate ReadRegisterSet()
and WriteRegisterSet() calls to maintain it. It includes
a FlushRegisterSets() method that needs to be called whenever the thread
is about to resume execution.

[mgorny]

Ok, this is going to have to wait. I've found some hard bug in NetBSD's xstate support and don't have time+resources to figure it out at the moment.

[labath]

I'm wondering if this could be implemented as some kind of a separate class which you "mix" into your register context when you want to do caching. Possibly something like this:

class NRCLinux_MyArch: public NRCLinux, private RCCache::Backend {
  RCCache m_cache;

  NRCLinux_MyArch(...) : m_cache(this) {}

  // NRC interface
  ReadRegister(...) override { return m_cache.ReadRegister(...); }
  ...

  // Backend interface
  ReadReally(...) override { action happens here }
};

That would enable individual register contexts to explicitly opt into caching, which might be cleaner in the long term, as I anticipate it will take a very long time to convert all registers contexts to the new way of doing things.

lldb/source/Plugins/Process/Utility/NativeRegisterContextRegisterInfo.h

25

I think using Optional<integral> would be better. Although, I'm not sure what are the cases where a register does not belong to any register set.

64–83

I think new interfaces should use llvm::Error/Expected

[mgorny]

Moved stuff into mixin-style class. Changing return types is next on the list.

[mgorny]

Switched GetRegisterSetForRegNum to llvm::Optional. Also noticed that NativeRegisterContextNetBSD_x86_64::GetRegisterSetCount() is entirely meaningless.

[mgorny]

Rework error handling to use llvm::Error in the new methods.

[mgorny]

Now with actual caching support.

[labath]

I guess this slipped off my radar.... What's the state of this? Are you still interested in the patch? If yes, we can try again after rebasing.

[mgorny]

I don't think I really want to work on this right now. Even if it still works reliably, I'm not sure if the gain is really worth the added complexity (and risks).

lldb/source/Plugins/Process/NetBSD/NativeRegisterContextNetBSD_x86_64.h

 class NativeRegisterContextNetBSD_x86_64 : public NativeRegisterContextNetBSD {
 public:
   NativeRegisterContextNetBSD_x86_64(const ArchSpec &target_arch,
                                      NativeThreadProtocol &native_thread);
   uint32_t GetRegisterSetCount() const override;
 
   const RegisterSet *GetRegisterSet(uint32_t set_index) const override;
 
-  Status ReadRegister(const RegisterInfo *reg_info,
-                      RegisterValue &reg_value) override;
-
-  Status WriteRegister(const RegisterInfo *reg_info,
-                       const RegisterValue &reg_value) override;
-
   Status ReadAllRegisterValues(lldb::DataBufferSP &data_sp) override;
 
   Status WriteAllRegisterValues(const lldb::DataBufferSP &data_sp) override;
 
   Status IsWatchpointHit(uint32_t wp_index, bool &is_hit) override;
 
   Status GetWatchpointHitIndex(uint32_t &wp_index,
                                lldb::addr_t trap_addr) override;
public:
 
   lldb::addr_t GetWatchpointAddress(uint32_t wp_index) override;
 
   uint32_t NumSupportedHardwareWatchpoints() override;
 
   Status
   CopyHardwareWatchpointsFrom(NativeRegisterContextNetBSD &source) override;
 
+protected:
+  uint32_t GetRegisterSetForRegNum(uint32_t reg_index) const override;
+
+  Status ReadRegisterSet(uint32_t set) override;
+  Status WriteRegisterSet(uint32_t set) override;
+
+  Status GetRegisterFromCache(const RegisterInfo *reg_info,
+                              RegisterValue &reg_value,
+                              uint32_t set) override;
+  Status SetRegisterInCache(const RegisterInfo *reg_info,
+                            const RegisterValue &reg_value,
+                            uint32_t set) override;
+
 private:
   // Private member types.
   enum { GPRegSet, FPRegSet, XStateRegSet, DBRegSet };
 
   // Private member variables.
   struct reg m_gpr;
 #if defined(__x86_64__)
   struct fpreg m_fpr;
 #else
   struct xmmregs m_fpr;
 #endif
   struct dbreg m_dbr;
 #ifdef HAVE_XSTATE
   struct xstate m_xstate;
 #endif
 
-  int GetSetForNativeRegNum(int reg_num) const;
   int GetDR(int num) const;
-
-  Status ReadRegisterSet(uint32_t set);
-  Status WriteRegisterSet(uint32_t set);
 };
 
 } // namespace process_netbsd
 } // namespace lldb_private
 
 #endif // #ifndef lldb_NativeRegisterContextNetBSD_x86_64_h
 
 #endif // defined(__x86_64__)

lldb/source/Plugins/Process/NetBSD/NativeRegisterContextNetBSD_x86_64.cpp

: NativeRegisterContextNetBSD(native_thread,
                                   CreateRegisterInfoInterface(target_arch)),
       m_gpr(), m_fpr(), m_dbr() {}
 
 // CONSIDER after local and llgs debugging are merged, register set support can
 // be moved into a base x86-64 class with IsRegisterSetAvailable made virtual.
 uint32_t NativeRegisterContextNetBSD_x86_64::GetRegisterSetCount() const {
   uint32_t sets = 0;
   for (uint32_t set_index = 0; set_index < k_num_register_sets; ++set_index) {
-    if (GetSetForNativeRegNum(set_index) != -1)
+    if (GetRegisterSetForRegNum(set_index) != LLDB_INVALID_REGNUM)
       ++sets;
   }
 
   return sets;
 }
 
 const RegisterSet *
 NativeRegisterContextNetBSD_x86_64::GetRegisterSet(uint32_t set_index) const {
static constexpr int RegNumX86ToX86_64(int regnum) {
   case lldb_dr6_i386:
   case lldb_dr7_i386:
     return lldb_dr0_x86_64 + regnum - lldb_dr0_i386;
   default:
     llvm_unreachable("Unhandled i386 register.");
   }
 }
 
-int NativeRegisterContextNetBSD_x86_64::GetSetForNativeRegNum(
-    int reg_num) const {
+uint32_t NativeRegisterContextNetBSD_x86_64::GetRegisterSetForRegNum(
+    uint32_t reg_num) const {
   switch (GetRegisterInfoInterface().GetTargetArchitecture().GetMachine()) {
   case llvm::Triple::x86:
     if (reg_num >= k_first_gpr_i386 && reg_num <= k_last_gpr_i386)
       return GPRegSet;
     if (reg_num >= k_first_fpr_i386 && reg_num <= k_last_fpr_i386)
       return FPRegSet;
     if (reg_num >= k_first_avx_i386 && reg_num <= k_last_avx_i386)
       return XStateRegSet; // AVX
     if (reg_num >= k_first_mpxr_i386 && reg_num <= k_last_mpxr_i386)
-      return -1; // MPXR
+      return LLDB_INVALID_REGSET; // MPXR
     if (reg_num >= k_first_mpxc_i386 && reg_num <= k_last_mpxc_i386)
-      return -1; // MPXC
+      return LLDB_INVALID_REGSET; // MPXC
     if (reg_num >= k_first_dbr_i386 && reg_num <= k_last_dbr_i386)
       return DBRegSet; // DBR
     break;
   case llvm::Triple::x86_64:
     if (reg_num >= k_first_gpr_x86_64 && reg_num <= k_last_gpr_x86_64)
       return GPRegSet;
     if (reg_num >= k_first_fpr_x86_64 && reg_num <= k_last_fpr_x86_64)
       return FPRegSet;
     if (reg_num >= k_first_avx_x86_64 && reg_num <= k_last_avx_x86_64)
       return XStateRegSet; // AVX
     if (reg_num >= k_first_mpxr_x86_64 && reg_num <= k_last_mpxr_x86_64)
-      return -1; // MPXR
+      return LLDB_INVALID_REGSET; // MPXR
     if (reg_num >= k_first_mpxc_x86_64 && reg_num <= k_last_mpxc_x86_64)
-      return -1; // MPXC
+      return LLDB_INVALID_REGSET; // MPXC
     if (reg_num >= k_first_dbr_x86_64 && reg_num <= k_last_dbr_x86_64)
       return DBRegSet; // DBR
     break;
   default:
     llvm_unreachable("Unhandled target architecture.");
   }
 
   llvm_unreachable("Register does not belong to any register set");

 #else
     return Status("XState is not supported by the kernel");
 #endif
   }
   llvm_unreachable("NativeRegisterContextNetBSD_x86_64::WriteRegisterSet");
 }
 
 Status
-NativeRegisterContextNetBSD_x86_64::ReadRegister(const RegisterInfo *reg_info,
-                                                 RegisterValue &reg_value) {
-  Status error;
+NativeRegisterContextNetBSD_x86_64::GetRegisterFromCache(
+    const RegisterInfo *reg_info, RegisterValue &reg_value, uint32_t set) {
 
-  if (!reg_info) {
-    error.SetErrorString("reg_info NULL");
-    return error;
-  }
+  assert(reg_info);
+  assert(set != LLDB_INVALID_REGSET);
 
+  Status error;
   uint32_t reg = reg_info->kinds[lldb::eRegisterKindLLDB];
-  if (reg == LLDB_INVALID_REGNUM) {
-    // This is likely an internal register for lldb use only and should not be
-    // directly queried.
-    error.SetErrorStringWithFormat("register \"%s\" is an internal-only lldb "
-                                   "register, cannot read directly",
-                                   reg_info->name);
-    return error;
-  }
-
-  int set = GetSetForNativeRegNum(reg);
-  if (set == -1) {
-    // This is likely an internal register for lldb use only and should not be
-    // directly queried.
-    error.SetErrorStringWithFormat("register \"%s\" is in unrecognized set",
-                                   reg_info->name);
-    return error;
-  }
 
   switch (GetRegisterInfoInterface().GetTargetArchitecture().GetMachine()) {
   case llvm::Triple::x86_64:
     break;
   case llvm::Triple::x86:
     reg = RegNumX86ToX86_64(reg);
     break;
   default:
     llvm_unreachable("Unhandled target architecture.");
   }
 
-  error = ReadRegisterSet(set);
-  if (error.Fail())
-    return error;
-
   switch (reg) {
 #if defined(__x86_64__)
   case lldb_rax_x86_64:
     reg_value = (uint64_t)m_gpr.regs[_REG_RAX];
     break;
   case lldb_rbx_x86_64:
     reg_value = (uint64_t)m_gpr.regs[_REG_RBX];
     break;
case lldb_dr7_x86_64:
     break;
   default:
     llvm_unreachable("Reading unknown/unsupported register");
   }
 
   return error;
 }
 
-Status NativeRegisterContextNetBSD_x86_64::WriteRegister(
-    const RegisterInfo *reg_info, const RegisterValue &reg_value) {
-
-  Status error;
+Status NativeRegisterContextNetBSD_x86_64::SetRegisterInCache(
+    const RegisterInfo *reg_info,
+    const RegisterValue &reg_value,
+    uint32_t set) {
 
-  if (!reg_info) {
-    error.SetErrorString("reg_info NULL");
-    return error;
-  }
+  assert(reg_info);
+  assert(set != LLDB_INVALID_REGSET);
 
+  Status error;
   uint32_t reg = reg_info->kinds[lldb::eRegisterKindLLDB];
-  if (reg == LLDB_INVALID_REGNUM) {
-    // This is likely an internal register for lldb use only and should not be
-    // directly queried.
-    error.SetErrorStringWithFormat("register \"%s\" is an internal-only lldb "
-                                   "register, cannot read directly",
-                                   reg_info->name);
-    return error;
-  }
-
-  int set = GetSetForNativeRegNum(reg);
-  if (set == -1) {
-    // This is likely an internal register for lldb use only and should not be
-    // directly queried.
-    error.SetErrorStringWithFormat("register \"%s\" is in unrecognized set",
-                                   reg_info->name);
-    return error;
-  }
 
   switch (GetRegisterInfoInterface().GetTargetArchitecture().GetMachine()) {
   case llvm::Triple::x86_64:
     break;
   case llvm::Triple::x86:
     reg = RegNumX86ToX86_64(reg);
     break;
   default:
     llvm_unreachable("Unhandled target architecture.");
   }
 
-  error = ReadRegisterSet(set);
-  if (error.Fail())
-    return error;
-
   switch (reg) {
 #if defined(__x86_64__)
   case lldb_rax_x86_64:
     m_gpr.regs[_REG_RAX] = reg_value.GetAsUInt64();
     break;
   case lldb_rbx_x86_64:
     m_gpr.regs[_REG_RBX] = reg_value.GetAsUInt64();
     break;
#endif
   case lldb_dr6_x86_64:
   case lldb_dr7_x86_64:
     m_dbr.dr[reg - lldb_dr0_x86_64] = reg_value.GetAsUInt64();
     break;
   default:
     llvm_unreachable("Reading unknown/unsupported register");
   }
 
-  return WriteRegisterSet(set);
+  return Status();
 }
 
 Status NativeRegisterContextNetBSD_x86_64::ReadAllRegisterValues(
     lldb::DataBufferSP &data_sp) {
   Status error;
 
   data_sp.reset(new DataBufferHeap(REG_CONTEXT_SIZE, 0));
   error = ReadRegisterSet(GPRegSet);

lldb/source/Plugins/Process/Utility/NativeRegisterContextRegisterInfo.h

 //===-- NativeRegisterContextRegisterInfo.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_SOURCE_PLUGINS_PROCESS_UTILITY_NATIVEREGISTERCONTEXTREGISTERINFO_H
 #define LLDB_SOURCE_PLUGINS_PROCESS_UTILITY_NATIVEREGISTERCONTEXTREGISTERINFO_H
 
+#include <cstdint>
+#include <map>
 #include <memory>
 
 #include "RegisterInfoInterface.h"
 #include "lldb/Host/common/NativeRegisterContext.h"
 
+enum class RegSetCacheStatus {
+  Empty,
+  Read,
+  Written,
+};
+
+#define LLDB_INVALID_REGSET UINT32_MAX
+
 namespace lldb_private {
 class NativeRegisterContextRegisterInfo : public NativeRegisterContext {
 public:
   ///
   /// Construct a NativeRegisterContextRegisterInfo, taking ownership
   /// of the register_info_interface pointer.
   ///
   NativeRegisterContextRegisterInfo(
       NativeThreadProtocol &thread,
       RegisterInfoInterface *register_info_interface);
 
   uint32_t GetRegisterCount() const override;
 
   uint32_t GetUserRegisterCount() const override;
 
   const RegisterInfo *GetRegisterInfoAtIndex(uint32_t reg_index) const override;
 
   const RegisterInfoInterface &GetRegisterInfoInterface() const;
 
+  Status ReadRegister(const RegisterInfo *reg_info,
+                      RegisterValue &reg_value) override;
+
+  Status WriteRegister(const RegisterInfo *reg_info,
+                       const RegisterValue &reg_value) override;
+
+  Status FlushRegisterSets();
+
 protected:
   std::unique_ptr<RegisterInfoInterface> m_register_info_interface_up;
-};
+
+  // APIs used to implement cached register access, they need to be defined
+  // only if ReadRegister() and WriteRegister() are not overriden
+
+  virtual uint32_t GetRegisterSetForRegNum(uint32_t reg_index) const {
+    return LLDB_INVALID_REGSET;
+  }
+
+  virtual Status ReadRegisterSet(uint32_t set) {
+    return Status("not implemented");
   }
+
+  virtual Status WriteRegisterSet(uint32_t set) {
+    return Status("not implemented");
+  }
+
+  virtual Status GetRegisterFromCache(const RegisterInfo *reg_info,
+                                      RegisterValue &reg_value, uint32_t set) {
+    return Status("not implemented");
+  }
+
+  virtual Status SetRegisterInCache(const RegisterInfo *reg_info,
+                                    const RegisterValue &reg_value,
+                                    uint32_t set) {
+    return Status("not implemented");
+  }
+
+  virtual Status FlushRegisterSet(uint32_t set);
+
+  // ---------------------------------------------------------------------------
+
+private:
+  std::map<int, RegSetCacheStatus> m_regset_cache_status;
+};
+
+} // namespace lldb_private
 #endif

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

const RegisterInfo *NativeRegisterContextRegisterInfo::GetRegisterInfoAtIndex(
   else
     return nullptr;
 }
 
 const RegisterInfoInterface &
 NativeRegisterContextRegisterInfo::GetRegisterInfoInterface() const {
   return *m_register_info_interface_up;
 }
+
+Status
+NativeRegisterContextRegisterInfo::ReadRegister(const RegisterInfo *reg_info,
+                                                RegisterValue &reg_value) {
+  Status error;
+
+  if (!reg_info) {
+    error.SetErrorString("reg_info NULL");
+    return error;
+  }
+
+  uint32_t reg = reg_info->kinds[lldb::eRegisterKindLLDB];
+  if (reg == LLDB_INVALID_REGNUM) {
+    // This is likely an internal register for lldb use only and should not be
+    // directly queried.
+    error.SetErrorStringWithFormat("register \"%s\" is an internal-only lldb "
+                                   "register, cannot read directly",
+                                   reg_info->name);
+    return error;
+  }
+
+  uint32_t set = GetRegisterSetForRegNum(reg);
+  if (set == LLDB_INVALID_REGSET) {
+    // This is likely an internal register for lldb use only and should not be
+    // directly queried.
+    error.SetErrorStringWithFormat("register \"%s\" is in unrecognized set",
+                                   reg_info->name);
+    return error;
+  }
+
+  error = ReadRegisterSet(set);
+  if (error.Fail())
+    return error;
+
+  return GetRegisterFromCache(reg_info, reg_value, set);
+}
+
+Status NativeRegisterContextRegisterInfo::WriteRegister(
+    const RegisterInfo *reg_info, const RegisterValue &reg_value) {
+
+  Status error;
+
+  if (!reg_info) {
+    error.SetErrorString("reg_info NULL");
+    return error;
+  }
+
+  uint32_t reg = reg_info->kinds[lldb::eRegisterKindLLDB];
+  if (reg == LLDB_INVALID_REGNUM) {
+    // This is likely an internal register for lldb use only and should not be
+    // directly queried.
+    error.SetErrorStringWithFormat("register \"%s\" is an internal-only lldb "
+                                   "register, cannot read directly",
+                                   reg_info->name);
+    return error;
+  }
+
+  uint32_t set = GetRegisterSetForRegNum(reg);
+  if (set == LLDB_INVALID_REGSET) {
+    // This is likely an internal register for lldb use only and should not be
+    // directly queried.
+    error.SetErrorStringWithFormat("register \"%s\" is in unrecognized set",
+                                   reg_info->name);
+    return error;
+  }
+
+  error = ReadRegisterSet(set);
+  if (error.Fail())
+    return error;
+
+  error = SetRegisterInCache(reg_info, reg_value, set);
+  if (error.Fail())
+    return error;
+
+  return WriteRegisterSet(set);
+}
+
+Status NativeRegisterContextRegisterInfo::FlushRegisterSets() {
+  Status error;
+
+  for (auto kv : m_regset_cache_status) {
+    if (kv.second == RegSetCacheStatus::Empty)
+      continue;
+    error = FlushRegisterSet(kv.first);
+    if (error.Fail())
+      break;
+  }
+
+  return error;
+}
+
+Status NativeRegisterContextRegisterInfo::FlushRegisterSet(uint32_t set) {
+  m_regset_cache_status[set] = RegSetCacheStatus::Empty;
+  return Status();
+}