Don't keep a global ABI plugin per architecture

[jingham]

Summary

For reasons that are unclear to me, when the ABIXXX::CreateInstance function is called to make a new ABI for a given process and ArchSpec, we would only make the plugin once, with the initial process and architecture. Then if we got called with a different process, we would check to see if we already made one and if we did, hand back the one we had made - even though that was for a different process.

If there were ever anything per-process that effected the ABI plugin's behavior (for instance if it relied on a Process property) you could very well use the wrong processes setting. Even worse, since the ABI's hold onto a process through a weak pointer, if the initial process had gone away, you would not be able to get to any process at all, and silently fall back on some default behavior.

This caching goes back to prehistoric days in lldb, but I can't think of any reason why we would do this. It seems clearly wrong, and ABI plugins are really cheap to make - they pretty much just copy the process SP to a weak pointer and that's about all. So this also seems like an unnecessary optimization.

Greg or Jason, do you remember why we did this?

[jasonmolenda]

LGTM, my guess is that this was Greg's code originally. He may have made a singleton object out of caution.

[zturner]

If there were ever anything per-process that effected the ABI plugin's behavior (for instance if it relied on a Process property) you could very well use the wrong processes setting. Even worse, since the ABI's hold onto a process through a weak pointer, if the initial process had gone away, you would not be able to get to any process at all, and silently fall back on some default behavior.

This sounds like something testable. Start a process, force an operation that goes through the ABI, kill the process, start a new one, do the same thing again. Presumably this crashes in current LLDB? Maybe this makes for a good test case?

BTW, as a general rule of thumb, std::make_shared<T>(constructor_args) is preferred over shared_ptr<T>(new T(constructor_args)); unless it's impossible (the only time being where a constructor is private or protected). The biggest reason is that it's every so slightly more efficient. This usually doesn't matter, but it's one of those "well, why not?" kinda things. Feel free to use the current syntax though, just pointing it out in case it strikes your fancy.

[clayborg]

Anything that takes a process shared pointer should be per process. A long time ago these plug-ins didn't take process, and as time went on they did take a process:

306633   jmolenda   ABI(lldb::ProcessSP process_sp) {
306633   jmolenda     if (process_sp.get())
306633   jmolenda         m_process_wp = process_sp;
306633   jmolenda   }
245277     labath 
306633   jmolenda   lldb::ProcessWP m_process_wp;
306633   jmolenda

The ABI plug-in started off just figuring out where return values were and and where argument values went.. If there was no process, then each ABI plug-in really is per architecture and they could be shared. Not sure if there is anything cached in any ABI plug-ins that is process specific. The change log says:

Change the ABI class to have a weak pointer to its Process;
some methods in the ABI need a Process to do their work.
Instead of passing it in as a one-off argument to those
methods, this patch puts it in the base class and the methods
can retrieve if it needed.

Note that ABI's are sometimes built without a Process 
(e.g. SBTarget::GetStackRedZoneSize) so it's entirely
possible that the process weak pointer will not be
able to reconsistitue into a strong pointer.

<rdar://problem/32526754>

So to avoid having to pass the process to ABI functions that require it, it sounds like Jason just put it into the class. We can either take the process out of the base class and pass it in where it is needed to each method and keep one copy, or we can make new ones for each process. They aren't huge.

[jingham]

Okay, I think I'll keep what Jason did and just make them per-process. As Greg said, they are very cheap to make, since they just take a Process SP and get the weak pointer from it. None of them have any other state.

[jingham]

As for testing, Jason was careful to use a WP, so nothing crashes. The process's SP doesn't resolve and then you get the default setting of any process specific setting that uses the ABI. Since there aren't any of those yet - this currently has no observable consequences. But there will be soon, and we can use that to test that we are getting this process's settings easily once that gets checked in.

