[PATCH] D157459: Make DWARFContext more thread safe.

[Alex Langford via Phabricator via llvm-commits]

bulbazord added a comment.

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

> In D157459#4589175 <https://reviews.llvm.org/D157459#4589175>, @bulbazord wrote:
>
>> I think I'm somewhat on the same page as David here. The big concern I have is that although this probably works today, it's not very resilient or future-proof. It's difficult to guarantee future contributors or future reviewers will know or remember to guard member variable mutations with the mutex. I won't block this patch because I know the work to refactor the abstraction to be concurrency-safe is non-trivial, but I am concerned about the long term implications.
>
> IMHO this is just creating thread safe code and is no different that any other patch. Unless there is a mandate in LLVM code to not create thread safe code for performance reasons? The implications would be that we, as reviewers, would be aware of this and any modifications to this file we would just tell people to use a LockGuard.

Apologies if I came across as pointed. I appreciate that you are working to make LLVM more thread-safe and I don't think any mandate that would prioritize performance over safety would stand up to scrutiny. I agree that the implication would be that reviewers should and hopefully would understand that access to member variables of `DWARFContext` need to be synchronized and guarded with a mutex. What I wanted to convey was what @JDevlieghere and @dblaikie are saying.

I also think that in the case of `DWARFDebugLine::LineTable` a mutex is not enough because you can pass out a `const DWARFDebugLine::LineTable *` to somebody on one thread and on another thread right afterwards you can call `DWARFDebugLine::clearLineTableForUnit`, meaning the first thread is left holding a dangling pointer. Future changes to DWARFContext and introduce similar issues with other member variables and it may be difficult to notice.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157459