> On Jun 21, 2018, at 8:32 AM, Frédéric Riss <fr...@apple.com> wrote: > > > >> On Jun 21, 2018, at 3:18 AM, Pavel Labath via Phabricator >> <revi...@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
_______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