source/Plugins/ABI/MacOSX-arm/ABIMacOSX_arm.cpp

 size_t ABIMacOSX_arm::GetRedZoneSize() const { return 0; }
 
 //------------------------------------------------------------------
 // Static Functions
 //------------------------------------------------------------------
 
 ABISP
 ABIMacOSX_arm::CreateInstance(ProcessSP process_sp, const ArchSpec &arch) {
-  static ABISP g_abi_sp;
   const llvm::Triple::ArchType arch_type = arch.GetTriple().getArch();
   const llvm::Triple::VendorType vendor_type = arch.GetTriple().getVendor();
 
   if (vendor_type == llvm::Triple::Apple) {
     if ((arch_type == llvm::Triple::arm) ||
         (arch_type == llvm::Triple::thumb)) {
-      if (!g_abi_sp)
-        g_abi_sp.reset(new ABIMacOSX_arm(process_sp));
-      return g_abi_sp;
+      return ABISP(new ABIMacOSX_arm(process_sp));
     }
   }
 
   return ABISP();
 }
 
 bool ABIMacOSX_arm::PrepareTrivialCall(Thread &thread, addr_t sp,
                                        addr_t function_addr, addr_t return_addr,

source/Plugins/ABI/MacOSX-arm64/ABIMacOSX_arm64.cpp

 size_t ABIMacOSX_arm64::GetRedZoneSize() const { return 128; }
 
 //------------------------------------------------------------------
 // Static Functions
 //------------------------------------------------------------------
 
 ABISP
 ABIMacOSX_arm64::CreateInstance(ProcessSP process_sp, const ArchSpec &arch) {
-  static ABISP g_abi_sp;
   const llvm::Triple::ArchType arch_type = arch.GetTriple().getArch();
   const llvm::Triple::VendorType vendor_type = arch.GetTriple().getVendor();
 
   if (vendor_type == llvm::Triple::Apple) {
     if (arch_type == llvm::Triple::aarch64) {
-      if (!g_abi_sp)
-        g_abi_sp.reset(new ABIMacOSX_arm64(process_sp));
-      return g_abi_sp;
+      return ABISP(new ABIMacOSX_arm64(process_sp));
     }
   }
 
   return ABISP();
 }
 
 bool ABIMacOSX_arm64::PrepareTrivialCall(
     Thread &thread, lldb::addr_t sp, lldb::addr_t func_addr,

source/Plugins/ABI/MacOSX-i386/ABIMacOSX_i386.cpp

 size_t ABIMacOSX_i386::GetRedZoneSize() const { return 0; }
 
 //------------------------------------------------------------------
 // Static Functions
 //------------------------------------------------------------------
 
 ABISP
 ABIMacOSX_i386::CreateInstance(lldb::ProcessSP process_sp, const ArchSpec &arch) {
-  static ABISP g_abi_sp;
   if ((arch.GetTriple().getArch() == llvm::Triple::x86) &&
       (arch.GetTriple().isMacOSX() || arch.GetTriple().isiOS() ||
        arch.GetTriple().isWatchOS())) {
-    if (!g_abi_sp)
-      g_abi_sp.reset(new ABIMacOSX_i386(process_sp));
-    return g_abi_sp;
+    return ABISP(new ABIMacOSX_i386(process_sp));
   }
   return ABISP();
 }
 
 bool ABIMacOSX_i386::PrepareTrivialCall(Thread &thread, addr_t sp,
                                         addr_t func_addr, addr_t return_addr,
                                         llvm::ArrayRef<addr_t> args) const {
   RegisterContext *reg_ctx = thread.GetRegisterContext().get();

source/Plugins/ABI/SysV-arm/ABISysV_arm.cpp

 size_t ABISysV_arm::GetRedZoneSize() const { return 0; }
 
 //------------------------------------------------------------------
 // Static Functions
 //------------------------------------------------------------------
 
 ABISP
 ABISysV_arm::CreateInstance(lldb::ProcessSP process_sp, const ArchSpec &arch) {
-  static ABISP g_abi_sp;
   const llvm::Triple::ArchType arch_type = arch.GetTriple().getArch();
   const llvm::Triple::VendorType vendor_type = arch.GetTriple().getVendor();
 
   if (vendor_type != llvm::Triple::Apple) {
     if ((arch_type == llvm::Triple::arm) ||
         (arch_type == llvm::Triple::thumb)) {
-      if (!g_abi_sp)
-        g_abi_sp.reset(new ABISysV_arm(process_sp));
-      return g_abi_sp;
+      return ABISP(new ABISysV_arm(process_sp));
     }
   }
 
   return ABISP();
 }
 
 bool ABISysV_arm::PrepareTrivialCall(Thread &thread, addr_t sp,
                                      addr_t function_addr, addr_t return_addr,

source/Plugins/ABI/SysV-arm64/ABISysV_arm64.cpp

 size_t ABISysV_arm64::GetRedZoneSize() const { return 128; }
 
 //------------------------------------------------------------------
 // Static Functions
 //------------------------------------------------------------------
 
 ABISP
 ABISysV_arm64::CreateInstance(lldb::ProcessSP process_sp, const ArchSpec &arch) {
-  static ABISP g_abi_sp;
   const llvm::Triple::ArchType arch_type = arch.GetTriple().getArch();
   const llvm::Triple::VendorType vendor_type = arch.GetTriple().getVendor();
 
   if (vendor_type != llvm::Triple::Apple) {
     if (arch_type == llvm::Triple::aarch64) {
-      if (!g_abi_sp)
-        g_abi_sp.reset(new ABISysV_arm64(process_sp));
-      return g_abi_sp;
+      return ABISP(new ABISysV_arm64(process_sp));
     }
   }
 
   return ABISP();
 }
 
 bool ABISysV_arm64::PrepareTrivialCall(Thread &thread, addr_t sp,
                                        addr_t func_addr, addr_t return_addr,

source/Plugins/ABI/SysV-hexagon/ABISysV_hexagon.cpp

 size_t ABISysV_hexagon::GetRedZoneSize() const { return 0; }
 
 //------------------------------------------------------------------
 // Static Functions
 //------------------------------------------------------------------
 
 ABISP
 ABISysV_hexagon::CreateInstance(lldb::ProcessSP process_sp, const ArchSpec &arch) {
-  static ABISP g_abi_sp;
   if (arch.GetTriple().getArch() == llvm::Triple::hexagon) {
-    if (!g_abi_sp)
-      g_abi_sp.reset(new ABISysV_hexagon(process_sp));
-    return g_abi_sp;
+    return ABISP(new ABISysV_hexagon(process_sp));
   }
   return ABISP();
 }
 
 bool ABISysV_hexagon::PrepareTrivialCall(Thread &thread, lldb::addr_t sp,
                                          lldb::addr_t pc, lldb::addr_t ra,
                                          llvm::ArrayRef<addr_t> args) const {
   // we don't use the traditional trivial call specialized for jit

source/Plugins/ABI/SysV-i386/ABISysV_i386.cpp

 }
 
 //------------------------------------------------------------------
 // Static Functions
 //------------------------------------------------------------------
 
 ABISP
 ABISysV_i386::CreateInstance(lldb::ProcessSP process_sp, const ArchSpec &arch) {
-  static ABISP g_abi_sp;
   if (arch.GetTriple().getVendor() != llvm::Triple::Apple) {
     if (arch.GetTriple().getArch() == llvm::Triple::x86) {
-      if (!g_abi_sp)
-        g_abi_sp.reset(new ABISysV_i386(process_sp));
-      return g_abi_sp;
+      return ABISP(new ABISysV_i386(process_sp));
     }
   }
   return ABISP();
 }
 
 bool ABISysV_i386::PrepareTrivialCall(Thread &thread, addr_t sp,
                                       addr_t func_addr, addr_t return_addr,
                                       llvm::ArrayRef<addr_t> args) const {

source/Plugins/ABI/SysV-mips/ABISysV_mips.cpp

 size_t ABISysV_mips::GetRedZoneSize() const { return 0; }
 
 //------------------------------------------------------------------
 // Static Functions
 //------------------------------------------------------------------
 
 ABISP
 ABISysV_mips::CreateInstance(lldb::ProcessSP process_sp, const ArchSpec &arch) {
-  static ABISP g_abi_sp;
   const llvm::Triple::ArchType arch_type = arch.GetTriple().getArch();
   if ((arch_type == llvm::Triple::mips) ||
       (arch_type == llvm::Triple::mipsel)) {
-    if (!g_abi_sp)
-      g_abi_sp.reset(new ABISysV_mips(process_sp));
-    return g_abi_sp;
+    return ABISP(new ABISysV_mips(process_sp));
   }
   return ABISP();
 }
 
 bool ABISysV_mips::PrepareTrivialCall(Thread &thread, addr_t sp,
                                       addr_t func_addr, addr_t return_addr,
                                       llvm::ArrayRef<addr_t> args) const {
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_EXPRESSIONS));

source/Plugins/ABI/SysV-mips64/ABISysV_mips64.cpp

 size_t ABISysV_mips64::GetRedZoneSize() const { return 0; }
 
 //------------------------------------------------------------------
 // Static Functions
 //------------------------------------------------------------------
 
 ABISP
 ABISysV_mips64::CreateInstance(lldb::ProcessSP process_sp, const ArchSpec &arch) {
-  static ABISP g_abi_sp;
   const llvm::Triple::ArchType arch_type = arch.GetTriple().getArch();
   if ((arch_type == llvm::Triple::mips64) ||
       (arch_type == llvm::Triple::mips64el)) {
-    if (!g_abi_sp)
-      g_abi_sp.reset(new ABISysV_mips64(process_sp));
-    return g_abi_sp;
+    return ABISP(new ABISysV_mips64(process_sp));
   }
   return ABISP();
 }
 
 bool ABISysV_mips64::PrepareTrivialCall(Thread &thread, addr_t sp,
                                         addr_t func_addr, addr_t return_addr,
                                         llvm::ArrayRef<addr_t> args) const {
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_EXPRESSIONS));

source/Plugins/ABI/SysV-ppc/ABISysV_ppc.cpp

 size_t ABISysV_ppc::GetRedZoneSize() const { return 224; }
 
 //------------------------------------------------------------------
 // Static Functions
 //------------------------------------------------------------------
 
 ABISP
 ABISysV_ppc::CreateInstance(lldb::ProcessSP process_sp, const ArchSpec &arch) {
-  static ABISP g_abi_sp;
   if (arch.GetTriple().getArch() == llvm::Triple::ppc) {
-    if (!g_abi_sp)
-      g_abi_sp.reset(new ABISysV_ppc(process_sp));
-    return g_abi_sp;
+     return ABISP(new ABISysV_ppc(process_sp));
   }
   return ABISP();
 }
 
 bool ABISysV_ppc::PrepareTrivialCall(Thread &thread, addr_t sp,
                                      addr_t func_addr, addr_t return_addr,
                                      llvm::ArrayRef<addr_t> args) const {
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_EXPRESSIONS));

source/Plugins/ABI/SysV-s390x/ABISysV_s390x.cpp

 size_t ABISysV_s390x::GetRedZoneSize() const { return 0; }
 
 //------------------------------------------------------------------
 // Static Functions
 //------------------------------------------------------------------
 
 ABISP
 ABISysV_s390x::CreateInstance(lldb::ProcessSP process_sp, const ArchSpec &arch) {
-  static ABISP g_abi_sp;
   if (arch.GetTriple().getArch() == llvm::Triple::systemz) {
-    if (!g_abi_sp)
-      g_abi_sp.reset(new ABISysV_s390x(process_sp));
-    return g_abi_sp;
+    return ABISP(new ABISysV_s390x(process_sp));
   }
   return ABISP();
 }
 
 bool ABISysV_s390x::PrepareTrivialCall(Thread &thread, addr_t sp,
                                        addr_t func_addr, addr_t return_addr,
                                        llvm::ArrayRef<addr_t> args) const {
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_EXPRESSIONS));

source/Plugins/ABI/SysV-x86_64/ABISysV_x86_64.cpp

 size_t ABISysV_x86_64::GetRedZoneSize() const { return 128; }
 
 //------------------------------------------------------------------
 // Static Functions
 //------------------------------------------------------------------
 
 ABISP
 ABISysV_x86_64::CreateInstance(lldb::ProcessSP process_sp, const ArchSpec &arch) {
-  static ABISP g_abi_sp;
   if (arch.GetTriple().getArch() == llvm::Triple::x86_64) {
-    if (!g_abi_sp)
-      g_abi_sp.reset(new ABISysV_x86_64(process_sp));
-    return g_abi_sp;
+    return ABISP(new ABISysV_x86_64(process_sp));
   }
   return ABISP();
 }
 
 bool ABISysV_x86_64::PrepareTrivialCall(Thread &thread, addr_t sp,
                                         addr_t func_addr, addr_t return_addr,
                                         llvm::ArrayRef<addr_t> args) const {
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_EXPRESSIONS));