[PATCH] D157459: Make DWARFContext more thread safe.

[Greg Clayton via Phabricator via llvm-commits]

clayborg added a comment.

> It *is* more complex, I agree. The trade-off is that nobody can take touch `TUIndex` directly from within `DWARFContext`. They are required to go through `Impl` to actually get the TUIndex. The point is to have a safeguard other than code review, something that is difficult to write in an unsafe way. C++ doesn't give us a ton of guard rails so I'm suggesting we write our own.

It is one possible solution yes.

>>> - Some kind of documentation related to the expectations of `DWARFContext` in a multithreaded environment go a long way. Even something simple as "This class is considered thread-safe" so that folks know that they can call `DWARFContext::getTUIndex` without worrying about this exact scenario.
>>
>> Seems like this is pretty clear from the places the mutex is used, but I could add documentation if needed.
>
> I would argue that it's possible to figure out if you read the implementation. Reading the documentation or the header doesn't tell me what the guarantees and expected behaviors are. `LLVMContext` is a good example -- The documentation states that it's not safe to touch in a concurrent fashion and users must guard access to it. DWARFContext currently has nothing written down in this regard. This patch is making a decision about the use of `DWARFContext` in a concurrent fashion, we should write down that decision so there are clear expectations. It would be the difference between a bug report ("My code crashed when using DWARFContext in a multithreaded program. It's supposed to be thread-safe so this is a bug.") versus a user misusing an API ("My code crashed when using LLVMContext in a multithreaded program. The documentation says it's not safe to do this, I need to add locks myself").

if DWARFContext was a class that was used on its own this would make more sense to have us allow it to claim to be not usable in threaded code. The problem is its use is intertwined in many of the DWARF classes. The DWARFUnit and the DWARFDie and other classes access this from within their own code so stating that DWARFContext is not threadsafe and your need to guard access to it on your own can't and doesn't work, this is what we have now. When we have a split DWARF binary we have 1 executable and N number of .dwo files which each have their own DWARFContext, which can and will request things from the main DWARFContext if and when needed (like access to the .debug_addr section, TUIndex, CUIndex, etc) and can cause crashes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157459