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

[Francis Ricci via Phabricator via llvm-commits]

fjricci added a comment.

In https://reviews.llvm.org/D29588#669898, @dvyukov wrote:

> In https://reviews.llvm.org/D29588#669813, @fjricci wrote:
>
> > Correct. We know that the data won't change in this particular case, because we know the parent isn't changing the data, and we know that our parent is holding the lock. However, this isn't what CheckLocked() is checking. CheckLocked() checks that *any* thread holds the lock, it doesn't need to be our parent to pass the check.
>
>
> I still don't get the problem.
>  How is CheckLocked related? I don't see any CheckLocked calls in lsan code.


`CheckLocked()` is called via: `__lsan::CheckForLeaksCallback()` -> `__lsan::GetThreadRangesLocked()` -> `__sanitizer::ThreadRegistry::FindThreadContextByOsIDLocked()` -> `CheckLocked()`

I think I should probably do a better job of explaining the motivations behind these two changes.

`BlockingMutex::CheckLocked()` is part of an OS-independent API, called primarily from OS-independent code. To me, this means that its behavior should be the same regardless of platform. Currently this is not the case. If a platform-independent function calls `CheckLocked()`, its behavior will be different depending on the OS. So in practice, when `FindThreadContextByOsIDLocked()` calls `CheckLocked()`, if the code is running on linux, the check will pass as long as *any thread* holds the mutex. On mac or windows, the check will only pass if the *calling thread* holds the mutex.

Essentially, the problem I'm trying to solve is solidifying the expected behavior of `CheckLocked()`. If we think that the mac and windows implementations are representative of the correct API, then we should also only pass the check if the calling thread holds the mutex on linux, which requires these two changes. If we think the linux implementation is representative, then on mac and windows, we should pass as long as any thread holds the mutex, and not fail if the current thread isn't the holder. I'm happy with either API, although the current stricter approach seemed more correct to me, due to the reasons I've attempted to outline in previous comments.

It would probably be productive to decide what we want the general behavior of `CheckLocked()` to be. From there, we can decide whether we want to use this patch or a different fix.


https://reviews.llvm.org/D29588