[PATCH] D157459: Make DWARFContext more thread safe.

[Jonas Devlieghere via Phabricator via llvm-commits]

JDevlieghere added a comment.

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

> 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 current implementation uses one mutex for all of them so I'd expect this to do the same.

> 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.
>
>> - You don't pay anything when you use the non-thread safe variant. (Dave & my concern)
>
> Fixed by above solution with no templates needed

Almost, but you'd still need to check if you need to lock the Mutex on every call, right? Definitely cheaper than locking the real mutex but not entirely free.

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

Yes, that protects the pointer, but not the abstraction it points to. My original comment was a reference to my big picture question of what threading guarantees this class provides, to which "it protects the pointers" is a totally fair answer.

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

> Any interest in this happening, or should I abandon and change llvm-gsymutil to just avoid threading?

I've been known to like templates so it won't be a surprise that I prefer that solution, so I'd like others to chime in and express their opinions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157459