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.

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.

I have never seen that before and it seems pretty interesting. All that work I just did to find which paths to that declaration function do not have adequate locking seemed like a fairly mechanical task that could have been automated. 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. 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. That said, I think this copying of DUChain::lock() is actually a flaw with the DUChain locking system right now. The reason we do this copying of DUChain::lock() instead of just making it a global variable or something is because DUChain::lock() is stored in the static global variable sdDUChainPrivate which is defined with Q_GLOBAL_STATIC(DUChainPrivate, sdDUChainPrivate). The aim of which is to reduce startup load time by not initializing sdDUChainPrivate (i.e. the global object which contains this lock) until it is used for the first time, e.g. the first use of DUChain::lock(). The reason we hide this lock in a private class in the first place is just to maintain a "separation of concerns." Several things can be done about this. We could keep this global static DUChainPrivate thing as it is and create a fake global, like g_DUChainLock or whatever and annotate all of these DUChain{Read,Write}Locker instance method declarations with those Clang Thread Safety annotations, but don't actually ever use that global variable. Since I have never seen DUChain{Read,Write}Locker used with anything other than DUChain::lock() this shouldn't be cause for false positive deadlock detection. An alternative to this is to actually make our DUChain lock a global variable and keep it separate from this DUChainPrivate state. This would eliminate some minuscule overhead in instantiation of our DUChain{Read,Write}Lock's and their respective locking and unlocking methods, but does make this lock a publicly visible entity.

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

Interesting. I have experimented with this a bit and I cannot find a way to articulate to the thread safety system that my lock and unlock functions are idempotent. If I call lock.unlock() and later return without locking it again, I get a warning (or rather an error because I set -Werror=thread-safety) that complains that I am releasing a lock that isn't held because the deconstructor for my locker is also annotated with release_capability(..). So would -Wno-thread-safety-negative or -Wno-error=thread-safety-negative allow us to get around that or is there possibly some other solution?

In D22182#490532, @mswan wrote:

Interesting. I have experimented with this a bit and I cannot find a way to articulate to the thread safety system that my lock and unlock functions are idempotent. If I call lock.unlock() and later return without locking it again, I get a warning (or rather an error because I set -Werror=thread-safety) that complains that I am releasing a lock that isn't held. So would -Wno-error=thread-safety-negative allow us to get around that or is there possibly some other solution?

Lock and unlock shouldn't be idempotent. There are recursive/reentrant mutexes, which can be locked multiple times, but must be unlocked the same number of times. They are rarely needed though, do we want that here?

If you assume the lock is initially held, and want to return with the lock released, you need to annotate the function with __attribute__((release_capability(lock))).

I'm just having a lock at the DUChain locking and try to figure out what the best way would be. It's a bit of a problem that we're not exactly doing it "by the book" sometimes, such as when scoped locks are handed as parameters to functions. But there is usually a way around that.

In D22182#490533, @aaronpuchert wrote:

Lock and unlock shouldn't be idempotent.

I agree, but in the current implementation they are see DUChainReadLocker::lock() and DUChainReadLocker::unlock(), they both check if we are already locked before continuing to lock or unlock respectively.

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#490534, @mswan wrote:

I agree, but in the current implementation they are see DUChainReadLocker::lock() and DUChainReadLocker::unlock(), they both check if we are already locked before continuing to lock or unlock respectively.

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

In D22182#490535, @mswan wrote:

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.

That's no problem, the destructor doesn't need the lock being held. See https://github.com/llvm/llvm-project/blob/llvmorg-8.0.0/clang/lib/Analysis/ThreadSafety.cpp#L965-L967: the warning is only emitted "if we're not destroying the scoped object."

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.

In D22182#490536, @aaronpuchert wrote:

That's no problem, the destructor doesn't need the lock being held. See https://github.com/llvm/llvm-project/blob/llvmorg-8.0.0/clang/lib/Analysis/ThreadSafety.cpp#L965-L967: the warning is only emitted "if we're not destroying the scoped object."

I have actually tried changing my experiment with clang's thread safety to use the scoping feature and I seem to be seeing a new issue with these destructors with an error like "lock using shared access, expected exclusive access."

