[PATCH] D157459: Make DWARFContext more thread safe.

Wed Aug 16 09:54:18 PDT 2023

clayborg added a comment.

In D157459#4590168 <https://reviews.llvm.org/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 <https://reviews.llvm.org/D157459#4590223>, @dblaikie wrote:

> In D157459#4590028 <https://reviews.llvm.org/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 <https://reviews.llvm.org/D157459#4590470>, @bulbazord wrote:

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

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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157459