Page MenuHomePhabricator

Restrict the set of plugins used for ProcessMinidump
ClosedPublic

Authored by lemo on Aug 23 2018, 10:58 AM.

Details

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)

Diff Detail

Event Timeline

lemo created this revision.Aug 23 2018, 10:58 AM
clayborg requested changes to this revision.Aug 23 2018, 11:07 AM

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.

This revision now requires changes to proceed.Aug 23 2018, 11:07 AM

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 added a comment.Aug 23 2018, 11:17 AM

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)

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.

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 added a comment.Aug 23 2018, 11:37 AM

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 added a comment.Aug 23 2018, 11:40 AM

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)

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 added a comment.Aug 23 2018, 1:07 PM

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 accepted this revision.Aug 23 2018, 1:17 PM

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

This revision is now accepted and ready to land.Aug 23 2018, 1:17 PM

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...

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().

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.

lemo edited the summary of this revision. (Show Details)Aug 23 2018, 1:38 PM
zturner accepted this revision.Aug 23 2018, 2:00 PM
zturner added inline comments.
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 updated this revision to Diff 162270.Aug 23 2018, 2:31 PM

Added the comment requested by zturner

lemo marked an inline comment as done.Aug 23 2018, 2:32 PM

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.

This revision was automatically updated to reflect the committed changes.