[Lldb-commits] [PATCH] D148380: [lldb] Lock accesses to PathMappingLists's internals

[Alex Langford via Phabricator via lldb-commits]

bulbazord added a comment.

In D148380#4270090 <https://reviews.llvm.org/D148380#4270090>, @JDevlieghere wrote:

> In D148380#4270085 <https://reviews.llvm.org/D148380#4270085>, @bulbazord wrote:
>
>> In D148380#4269876 <https://reviews.llvm.org/D148380#4269876>, @bulbazord wrote:
>>
>>> In D148380#4269862 <https://reviews.llvm.org/D148380#4269862>, @JDevlieghere wrote:
>>>
>>>> Does this actually have to be a //recursive// mutex?
>>>
>>> Good point, I don't think it does. I'll update this.
>>
>> Scratch that -- It unfortunately does need to be a recursive mutex. `PathMappingList` has a callback that may try to perform another operation on the list itself. This happens if you try to use `target modules search-paths add` -- We append to the list, the specified callback tries to read the size of the list. With a std::mutex we deadlock.
>
> I see, and I assume it doesn't make sense to unlock the mutex before the callback?

I modified this patch locally to use `std::mutex` and release the mutex before invoking the callback. The test suite seems to like it, but it's still technically incorrect because of `AppendUnique`. Using std::mutex, we'd have to first lock the mutex, check to see if the given paths are already in `m_pairs`, and if they are not, unlock the mutex and call Append. If 2 things try to AppendUnique the same things at the same time and the stars align, they're going to Append the same thing twice (which is what happens without this patch anyway). We'd need to do some refactors if we wanted std::mutex to work correctly instead of std::recursive_mutex. That may be worth doing in the future, but I'm going to land this as-is for now.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148380/new/

https://reviews.llvm.org/D148380