[PATCH] D157459: Make DWARFContext more thread safe.

[Alexander Yermolovich via Phabricator via llvm-commits]

ayermolo added a comment.

In D157459#4580929 <https://reviews.llvm.org/D157459#4580929>, @dblaikie wrote:

> In D157459#4578926 <https://reviews.llvm.org/D157459#4578926>, @ayermolo wrote:
>
>> In D157459#4573865 <https://reviews.llvm.org/D157459#4573865>, @dblaikie wrote:
>>
>>> Not sure how I feel about adding multithreading at this level, versus in a wrapper API for those that need it? (insert an interface (`DWARFContext`, then rename the current concrete implementation as `DWARFContextImpl`) then have a `ThreadSafeDWARFContext` that wraps (either as a template, or with a `unique_ptr<DWARFContext>` member) another `DWARFContext` and adds the locking) Though it's probably not the end of the world to add it here directly.
>>>
>>> Can probably use Class Template Argument Deduction when locking, like this:
>>>
>>>   std::unique_lock LockGuard(Mutex);
>>
>> Is there a downside just adding it here, instead of adding complexity of another class and wrapper APIs?
>
> Nothing drastic that I can think of - though perhaps a more vague "DWARFContext is already... not great (exactly which entry points you should use when, what guarantees they provide, when you need to parse things up-front versus lazily, how the memory is managed (mostly it just grows unbounded for instance/never gets cleaned up short of throwing the whole context away) and I worry that trying to figure out when/where multithreaded use is valid or not will add another dimension on an already uncertain and complex interface". But it's probably not the straw that breaks the camel's back, as it were - and adding a wrapper isn't really going to fix the underlying problems/make them especially easier to fix (it's probably a fairly broad rewrite at some point).

I see. Thank you for explanation.
Sounds like it might be nice to have, but not a blocker? As you said wrapper is not going to fix an underlying problem with this class.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157459