[PATCH] D29588: Use rw locks for sanitizer thread registry

[Dmitry Vyukov via Phabricator via llvm-commits]

dvyukov added a comment.

The contract for CheckLocked is: CheckLocked does nothing if the current thread holds the mutex, otherwise behavior is undefined.
And, yes, we are currently abusing the sloppy check in BlockingMutex on linux in CheckForLeaks.

However, your solution does not look right to me. There are several problems with it:

1. Besides ThreadRegistry mutex, CheckForLeaks also locks in a similar manner allocator mutexes and dl_iterate_phr mutex. They are no firing currently due to a number of coincidences (e.g. not calling CheckLocked where is should be called, using SpinMutex that has the same sloppy check).
2. StopTheWorld is also used in tsan and subject to all the same problems. We can't just start changing all mutexes to RWMutex to please the stricter check. And finally:
3. RWMutex has the same sloppy ownership check for readers as the one you are trying to eliminate in BlockingMutex. When/if we make the check in RWMutex strict (require the calling thread to hold read lock), it will all fall apart because you don't actually fix the problem, you just mask it again abusing the sloppy check in RWMutex.

The root problem here is that StopTheWorld does weird things with mutexes, clone and ptrace. But it does them for a reason. A proper solution to this problem would be: parent grabs all mutexes and starts the child process, child process stops all threads _besides_ the parent thread, child process instructs parent thread to unlock the mutexes, parent thread releases all mutexes and blocks on something, child process grabs all mutexes. But I don't think it's worth it.

I see 2 practical ways to resolve this:

1. Make all CheckLocked checks sloppy. It's actually not that bad, because we will catch any misuse eventually. It's also simple and fast.
2. Make all CheckLocked checks strict (including RWMutex and SpinMutex) but introduce a special global of per-thread flag for StopTheWorld that will say "back off, I am doing weird things here, so disable ownership checks for now".


https://reviews.llvm.org/D29588