EDIT: I have come across your oldish ticket regarding this: https://bugs.llvm.org/show_bug.cgi?id=33504. Evidently I need to annotate my DUChainReadLocker releasing functions with RELEASE(..) instead of RELEASE_SHARED(..). Not sure I understand why.

Do you have an IRC handle? This would be easiest to discuss in chat.

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#490537, @mswan wrote:

I have actually tried changing my experiment with clang's thread safety to use the scoping feature and I seem to be seeing a new issue with these destructors with an error like "lock using shared access, expected exclusive access."

EDIT: I have come across your oldish ticket regarding this: https://bugs.llvm.org/show_bug.cgi?id=33504. Evidently I need to annotate my DUChainReadLocker releasing functions with RELEASE(..) instead of RELEASE_SHARED(..). Not sure I understand why.

Glad you figured it out. There is actually a good reason for that. I think this should be mentioned in the documentation, but I haven't found the time to write it yet.

Do you have an IRC handle? This would be easiest to discuss in chat.

Perhaps I'll go online later today, you'll find me under the same name as here.

In D22182#490550, @mswan wrote:

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.

I think the biggest problem right now is that the lock is sometimes specified explicitly, sometimes not. In my opinion this is a readability issue independent from what we're trying to do here.

• If we make the lock explicit, we can just annotate the constructor with ACQUIRE[_SHARED](duChainLock) (using the macros from the docs). When we call it with DUChain::lock(), Clang assumes that whatever this function returns is now locked, so if something else is annotated with REQUIRES(DUChain::lock()), it should be able to figure out that this is fine.
• If we always make the lock implicit, we could use a “mock lock”. This would be a global dummy object that we annotate as mutex, and that we claim is acquired by DUChain{Read,Write}Locker. That this isn't actually a lock is irrelevant for the Analysis.

Both approaches should be workable in my opinion. The former has the advantage that it would generalize well to multiple locks, should the need ever arise. The latter means less code to write.

I'm slightly in favor of the former approach, because we wouldn't lose the ability to change things later, and because it would be less confusing.

I think you're going to run into two issues:

• Sometimes scoped locks are passed around as parameters. Perhaps you can replace that with annotations REQUIRES(DUChain::lock()) and by allowing adopting of a held lock by the *Locker classes, when needed.
• The constructor of the scoped locks is actually a try-lock. That is currently not supported. Perhaps we can rewrite the few places with timeout with adopting as well.

Adopting means that we don't lock the mutex, we just take over an already locked mutex. (See e.g. the second constructor of a lock_guard.) The annotation for that is REQUIRES(...) on the constructor.

Then you can do:

if (!DUChain::lock().lockForRead())
    return; // or error handling...
DUChain{Read,Write}Locker lock(DUChain::lock(), std::adopt_lock);

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 don't suspect our ..Locker constructors will ever (i.e. not anytime prior to the heat death of the universe) take a different lock. For that reason I am partial to making the lock implicit wherever possible.

When we call it with DUChain::lock(), Clang assumes that whatever this function returns is now locked, so if something else is annotated with REQUIRES(DUChain::lock()), it should be able to figure out that this is fine.

You are right. I actually watched a talk on the first iteration of Clang Thread Safety which confirms this point, i.e. https://www.youtube.com/watch?v=5Xx0zktqSs4&t=15m46s.

If we always make the lock implicit, we could use a “mock lock”. This would be a global dummy object that we annotate as mutex, and that we claim is acquired by DUChain{Read,Write}Locker. That this isn't actually a lock is irrelevant for the Analysis.

This is actually what I initially had in mind.

Both approaches should be workable in my opinion. The former has the advantage that it would generalize well to multiple locks, should the need ever arise. The latter means less code to write.

I'm slightly in favor of the former approach, because we wouldn't lose the ability to change things later, and because it would be less confusing.

Would we be able to change the constructor to default to DUChain::lock()? I'm not familiar with C++'s rules w.r.t. default function/constructor values. If we can't make it a function call, would we be able to make it a plain-old global variable? Regardless, I would be in favor of the mock lock approach because I like keeping verbosity down when it really doesn't suit an immediate need.

