D22182: Remove anchorClicked lock which causes deadlock

[Michael Swan]

mswan added a comment.


  In D22182#489868 <https://phabricator.kde.org/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 <https://clang.llvm.org/docs/ThreadSafetyAnalysis.html> could be of any help in KDevelop, perhaps it is.
  
  
  I have never seen that 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.

REPOSITORY
  R32 KDevelop

REVISION DETAIL
  https://phabricator.kde.org/D22182

To: mswan, #kdevelop
Cc: aaronpuchert, kdevelop-devel, hmitonneau, christiant, glebaccon, domson, antismap, iodelay, alexeymin, geetamc, Pilzschaf, akshaydeo, surgenight, arrowd
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20190703/fa5b1fd9/attachment.html>