Restrict the set of plugins used for ProcessMinidump

[lemo]

Summary

1. The dynamic loaders should not be needed for loading minidumps and they may create problems (ex. the macOS loader resets the list of loaded sections)
2. In general, the extra plugins can do extraneous work which hurts performance (ex. trying to set up implicit breakpoints, which in turn will trigger extra symbol loading)

[clayborg]

Dynamic loaders are needed for loading breakpad minidumps that are for MacOSX and iOS processes. They should also be needed for loading any minidumps that have stack traces.

[clayborg]

No dynamic loader plug-ins should be affecting the module list during the plug-in loading/selection, if that is happening, that is a bug and it should be fixed.

[lemo]

Dynamic loaders are needed for loading breakpad minidumps that are for MacOSX and iOS processes. They should also be needed for loading any minidumps that have stack traces.

Thanks. I just validated the change against a macOS minidump and everything works fine in my local test. What parts of the dynamic loader is relevant to minidump loading? (ie. anything specific I should pay attention to?)

No dynamic loader plug-ins should be affecting the module list during the plug-in loading/selection, if that is happening, that is a bug and it should be fixed.

I agree. Although that's outside the scope of this change if I'm right in that we can avoid the dynamic loader plugins completely. Here's an example where the DynamicLoaderDarwin is misbehaving
(note the DynamicLoaderDarwin::PrivateInitialize() call to Target::ClearAllLoadedSections())