Sometimes scoped locks are passed around as parameters.

I have also noticed this in what I think is its singular incarnation, namely DUChainPrivate::storeAllInformation which is part of the fairly complex DUChainPrivate::doMoreCleanup procedure. In this case, it is unsurprisingly referring to DUChain::lock() and so should be trivial to support lock adoption by this method, regardless of which approach we decide on.

The constructor of the scoped locks is actually a try-lock.

That is a good point and I think we would be better off making different constructors. One that accepts a timeout, one that doesn't. The former does not take any default values so it is not ambiguous.

That is currently not supported.

That sounds like it could be an interesting project. It looks like they are pretty close since they have this whole "TRY_ACQUIRE" annotation which isn't exactly what we're looking for but almost. If it could be instead predicated on, say, the return value of a public boolean method or instance variable post-invocation, that would be enough for our needs. I have not looked into how this works and what kinds of things it can check. I assume it does a very basic check at the caller to see that the return value is immediately used and that the failed-to-lock branch doesn't require the lock. That is mere speculation.

Perhaps we can rewrite the few places with timeout with adopting as well.

Not sure that will be needed if one of us adds support for that slight augmentation to clang's existing TRY_ACQUIRE annotation. Currently whenever this timeout try-lock behaviour is performed, the locked() instance method is immediately invoked and usually leads to either returning or continuing with the rest of the function.

Adopting means that we don't lock the mutex, we just take over an already locked mutex. (See e.g. the second constructor of a lock_guard.) The annotation for that is REQUIRES(...) on the constructor.

Then you can do:

if (!DUChain::lock().lockForRead())
    return; // or error handling...
DUChain{Read,Write}Locker lock(DUChain::lock(), std::adopt_lock);

I am not sure I follow your example. Obviously we would just invoke DUChain{Read,Write}Locker instead of going through this sadistic exercise. It is unclear to me when we would actually want to adopt a lock in a DUChain{Read,Write}Locker constructor invocation. That said, adopting a lock seems plenty relevant to the many functions present in KDevelop that expect prior locking, e.g. methods beginning with ENSURE_CAN_READ.

In D22182#491716, @mswan wrote:

I don't suspect our ..Locker constructors will ever (i.e. not anytime prior to the heat death of the universe) take a different lock. For that reason I am partial to making the lock implicit wherever possible.

That's a bold statement. ;)

Would we be able to change the constructor to default to DUChain::lock()? I'm not familiar with C++'s rules w.r.t. default function/constructor values. If we can't make it a function call, would we be able to make it a plain-old global variable?

In principle we could have that as default argument, although that requires including duchain.h in duchainlock.h.

But I would actually advise against this. I would either make the argument explicit (meaning there is no default), or always implicit (meaning there is no way to specify another lock). If we make it implicit, then we don't have to use DUChain::lock() in the annotation, because that effectively becomes an implementation detail and can use a mock object instead.

Regardless, I would be in favor of the mock lock approach because I like keeping verbosity down when it really doesn't suit an immediate need.

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.

That is currently not supported.

That sounds like it could be an interesting project. It looks like they are pretty close since they have this whole "TRY_ACQUIRE" annotation which isn't exactly what we're looking for but almost. If it could be instead predicated on, say, the return value of a public boolean method or instance variable post-invocation, that would be enough for our needs. I have not looked into how this works and what kinds of things it can check.

It would require adding an additional attribute, which I'm hesitant to do, since we have a good workaround.

I assume it does a very basic check at the caller to see that the return value is immediately used and that the failed-to-lock branch doesn't require the lock. That is mere speculation.

It doesn't work like that actually, the branch point can be considerably later. Basically we check if a branch condition correlates to the return value of a try-acquire call and if it does, we assumes the lock is acquired on that branch.

void k() {
    bool hold  = mu.TryLock();
    bool hold2 = hold;
    if (b)
        hold = true;
    if (hold) {
        a = 8;    // warning: writing variable 'a' requires holding mutex 'mu' exclusively [-Wthread-safety-analysis]
    }
    if (hold2) {
        a = 8;    // No warning here.
        mu.Unlock();
    }
}

