> 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? Fred _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
