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

[Greg Clayton via lldb-commits]

> On Jun 21, 2018, at 8:32 AM, Frédéric Riss <friss at apple.com> wrote:
> 
> 
> 
>> On Jun 21, 2018, at 3:18 AM, Pavel Labath via Phabricator <reviews at reviews.llvm.org> wrote:
>> 
>> labath added a comment.
>> 
>> 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.
>> 
>> So, I think that locking at a higher level would be better. Doing that will certainly be tricky though…
> 
> You are absolutely correct. I had quickly thought about this, but thought that we would just be duplicating work. Seeing how DIE_IS_BEING_PARSED is used this is actually a correctness issue.
> 
> While looking at this and especially the DIE_BEING_PARSED stuff, I was wondering if we couldn’t use a lockless data-structure like a hand-rolled bit-vector instead of using the map to store this information. What if we do something like this, but we make the DIE_IS_BEING_PARSED data-structure thread-local? In this case, I suppose you would potentially duplicate some work, but I think it should yield a correct result. WDYT?

The main issue with that approach is we might end up adding the one class multiple times to the clang::ASTContext. This would be bad and cause all sorts of expressions parsing issues. 

> 
> Fred

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20180621/211ed7a9/attachment.html>