lldb_private::SectionLoadList::Clear(lldb_private::SectionLoadList * this) (lldb/source/Target/SectionLoadList.cpp:47)
lldb_private::SectionLoadList::~SectionLoadList(lldb_private::SectionLoadList * this) (lldb/include/lldb/Target/SectionLoadList.h:39)
std::_Sp_counted_ptr<lldb_private::SectionLoadList*, (gnu_cxx::_Lock_policy)2>::_M_dispose(std::_Sp_counted_ptr<lldb_private::SectionLoadList*, gnu_cxx::_S_atomic> * this) (libstdcxx/include/bits/shared_ptr_base.h:371)
std::_Sp_counted_base<(gnu_cxx::_Lock_policy)2>::_M_release(std::_Sp_counted_base<gnu_cxx::_S_atomic> * this) (libstdcxx/include/bits/shared_ptr_base.h:149)
std::shared_count<(gnu_cxx::_Lock_policy)2>::~shared_count(std::shared_count<gnu_cxx::_S_atomic> * this) (libstdcxx/include/bits/shared_ptr_base.h:664)
std::shared_ptr<lldb_private::SectionLoadList, (gnu_cxx::_Lock_policy)2>::~shared_ptr(std::shared_ptr<lldb_private::SectionLoadList, gnu_cxx::_S_atomic> * this) (libstdcxx/include/bits/shared_ptr_base.h:912)
std::shared_ptr<lldb_private::SectionLoadList>::~shared_ptr(std::shared_ptr<lldb_private::SectionLoadList> * this) (libstdcxx/include/bits/shared_ptr_base.h:342)
std::pair<unsigned int const, std::shared_ptr<lldb_private::SectionLoadList> >::~pair(std::pair<unsigned int const, std::shared_ptr<lldb_private::SectionLoadList> > * this) (libstdcxx/include/bits/stl_pair.h:96)
gnu_cxx::new_allocator<std::_Rb_tree_node<std::pair<unsigned int const, std::shared_ptr<lldb_private::SectionLoadList> > > >::destroy<std::pair<unsigned int const, std::shared_ptr<lldb_private::SectionLoadList> > >(gnu_cxx::new_allocator<std::_Rb_tree_node<std::pair<unsigned int const, std::shared_ptr<lldb_private::SectionLoadList> > > > * this, std::pair<unsigned int const, std::shared_ptr<lldb_private::SectionLoadList> > * p) (libstdcxx/include/ext/new_allocator.h:165)
std::allocator_traits<std::allocator<std::_Rb_tree_node<std::pair<unsigned int const, std::shared_ptr<lldb_private::SectionLoadList> > > > >::destroy<std::pair<unsigned int const, std::shared_ptr<lldb_private::SectionLoadList> > >(std::allocator_traits<std::allocator<std::_Rb_tree_node<std::pair<unsigned int const, std::shared_ptr<lldb_private::SectionLoadList> > > > >::allocator_type & a, std::pair<unsigned int const, std::shared_ptr<lldb_private::SectionLoadList> > * p) (libstdcxx/include/bits/alloc_traits.h:539)
std::_Rb_tree<unsigned int, std::pair<unsigned int const, std::shared_ptr<lldb_private::SectionLoadList> >, std::_Select1st<std::pair<unsigned int const, std::shared_ptr<lldb_private::SectionLoadList> > >, std::map<unsigned int, std::shared_ptr<lldb_private::SectionLoadList>, std::less<unsigned int>, std::allocator<std::pair<unsigned int const, std::shared_ptr<lldb_private::SectionLoadList> > > >::_ConstCompare<std::less<unsigned int> >, std::allocator<std::pair<unsigned int const, std::shared_ptr<lldb_private::SectionLoadList> > > >::_M_destroy_node(std::_Rb_tree<unsigned int, std::pair<unsigned int const, std::shared_ptr<lldb_private::SectionLoadList> >, std::_Select1st<std::pair<unsigned int const, std::shared_ptr<lldb_private::SectionLoadList> > >, std::map<unsigned int, std::shared_ptr<lldb_private::SectionLoadList>, std::less<unsigned int>, std::allocator<std::pair<unsigned int const, std::shared_ptr<lldb_private::SectionLoadList> > > >::_ConstCompare<std::less<unsigned int> >, std::allocator<std::pair<unsigned int const, std::shared_ptr<lldb_private::SectionLoadList> > > > * this, std::_Rb_tree<unsigned int, std::pair<unsigned int const, std::shared_ptr<lldb_private::SectionLoadList> >, std::_Select1st<std::pair<unsigned int const, std::shared_ptr<lldb_private::SectionLoadList> > >, std::map<unsigned int, std::shared_ptr<lldb_private::SectionLoadList>, std::less<unsigned int>, std::allocator<std::pair<unsigned int const, std::shared_ptr<lldb_private::SectionLoadList> > > >::_ConstCompare<std::less<unsigned int> >, std::allocator<std::pair<unsigned int const, std::shared_ptr<lldb_private::SectionLoadList> > > >::_Link_type p) (libstdcxx/include/bits/stl_tree.h:435)
std::_Rb_tree<unsigned int, std::pair<unsigned int const, std::shared_ptr<lldb_private::SectionLoadList> >, std::_Select1st<std::pair<unsigned int const, std::shared_ptr<lldb_private::SectionLoadList> > >, std::map<unsigned int, std::shared_ptr<lldb_private::SectionLoadList>, std::less<unsigned int>, std::allocator<std::pair<unsigned int const, std::shared_ptr<lldb_private::SectionLoadList> > > >::_ConstCompare<std::less<unsigned int> >, std::allocator<std::pair<unsigned int const, std::shared_ptr<lldb_private::SectionLoadList> > > >::_M_erase(std::_Rb_tree<unsigned int, std::pair<unsigned int const, std::shared_ptr<lldb_private::SectionLoadList> >, std::_Select1st<std::pair<unsigned int const, std::shared_ptr<lldb_private::SectionLoadList> > >, std::map<unsigned int, std::shared_ptr<lldb_private::SectionLoadList>, std::less<unsigned int>, std::allocator<std::pair<unsigned int const, std::shared_ptr<lldb_private::SectionLoadList> > > >::_ConstCompare<std::less<unsigned int> >, std::allocator<std::pair<unsigned int const, std::shared_ptr<lldb_private::SectionLoadList> > > > * this, std::_Rb_tree<unsigned int, std::pair<unsigned int const, std::shared_ptr<lldb_private::SectionLoadList> >, std::_Select1st<std::pair<unsigned int const, std::shared_ptr<lldb_private::SectionLoadList> > >, std::map<unsigned int, std::shared_ptr<lldb_private::SectionLoadList>, std::less<unsigned int>, std::allocator<std::pair<unsigned int const, std::shared_ptr<lldb_private::SectionLoadList> > > >::_ConstCompare<std::less<unsigned int> >, std::allocator<std::pair<unsigned int const, std::shared_ptr<lldb_private::SectionLoadList> > > >::_Link_type __x) (libstdcxx/include/bits/stl_tree.h:1283)
std::_Rb_tree<unsigned int, std::pair<unsigned int const, std::shared_ptr<lldb_private::SectionLoadList> >, std::_Select1st<std::pair<unsigned int const, std::shared_ptr<lldb_private::SectionLoadList> > >, std::map<unsigned int, std::shared_ptr<lldb_private::SectionLoadList>, std::less<unsigned int>, std::allocator<std::pair<unsigned int const, std::shared_ptr<lldb_private::SectionLoadList> > > >::_ConstCompare<std::less<unsigned int> >, std::allocator<std::pair<unsigned int const, std::shared_ptr<lldb_private::SectionLoadList> > > >::clear(std::_Rb_tree<unsigned int, std::pair<unsigned int const, std::shared_ptr<lldb_private::SectionLoadList> >, std::_Select1st<std::pair<unsigned int const, std::shared_ptr<lldb_private::SectionLoadList> > >, std::map<unsigned int, std::shared_ptr<lldb_private::SectionLoadList>, std::less<unsigned int>, std::allocator<std::pair<unsigned int const, std::shared_ptr<lldb_private::SectionLoadList> > > >::_ConstCompare<std::less<unsigned int> >, std::allocator<std::pair<unsigned int const, std::shared_ptr<lldb_private::SectionLoadList> > > > * this) (libstdcxx/include/bits/stl_tree.h:944)
std::map<unsigned int, std::shared_ptr<lldb_private::SectionLoadList>, std::less<unsigned int>, std::allocator<std::pair<unsigned int const, std::shared_ptr<lldb_private::SectionLoadList> > > >::clear(std::map<unsigned int, std::shared_ptr<lldb_private::SectionLoadList>, std::less<unsigned int>, std::allocator<std::pair<unsigned int const, std::shared_ptr<lldb_private::SectionLoadList> > > > * this) (libstdcxx/include/bits/stl_map.h:862)
lldb_private::SectionLoadHistory::Clear(lldb_private::SectionLoadHistory * this) (lldb/source/Target/SectionLoadHistory.cpp:29)
lldb_private::Target::ClearAllLoadedSections(lldb_private::Target * this) (lldb/source/Target/Target.cpp:2952)
lldb_private::DynamicLoaderDarwin::PrivateInitialize(lldb_private::DynamicLoaderDarwin * this, lldb_private::Process * process) (lldb/source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderDarwin.cpp:768)
lldb_private::DynamicLoaderDarwin::DidAttach(lldb_private::DynamicLoaderDarwin * this) (lldb/source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderDarwin.cpp:73)
lldb_private::Process::LoadCore(lldb_private::Process * this) (lldb/source/Target/Process.cpp:2847)
lldb::SBTarget::LoadCore(lldb::SBTarget * this, const char * core_file, lldb::SBError & error) (lldb/source/API/SBTarget.cpp:218)

