[Lldb-commits] [PATCH] D52543: [DWARFSymbolFile] Adds the module lock to all virtual methods from SymbolFile

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

In https://reviews.llvm.org/D52543#1246877, @clayborg wrote:

> It is the SymbolVendor's job to do the locking. And in many cases it already is. I stopped with inlined comments after a few comments as it would apply to this entire patch.


Thanks Greg. Honestly I don't know the code well enough to be sure where we *need* a lock and where we can get away without. Since it's a recursive mutex I figured there's no harm in playing it safe. Not everything goes through the symbol vendor though, for example the backtrace below is the result of a corruption and it looks like we're calling directly into SymbolFileDWARF.

  >  1 com.apple.LLDB.framework       0x02f8dcf5 bool llvm::DenseMapBase<llvm::DenseMap<void*, DIERef, llvm::DenseMapInfo<void*>, llvm::detail::DenseMapPair<void*, DIERef> >, void*, DIERef, llvm::DenseMapInfo<void*>, llvm::detail::DenseMapPair<void*, DIERef> >::LookupBucketFor<void*>(void* const&, llvm::detail::DenseMapPair<void*, DIERef> const*&) const + 45
     2 com.apple.LLDB.framework       0x02f8e91e llvm::DenseMapBase<llvm::DenseMap<DWARFDebugInfoEntry const*, clang::Decl*, llvm::DenseMapInfo<DWARFDebugInfoEntry const*>, llvm::detail::DenseMapPair<DWARFDebugInfoEntry const*, clang::Decl*> >, DWARFDebugInfoEntry const*, clang::Decl*, llvm::DenseMapInfo<DWARFDebugInfoEntry const*>, llvm::detail::DenseMapPair<DWARFDebugInfoEntry const*, clang::Decl*> >::FindAndConstruct(DWARFDebugInfoEntry const*&&) + 28
     3 com.apple.LLDB.framework       0x02f81f94 DWARFASTParserClang::ParseTypeFromDWARF(lldb_private::SymbolContext const&, DWARFDIE const&, lldb_private::Log*, bool*) + 6144
     4 com.apple.LLDB.framework       0x03150f75 SymbolFileDWARF::ParseType(lldb_private::SymbolContext const&, DWARFDIE const&, bool*) + 209
     5 com.apple.LLDB.framework       0x0314a70e SymbolFileDWARF::GetTypeForDIE(DWARFDIE const&, bool) + 486
     6 com.apple.LLDB.framework       0x03149ee2 SymbolFileDWARF::ResolveType(DWARFDIE const&, bool, bool) + 68
     7 com.apple.LLDB.framework       0x02f88728 DWARFASTParserClang::GetClangDeclContextForDIE(DWARFDIE const&) + 176
     8 com.apple.LLDB.framework       0x02f8c0ae DWARFASTParserClang::GetDeclContextForUIDFromDWARF(DWARFDIE const&) + 24
     9 com.apple.LLDB.framework       0x031636fe DWARFDIE::GetDeclContext() const + 60
    10 com.apple.LLDB.framework       0x03149dd5 SymbolFileDWARF::GetDeclContextForUID(unsigned long long) + 53
    11 com.apple.LLDB.framework       0x0315c6ed SymbolFileDWARFDebugMap::GetDeclContextForUID(u



> It would be great if we can replace all of the locations where you added lock guards with lldbasserts to assert that the lock is already taken. Then we might be able to catch places where the locks are not being respected. But in general the symbol vendor should take the lock, use N number of SymbolFile instances to complete the request, and then release the lock. Makes things easier on the lldb_private::SymbolFile subclasses this way.

That sounds like a really good idea, I'll definitely do that.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D52543