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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In https://reviews.llvm.org/D48393#1243195, @JDevlieghere wrote:

> In https://reviews.llvm.org/D48393#1139327, @labath wrote:
>
> > 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.
>
>
> I gave this a shot in https://reviews.llvm.org/D52406 but I'm afraid it's too simple to be correct. Pavel, could you give it a look and let me know whether that was what you had in mind?


Yeah, I don't think this will make things fully correct. This avoids the problem when two threads are misinterpreting the DIE_IS_BEING_PARSED markers left by the other thread. However, it is still not correct when two threads are parsing the same DIE concurrently. Imagine the following sequence of events:

- thread A starts parsing DIE 1, checks that it still hasn't been parsed, inserts the DIE_IS_BEING_PARSED into the local map.
- thread B does the same
- thread A finishes parsing DIE 1. constructs a clang AST (C1) for it sets it as the map value
- thread B does the same overwrites the value with C2
- thread A carries on using C1
- thread B carries on using C2

This sounds like something that is not supposed to happen, though I don't really know what the consequences of that are. Also I am not entirely convinced that the mere construction of C1 and C2 does not touch some global structures (which would not be protected by a lock).

I agree with Greg that it would be best to restrict things such that there is only one instance of parsing going on at any given time for a single module. I think this was pretty much the state we reached when this thread fizzled out the last time (there are some extra emails that are not showing in phabricator history, the interesting ones start around here: http://lists.llvm.org/pipermail/lldb-commits/Week-of-Mon-20180618/041937.html). I think the main part that needed to be resolved is whether we need to go anything special when resolving debug info references *between* modules (e.g. to prevent A/B deadlocks).


https://reviews.llvm.org/D48393