[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/source/Plugins/Process/NetBSD/NativeRegisterContextNetBSD.h

protected:
   virtual Status WriteGPR();
 
   virtual Status ReadFPR();
   virtual Status WriteFPR();
 
   virtual Status ReadDBR();
   virtual Status WriteDBR();
 
-  virtual void *GetGPRBuffer() { return nullptr; }
+  virtual void *GetGPRBuffer() = 0;
   virtual size_t GetGPRSize() {
     return GetRegisterInfoInterface().GetGPRSize();
   }
 
-  virtual void *GetFPRBuffer() { return nullptr; }
-  virtual size_t GetFPRSize() { return 0; }
+  virtual void *GetFPRBuffer() = 0;
 
-  virtual void *GetDBRBuffer() { return nullptr; }
-  virtual size_t GetDBRSize() { return 0; }
+  virtual void *GetDBRBuffer() = 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);

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

 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());
 }
 

lldb/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; }
+  void *GetGPRBuffer() { return &m_gpr_x86_64; }
+  void *GetFPRBuffer() { return &m_fpr_x86_64; }
+  void *GetDBRBuffer() { 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;