[Lldb-commits] [PATCH] D41909: Fix deadlock in dwarf logging

[Tamas Berghammer via Phabricator via lldb-commits]

tberghammer added a comment.

I think it isn't an A/B locking issue. The problem is that 2 different thread (main thread and 1 of the DWARF indexer thread) want to lock the mutex at the same time.

I have a mixed feeling about this change because introducing any sort of race condition (even if it is used only during logging) is problematic as it can read to undefined behavior and then crashes for example by reading and then dereferencing an  incorrect const char*. In this specific case I am pretty the code is actually thread safe on all architectures we care about (unless the compiler do something very crazy) because we read a set of ConstString's what is equivalent to reading a single pointer what I think is atomic on all architectures we care about (assuming it is aligned) and then we read data from the ConstString data pool what is read only and thread safe. My concern is that changing something fairly trivial (e.g. change a ConstString to an std::string inside FileSpec) can make this code not thread safe and introduce some new crashes so if possible I think we should try to keep the mutex here.

Why do we need to lock the Module mutex in SymbolVendor::FindFunctions? I think a better option would be to remove that lock and if it is needed then lock it just for the calls where it necessary. The fact that SymbolVendor locks a mutex inside a Module feels like a pretty bad layering violation for me what can cause many other deadlocks so it would be nice to fix that instead of hacking it around here.


https://reviews.llvm.org/D41909