[lldb] [Process/NetBSD] Remove unnecessary register buffer abstraction

[mgorny]

Summary

Remove most of the abstraction over ptrace() register operations,
as it has little value and introduces more code than it saves.
Instead, leave a single ptrace() wrapper method and call it directly
from ReadRegisterSet() and WriteRegisterSet() with correct PT_* request
and buffer.

Remove the remaining direct ReadGPR() and WriteGPR() invocations
with ReadRegisterSet() and WriteRegisterSet().

Cleanup suggested by Pavel Labath in D63545.

[labath]

How about we go even a bit further? The non-Do functions are now so trivial that the Do functions could be inlined into them, producing something like return NativeProcessNetBSD::PtraceWrapper(PT_GETFPREGS, GetProcessPid(), GetFPRBuffer(),m_thread.GetID());. If you wanted to be really fancy, you could define a function like ReadRegisterSet(T regset, void *buffer), and call make that be ReadRegisterSet(PT_GETFPREGS, GetFPRBuffer());

[labath]

BTW, is ReadGPR even called from some other place than NativeRegisterContextNetBSD_x86_64::ReadRegisterSet ? If not, then we could remove all of these functions (except the ReadRegisterSet I suggest above), and inline everything into NativeRegisterContextNetBSD_x86_64::ReadRegisterSet (one of these should be renamed, obviously), removing about 5 layers of indirection...

[mgorny]

In D63594#1551702, @labath wrote:

BTW, is ReadGPR even called from some other place than NativeRegisterContextNetBSD_x86_64::ReadRegisterSet ? If not, then we could remove all of these functions (except the ReadRegisterSet I suggest above), and inline everything into NativeRegisterContextNetBSD_x86_64::ReadRegisterSet (one of these should be renamed, obviously), removing about 5 layers of indirection...

I think the idea is that they will be reused when we introduce additional architectures.

[labath]

In D63594#1551715, @mgorny wrote:

In D63594#1551702, @labath wrote:

BTW, is ReadGPR even called from some other place than NativeRegisterContextNetBSD_x86_64::ReadRegisterSet ? If not, then we could remove all of these functions (except the ReadRegisterSet I suggest above), and inline everything into NativeRegisterContextNetBSD_x86_64::ReadRegisterSet (one of these should be renamed, obviously), removing about 5 layers of indirection...

I think the idea is that they will be reused when we introduce additional architectures.

How exactly would that work? If the other architectures follow the pattern of x86_64, then they will also have a ReadRegisterSet function, which can be used for reading everything. And doing things this way doesn't force every architecture to fit into predefined set of register sets.

This is your code, so that's ultimately up to you, but I'd try to aim for simplicity first, and if it later turns out to be useful to reintroduce these functions, then so be it...

[mgorny]

I'll try to simplify it in a few steps and see how that goes. All I'm saying is that ultimately having some DoRegisterSet() wrapper that calls ptrace() and avoids repeating getting PID/TID may help. Though admittely it saves very little actual code.

[mgorny]

How about this? I've removed almost all abstraction, leaving only simple ptrace() wrapper.

[labath]

Yes, that's exactly what I had in mind. Let's ship it.

lldb/trunk/source/Plugins/Process/NetBSD/NativeRegisterContextNetBSD.h

public:
   // NativeRegisterContextNetBSD. The implementations can't collide as only one
   // NativeRegisterContextNetBSD_* variant should be compiled into the final
   // executable.
   static NativeRegisterContextNetBSD *
   CreateHostNativeRegisterContextNetBSD(const ArchSpec &target_arch,
                                         NativeThreadProtocol &native_thread);
 
 protected:
-  virtual Status ReadGPR();
-  virtual Status WriteGPR();
-
-  virtual Status ReadFPR();
-  virtual Status WriteFPR();
-
-  virtual Status ReadDBR();
-  virtual Status WriteDBR();
-
-  virtual void *GetGPRBuffer() { return nullptr; }
-  virtual size_t GetGPRSize() {
-    return GetRegisterInfoInterface().GetGPRSize();
-  }
-
-  virtual void *GetFPRBuffer() { return nullptr; }
-  virtual size_t GetFPRSize() { return 0; }
-
-  virtual void *GetDBRBuffer() { return nullptr; }
-  virtual size_t GetDBRSize() { return 0; }
-
-  virtual Status DoReadGPR(void *buf);
-  virtual Status DoWriteGPR(void *buf);
-
-  virtual Status DoReadFPR(void *buf);
-  virtual Status DoWriteFPR(void *buf);
-
-  virtual Status DoReadDBR(void *buf);
-  virtual Status DoWriteDBR(void *buf);
-
+  Status DoRegisterSet(int req, void *buf);
   virtual NativeProcessNetBSD &GetProcess();
   virtual ::pid_t GetProcessPid();
 };
 
 } // namespace process_netbsd
 } // namespace lldb_private
 
 #endif // #ifndef lldb_NativeRegisterContextNetBSD_h

