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



_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to