[Lldb-commits] [PATCH] D80105: [LLDB] Combine multiple defs of arm64 register sets

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I like this.



================
Comment at: lldb/source/Plugins/Process/FreeBSD/FreeBSDThread.cpp:167
     assert(target_arch.GetTriple().getOS() == llvm::Triple::FreeBSD);
     switch (target_arch.GetMachine()) {
     case llvm::Triple::aarch64:
----------------
It would be good to merge these two switches. Then the reg(set)_interface variables would be local to each case label and it would not be so weird that we sometimes use one and sometimes the other.


================
Comment at: lldb/source/Plugins/Process/FreeBSD/RegisterContextPOSIXProcessMonitor_arm64.h:21
       lldb_private::Thread &thread, uint32_t concrete_frame_idx,
-      lldb_private::RegisterInfoInterface *register_info);
+      lldb_private::RegisterInfoAndSetInterface *register_info);
 
----------------
While we're changing interfaces, let's also make this unique_ptr<RegisterInfoAndSetInterface> to document the ownership transfer. (I might also drop the concrete_frame_idx argument, as that is always zero.)


================
Comment at: lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp:71-72
+
+  m_regset_interface_up = std::static_pointer_cast<RegisterInfoAndSetInterface>(
+      m_register_info_interface_up);
 }
----------------
The way I'd recommend dealing with this is to have a `RegisterInfoPOSIX_arm64& GetRegisterInfo() const` method which performs a cast on the `m_register_info_interface_up` pointer, and then have everything call that. If you place that method in close proximity to this constructor, then it should be clear that this operation is always safe. There's already something similar being done in e.g. `NativeThreadLinux::GetProcess`.


================
Comment at: lldb/source/Plugins/Process/Utility/NativeRegisterContextRegisterInfo.h:36
 
-private:
-  std::unique_ptr<RegisterInfoInterface> m_register_info_interface_up;
+  std::shared_ptr<RegisterInfoInterface> m_register_info_interface_up;
 };
----------------
With the above idea, it should no longer be needed to change this into a shared_ptr.


================
Comment at: lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_arm64.h:23-24
   RegisterContextPOSIX_arm64(
       lldb_private::Thread &thread, uint32_t concrete_frame_idx,
-      lldb_private::RegisterInfoInterface *register_info);
+      lldb_private::RegisterInfoAndSetInterface *register_info);
 
----------------
similar thoughts about unique_ptr and concrete_frame_idx. In fact, thinking ahead to the next patch, I think we could make this an `unique_ptr<RegisterInfoPOSIX_arm64>` even.


================
Comment at: lldb/source/Plugins/Process/Utility/RegisterInfoAndSetInterface.h:24
+      : RegisterInfoInterface(target_arch) {}
+  virtual ~RegisterInfoAndSetInterface() {}
+
----------------
s/virtual/override (or just omit the destructor completely)


================
Comment at: lldb/source/Plugins/Process/Utility/RegisterInfoAndSetInterface.h:35-37
+  virtual uint32_t GetRegisterOffset(uint32_t reg_index) const = 0;
+
+  virtual uint32_t GetRegisterSize(uint32_t reg_index) const = 0;
----------------
These aren't really register *set* related, are they? Could they go on the base interface instead?

Although, THB, the way I'd handle this is by having something a `ArrayRef<RegisterInfo> RegInfos()` interface, and then having the callers do `reg_info_interface->RegInfos()[reg_index].offset`


================
Comment at: lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.h:16
 
-class RegisterInfoPOSIX_arm64 : public lldb_private::RegisterInfoInterface {
+enum { RegisterSetPOSIX_ARM64GPR = 0, RegisterSetPOSIX_ARM64FPR };
+
----------------
How about we put this inside the `RegisterInfoPOSIX_arm64` class, and drop `RegisterSetPOSIX_ARM64` from the name. (`RegisterInfoPOSIX_arm64::GPRegSet` ?) (there's already something similar inside `NativeRegisterContextNetBSD_x86_64`).


================
Comment at: lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.h:21-29
+  struct RegInfo {
+    uint32_t num_registers;
+    uint32_t num_gpr_registers;
+    uint32_t num_fpr_registers;
+
+    uint32_t last_gpr;
+    uint32_t first_fpr;
----------------
I'm not sure what's the advantage of making this a struct vs. just splating it into the containing class. Maybe that made more sense when originally when the class was doing a lot more work, but here I'd just replace this by a list of members.


================
Comment at: lldb/source/Plugins/Process/elf-core/ThreadElfCore.cpp:82
 
     switch (arch.GetTriple().getOS()) {
     case llvm::Triple::FreeBSD: {
----------------
The reg(set)_interface and register_context switches could be merged here too in a similar way...


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80105/new/

https://reviews.llvm.org/D80105