lldb/trunk/source/Plugins/Process/NetBSD/NativeRegisterContextNetBSD.cpp

 // clang-format on
 
 NativeRegisterContextNetBSD::NativeRegisterContextNetBSD(
     NativeThreadProtocol &native_thread,
     RegisterInfoInterface *reg_info_interface_p)
     : NativeRegisterContextRegisterInfo(native_thread,
                                         reg_info_interface_p) {}
 
-Status NativeRegisterContextNetBSD::ReadGPR() {
-  void *buf = GetGPRBuffer();
-  if (!buf)
-    return Status("GPR buffer is NULL");
-
-  return DoReadGPR(buf);
-}
-
-Status NativeRegisterContextNetBSD::WriteGPR() {
-  void *buf = GetGPRBuffer();
-  if (!buf)
-    return Status("GPR buffer is NULL");
-
-  return DoWriteGPR(buf);
-}
-
-Status NativeRegisterContextNetBSD::ReadFPR() {
-  void *buf = GetFPRBuffer();
-  if (!buf)
-    return Status("FPR buffer is NULL");
-
-  return DoReadFPR(buf);
-}
-
-Status NativeRegisterContextNetBSD::WriteFPR() {
-  void *buf = GetFPRBuffer();
-  if (!buf)
-    return Status("FPR buffer is NULL");
-
-  return DoWriteFPR(buf);
-}
-
-Status NativeRegisterContextNetBSD::ReadDBR() {
-  void *buf = GetDBRBuffer();
-  if (!buf)
-    return Status("DBR buffer is NULL");
-
-  return DoReadDBR(buf);
-}
-
-Status NativeRegisterContextNetBSD::WriteDBR() {
-  void *buf = GetDBRBuffer();
-  if (!buf)
-    return Status("DBR buffer is NULL");
-
-  return DoWriteDBR(buf);
-}
-
-Status NativeRegisterContextNetBSD::DoReadGPR(void *buf) {
-  return NativeProcessNetBSD::PtraceWrapper(PT_GETREGS, GetProcessPid(), buf,
-                                            m_thread.GetID());
-}
-
-Status NativeRegisterContextNetBSD::DoWriteGPR(void *buf) {
-  return NativeProcessNetBSD::PtraceWrapper(PT_SETREGS, GetProcessPid(), buf,
-                                            m_thread.GetID());
-}
-
-Status NativeRegisterContextNetBSD::DoReadFPR(void *buf) {
-  return NativeProcessNetBSD::PtraceWrapper(PT_GETFPREGS, GetProcessPid(), buf,
-                                            m_thread.GetID());
-}
-
-Status NativeRegisterContextNetBSD::DoWriteFPR(void *buf) {
-  return NativeProcessNetBSD::PtraceWrapper(PT_SETFPREGS, GetProcessPid(), buf,
-                                            m_thread.GetID());
-}
-
-Status NativeRegisterContextNetBSD::DoReadDBR(void *buf) {
-  return NativeProcessNetBSD::PtraceWrapper(PT_GETDBREGS, GetProcessPid(), buf,
-                                            m_thread.GetID());
-}
-
-Status NativeRegisterContextNetBSD::DoWriteDBR(void *buf) {
-  return NativeProcessNetBSD::PtraceWrapper(PT_SETDBREGS, GetProcessPid(), buf,
+Status NativeRegisterContextNetBSD::DoRegisterSet(int ptrace_req, void *buf) {
+  return NativeProcessNetBSD::PtraceWrapper(ptrace_req, GetProcessPid(), buf,
                                             m_thread.GetID());
 }
 
 NativeProcessNetBSD &NativeRegisterContextNetBSD::GetProcess() {
   return static_cast<NativeProcessNetBSD &>(m_thread.GetProcess());
 }
 
 ::pid_t NativeRegisterContextNetBSD::GetProcessPid() {
   return GetProcess().GetID();
 }

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

public:
 
   uint32_t SetHardwareWatchpoint(lldb::addr_t addr, size_t size,
                                  uint32_t watch_flags) override;
 
   lldb::addr_t GetWatchpointAddress(uint32_t wp_index) override;
 
   uint32_t NumSupportedHardwareWatchpoints() override;
 
-protected:
-  void *GetGPRBuffer() override { return &m_gpr_x86_64; }
-  void *GetFPRBuffer() override { return &m_fpr_x86_64; }
-  void *GetDBRBuffer() override { return &m_dbr_x86_64; }
-
 private:
   // Private member types.
   enum { GPRegSet, FPRegSet, DBRegSet };
 
   // Private member variables.
   struct reg m_gpr_x86_64;
   struct fpreg m_fpr_x86_64;
   struct dbreg m_dbr_x86_64;

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

 #include "lldb/Utility/Log.h"
 #include "lldb/Utility/RegisterValue.h"
 #include "lldb/Utility/Status.h"
 
 #include "Plugins/Process/Utility/RegisterContextNetBSD_x86_64.h"
 
 // clang-format off
 #include <sys/types.h>
+#include <sys/ptrace.h>
 #include <sys/sysctl.h>
 #include <x86/cpu.h>
 #include <elf.h>
 #include <err.h>
 #include <stdint.h>
 #include <stdlib.h>
 // clang-format on
 
else if (reg_num <= lldb_dr7_x86_64)
     return DBRegSet; // DBR
   else
     return -1;
 }
 
 Status NativeRegisterContextNetBSD_x86_64::ReadRegisterSet(uint32_t set) {
   switch (set) {
   case GPRegSet:
-    return ReadGPR();
+    return DoRegisterSet(PT_GETREGS, &m_gpr_x86_64);
   case FPRegSet:
-    return ReadFPR();
+    return DoRegisterSet(PT_GETFPREGS, &m_fpr_x86_64);
   case DBRegSet:
-    return ReadDBR();
+    return DoRegisterSet(PT_GETDBREGS, &m_dbr_x86_64);
   }
   llvm_unreachable("NativeRegisterContextNetBSD_x86_64::ReadRegisterSet");
 }
 
 Status NativeRegisterContextNetBSD_x86_64::WriteRegisterSet(uint32_t set) {
   switch (set) {
   case GPRegSet:
-    return WriteGPR();
+    return DoRegisterSet(PT_SETREGS, &m_gpr_x86_64);
   case FPRegSet:
-    return WriteFPR();
+    return DoRegisterSet(PT_SETFPREGS, &m_fpr_x86_64);
   case DBRegSet:
-    return WriteDBR();
+    return DoRegisterSet(PT_SETDBREGS, &m_dbr_x86_64);
   }
   llvm_unreachable("NativeRegisterContextNetBSD_x86_64::WriteRegisterSet");
 }
 
 Status
 NativeRegisterContextNetBSD_x86_64::ReadRegister(const RegisterInfo *reg_info,
                                                  RegisterValue &reg_value) {
   Status error;
Status NativeRegisterContextNetBSD_x86_64::ReadAllRegisterValues(
   data_sp.reset(new DataBufferHeap(REG_CONTEXT_SIZE, 0));
   if (!data_sp) {
     error.SetErrorStringWithFormat(
         "failed to allocate DataBufferHeap instance of size %" PRIu64,
         REG_CONTEXT_SIZE);
     return error;
   }
 
-  error = ReadGPR();
+  error = ReadRegisterSet(GPRegSet);
   if (error.Fail())
     return error;
 
   uint8_t *dst = data_sp->GetBytes();
   if (dst == nullptr) {
     error.SetErrorStringWithFormat("DataBufferHeap instance of size %" PRIu64
                                    " returned a null pointer",
                                    REG_CONTEXT_SIZE);
if (src == nullptr) {
     error.SetErrorStringWithFormat("NativeRegisterContextNetBSD_x86_64::%s "
                                    "DataBuffer::GetBytes() returned a null "
                                    "pointer",
                                    __FUNCTION__);
     return error;
   }
   ::memcpy(&m_gpr_x86_64, src, GetRegisterInfoInterface().GetGPRSize());
 
-  error = WriteGPR();
+  error = WriteRegisterSet(GPRegSet);
   if (error.Fail())
     return error;
   src += GetRegisterInfoInterface().GetGPRSize();
 
   return error;
 }
 
 Status NativeRegisterContextNetBSD_x86_64::IsWatchpointHit(uint32_t wp_index,