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

[Muhammad Omair Javaid via Phabricator via lldb-commits]

omjavaid marked 10 inline comments as done.
omjavaid added inline comments.


================
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:
----------------
labath wrote:
> 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.
I have tried a couple of options but the no of different combinations involved I feel current implementation should stay untill we incrementally move remaining architectures to user RegisterInfosAndSet interface.


================
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);
 
----------------
labath wrote:
> 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.)
Agreed. I will update this in updated revision.


================
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);
 }
----------------
labath wrote:
> 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`.
yes this would be a lot cleaner than what I am doing currently. I am going to update this in next revision.


================
Comment at: lldb/source/Plugins/Process/elf-core/ThreadElfCore.cpp:82
 
     switch (arch.GetTriple().getOS()) {
     case llvm::Triple::FreeBSD: {
----------------
labath wrote:
> The reg(set)_interface and register_context switches could be merged here too in a similar way...
Here also incremental merger will be a better approach IMO.


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

https://reviews.llvm.org/D80105