D22182: Remove anchorClicked lock which causes deadlock

[Aaron Puchert]

aaronpuchert added a comment.


  In D22182#490537 <https://phabricator.kde.org/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. I think this should be mentioned in the documentation, but I haven't found the time to write this 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 <https://phabricator.kde.org/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 <https://en.cppreference.com/w/cpp/thread/lock_guard/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);

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/20190704/ab7109bd/attachment.html>