[Lldb-commits] [PATCH] D48393: Make DWARFParsing more thread-safe

[Frédéric Riss via lldb-commits]

> On Jun 21, 2018, at 8:03 AM, Pavel Labath via Phabricator <reviews at reviews.llvm.org> wrote:
> 
> labath added a comment.
> 
> In https://reviews.llvm.org/D48393#1139270, @clayborg wrote:
> 
>> In https://reviews.llvm.org/D48393#1138989, @labath wrote:
>> 
>>> I am not sure this will actually solve the problems you are seeing. This may avoid corrupting the internal DenseMap data structures, but it will not make the algorithm using them actually correct.
>>> For example the pattern in `ParseTypeFromDWARF` is:
>>> 
>>> 1. check the "already parsed map". If the DIE is already parsed then you're done.
>>> 2. if the map contains the magic "DIE_IS_BEING_PARSED" key, abort (recursive dwarf references)
>>> 3. otherwise, insert the  "DIE_IS_BEING_PARSED" key into the map
>>> 4. do the parsing, which potentially involves recursive `ParseTypeFromDWARF` calls
>>> 5. insert the parsed type into the map
>>> 
>>>  What you do is make each of the steps (1), (3), (5) atomic individually. However, the whole algorithm is not correct unless the whole sequence is atomic as a whole. Otherwise, if you have two threads trying to parse the same DIE (directly or indirectly), one of them could see the intermediate DIE_IS_BEING_PARSED and incorrectly assume that it encountered recursive types.
>> 
>> 
>> We need to make #1 atomic.
>> #2 would need to somehow know if the type is already being parsed recursively by the current thread. If so, then do what we do now. If not, we need a way to wait on the completion of this type so the other parsing thread can complete it and put it into the map, at which time we grab the right value from the map
>> So #6 step would need to be added so after we do put it into the map, we can notify other threads that might be waiting
> 
> 
> 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.

Doh, that sounds like what I just tried to describe in my other mail. I should read threads completely before replying.

Fred