[PATCH] D157459: Make DWARFContext more thread safe.

[Greg Clayton via Phabricator via llvm-commits]

clayborg added a comment.

In D157459#4597411 <https://reviews.llvm.org/D157459#4597411>, @JDevlieghere wrote:

> Can we do something like this?
>
>   class DWARFContext {
>     public:
>       void foo();
>       void bar();
>   };
>   
>   template <typename T>
>   class ThreadSafeDWARFContext : public T {
>     public:
>       void foo() {
>         std::lock_guard<std::mutex> L(M);
>         return T::foo();
>       }
>   
>     private:
>       std::mutex M;
>   };

Does this mean there will be one mutex for all member variables, or one mutex per member variable? If it is one mutex per variable, then we will deadlock with A/B locking. If it is one mutex for all member variables, then this can work. I am assuming since this is templatized that it means there is one mutex per variable.

The other idea is to enable threading support by using a thread safe context but having the mutex be optional and controlled by how we initialize DWARFContextState with a boolean "EnableThreading":

  class OptionalLockGuard {
    std::unique_lock<std::mutex> LockGuard;
  public:
    OptionalLockGuard(std::optional<std::mutex> &OptMutex) {
      if (OptMutex) 
        LockGuard.swap(std::unique_lock<std::mutex>(*OptMutex);
    }
  };
  
  class DWARFContextState {
    std::optional<std::mutex> OptMutex; // If this has a value, then thread safety is enabled, else it isn't
    TUIndex TU;
    CUIndex CU;
    ... (add all other protected member variables here)
    public:
      DWARFContextState(DWARFContext &D, bool EnableThreading) DC(D) {
        if (EnableThreading)
          OptMutex = std::mutex(); // Enable thread safety conditionally
      }
  
      void getTUIndex() {
        OptionalLockGuard LockGuard(OptMutex);
        if (TU)
           return TU;
        // Parse and populate TU
        return TU;
      }
  
    private:
      DWARFContext &DC;
      std::mutex M;
  };

This would allow us to enable and disable threading support as needed. We would need to pass in a "bool EnableThreading" to the DWARFContext on creation so it can be passed down into DWARFContextState.

> I think this would address everyone's concern:
>
> - You don't pay anything when you use the non-thread safe variant. (Dave & my concern)

Fixed by above solution with no templates needed

> - Thread safety is localized in the wrapper. (Dave & Alex's concern)

Should be easy to do, but I still think we might need a recursive mutex since we can't have multiple locks unless the data that gets constructed is guaranteed to not access any other DWARFContext variables. But I can look at the API. If we can get away with multiple locks for things that don't access other DWARFContext calls, then we can use unique locks per item, if some have interdependencies, then we need a common recursive lock for those ivars.

> - This works everywhere where a regular `DWARFContext` works. (Greg's concern)

yes

> It still doesn't solve the problem that you might be handing out other non thread safe abstractions (like the line table) but that's a bigger problem.

That can be solved by storing line tables in shared pointers and handing out a shared pointer from this API so it is actually safe.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157459