Page MenuHomePhabricator

Make DIE_IS_BEING_PARSED local to the current thread.
AbandonedPublic

Authored by JDevlieghere on Sep 24 2018, 2:35 AM.

Details

Reviewers
labath
clayborg
Summary

This is an attempt to realize Pavel's suggestion from D48393

It's even more complicated than that, in case you really have reference cycles, you can have multiple threads starting parsing from different points in that cycle, and getting deadlocked waiting for the DIE_IS_BEING_PARSED results from each other.

The only sane algorithm I can come up right now is to make the list of parsed dies local to each thread/parsing entity (e.g. via a "visited" list), and only update the global map once the parsing has completed (successfully or not). This can potentially duplicate some effort where one thread parses a type only to find out that it has already been parsed, but hopefully that is not going to be the common case. The alternative is some complicated resource cycle detection scheme.

Diff Detail

Repository
rLLDB LLDB

Event Timeline

JDevlieghere created this revision.Sep 24 2018, 2:35 AM
JDevlieghere edited the summary of this revision. (Show Details)Sep 24 2018, 2:35 AM
clayborg requested changes to this revision.Sep 24 2018, 8:55 AM
clayborg added a subscriber: clayborg.

A little background might help here: The lldb_private::Module lock is used to prevent multiple queries into the DWARF from stomping on each other.

Multi-threaded DWARF parsing was primarily added to speed up indexing and the only place it is used. Is that not true? Indexing is just creating name tables and the accelerator tables we need so that we can partially parse the DWARF later. No type parsing should be happening when indexing. All other accesses to the DWARF must be done via a SymbolFile API that takes the module lock to stop multiple threads from stomping on each other.

This fix will not work for a few reasons:

  • multiple threads should not be allowed to parse types at the same time, the module lock must be taken, if that isn't the case, then this needs to be fixed
  • even if we were to allow this fix and if multiple threads are able to parse DWARF at the same time, this fix wouldn't work. We have one clang AST per symbol and we convert DWARF into clang ASTs. We can't have multiple threads creating a "class Foo" at the same time and populating them at anytime.

So my main point is we need to use the module lock to avoid having multiple threads doing important work that will cause crashes.

This revision now requires changes to proceed.Sep 24 2018, 8:55 AM
JDevlieghere abandoned this revision.Sep 26 2018, 2:04 AM