I would like to get this merged in if possible. The work discussed for flushing out future deadlocks should also be done, but deserves its own ticket.

I have been mega busy for the last while and intend to contribute more to this project when my situation stabilizes in like two weeks. Whenever you have time, I would definitely be interested to hear what sort of challenges you have come across through experimentation. An issue which I have suspected from the onset is actually determining all that needs to be protected by DUChain's lock and those which don't since KDevelop source makes use of these locks so liberally. I'm wondering if in your experimentation there are open questions about the DUChain locking scope.

Okay, that all makes sense to me. Also, in spite of what I said earlier, I think the explicit DUChain locking should be done and there eventually should be some work done to determine which objects will only interact with one specific DUChain per instance of that object, i.e. an object that should reference a specific DUChain, e.g. ClangSupport. We could make the first pass at this involve replacing all of those DUChainReadLocker lock; lines with DUChainReaderLocker lock(DUChain::lock());, but ideally we would eventually come to some understanding about which objects are instantiated within a specific DUChain context such that multiple instances can coexist with entirely independent DUChain interactions. The eventual use case for this is something like T11223 which at this point has so very many unknowns that it would be difficult to start tailoring changes like proposed here to that end. At this stage with the desired changes loosely described above, I am inclined to think that there needs to be some explicitly stated semantics of DUChain::lock(), i.e. it locks all DUChain instances. If we eventually start adding support for multiple DUChain instances we would at least not have to worry about any of these existing locks being overly permissive given the suggested default semantics. I don't know if anything I have just said changes anything about how we would go about applying Clang Thread Safety annotations to this project, but I suspect that it does. Thoughts?

Even in contexts where you are specifying the struct itself, it is still matching against the typedef.

In D22182#492046, @aaronpuchert wrote:

I don't find it terribly verbose. Also there are other locks used in the du-chain code, and I think it doesn't hurt to be a little bit more explicit.

In D22182#491022, @aaronpuchert wrote:

If we make the lock explicit, we can just annotate the constructor with ACQUIRE[_SHARED](duChainLock) (using the macros from the docs).

I think this DUChain::lock() probably still needs to be exposed as a variable or perhaps it needs to be annotated some way because I suspect that clang is not going to figure out that every value returned from this function is the exact same lock. If I start adding annotations to functions that expect the effectively global DUChain lock to be held beforehand, I would have to write something like REQUIRES(DUChain::lock()) which as I said likely doesn't do what I want.

In D22182#490536, @aaronpuchert wrote:

Hmm, you're right. Maybe we can get rid of that when we're certain through static checking that this isn't needed anymore.

Well at least the destructor will need that behaviour because it is certainly sane behaviour for someone to unlock a lock somewhere along the line and then leave it unlocked till the end of the function, e.g. the code I included with this ticket. There didn't seem to be a way that I could scope that lock because this lock was needed for a local constructor invocation.

So it might make sense to try to refactor this DUChain locking abstraction entirely and possibly change all call-sites to respect the new interface. The first obvious question is how do you make this locker thing achieve the same end. It is somewhat convenient that the destructor idempotent behaviour is the way it is. I don't want to invoke lock.lock() at the end of every function to satisfy that destructor and I also wouldn't want to get rid of that destructor behaviour.

In D22182#490533, @aaronpuchert wrote:

Lock and unlock shouldn't be idempotent.

In D22182#490522, @aaronpuchert wrote:

In D22182#489873, @mswan wrote:

This should definitely be tried and I would be interested in the task. There is definitely quite alot of stuff that needs to be annotated to make this work so it might need several hands working on it.

I'd be delighted and would happily work with you on this. I worked on the Thread Safety Analysis feature for a while.

The one problem with adopting that system in KDevelop is that we copy DUChain::lock() into each instance of DUChain{Read,Write}Locker which means that our annotations wouldn't see the DUChainReadLocker in one instance as the same lock in another instance.

I don't think the lock is copied, it's just a pointer to it. That's not an issue, std::lock_guard does that as well. Note that DUChainLock would be annotated with __attribute__((capability("mutex"))), but DUChainReadLocker with __attribute__((scoped_lockable)). The analysis tracks which scoped capability owns which capability.

Since I have never seen DUChain{Read,Write}Locker used with anything other than DUChain::lock()

Wondered about this as well—the constructor allows specifying any lock, but this is used only with DUChain::lock().

this shouldn't be cause for false positive deadlock detection.

Deadlock detection needs to turned on separately anyway. (With -Wthread-safety-negative, because it requires "negative capabilities", i.e. someone not having a lock as opposed to having it.)

I wouldn't worry too much about that for now.

In D22182#489868, @aaronpuchert wrote:

I looked through all of the paths to the failed assertion discussed in that latter ticket and it appears that all but the invocation in PotentialBuddyCollector::accept is verified to have the reader lock held prior to invocation. There were two paths which needed a lock added given my change, so this change set should not cause any regression on bug #386901.

I've always wondered whether Clang's Thread Safety Analysis could be of any help in KDevelop, perhaps it is.

Add now necessary locks to avoid recurrence of bug 386901.