Make DWARFContext more thread safe.

[clayborg]

Summary

llvm-gsymutil uses a DWARFContext from multiple threads as it parses each compile unit. As it finds issues it might end up dumping a DIE to an output stream which can cause accesses to the DWARFContext from multiple threads. In llvm-gsymutil it can end up dumping a DIE from multiple threads into thread specific stream which was causing DWARFContext::getTUIndex() to be called and can crash the process.

This fix puts a recursive mutex into the DWARFContext class and makes most APIs threadsafe for access. Many of the methods in DWARFContext will check if a member variable has been filled in yet, and parse what is needed and populate a member variagle with the results. Now a mutex protects these functions.

[dblaikie]

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);

[clayborg]

Switch to using "std::unique_lock LockGuard(Mutex);" and let template argument deduction happen.

[clayborg]

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

I am fine with either way. The main issue about having a class that wraps a DWARFContext is internally the DWARFDie and DWARFUnit and possibly others, have access to their DWARFContext via the DWARFUnit and they can call some of these APIs. So wrapping won't really help for these important cases. Seems cleaner to me to have the mutex live in this class, but I am open to everyone's comments to figure out what is best for the codebase.

Can probably use Class Template Argument Deduction when locking, like this:

std::unique_lock LockGuard(Mutex);

Good idea, I fixed all locations as suggested.

[clayborg]

I ran into this issue when I had multiple threads trying to call DWARFDie::dump(...). This internally got ahold of the DWARFContext and calls DWARFContext::getTUIndex() when trying to dump a DW_AT_type attribute since it prints the DIE offset and then finds the referenced DIE to get the name and print the typename.

[ayermolo]

In 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?

[dblaikie]

In D157459#4578926, @ayermolo wrote:

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

[ayermolo]

In D157459#4580929, @dblaikie wrote:

In D157459#4578926, @ayermolo wrote:

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

[aprantl]

This patch is protecting access to the fields of DWARFContext. For all the methods returning section pointers, such as const DWARFDebugLoc *DWARFContext::getDebugLoc() what are concurrency requirements/guarantees? Do these objects all carry their own mutexes?

llvm/include/llvm/DebugInfo/DWARF/DWARFContext.h

50

Nit: ///

[clayborg]

In D157459#4585664, @aprantl wrote:

This patch is protecting access to the fields of DWARFContext.

Yes, this is designed to only protect members of DWARFContext.

For all the methods returning section pointers, such as const DWARFDebugLoc *DWARFContext::getDebugLoc() what are concurrency requirements/guarantees? Do these objects all carry their own mutexes?

If you add more than one mutex then you can deadlock your program due to A/B locking issues. The current fix in DWARFContext::getDebugLoc() ensures that a complete "Loc" member variable is populated in the current DWARFContext object and ensure any other threads wait until the first thread completes the "Loc" contents.

[bulbazord]

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.

llvm/lib/DebugInfo/DWARF/DWARFContext.cpp

1070–1094

I think either all of these getters either need to be protected or getAccelTable needs to be protected. They can all mutate these member variables.

[clayborg]

Make all accelerator table get accessors thread safe as they cache results in member variables.

[clayborg]

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

llvm/lib/DebugInfo/DWARF/DWARFContext.cpp

1070–1094

Yes, these should be protected as well.

[JDevlieghere]

To Adrian's point: what are the guarantees provided by the DWARFContext? I'm slightly worried about overhead but care more about providing clear expectations. For example, the LLVMContext explicitly provides no locking guarantees [1] and leaves it up to the consumer.

[1] https://llvm.org/doxygen/classllvm_1_1LLVMContext.html#details

llvm/include/llvm/DebugInfo/DWARF/DWARFContext.h

51

Does this have to be a recursive mutex? I was under the impression that they're somewhat frowned upon. I think the CppCoreGuidelines called it something along the lines of working around bad reasoning and adding unnecessary overhead.

[dblaikie]

In D157459#4590028, @clayborg wrote:

IMHO this is just creating thread safe code and is no different that any other patch.

Ish. It's adding a fairly subtle & incomplete invariant to a fairly complex API (with a bunch of fairly subtle invariants already). Even if this weren't about multithreading, but some other similarly subtle invariant, I think there'd still be similar reasonable concerns.

(DWARFContext's API is wide, this locking only protects it from certain multithreading, and not others - that's what I mean by subtle and incomplete - and so it's going to be difficult to keep track of/maintain this invariant going forward - as @JDevlieghere points out, I think, it'd be hard to document what DWARFContext's guarantees are after/with this patch, because I think it's still a fairly arbitrary selection of functions that can be used in a multithreaded environment)

[bulbazord]

In D157459#4590028, @clayborg wrote:

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

[clayborg]

In D157459#4590168, @JDevlieghere wrote:

To Adrian's point: what are the guarantees provided by the DWARFContext? I'm slightly worried about overhead but care more about providing clear expectations. For example, the LLVMContext explicitly provides no locking guarantees [1] and leaves it up to the consumer.

