[PATCH] D157459: Make DWARFContext more thread safe.

[Alex Langford via Phabricator via llvm-commits]

bulbazord added a comment.

In D157459#4596891 <https://reviews.llvm.org/D157459#4596891>, @clayborg wrote:

> In D157459#4596842 <https://reviews.llvm.org/D157459#4596842>, @bulbazord wrote:
>
>> - It looks like the "blessed" way of accessing these members is through getter methods. Future contributors may not be aware of this restriction (partially because it's not written down anywhere, partially because it's not obvious by reading the code). Even if you do document it, it's unenforceable except through code review. To address this, I think we can add an abstraction where all the member variables are owned by some other class/struct (e.g. `DWARFContextImpl`) where all the members are owned by that `Impl` object and are marked private. Access to those variables would be strictly through getter methods. DWARFContext would then contain an instance of this `DWARFContextImpl` class so nobody can use the member variables in an unsynchronized fashion. To be more explicit, anytime a method in `DWARFContext` wants to get the `TUIndex`, it would have to call something like `Impl.getTUIndex()`. It's possible people can work around this and still do bad things, but they would have to consciously think about breaking the abstraction to do that.
>
> That sounds very complex and would make the class even harder to work on and maintain?

Here would be a very simplified version to illustrate my idea:

  class ThreadSafeDWARFContextImpl {
  private:
  MutexType Mutex;
  std::unique_ptr<DWARFUnitIndex> TUIndex;
  public:
  const DWARFUnitIndex &getTUIndex();
  };
  
  class DWARFContext {
  private:
  std::unique_ptr<ThreadSafeDWARFContextImpl> Impl;
  public:
  const DWARFUnitIndex &getTUIndex();
  };
  
  const DWARFUnitIndex &ThreadSafeDWARFContextImpl::getTUIndex() {
    // Perform lock of Mutex
    if (TUIndex)
      return TUIndex.get();
    // Else, do the work to compute it
    return *TUIndex;
  }
  
  const DWARFUnitIndex &DWARFContext::getTUIndex() {
    return Impl->getTUIndex();
  }

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.

>> - 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").


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157459