There are some deficits in the analysis of try-acquires, but that's mostly because it just isn't used very often.

Not sure that will be needed if one of us adds support for that slight augmentation to clang's existing TRY_ACQUIRE annotation. Currently whenever this timeout try-lock behaviour is performed, the locked() instance method is immediately invoked and usually leads to either returning or continuing with the rest of the function.

I find the current behavior confusing actually. A constructor is meant to construct a "valid" object or fail (that's the idea of RAII), yet here it might not be valid and we always have to check with some function. This isn't very idiomatic and also error-prone. Getting rid of this pattern is a value in and of itself, independent of whether we want to pursue the analysis.

Adopting means that we don't lock the mutex, we just take over an already locked mutex. (See e.g. the second constructor of a lock_guard.) The annotation for that is REQUIRES(...) on the constructor.

Then you can do:

if (!DUChain::lock().lockForRead())
    return; // or error handling...
DUChain{Read,Write}Locker lock(DUChain::lock(), std::adopt_lock);

I am not sure I follow your example. Obviously we would just invoke DUChain{Read,Write}Locker instead of going through this sadistic exercise. It is unclear to me when we would actually want to adopt a lock in a DUChain{Read,Write}Locker constructor invocation.

You are aware that the code this is meant to replace isn't just a constructor call? This is the pattern that we find right now:

DUChain{Read,Write}Locker lock(DUChain::lock(), /*timeout*/ 100);
if (!lock.locked())
    return; // or error handling...

What do you find sadistic about my proposed replacement?

That said, adopting a lock seems plenty relevant to the many functions present in KDevelop that expect prior locking, e.g. methods beginning with ENSURE_CAN_READ.

Adopting means the lock will be released upon leaving the function. That's not what we want there, we want an annotation on the function REQUIRES_SHARED(...). Remember that the main purpose of scoped locks is to ensure release on all code paths. If you don't want to release anything, there is no reason for a scoped lock.

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.

I agree, it doesn't really hurt readability but it just seems like the same constructor argument on repeat through the whole code seems like a blemish to me. It is really a triviality of syntax so if there are practical reasons to make it explicit then that is obviously what should be done.

That sounds like it could be an interesting project. It looks like they are pretty close since they have this whole "TRY_ACQUIRE" annotation which isn't exactly what we're looking for but almost. If it could be instead predicated on, say, the return value of a public boolean method or instance variable post-invocation, that would be enough for our needs. I have not looked into how this works and what kinds of things it can check.

It would require adding an additional attribute, which I'm hesitant to do, since we have a good workaround.

The workaround being that we eliminate the try-lock behaviour in our locker constructors and move that timeout behaviour elsewhere?

I assume it does a very basic check at the caller to see that the return value is immediately used and that the failed-to-lock branch doesn't require the lock. That is mere speculation.

It doesn't work like that actually, the branch point can be considerably later. Basically we check if a branch condition correlates to the return value of a try-acquire call and if it does, we assumes the lock is acquired on that branch.

void k() {
    bool hold  = mu.TryLock();
    bool hold2 = hold;
    if (b)
        hold = true;
    if (hold) {
        a = 8;    // warning: writing variable 'a' requires holding mutex 'mu' exclusively [-Wthread-safety-analysis]
    }
    if (hold2) {
        a = 8;    // No warning here.
        mu.Unlock();
    }
}

There are some deficits in the analysis of try-acquires, but that's mostly because it just isn't used very often.

Interesting. Good to know.

Not sure that will be needed if one of us adds support for that slight augmentation to clang's existing TRY_ACQUIRE annotation. Currently whenever this timeout try-lock behaviour is performed, the locked() instance method is immediately invoked and usually leads to either returning or continuing with the rest of the function.

I find the current behavior confusing actually. A constructor is meant to construct a "valid" object or fail (that's the idea of RAII), yet here it might not be valid and we always have to check with some function. This isn't very idiomatic and also error-prone. Getting rid of this pattern is a value in and of itself, independent of whether we want to pursue the analysis.

C++ idioms are relatively foreign to me so I am not sure what a constructor like this "should" do but I see your point. We could have those very few cases where timeout is used invoke DUChainLock functions directly instead of us having to worry about that timeout try-lock weirdness in our locker implementation. I know of two places where there is a timeout being passed to the DUChainReadLocker which is not necessarily cause for changing the semantics of its 300+ other invocations.

You are aware that the code this is meant to replace isn't just a constructor call? This is the pattern that we find right now:

DUChain{Read,Write}Locker lock(DUChain::lock(), /*timeout*/ 100);
if (!lock.locked())
    return; // or error handling...

What do you find sadistic about my proposed replacement?

I see, that makes more sense. So, your proposal is probably what should be done in those few places where we actually use this timeout functionality. I mean, we are currently talking about like five callsites between both the read and write lockers so I'm not too worried about how this timeout code looks so long as it works. Ideally we could also test those callsites with clang thread safety but that too is not as essential as the hundreds of non-timeout invocations.

That said, adopting a lock seems plenty relevant to the many functions present in KDevelop that expect prior locking, e.g. methods beginning with ENSURE_CAN_READ.

Adopting means the lock will be released upon leaving the function.

I see, I did not realize that.

That's not what we want there, we want an annotation on the function REQUIRES_SHARED(...). Remember that the main purpose of scoped locks is to ensure release on all code paths. If you don't want to release anything, there is no reason for a scoped lock.

Err, so now I'm confused again. So, does REQUIRES_SHARED(...) require that this share of the lock is no longer held upon completion of this function? If so then is it possible to annotate a function in a semantically identical way to ENSURE_CAN_READ which implies that a read (i.e. shared) lock is held before and (possibly) after the function evaluates, or does that kind of behaviour break this whole thing? I imagine then we would have to be much more fine-grained with our usage of DUChain locks and move these lockers closer to the actual mutations to DUChain.

In D22182#492087, @mswan wrote:

The workaround being that we eliminate the try-lock behaviour in our locker constructors and move that timeout behaviour elsewhere?

Exactly.

That's not what we want there, we want an annotation on the function REQUIRES_SHARED(...). Remember that the main purpose of scoped locks is to ensure release on all code paths. If you don't want to release anything, there is no reason for a scoped lock.

Err, so now I'm confused again. So, does REQUIRES_SHARED(...) require that this share of the lock is no longer held upon completion of this function? If so then is it possible to annotate a function in a semantically identical way to ENSURE_CAN_READ which implies that a read (i.e. shared) lock is held before and (possibly) after the function evaluates, or does that kind of behaviour break this whole thing? I imagine then we would have to be much more fine-grained with our usage of DUChain locks and move these lockers closer to the actual mutations to DUChain.

Maybe the terminology is a bit confusing: a "shared" lock is a read lock (many readers can happily coexist), while an "exclusive" lock is a write lock. (The "exclusive" is omitted from the attributes because standard mutexes are always exclusive.) The annotation REQUIRES_SHARED(m) means that the function can only be called when m is locked in shared mode. Also, m should still be held in shared mode a the end of the function. Basically the attributes encode state transitions:

where the rows are the state of a mutex at the beginning of a function, and the columns are the state at the end of the function.

So if a function doesn't lock or unlock and just calls ENSURE_CAN_READ, it's a hot candidate for REQUIRES_SHARED. If we can fix all thread safety warnings and turn them on as errors, we might be able to remove these ENSURE_* assertions completely, because we enforce that at compile-time.

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?

I don't know what would be required to support multiple DUChains, but it seems plausible that they can be locked separately. In this case it would help to have the explicit argument, I guess. It could even be inevitable, when there is no single global DUChain anymore.

In D22182#495823, @mswan wrote:

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());, [...]

By the way, I've started working on that, so you don't need to bother yourself doing that. When I have some annotations done I will have a look at this again and properly review your change.

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.

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.

@mswan, there is renewed interest in these fixes: https://invent.kde.org/kdevelop/kdevelop/-/merge_requests/277. But we no longer use Phabricator for code reviews. Would you like to convert this review into a merge request at KDE's GitLab instance yourself? Or let someone else do that?

The replacement merge request was just merged into release/22.04. Therefore this review is obsolete and should be abandoned.