I hear you on the complexity and by no means is this a patch that adds complete thread safety to the full DWARF parser.

If an API is simple for someone to wrap up in a thread safe class that contains this object, not sure if LLVMContext is or not, then it is ok to say "I will leave this up to the user". But right now any objects owned by DWARFContext, like DWARFUnit and DWARFDie, can make requests from the DWARFContext at any time that cause crashes as I found in llvm-gsymutil when it parses DWARFDie from each DWARFUnit on separate threads. So it would be hard to wrap up any of the DWARF classes effectively. The crash I ran into was because two different threads tried to dump a DWARFDie and this causes a DWARFContext to mess itself up when calling one of the functions that are now protected by the mutex.

In D157459#4590223, @dblaikie wrote:

In D157459#4590028, @clayborg wrote:

IMHO this is just creating thread safe code and is no different that any other patch.

Ish. It's adding a fairly subtle & incomplete invariant to a fairly complex API (with a bunch of fairly subtle invariants already). Even if this weren't about multithreading, but some other similarly subtle invariant, I think there'd still be similar reasonable concerns.

(DWARFContext's API is wide, this locking only protects it from certain multithreading, and not others - that's what I mean by subtle and incomplete - and so it's going to be difficult to keep track of/maintain this invariant going forward - as @JDevlieghere points out, I think, it'd be hard to document what DWARFContext's guarantees are after/with this patch, because I think it's still a fairly arbitrary selection of functions that can be used in a multithreaded environment).

Indeed. I have a specific crash I am trying to fix here for llvm-gsymutil so it can dump DWARFDie objects into error messages. Right now, if you look at the DWARFTransformer, the current code completely just tries to avoid doing anything that might cause threading issues by doing things serially like:

• parsing all CU DIE trees up front on the main thread to avoid crashing
• parse all line tables for each CU on the main thread to avoid crashing

I was able to make this work, but it is far from efficient, but I worked around it using the "it is up to the client to handle threading issues" mantra. But when I ran into problems being able to dump DIEs, that is something I can't do up front. The issue I ran into was the DWARFDie class was asking for the TUIndex from the DWARFContext, so I tried to fix this by parsing all of the TUIndexes on the main thread, but it still crashed due to split DWARF as each .dwo file had its own DWARFContext. So the only solution was to then parse all compile units up front, then grab the DWARFContext from each .dwo file and call all of the needed functions on it to avoid crashes (like calling the getTUIndex() method) and hope this is enough to avoid crashing. So it got quite messy and I ended up doing a ton of work on the main thread. This fix works and removes all the need for any pre-parsing, so it was much cleaner.

In D157459#4590470, @bulbazord wrote:

In D157459#4590028, @clayborg wrote:

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

Agreed. It isn't perfect, but it does allow llvm-gsymutil to parse DWARF on multiple threads much more efficiently without having to do the "I must do X, Y and Z on the main thread before allowing all threads to parse the DWARF".

If we don't want to tackle thread safety with this patch I can modify llvm-gsymutil to not print out the DWARFDie information in llvm-gsymutil on the threads that parse the DWARF to avoid the crash, but I would really like to be able to if possible.

[clayborg]

I know this patch is far from adding complete thread safety to this class, but it does address some fairly simple issues that can cause things to crash. This patch protects any function that has the format of:

if (MemberVar)
  return MemberVar;
// Initialize MemberVar and then do expensive work to populate MemberVar 
return MemberVar;

The expensive work to populate can take some time and protecting this with a mutex seems like the right thing to do. Any other threads can easily use the member variable that gets returned in an incomplete state.

Are there any concrete steps I can take to help make this patch work for everyone? I have heard concerns but no real steps or tips on what it would take for people to move this patch along.

[bulbazord]

In D157459#4596356, @clayborg wrote:

I know this patch is far from adding complete thread safety to this class, but it does address some fairly simple issues that can cause things to crash. This patch protects any function that has the format of:

if (MemberVar)
  return MemberVar;
// Initialize MemberVar and then do expensive work to populate MemberVar 
return MemberVar;

The expensive work to populate can take some time and protecting this with a mutex seems like the right thing to do. Any other threads can easily use the member variable that gets returned in an incomplete state.

Are there any concrete steps I can take to help make this patch work for everyone? I have heard concerns but no real steps or tips on what it would take for people to move this patch along.

I can't speak for everyone, but here are a few concrete ways I think this change can be improved:

• This change is probably a regression performance-wise. You have a recursive mutex locking access to all member variables of a given DWARFContext on every access. One thread may try to access the already-calculated TUIndex while another is trying to do expensive work to calculate Abbrev. While this technically will give you correct results, I think we can do better in terms of performance. I'm not 100% sure which members depend on which, but it would be excellent if we could figure that out and guard every member behind its own mutex so we're not stalling on unrelated requests. The only downside I can think of is if we have mutually-dependent requests where thread A locks mutex 1 and then 2 while thread B locks mutex 2 and then 1 (deadlock). Is this possible to do?
• 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.
• 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.