[jingham]

That's not changing the module list, that's changing the loaded section information. It is the dynamic loader's job to figure out what got loaded where, plus your stack trace show this happening after we've selected a plugin, not in the process of negotiating for the plugin. Clearing the section load list before setting to work seems like an appropriate thing for a selected plugin to do.

[clayborg]

Dynamic loaders will fill out the section load list in the target that saids "__TEXT" from "/tmp/a.out" was loaded at address 0x1000020200. So yes they are needed. If the process minidump is manually setting the section load list itself, then there really is no need for a dynamic loader and this fix might work.

[lemo]

That's not changing the module list, that's changing the loaded section information. It is the dynamic loader's job to figure out what got loaded where, plus your stack trace show this happening after we've selected a plugin, not in the process of negotiating for the plugin. Clearing the section load list before setting to work seems like an appropriate thing for a selected plugin to do.

Correct, sorry - I meant the loaded sections list. This reset is problematic for minidumps since we build the loaded sections list while loading the minidump.

Is the dynamic loader relevant to loading the minidumps in any way? I'm open to any other ides on how to avoid this conflict between the minidump loading and the dynamic loader, although I'd strongly prefer to minimize the code path to the strictly minimum required. For example this dynamic loader issue is particularly unfortunate since it only happens for macOS minidumps. So one immediate consequence at least is that it complicates the test matrix (BTW, we should probably have at least one macOS & iOS minidumps for LLDB tests, what do you think?)

[lemo]

In D51176#1211307, @clayborg wrote:

Dynamic loaders will fill out the section load list in the target that saids "__TEXT" from "/tmp/a.out" was loaded at address 0x1000020200. So yes they are needed. If the process minidump is manually setting the section load list itself, then there really is no need for a dynamic loader and this fix might work.

That was my original thought: for minidumps we don't really have "load addresses", the memory map is recorded in the minidump and that's what we use (see PlaceholderModule::CreateImageSection)

[clayborg]

So this might actually work. Take a look around and see what else the dynamic loader is used for and make sure that they won't be needed for anything else. If not, this fix should work, but we should document it.

[lemo]

In D51176#1211358, @clayborg wrote:

So this might actually work. Take a look around and see what else the dynamic loader is used for and make sure that they won't be needed for anything else. If not, this fix should work, but we should document it.

I took another look and I don't see anything where the dynamic loader is required in the context of loading minidumps.

[clayborg]

I am fine with this change then. Probably best to get the OK from Zach as well.

[jingham]

The other option would be to move the code that populates the section load list into the mini dump dynamic loader. That has the benefit of keeping this consistent with the other process plugins, but OTOH is just moving code around...

[clayborg]

In D51176#1211433, @jingham wrote:

The other option would be to move the code that populates the section load list into the mini dump dynamic loader. That has the benefit of keeping this consistent with the other process plugins, but OTOH is just moving code around...

Yes the dynamic loader plug-ins aren't hard to write and the code already exists in the ProcessMinidump. Then you would request the plug-in by name instead of passing a nullptr as the name in ProcessMinidump::GetDynamicLoader().

[lemo]

It's an interesting idea, thanks! I don't object moving code around if
there's a strong case for it, but I'd like to keep the fix small and simple
for now, but it's worth considering if the current minidump loading path
will need more flexibility.

[zturner]

source/Plugins/Process/minidump/ProcessMinidump.cpp

399–404

Can you put a comment in here explaining that the base class implementation does additional work that is unnecessary for a minidump plugin, so this is basically a performance optimization?

[lemo]

Added the comment requested by zturner

[lemo]

In D51176#1211433, @jingham wrote:

The other option would be to move the code that populates the section load list into the mini dump dynamic loader. That has the benefit of keeping this consistent with the other process plugins, but OTOH is just moving code around...

It's an interesting idea, thanks. I'd like to keep the fix small and simple for now, but it's worth considering if the current minidump loading path will need more flexibility.

source/Plugins/Process/minidump/ProcessMinidump.h

public:
 
   ~ProcessMinidump() override;
 
   bool CanDebug(lldb::TargetSP target_sp,
                 bool plugin_specified_by_name) override;
 
   Status DoLoadCore() override;
 
-  DynamicLoader *GetDynamicLoader() override;
+  DynamicLoader *GetDynamicLoader() override { return nullptr; }
 
   ConstString GetPluginName() override;
 
   uint32_t GetPluginVersion() override;
 
   SystemRuntime *GetSystemRuntime() override { return nullptr; }
 
   Status DoDestroy() override;

 protected:
   void Clear();
 
   bool UpdateThreadList(ThreadList &old_thread_list,
                         ThreadList &new_thread_list) override;
 
   void ReadModuleList();
 
+  JITLoaderList &GetJITLoaders() override;
+
 private:
   FileSpec m_core_file;
   llvm::ArrayRef<MinidumpThread> m_thread_list;
   const MinidumpExceptionStream *m_active_exception;
   bool m_is_wow64;
 };
 
 } // namespace minidump
 } // namespace lldb_private
 
 #endif // liblldb_ProcessMinidump_h_

source/Plugins/Process/minidump/ProcessMinidump.cpp

 #include "ProcessMinidump.h"
 #include "ThreadMinidump.h"
 
 // Other libraries and framework includes
 #include "lldb/Core/Module.h"
 #include "lldb/Core/ModuleSpec.h"
 #include "lldb/Core/PluginManager.h"
 #include "lldb/Core/Section.h"
-#include "lldb/Target/DynamicLoader.h"
+#include "lldb/Target/JITLoaderList.h"
 #include "lldb/Target/MemoryRegionInfo.h"
 #include "lldb/Target/SectionLoadList.h"
 #include "lldb/Target/Target.h"
 #include "lldb/Target/UnixSignals.h"
 #include "lldb/Utility/DataBufferLLVM.h"
 #include "lldb/Utility/LLDBAssert.h"
 #include "lldb/Utility/Log.h"
 #include "lldb/Utility/State.h"
if (!pid) {
     error.SetErrorString("failed to parse PID");
     return error;
   }
   SetID(pid.getValue());
 
   return error;
 }
 
-DynamicLoader *ProcessMinidump::GetDynamicLoader() {
-  if (m_dyld_ap.get() == nullptr)
-    m_dyld_ap.reset(DynamicLoader::FindPlugin(this, nullptr));
-  return m_dyld_ap.get();
-}
-
 ConstString ProcessMinidump::GetPluginName() { return GetPluginNameStatic(); }
 
 uint32_t ProcessMinidump::GetPluginVersion() { return 1; }
 
 Status ProcessMinidump::DoDestroy() { return Status(); }
 
 void ProcessMinidump::RefreshStateAfterStop() {
   if (!m_active_exception)
bool ProcessMinidump::GetProcessInfo(ProcessInstanceInfo &info) {
   lldb::ModuleSP module_sp = GetTarget().GetExecutableModule();
   if (module_sp) {
     const bool add_exe_file_as_first_arg = false;
     info.SetExecutableFile(GetTarget().GetExecutableModule()->GetFileSpec(),
                            add_exe_file_as_first_arg);
   }
   return true;
 }
+
+// For minidumps there's no runtime generated code so we don't need JITLoader(s)
+// Avoiding them will also speed up minidump loading since JITLoaders normally
+// try to set up symbolic breakpoints, which in turn may force loading more
+// debug information than needed.
+JITLoaderList &ProcessMinidump::GetJITLoaders() {
+  if (!m_jit_loaders_ap) {
+    m_jit_loaders_ap = llvm::make_unique<JITLoaderList>();
+  }
+  return *m_jit_loaders_ap;
+}