What do y'all think @JDevlieghere @dblaikie @aprantl?

[clayborg]

In D157459#4596842, @bulbazord wrote:

In D157459#4596356, @clayborg wrote:

Are there any concrete steps I can take to help make this patch work for everyone? I have heard concerns but no real steps or tips on what it would take for people to move this patch along.

I can't speak for everyone, but here are a few concrete ways I think this change can be improved:

• This change is probably a regression performance-wise. You have a recursive mutex locking access to all member variables of a given DWARFContext on every access. One thread may try to access the already-calculated TUIndex while another is trying to do expensive work to calculate Abbrev. While this technically will give you correct results, I think we can do better in terms of performance. I'm not 100% sure which members depend on which, but it would be excellent if we could figure that out and guard every member behind its own mutex so we're not stalling on unrelated requests. The only downside I can think of is if we have mutually-dependent requests where thread A locks mutex 1 and then 2 while thread B locks mutex 2 and then 1 (deadlock). Is this possible to do?

I worry about thread deadlocking due to the A/B locking as you stated in these kinds of requests. We went this road early in LLDB's development but as the code evolved and people added more locks we ended up deadlocking. We used to have a lock on the lldb_private::Module and one on lldb_private::ObjectFile and one in lldb_private::SymbolFile and ran into deadlocks as people added new code. We eventually made all of these classes, that are related, use a single mutex and things were much better.

The main reason for the recursive nature is if any of the currently protected methods call another you can and will deadlock your program and I think some of the current calls do call others IIRC.

I

• 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?

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

[ayermolo]

I am somewhat confused about how this is a performance regression. Doesn't sound like it currently working in multi threaded environment anyway. If it is implemented as is, I don't believe most tools that process debug information are multi threaded, and even if they are majority of time is spent actually parsing debug information.

[bulbazord]

In D157459#4596891, @clayborg wrote:

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

[bulbazord]

In D157459#4596989, @ayermolo wrote:

I am somewhat confused about how this is a performance regression. Doesn't sound like it currently working in multi threaded environment anyway. If it is implemented as is, I don't believe most tools that process debug information are multi threaded, and even if they are majority of time is spent actually parsing debug information.

I have not measured this change myself, so I can't definitely claim it is 100% a performance regression. However, there are 2 reasons I suspect this:

1. recursive_mutex is usually less performant because every time you try to acquire a lock with it, you need to do extra work to determine if the mutex is already acquired on the calling thread. How much slower, I cannot say as this is probably dependent on many factors (hardware, os, stdlib implementation, etc). I won't say not to use recursive_mutex because sometimes the use is completely justified and unavoidable. We should be aware of the penalty though.
2. One mutex locking the access to every member variable means that only one member can be parsed at a time. If one thread calls DWARFContext::getDebugAbbrev while another tries to parse an unrelated section, you're leaving performance on the table. Maybe that's not a priority or not the point, but if we're going to go this route we should be aware of the tradeoffs.

[bulbazord]

I understand my suggestions may not be trivial to implement, perhaps we could benefit from starting a discussion or RFC on the LLVM discourse? Perhaps others have ideas about how to approach this while addressing our concerns.

[clayborg]

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.

It is one possible solution yes.

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

if DWARFContext was a class that was used on its own this would make more sense to have us allow it to claim to be not usable in threaded code. The problem is its use is intertwined in many of the DWARF classes. The DWARFUnit and the DWARFDie and other classes access this from within their own code so stating that DWARFContext is not threadsafe and your need to guard access to it on your own can't and doesn't work, this is what we have now. When we have a split DWARF binary we have 1 executable and N number of .dwo files which each have their own DWARFContext, which can and will request things from the main DWARFContext if and when needed (like access to the .debug_addr section, TUIndex, CUIndex, etc) and can cause crashes.

[JDevlieghere]

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;
};

I think this would address everyone's concern:

• You don't pay anything when you use the non-thread safe variant. (Dave & my concern)
• Thread safety is localized in the wrapper. (Dave & Alex's concern)
• This works everywhere where a regular DWARFContext works. (Greg's concern)

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.

[clayborg]

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

[clayborg]

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

[JDevlieghere]

In 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, @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.

[bulbazord]

In D157459#4604989, @clayborg wrote:

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

I'm still interested in seeing this happen, parsing DWARF in concurrent contexts is important. I don't think the current proposed solution addresses my concerns, but I don't particularly care if you go with my suggestion or Jonas's. I'd be interested in hearing what @dblaikie has to say.

[clayborg]

Ok, I wanted to try and protect just the call to "const DWARFUnitIndex &DWARFContext::getCUIndex()" using examples people have mentioned and I came up with this solution to see if anyone likes it:

In DWARFContext.h:

class DWARFContextStateImpl {
public:
  virtual ~DWARFContextStateImpl() = default;
  virtual const DWARFUnitIndex &getCUIndex(DWARFContext &D) = 0;
};

class DWARFContextStateSingleThreaded : public DWARFContextStateImpl {
  std::unique_ptr<DWARFUnitIndex> CUIndex;
public:
  const DWARFUnitIndex &getCUIndex(DWARFContext &D) override;
};

class DWARFContextStateMultiThreaded : public DWARFContextStateSingleThreaded {
  std::recursive_mutex Mutex;
public:
  const DWARFUnitIndex &getCUIndex(DWARFContext &D) override;
};

// DWARFContext member variables
...
// std::unique_ptr<DWARFUnitIndex> CUIndex; // This was the old ivar which is now contained in a subclass of DWARFContextStateImpl
std::unique_ptr<DWARFContextStateImpl> State;
...

Then to implement this we can do the following in the DWARContext.cpp file:

const DWARFUnitIndex &DWARFContext::getCUIndex() {
  return State->getCUIndex(*this);
}

const DWARFUnitIndex &
DWARFContext::DWARFContextStateSingleThreaded::getCUIndex(DWARFContext &D) {
  if (CUIndex)
    return *CUIndex;

  DataExtractor CUIndexData(D.DObj->getCUIndexSection(), D.isLittleEndian(), 0);
  CUIndex = std::make_unique<DWARFUnitIndex>(DW_SECT_INFO);
  bool IsParseSuccessful = CUIndex->parse(CUIndexData);
  if (IsParseSuccessful)
    fixupIndex(D, *CUIndex);
  return *CUIndex;
}

const DWARFUnitIndex &
DWARFContext::DWARFContextStateMultiThreaded::getCUIndex(DWARFContext &D) {
  std::unique_lock LockGuard(Mutex);
  return DWARFContextStateSingleThreaded::getCUIndex(D);
}

Then if we enable threading with a bool in the constructor:

DWARFContext::DWARFContext(..., bool EnableMultiThreading) {
  if (EnableMultiThreading)
    State.reset(new DWARFContextStateMultiThreaded());
  else
    State.reset(new DWARFContextStateSingleThreaded());
}

If we like this idea, I can add all member variables that we want the thread protect into DWARFContextStateSingleThreaded and implement the wrappers for each accessor. Does this kind of solution work for anyone?

[JDevlieghere]

In D157459#4612553, @clayborg wrote:

Ok, I wanted to try and protect just the call to "const DWARFUnitIndex &DWARFContext::getCUIndex()" using examples people have mentioned and I came up with this solution to see if anyone likes it:

In DWARFContext.h:

class DWARFContextStateImpl {
public:
  virtual ~DWARFContextStateImpl() = default;
  virtual const DWARFUnitIndex &getCUIndex(DWARFContext &D) = 0;
};

class DWARFContextStateSingleThreaded : public DWARFContextStateImpl {
  std::unique_ptr<DWARFUnitIndex> CUIndex;
public:
  const DWARFUnitIndex &getCUIndex(DWARFContext &D) override;
};

class DWARFContextStateMultiThreaded : public DWARFContextStateSingleThreaded {
  std::recursive_mutex Mutex;
public:
  const DWARFUnitIndex &getCUIndex(DWARFContext &D) override;
};

// DWARFContext member variables
...
// std::unique_ptr<DWARFUnitIndex> CUIndex; // This was the old ivar which is now contained in a subclass of DWARFContextStateImpl
std::unique_ptr<DWARFContextStateImpl> State;
...

Then to implement this we can do the following in the DWARContext.cpp file:

const DWARFUnitIndex &DWARFContext::getCUIndex() {
  return State->getCUIndex(*this);
}

const DWARFUnitIndex &
DWARFContext::DWARFContextStateSingleThreaded::getCUIndex(DWARFContext &D) {
  if (CUIndex)
    return *CUIndex;

  DataExtractor CUIndexData(D.DObj->getCUIndexSection(), D.isLittleEndian(), 0);
  CUIndex = std::make_unique<DWARFUnitIndex>(DW_SECT_INFO);
  bool IsParseSuccessful = CUIndex->parse(CUIndexData);
  if (IsParseSuccessful)
    fixupIndex(D, *CUIndex);
  return *CUIndex;
}

const DWARFUnitIndex &
DWARFContext::DWARFContextStateMultiThreaded::getCUIndex(DWARFContext &D) {
  std::unique_lock LockGuard(Mutex);
  return DWARFContextStateSingleThreaded::getCUIndex(D);
}

Then if we enable threading with a bool in the constructor:

DWARFContext::DWARFContext(..., bool EnableMultiThreading) {
  if (EnableMultiThreading)
    State.reset(new DWARFContextStateMultiThreaded());
  else
    State.reset(new DWARFContextStateSingleThreaded());
}

If we like this idea, I can add all member variables that we want the thread protect into DWARFContextStateSingleThreaded and implement the wrappers for each accessor. Does this kind of solution work for anyone?

I like this approach 👍

[bulbazord]

In D157459#4612553, @clayborg wrote:

Ok, I wanted to try and protect just the call to "const DWARFUnitIndex &DWARFContext::getCUIndex()" using examples people have mentioned and I came up with this solution to see if anyone likes it:

In DWARFContext.h:

class DWARFContextStateImpl {
public:
  virtual ~DWARFContextStateImpl() = default;
  virtual const DWARFUnitIndex &getCUIndex(DWARFContext &D) = 0;
};

class DWARFContextStateSingleThreaded : public DWARFContextStateImpl {
  std::unique_ptr<DWARFUnitIndex> CUIndex;
public:
  const DWARFUnitIndex &getCUIndex(DWARFContext &D) override;
};

class DWARFContextStateMultiThreaded : public DWARFContextStateSingleThreaded {
  std::recursive_mutex Mutex;
public:
  const DWARFUnitIndex &getCUIndex(DWARFContext &D) override;
};

// DWARFContext member variables
...
// std::unique_ptr<DWARFUnitIndex> CUIndex; // This was the old ivar which is now contained in a subclass of DWARFContextStateImpl
std::unique_ptr<DWARFContextStateImpl> State;
...

Then to implement this we can do the following in the DWARContext.cpp file:

const DWARFUnitIndex &DWARFContext::getCUIndex() {
  return State->getCUIndex(*this);
}

const DWARFUnitIndex &
DWARFContext::DWARFContextStateSingleThreaded::getCUIndex(DWARFContext &D) {
  if (CUIndex)
    return *CUIndex;

  DataExtractor CUIndexData(D.DObj->getCUIndexSection(), D.isLittleEndian(), 0);
  CUIndex = std::make_unique<DWARFUnitIndex>(DW_SECT_INFO);
  bool IsParseSuccessful = CUIndex->parse(CUIndexData);
  if (IsParseSuccessful)
    fixupIndex(D, *CUIndex);
  return *CUIndex;
}

const DWARFUnitIndex &
DWARFContext::DWARFContextStateMultiThreaded::getCUIndex(DWARFContext &D) {
  std::unique_lock LockGuard(Mutex);
  return DWARFContextStateSingleThreaded::getCUIndex(D);
}

Then if we enable threading with a bool in the constructor:

DWARFContext::DWARFContext(..., bool EnableMultiThreading) {
  if (EnableMultiThreading)
    State.reset(new DWARFContextStateMultiThreaded());
  else
    State.reset(new DWARFContextStateSingleThreaded());
}

If we like this idea, I can add all member variables that we want the thread protect into DWARFContextStateSingleThreaded and implement the wrappers for each accessor. Does this kind of solution work for anyone?

This approach works for me. I know it is more complex than what exists today so I appreciate you taking the time to address our concerns.

[clayborg]

Thanks for the feedback, I will update this patch with the updated changes soon!

[clayborg]

Created a DWARFContextState pure virtual class that contains all of the thread sensitive member variables for the DWARFContextClass.

Multi threaded access can be enabled by passing boolean to enable it in the DWARFContext constructor and create static functions. If threading is disabled we instantiate a SingleThreadedState class that contains the ivars and no mutex for thread safety. If we enable threading, a ThreadSafeState is created that uses a mutex to protect access to ivars and calls the work functions in the SingleThreadedState.

[ayermolo]

@dblaikie @bulbazord @JDevlieghere WDYT of this approach?
@clayborg One suggestion, maybe move EnableMultiThreading optional paramenter to the end of argument list? That way constructors in other projects don't need to be modified? Right now this will break LLD and BOLT builds.

[JDevlieghere]

This looks fine to me. My only qualm is with the use of Single/MultiThreading. This could leave someone with the impression that we'll do work in parallel in the DWARFContext. Th is patch is all about thread safety. We have some prior art in llvm that uses the ThreadSafe and you used that for the thread safe context as well. I left some concrete suggestions inline.

llvm/include/llvm/DebugInfo/DWARF/DWARFContext.h

120

How about bool ThreadSafe?

llvm/lib/DebugInfo/DWARF/DWARFContext.cpp

246

How about UnsafeDWARFContextState?

[clayborg]

llvm/include/llvm/DebugInfo/DWARF/DWARFContext.h

120

works for me

llvm/lib/DebugInfo/DWARF/DWARFContext.cpp

246

Following your previous suggestion maybe "ThreadUnsafeDWARFContextState"?

[JDevlieghere]

llvm/lib/DebugInfo/DWARF/DWARFContext.cpp

246

Sounds good!

[clayborg]

• Rename single threaded DWARFContextState to ThreadUnsafeState
• Move default paramter for DWARFContext and DWARFContext::create functions to the end
• Rename EnableMultiThreading to ThreadSafe

[clayborg]

Add getTypeUnitMap to the ThreadSafeContext, it was missing.

[JDevlieghere]

LGTM!

[bulbazord]

My concerns were addressed, thanks for working on this!

llvm/include/llvm/DebugInfo/DWARF/DWARFContext.h

 #include "llvm/DebugInfo/DWARF/DWARFUnit.h"
 #include "llvm/Object/Binary.h"
 #include "llvm/Object/ObjectFile.h"
 #include "llvm/Support/DataExtractor.h"
 #include "llvm/Support/Error.h"
 #include "llvm/TargetParser/Host.h"
 #include <cstdint>
 #include <memory>
+#include <mutex>
 
 namespace llvm {
 
 class MemoryBuffer;
 class AppleAcceleratorTable;
 class DWARFCompileUnit;
 class DWARFDebugAbbrev;
 class DWARFDebugAranges;
 class DWARFDebugFrame;
 class DWARFDebugLoc;
 class DWARFDebugMacro;
 class DWARFDebugNames;
 class DWARFGdbIndex;
 class DWARFTypeUnit;
 class DWARFUnitIndex;
 
 /// DWARFContext
 /// This data structure is the top level entity that deals with dwarf debug
 /// information parsing. The actual data is supplied through DWARFObj.
 class DWARFContext : public DIContext {
   DWARFUnitVector NormalUnits;
+  // Mutex protecting multi-threaded access to the DWARFContext.
+  std::recursive_mutex Mutex;
   std::optional<DenseMap<uint64_t, DWARFTypeUnit *>> NormalTypeUnits;
   std::unique_ptr<DWARFUnitIndex> CUIndex;
   std::unique_ptr<DWARFGdbIndex> GdbIndex;
   std::unique_ptr<DWARFUnitIndex> TUIndex;
   std::unique_ptr<DWARFDebugAbbrev> Abbrev;
   std::unique_ptr<DWARFDebugLoc> Loc;
   std::unique_ptr<DWARFDebugAranges> Aranges;
   std::unique_ptr<DWARFDebugLine> Line;
class DWARFContext : public DIContext {
   };
 
   // When set parses debug_info.dwo/debug_abbrev.dwo manually and populates CU
   // Index, and TU Index for DWARF5.
   bool ParseCUTUIndexManually = false;
 
 public:
   DWARFContext(std::unique_ptr<const DWARFObject> DObj,
                std::string DWPName = "",
                std::function<void(Error)> RecoverableErrorHandler =
                    WithColor::defaultErrorHandler,
                std::function<void(Error)> WarningHandler =
                    WithColor::defaultWarningHandler);
   ~DWARFContext() override;
 
   DWARFContext(DWARFContext &) = delete;
   DWARFContext &operator=(DWARFContext &) = delete;

llvm/lib/DebugInfo/DWARF/DWARFContext.cpp

if (Error Err = AddrTable.extract(AddrData, &Offset, Version, AddrSize,
       }
       break;
     }
     AddrTable.dump(OS, DumpOpts);
   }
 }
 
 // Dump the .debug_rnglists or .debug_rnglists.dwo section (DWARF v5).
 static void dumpRnglistsSection(
     raw_ostream &OS, DWARFDataExtractor &rnglistData,
     llvm::function_ref<std::optional<object::SectionedAddress>(uint32_t)>
         LookupPooledAddress,
     DIDumpOptions DumpOpts) {
   uint64_t Offset = 0;
   while (rnglistData.isValidOffset(Offset)) {
     llvm::DWARFDebugRnglistTable Rnglists;
     uint64_t TableOffset = Offset;
void fixupIndex(const DWARFObject &DObj, DWARFContext &C,
                 DWARFUnitIndex &Index) {
   if (Index.getVersion() < 5)
     fixupIndexV4(DObj, C, Index);
   else
     fixupIndexV5(DObj, C, Index);
 }
 
 const DWARFUnitIndex &DWARFContext::getCUIndex() {
+  std::unique_lock LockGuard(Mutex);
   if (CUIndex)
     return *CUIndex;
 
   DataExtractor CUIndexData(DObj->getCUIndexSection(), isLittleEndian(), 0);
   CUIndex = std::make_unique<DWARFUnitIndex>(DW_SECT_INFO);
   bool IsParseSuccessful = CUIndex->parse(CUIndexData);
   if (IsParseSuccessful)
     fixupIndex(*DObj, *this, *CUIndex);
   return *CUIndex;
 }
 
 const DWARFUnitIndex &DWARFContext::getTUIndex() {
+  std::unique_lock LockGuard(Mutex);
   if (TUIndex)
     return *TUIndex;
 
   DataExtractor TUIndexData(DObj->getTUIndexSection(), isLittleEndian(), 0);
   TUIndex = std::make_unique<DWARFUnitIndex>(DW_SECT_EXT_TYPES);
   bool isParseSuccessful = TUIndex->parse(TUIndexData);
   // If we are parsing TU-index and for .debug_types section we don't need
   // to do anything.
   if (isParseSuccessful && TUIndex->getVersion() != 2)
     fixupIndex(*DObj, *this, *TUIndex);
   return *TUIndex;
 }
 
 DWARFGdbIndex &DWARFContext::getGdbIndex() {
+  std::unique_lock LockGuard(Mutex);
   if (GdbIndex)
     return *GdbIndex;
 
   DataExtractor GdbIndexData(DObj->getGdbIndexSection(), true /*LE*/, 0);
   GdbIndex = std::make_unique<DWARFGdbIndex>();
   GdbIndex->parse(GdbIndexData);
   return *GdbIndex;
 }
 
 const DWARFDebugAbbrev *DWARFContext::getDebugAbbrev() {
+  std::unique_lock LockGuard(Mutex);
   if (Abbrev)
     return Abbrev.get();
 
   DataExtractor abbrData(DObj->getAbbrevSection(), isLittleEndian(), 0);
   Abbrev = std::make_unique<DWARFDebugAbbrev>(abbrData);
   return Abbrev.get();
 }
 
 const DWARFDebugAbbrev *DWARFContext::getDebugAbbrevDWO() {
+  std::unique_lock LockGuard(Mutex);
   if (AbbrevDWO)
     return AbbrevDWO.get();
 
   DataExtractor abbrData(DObj->getAbbrevDWOSection(), isLittleEndian(), 0);
   AbbrevDWO = std::make_unique<DWARFDebugAbbrev>(abbrData);
   return AbbrevDWO.get();
 }
 
 const DWARFDebugLoc *DWARFContext::getDebugLoc() {
+  std::unique_lock LockGuard(Mutex);
   if (Loc)
     return Loc.get();
 
   // Assume all units have the same address byte size.
   auto LocData =
       getNumCompileUnits()
           ? DWARFDataExtractor(*DObj, DObj->getLocSection(), isLittleEndian(),
                                getUnitAtIndex(0)->getAddressByteSize())
           : DWARFDataExtractor("", isLittleEndian(), 0);
   Loc.reset(new DWARFDebugLoc(std::move(LocData)));
   return Loc.get();
 }
 
 const DWARFDebugAranges *DWARFContext::getDebugAranges() {
+  std::unique_lock LockGuard(Mutex);
   if (Aranges)
     return Aranges.get();
 
   Aranges.reset(new DWARFDebugAranges());
   Aranges->generate(this);
   return Aranges.get();
 }
 
 Expected<const DWARFDebugFrame *> DWARFContext::getDebugFrame() {
+  std::unique_lock LockGuard(Mutex);
   if (DebugFrame)
     return DebugFrame.get();
 
   const DWARFSection &DS = DObj->getFrameSection();
 
   // There's a "bug" in the DWARFv3 standard with respect to the target address
   // size within debug frame sections. While DWARF is supposed to be independent
   // of its container, FDEs have fields with size being "target address size",
Expected<const DWARFDebugFrame *> DWARFContext::getDebugFrame() {
   if (Error E = DF->parse(DebugFrameData))
     return std::move(E);
 
   DebugFrame.swap(DF);
   return DebugFrame.get();
 }
 
 Expected<const DWARFDebugFrame *> DWARFContext::getEHFrame() {
+  std::unique_lock LockGuard(Mutex);
   if (EHFrame)
     return EHFrame.get();
 
   const DWARFSection &DS = DObj->getEHFrameSection();
   DWARFDataExtractor DebugFrameData(*DObj, DS, isLittleEndian(),
                                     DObj->getAddressSize());
 
   auto DF =
       std::make_unique<DWARFDebugFrame>(getArch(), /*IsEH=*/true, DS.Address);
   if (Error E = DF->parse(DebugFrameData))
     return std::move(E);
   DebugFrame.swap(DF);
   return DebugFrame.get();
 }
 
 const DWARFDebugMacro *DWARFContext::getDebugMacro() {
+  std::unique_lock LockGuard(Mutex);
   if (!Macro)
     Macro = parseMacroOrMacinfo(MacroSection);
   return Macro.get();
 }
 
 const DWARFDebugMacro *DWARFContext::getDebugMacroDWO() {
+  std::unique_lock LockGuard(Mutex);
   if (!MacroDWO)
     MacroDWO = parseMacroOrMacinfo(MacroDwoSection);
   return MacroDWO.get();
 }
 
 const DWARFDebugMacro *DWARFContext::getDebugMacinfo() {
+  std::unique_lock LockGuard(Mutex);
   if (!Macinfo)
     Macinfo = parseMacroOrMacinfo(MacinfoSection);
   return Macinfo.get();
 }
 
 const DWARFDebugMacro *DWARFContext::getDebugMacinfoDWO() {
+  std::unique_lock LockGuard(Mutex);
   if (!MacinfoDWO)
     MacinfoDWO = parseMacroOrMacinfo(MacinfoDwoSection);
   return MacinfoDWO.get();
 }
 
 template <typename T>
 static T &getAccelTable(std::unique_ptr<T> &Cache, const DWARFObject &Obj,
                         const DWARFSection &Section, StringRef StringSection,
                         bool IsLittleEndian) {
   if (Cache)
     return *Cache;
   DWARFDataExtractor AccelSection(Obj, Section, IsLittleEndian, 0);
   DataExtractor StrData(StringSection, IsLittleEndian, 0);
   Cache.reset(new T(AccelSection, StrData));
   if (Error E = Cache->extract())
     llvm::consumeError(std::move(E));
   return *Cache;
 }
 
 const DWARFDebugNames &DWARFContext::getDebugNames() {
   return getAccelTable(Names, *DObj, DObj->getNamesSection(),
                        DObj->getStrSection(), isLittleEndian());
 }
 
 const AppleAcceleratorTable &DWARFContext::getAppleNames() {
   return getAccelTable(AppleNames, *DObj, DObj->getAppleNamesSection(),
                        DObj->getStrSection(), isLittleEndian());
 }
 
 const AppleAcceleratorTable &DWARFContext::getAppleTypes() {
   return getAccelTable(AppleTypes, *DObj, DObj->getAppleTypesSection(),
                        DObj->getStrSection(), isLittleEndian());
 }
 
 const AppleAcceleratorTable &DWARFContext::getAppleNamespaces() {
   return getAccelTable(AppleNamespaces, *DObj,
                        DObj->getAppleNamespacesSection(),
                        DObj->getStrSection(), isLittleEndian());
 }
 
 const AppleAcceleratorTable &DWARFContext::getAppleObjC() {
   return getAccelTable(AppleObjC, *DObj, DObj->getAppleObjCSection(),
                        DObj->getStrSection(), isLittleEndian());
 }
 
 const DWARFDebugLine::LineTable *
 DWARFContext::getLineTableForUnit(DWARFUnit *U) {
   Expected<const DWARFDebugLine::LineTable *> ExpectedLineTable =
       getLineTableForUnit(U, WarningHandler);
   if (!ExpectedLineTable) {
     WarningHandler(ExpectedLineTable.takeError());
     return nullptr;
   }
   return *ExpectedLineTable;
 }
 
 Expected<const DWARFDebugLine::LineTable *> DWARFContext::getLineTableForUnit(
     DWARFUnit *U, function_ref<void(Error)> RecoverableErrorHandler) {
+  std::unique_lock LockGuard(Mutex);
   if (!Line)
     Line.reset(new DWARFDebugLine);
 
   auto UnitDIE = U->getUnitDIE();
   if (!UnitDIE)
     return nullptr;
 
   auto Offset = toSectionOffset(UnitDIE.find(DW_AT_stmt_list));
Expected<const DWARFDebugLine::LineTable *> DWARFContext::getLineTableForUnit(
   // We have to parse it first.
   DWARFDataExtractor lineData(*DObj, U->getLineSection(), isLittleEndian(),
                               U->getAddressByteSize());
   return Line->getOrParseLineTable(lineData, stmtOffset, *this, U,
                                    RecoverableErrorHandler);
 }
 
 void DWARFContext::clearLineTableForUnit(DWARFUnit *U) {
+  std::unique_lock LockGuard(Mutex);
   if (!Line)
     return;
 
   auto UnitDIE = U->getUnitDIE();
   if (!UnitDIE)
     return;
 
   auto Offset = toSectionOffset(UnitDIE.find(DW_AT_stmt_list));
   if (!Offset)
     return;
 
   uint64_t stmtOffset = *Offset + U->getLineTableOffset();
   Line->clearLineTable(stmtOffset);
 }
 
 void DWARFContext::parseNormalUnits() {
+  std::unique_lock LockGuard(Mutex);
   if (!NormalUnits.empty())
     return;
   DObj->forEachInfoSections([&](const DWARFSection &S) {
     NormalUnits.addUnitsForSection(*this, S, DW_SECT_INFO);
   });
   NormalUnits.finishedInfoUnits();
   DObj->forEachTypesSections([&](const DWARFSection &S) {
     NormalUnits.addUnitsForSection(*this, S, DW_SECT_EXT_TYPES);
   });
 }
 
 void DWARFContext::parseDWOUnits(bool Lazy) {
+  std::unique_lock LockGuard(Mutex);
   if (!DWOUnits.empty())
     return;
   DObj->forEachInfoDWOSections([&](const DWARFSection &S) {
     DWOUnits.addUnitsForDWOSection(*this, S, DW_SECT_INFO, Lazy);
   });
   DWOUnits.finishedInfoUnits();
   DObj->forEachTypesDWOSections([&](const DWARFSection &S) {
     DWOUnits.addUnitsForDWOSection(*this, S, DW_SECT_EXT_TYPES, Lazy);
for (uint32_t i = 0, n = InlinedChain.size(); i != n; i++) {
     }
     InliningInfo.addFrame(Frame);
   }
   return InliningInfo;
 }
 
 std::shared_ptr<DWARFContext>
 DWARFContext::getDWOContext(StringRef AbsolutePath) {
+  std::unique_lock LockGuard(Mutex);
   if (auto S = DWP.lock()) {
     DWARFContext *Ctxt = S->Context.get();
     return std::shared_ptr<DWARFContext>(std::move(S), Ctxt);
   }
 
   std::weak_ptr<DWOFile> *Entry = &DWOFiles[AbsolutePath];
 
   if (auto S = Entry->lock()) {